This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [rfa/6.0] Better handle unspecified CFI values
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
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, ®);
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, ®);
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;