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

Andrew Cagney
2003-09-09T03:00:40Z

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]

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

Ah! That also explains why GCC has problems. It's combined two
different states :-/ I've added comments to that effect.

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

On the platforms I tested (amd64 & i386) all [0 .. max "column") were
specified. The problems were with numbers outside of that range. I'll
re-word the comment.

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

Yes. There isn't a way of iterating over it. Oh, and btw, it can also
be sparse - I believe that the e500 (ppc variant) has has dwarf2 numbers
>1000. Something to address in a later iteration.

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

Yes, bit of a loose-loose situtation. If it's really "undefined" GDB
can't use "CFI" as that would mislead the user into thinking that a
bogus backtrace is valid.

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

I think they are fixed. I've attached the diff I'll commit.

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 9795 bytes --]

2003-09-05 Andrew Cagney <cagney@redhat.com>

* dwarf2-frame.c (enum dwarf2_reg_rule): New, replace anonymous
enum. Add REG_UNSPECIFIED, rename REG_UNSAVED to REG_UNDEFINED
and REG_UNMODIFIED to REG_SAME_VALUE.
(execute_cfa_program): Update.
(dwarf2_frame_cache): Update. Initialize table to
REG_UNSPECIFIED, complain if CFI fails to specify a register's
location.
(dwarf2_frame_prev_register): Update. Handle REG_UNSPECIFIED.

--- /home/scratch/GDB/src/gdb/dwarf2-frame.c Fri Sep 5 15:55:04 2003
+++ dwarf2-frame.c Mon Sep 8 19:37:24 2003
@@ -97,6 +97,28 @@

/* Structure describing a frame state. */

