[RFA] dwarf2cfi.c improvements

[Elena Zannoni]

Michal Ludvig writes:
 > Elena Zannoni wrote:
 > > Michal Ludvig writes:
 > >  >  > 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 have introducend new function pointer_encoding() that the function 
 > calling read_encoded_pointer() may invoke to see what was returned and 
 > how to handle it. I'm convinced, that it's the caller's responsibility 
 > to deal with different PE's (pointer encodings), because sometimes some 
 > of them are valid, sometimes not. If I'd just pass base_addr into the 
 > read_enc_ptr() how would I know whether it was handled correctly. There 
 > might be a situation, that the pointer would be encoded either in pcrel 
 > or in funcrel format - then I should decide afterwards how to compute 
 > the base address, shouldn't I?
 > 

Much better. This makes the code a bit easier to understand. The
previous version of the function was trying to do too much.

 > > 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? 
 > 
 > No, because it depends on PE how much data should I eat up. I can have 
 > another function that would give me the number of bytes occupied by a 
 > given PE, but I don't believe it's a good idea. It's not called that 
 > often, that it could speed things up.
 > 

No need anymore, with the pointer_encoding function.

 > > But I see that gcc does
 > > some similar trick. BTW that function deals with alignment and the gdb
 > > version doesn't (yet?).
 > 
 > Hmm I didn't notice. I'll look at it. But it works as it is.
 > 

Keep it in mind if you see strange behaviors.

 > >  > - 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. 
 > 
 > It wasn't correct, anyway. I have written my reasons against this 
 > approach above.
 > 
 > > 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.
 > 
 > How can I know how to compute base_addr when I don't know whether the 
 > encoding is pcrel, textrel, funcrel, ...? I know these aren't used _now_ 
 > but at least hypothetically are possible.
 > 

Right. OK.

 > >  > - 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?
 > 
 > I must only know in how many bytes it's encoded so that I advance the 
 > data pointer correctly. The value itself, either if encoded as pcrel, is 
 > always absolute. At least I hope so :-)
 > 
 > 
 > > 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?
 > 
 > I can't because DW_EH_PE_indirect is independent on DW_EH_PE_*rel. Both 
 > PE_pcrel and PE_pcrel_indirect are possible. In fact, encoding variable 
 > contains three flags: size, encoding and (in)direct flag.
 > 

Ah, I see. 3 masks for 3 different things. Is there any way you can
add a comment about this?

 > All of them must be handled separately.
 > 
 > >  > > 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.  
 > 
 > I have changed computing of this parameter in dwarf2_build_frame_info so 
 > that it is more readable now, but I'd leave 'eh_frame' as it is, because 
 > in most cases I don't care whether it's 1 or 2. The hack with 
 > eh_frame==2 is there just for speeding up things when looking for 
 > duplicate FDEs. I eh_frame is the only section parsed, then I don't have 
 > to be afraid that we already have a given FDE and don't have to check it 
 > at all.
 > 

Could you add a comment explaining the differences between the
eh_frame section and the debug_frame section? Only a couple, as far as
I know.

 > >  > 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).
 > 
 > IS_EH_FRAME would be true for both possible cases with eh_frame.
 > As I already said - usually I don't care whether it's parsed first or 
 > after debug_frame.
 > 
 > > Could you send me a unified diff? (don't need to send it to the list
 > > just to me). I'd appreciate that.
 > 
 > Yes. Included. It's against cvs rev 1.9.
 > 

A few little things:

 > +
 > +enum ptr_encoding
 > +pointer_encoding (unsigned char encoding)
 > +{
 > +  int ret;
 >  
 > +  if (encoding & DW_EH_PE_indirect)
 > +    warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
 > +  
 > +  switch (encoding & 0x70)
 > +    {
 > +    case DW_EH_PE_absptr:
 > +    case DW_EH_PE_pcrel:
 > +    case DW_EH_PE_textrel:
 > +    case DW_EH_PE_datarel:
 > +    case DW_EH_PE_funcrel:
 > +      ret = encoding & 0x70;
 > +      break;
 > +    default:
 > +      internal_error (__FILE__, __LINE__,
 > +    		  "read_encoded_pointer: unknown pointer encoding");


The function name should be pointer_encoding.



 > +static void
 > +parse_frame_info (struct objfile *objfile, file_ptr frame_offset,
 > +		  unsigned int frame_size, int eh_frame)
 >  {
 >    bfd *abfd = objfile->obfd;
 > +  asection *curr_section_ptr;
 >    char *start = NULL;
 >    char *end = NULL;
 > -  int from_eh = 0;
 > +  char *frame_buffer = NULL;
 > +  char *curr_section_name, *aug_data;
 > +  struct cie_unit *last_cie = NULL;
 > +  int last_dup_fde = 0, aug_len, i;

Please put these on separate lines.



 > +			  if (eh_frame &&
 > +			      (cie->offset ==
 > +			       (unit_offset + bytes_read - cie_id)))

Please reformat this line.



 > +	      for (i = 0; eh_frame == 2 && i < fde_chunks.elems; i++)

Could you include the for loop in an if statement, instead of adding the
eh_frame==2 test?


 > +  int after_debug_frame=0;

Spaces.


ChangeLog?

Other than that approved.

Elena