Re: [rfa/6.0] Better handle unspecified CFI values

[Mark Kettenis <kettenis at chello dot nl>]

Andrew Cagney <ac131313@redhat.com> writes:

> This is a multi-part message in MIME format.
> --------------000900050201080102070403
> Content-Type: text/plain; charset=us-ascii; format=flowed
> Content-Transfer-Encoding: 7bit
> 
> Hello,
> 
> This patch is an attempt at improving GDB's behavior when GCC "treads 
> the boundaries of the CFI specification".
> 
> It does the following:
> 
> - changes the rules REG_UNMODIFIED -> REG_SAME_VALUE and REG_UNSAVED -> 
> REG_UNDEFINED so that they better match the corresponding CFI register 
> states (I could commit this separatly).  The other names confused me :-)

That's closer to the definitions used in the specification, so go
ahead.  I used REG_UNSAVED since that's what GCC's unwinder uses, so
perhaps you could add a comment that says this?

> 
> - it adds a new register rule - REG_UNSPECIFIED - which is used to 
> differentiate a register that is missing CFI info from a register that 
> CFI specified as "undefined" (nee UNSAVED).

Great!

> 
> - when unwinding, it treats REG_UNSPECIFIED registers like 
> REG_SAME_VALUE but with the additional hack to map an unspecified 
> SP_REGNUM onto the CFA.

OK!

> - if it detects an unspecified CFI entry it complains
> It isn't perfect though - since it doesn't know the full range of valid 
> debug info register numbers it can't check every entry.  Instead it 
> checks the range provided by CFI for unspecified holes and then 
> complains about that.  The reality is that GCC at least gets that bit 
> right (but consistently forgets the SP).

Sorry, GCC gets what right?

Anyway, the full range of valid debug info registers, is defined by
the DWARF register mapping that some ABI's define (at least the x86-64
ABI provides such a definition).  GDB sort of has this information in
that we have the dwarf2_reg_to_regnum function in the architecture
vector.

> 
> I'd like to commit the patch as is for the 6.0 branch.  For the mainline 
> though, I'd like to make the additional changes:
> 
> - delete the SP_REGNUM hack from the REG_UNDEFINED rule (it's no longer 
> needed, I think)

I guess that if the stack pointer is explicitly marked as being
"undefined", all will be lost on most architectures.

> - add a check/complaint for the SP v CFA problem.
> 
> Anyway, the end result is that on x86-64 and i386 store.exp now passes.
> 
> ok to commit?

Hmm, three "style" nits:

- The uppercase MEMSET in the comment on `enum dwarf2_reg_rule' is
  wrong.  Uppercase is reserved for when we're talking about the value
  of a symbol, not the symbol itself.  So please convert this into
  lowercase.

- I find the name `cfinum' a bit cryptic.  I understand that you want
  to avoid `reg', how about calling it `column'?  That's sort of CFI
  speak for a register number.

- Please remove the extra blank line before `case REG_SAME_VALUE:'.

With those changes this is OK (or can't I approve changes to a file
I've written in the first place?).  You can also leave it to me to
make those changes ;-).

Mark