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


Michal Ludvig writes:
 > 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.
 > 

No, you should run gdb_indent.sh instead. That is the approved format.

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

It is run every day by a script. It detects a set of generally
deprecated conventions and usages. More often than not it also
uncovers bugs.  I don't think you can run it yourself. But you can
look at the categories listed and have a feel for what is deprecated.
http://sources.redhat.com/gdb/ari/

 > > 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?

Sigh. OK, but it will take a bit longer, I am afraid. 

 > 
 >  > 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.

See below for a different suggestion.

 > 
 >  > 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.

OK. But still, the warning should come from inside the function
itself.  I was thinking you needed to do it outside, to have the
fs->pc available to be printed in the warning, but that value actually
comes from inside the function itself, so it doesn't need to be separate.

 > - When reading pers_addr I don't care about the return value at all. I 
 > just need to advance the pointer to data.

Actually, do you need to call this function at all, if all you need to
do is eat up some data? This is really overloading the function.
Could you just call one of the read_* functions?  But I see that gcc does
some similar trick. BTW that function deals with alignment and the gdb
version doesn't (yet?).

 > - Init_loc reading takes care of pcrel addressing.

Yes, the bit that does it though, was inside read_encoded_pointer, and
now it's not, and you also need to do an additional check, so I  would
prefer doing it as it was before. 
Could you instead add a new parmeter *base* instead of the flag?  In
the DW_CFA_set_loc case, base would be NULL while here it would be
base_offset, so you could still distinguish the two cases.

 > - 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.
 > 

So that would work anyway, right?

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

I don't undersand this change:
*** 530,541 ****
      }
  
    if (ret != 0)
!     switch (encoding & 0xf0)
        {
        case DW_EH_PE_absptr:
        break;
        case DW_EH_PE_pcrel:
!       ret += (CORE_ADDR) *p;
        break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
--- 531,543 ----
      }
  
    if (ret != 0)
!   {
!     switch (encoding & 0x70)
        {
        case DW_EH_PE_absptr:
        break;
        case DW_EH_PE_pcrel:
!       if (flag_pcrel) *flag_pcrel = 1;
        break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
*************** read_encoded_pointer (bfd *abfd, char **
*** 544,549 ****
--- 546,555 ----
        internal_error (__FILE__, __LINE__,
                        "read_encoded_pointer: unknown pointer encoding");
        }
+     
+     if (encoding & DW_EH_PE_indirect)
+       warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
+   }
  
    return ret;
  }

Why are you changing the mask from 0xf0 to 0x70 in the switch to avoid
catching DW_EH_PE_indirect? Could you just add it to the case statement?



 > > 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) ...
 > 

That is the idea, yes. It would be much more understandable to the
reader.  

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

How would those behave different from enums? If I am understanding your
suggestion correctly, that is (macros are usually not introduced
unless absolutely necessary).

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

Never mind, there is too much similarity between the 2 cases.

 > > 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.
 > 

Could you send me a unified diff? (don't need to send it to the list
just to me). I'd appreciate that.

thanks
Elena


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