+enum dwarf2_reg_rule
+{
+ /* Make certain that 0 maps onto the correct enum value - the
+ corresponding structure is being initialized using memset zero.
+ This indicates that CFI didn't provide any information at all
+ about a register - leaving how to obtain it's value totally
+ unspecified. */
+ REG_UNSPECIFIED = 0,
+ /* The term "undefined" comes from the DWARF2 CFI spec which this
+ code is moddeling - it indicates that the register's value is
+ "undefined". */
+ /* NOTE: cagney/2003-09-08: GCC uses the less formal term "unsaved"
+ - it's definition is a combination of REG_UNDEFINED and
+ REG_UNSPECIFIED - the failure to differentiate the two helps
+ explain a few problems with the CFI GCC outputs. */
+ REG_UNDEFINED,
+ REG_SAVED_OFFSET,
+ REG_SAVED_REG,
+ REG_SAVED_EXP,
+ REG_SAME_VALUE
+};
+
struct dwarf2_frame_state
{
/* Each register save state can be described in terms of a CFA slot,
@@ -111,13 +133,7 @@
unsigned char *exp;
} loc;
ULONGEST exp_len;
- enum {
- REG_UNSAVED,
- REG_SAVED_OFFSET,
- REG_SAVED_REG,
- REG_SAVED_EXP,
- REG_UNMODIFIED
- } how;
+ enum dwarf2_reg_rule how;
} *reg;
int num_regs;

@@ -354,13 +370,13 @@
case DW_CFA_undefined:
insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
- fs->regs.reg[reg].how = REG_UNSAVED;
+ fs->regs.reg[reg].how = REG_UNDEFINED;
break;

case DW_CFA_same_value:
insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
- fs->regs.reg[reg].how = REG_UNMODIFIED;
+ fs->regs.reg[reg].how = REG_SAME_VALUE;
break;

case DW_CFA_register:
@@ -460,11 +476,10 @@
dwarf2_frame_cache (struct frame_info *next_frame, void **this_cache)
{
struct cleanup *old_chain;
- int num_regs = NUM_REGS + NUM_PSEUDO_REGS;
+ const int num_regs = NUM_REGS + NUM_PSEUDO_REGS;
struct dwarf2_frame_cache *cache;
struct dwarf2_frame_state *fs;
struct dwarf2_fde *fde;
- int reg;

if (*this_cache)
return *this_cache;
@@ -535,34 +550,65 @@
internal_error (__FILE__, __LINE__, "Unknown CFA rule.");
}

- /* Save the register info in the cache. */
- for (reg = 0; reg < fs->regs.num_regs; reg++)
- {
- int regnum;
-
- /* Skip the return address column. */
- if (reg == fs->retaddr_column)
- /* NOTE: cagney/2003-06-07: Is this right? What if the
- RETADDR_COLUM corresponds to a real register (and, worse,
- that isn't the PC_REGNUM)? I'm guessing that the PC_REGNUM
- further down is trying to handle this. That can't be right
- though - PC_REGNUM may not be valid (it can be -ve). I
- think, instead when RETADDR_COLUM isn't a real register, it
- should map itself onto frame_pc_unwind. */
- continue;
-
- /* Use the GDB register number as index. */
- regnum = DWARF2_REG_TO_REGNUM (reg);
+ /* Initialize things so that all registers are marked as
+ unspecified. */
+ {
+ int regnum;
+ for (regnum = 0; regnum < num_regs; regnum++)
+ cache->reg[regnum].how = REG_UNSPECIFIED;
+ }

- if (regnum >= 0 && regnum < num_regs)
- cache->reg[regnum] = fs->regs.reg[reg];
- }
+ /* Go through the DWARF2 CFI generated table and save its register
+ location information in the cache. */
+ {
+ int column; /* CFI speak for "register number". */
+ for (column = 0; column < fs->regs.num_regs; column++)
+ {
+ int regnum;
+
+ /* Skip the return address column. */
+ if (column == fs->retaddr_column)
+ /* NOTE: cagney/2003-06-07: Is this right? What if
+ RETADDR_COLUMN corresponds to a real register (and,
+ worse, that isn't the PC_REGNUM)? I'm guessing that the
+ PC_REGNUM further down is trying to handle this. That
+ can't be right though - PC_REGNUM may not be valid (it
+ can be -ve). I think, instead when RETADDR_COLUM isn't a
+ real register, it should map itself onto frame_pc_unwind. */
+ continue;
+
+ /* Use the GDB register number as the destination index. */
+ regnum = DWARF2_REG_TO_REGNUM (column);
+
+ /* If there's no corresponding GDB register, ignore it. */
+ if (regnum < 0 || regnum >= num_regs)
+ continue;
+
+ /* NOTE: cagney/2003-09-05: CFI should specify the disposition
+ of all debug info registers. If it doesn't complain (but
+ not too loudly). It turns out that GCC, assumes that an
+ unspecified register implies "same value" when CFI (draft
+ 7) specifies nothing at all. Such a register could equally
+ be interpreted as "undefined". Also note that this check
+ isn't sufficient - it only checks that all registers in the
+ range [0 .. max column] are specified - and won't detect
+ problems when a debug info register falls outside of the
+ table. Need a way of iterating through all the valid
+ DWARF2 register numbers. */
+ if (fs->regs.reg[column].how == REG_UNSPECIFIED)
+ complaint (&symfile_complaints,
+ "Incomplete CFI data; unspecified registers at 0x%s",
+ paddr (fs->pc));
+
+ cache->reg[regnum] = fs->regs.reg[column];
+ }
+ }

/* Store the location of the return addess. If the return address
column (adjusted) is not the same as gdb's PC_REGNUM, then this
implies a copy from the ra column register. */
if (fs->retaddr_column < fs->regs.num_regs
- && fs->regs.reg[fs->retaddr_column].how != REG_UNSAVED)
+ && fs->regs.reg[fs->retaddr_column].how != REG_UNDEFINED)
{
/* See comment above about a possibly -ve PC_REGNUM. If this
assertion fails, it's a problem with this code and not the
@@ -572,7 +618,7 @@
}
else
{
- reg = DWARF2_REG_TO_REGNUM (fs->retaddr_column);
+ int reg = DWARF2_REG_TO_REGNUM (fs->retaddr_column);
if (reg != PC_REGNUM)
{
/* See comment above about PC_REGNUM being -ve. If this
@@ -611,7 +657,9 @@

switch (cache->reg[regnum].how)
{
- case REG_UNSAVED:
+ case REG_UNDEFINED:
+ /* If CFI explicitly specified that the value isn't defined,
+ mark it as optomized away - the value isn't available. */
*optimizedp = 1;
*lvalp = not_lval;
*addrp = 0;
@@ -636,6 +684,12 @@
very real posibility that CFA is an offset from some
other register, having nothing to do with the unwound SP
value. */
+ /* FIXME: cagney/2003-09-05: I think I do now. GDB was
+ lumping the two states "unspecified" and "undefined"
+ together. Here SP_REGNUM was "unspecified", GCC assuming
+ that in such a case CFA would be used. This code should
+ be deleted - this specific problem now handled as part of
+ REG_UNSPECIFIED case further down. */
*optimizedp = 0;
if (valuep)
{
@@ -687,7 +741,52 @@
}
break;

- case REG_UNMODIFIED:
+ case REG_UNSPECIFIED:
+ /* GCC, in its infinite wisdom decided to not provide unwind
+ information for registers that are "same value". Since
+ DWARF2 (3 draft 7) doesn't define such behavior, said
+ registers are actually undefined (which is different to CFI
+ "undefined"). Code above issues a complaint about this.
+ Here just fudge the books, assume GCC, and that the value is
+ more inner on the stack. */
+ if (SP_REGNUM >= 0 && regnum == SP_REGNUM)
+ {
+ /* Can things get worse? Yep! One of the registers GCC
+ forgot to provide unwind information for was the stack
+ pointer. Outch! GCC appears to assumes that the CFA
+ address can be used - after all it points to the inner
+ most address of the previous frame before the function
+ call and that's always the same as the stack pointer on
+ return, right? Wrong. See GCC's i386 STDCALL option for
+ an ABI that has a different entry and return stack
+ pointer. */
+ /* DWARF V3 Draft 7 p102: Typically, the CFA is defined to
+ be the value of the stack pointer at the call site in the
+ previous frame (which may be different from its value on
+ entry to the current frame). */
+ /* DWARF V3 Draft 7 p103: The first column of the rules
+ defines the rule which computes the CFA value; it may be
+ either a register and a signed offset that are added
+ together or a DWARF expression that is evaluated. */
+ /* FIXME: cagney/2003-09-05: Complain about this via
+ complaint(). */
+ *optimizedp = 0;
+ *lvalp = not_lval;
+ *addrp = 0;
+ *realnump = -1;
+ if (valuep)
+ /* Store the value. */
+ store_typed_address (valuep, builtin_type_void_data_ptr,
+ cache->cfa);
+ }
+ else
+ /* Assume that the register can be found in the next inner
+ most frame. */
+ frame_register_unwind (next_frame, regnum,
+ optimizedp, lvalp, addrp, realnump, valuep);
+ break;
+
+ case REG_SAME_VALUE:
frame_register_unwind (next_frame, regnum,
optimizedp, lvalp, addrp, realnump, valuep);
break;