This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] dwarf2cfi.c improvements


Elena Zannoni wrote:
Michal Ludvig writes:
> > Well, this is the patch without unrelated message changes. Can I commit > it? It seems to be quite stable in our everyday use...
>
Michal, sorry, not as is.
No problem, I'll rework it.

There are coding standard violations having to do with spaces before
'(' and after ')' in casts. Also, it is better to not have multiple
initializations in a single line, like this:
 > !   char *start = NULL, *end = NULL, *frame_buffer = NULL;
Changed. I've reindented the whole file. I hope running 'indent dwarf2cfi.c' is enough.


Conditional expressions are also flagged by the ARI.
What does 'AR Index' mean and how do I run the checker?

Can I suggest that you split the pcrel changes into a separate patch?
Well, I wouldn't like to. When you start parsing .eh_frame you need a working pcrel, otherwise it is useless. But without parsing .eh_frame you can't verify whether it works, because .debug_frame doesn't use relative addressing. May I ask you to approve both at once, please?

> About pcrel, I don't like the use of the flag like you have below.
> Could you find some other way to do it w/o passing the address of the
> flag around? Could you maybe change the function to return 0/1 and
> have the current return value as an out parameter instead?

That is possible, but it would make read_encoded_pointer() behave differently from read_pointer(). I'll better introduce a new function like pointer_type() that would return an enum of possible types.

> Or alternatively, could you have the function read_encoded_pointer
> emit the warning itself?

In some cases PC-relative addressing is valid, but sometimes it is not (or at least is not used). The warning is there only once.

> After all, none of
> the other error/warning messages are that detailed, they don't print
> the PC value. I noticed that in a couple of the calls to
> read_encoded_pointer the code doesn't care about emitting the warning,
> why? Or add a pc parameter for the purpose of the warning.

There are four occurences:
- In DW_CFA_set_loc I don't (yet) handle pcrel addressing, because I have never seen it in .*_frame. That's why I print a warning - If I'll ever see it I'll implement support for it.
- When reading pers_addr I don't care about the return value at all. I just need to advance the pointer to data.
- Init_loc reading takes care of pcrel addressing.
- fde->address_range is always an absolute value, but encoded in a pointer format. I don't care if it is in pcrel format or not - it's just a difference between two pointers.

There are no other calls to read_encoded_pointer() in dwarf2cfi.c


About the eh_frame part of the patch.
I don't really like the use of eh_frame as you have in dwarf2_build_frame_info.
Could you try to figure out a simpler way to deal with the two cases?
You could use an enumeration for eh_frame.
Well, yes, actually. But then I would have to change:
if(eh_frame) ...
to
if(frame == EH_ONLY || frame == EH_SECOND) ...

What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(), IS_EH_FRAME_ONLY(), ...?


Could you do 2 passes, one for each section?
Hm? I don't understand what you mean...

I am not able to apply your patch, which I usually do, to try a test
run. Is there anyway you can regenerate the patch? Maybe with a unified
diff instead? (it's just that I find it easier to read)
> I would like to give you more intelligent comments about the eh_frame
> part, but I need to apply the patch first.

The cfi-huge5.diff was made against clean rev 1.7 of dwarf2cfi.c file. For me it applies cleanly against this revision.

Michal Ludvig
--
* SuSE CR, s.r.o * mludvig@suse.cz
* +420 2 9654 5373 * http://www.suse.cz



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]