Re: [RFA] dwarf2cfi.c improvements

[Michal Ludvig <mludvig@suse.cz>]

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