[RFA] dwarf2cfi.c improvements

Michal Ludvig mludvig@suse.cz
Thu Jun 13 12:02:00 GMT 2002


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?

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

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

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

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

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.

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

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



More information about the Gdb-patches mailing list