From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28762 invoked by alias); 9 Sep 2003 03:00:40 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 28557 invoked from network); 9 Sep 2003 03:00:37 -0000 Received: from unknown (HELO localhost.redhat.com) (65.49.0.121) by sources.redhat.com with SMTP; 9 Sep 2003 03:00:37 -0000 Received: from redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 868812B8E; Mon, 8 Sep 2003 19:45:07 -0400 (EDT) Message-ID: <3F5D1483.1060302@redhat.com> Date: Tue, 09 Sep 2003 03:00:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.2) Gecko/20030820 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Mark Kettenis Cc: gdb-patches@sources.redhat.com Subject: Re: [rfa/6.0] Better handle unspecified CFI values References: <3F593115.4030407@redhat.com> <86oexwnyqe.fsf@elgar.kettenis.dyndns.org> Content-Type: multipart/mixed; boundary="------------030401000102090201040506" X-SW-Source: 2003-09/txt/msg00142.txt.bz2 This is a multi-part message in MIME format. --------------030401000102090201040506 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2839 > 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 --------------030401000102090201040506 Content-Type: text/plain; name="diffs" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="diffs" Content-length: 9795 2003-09-05 Andrew Cagney * 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; --------------030401000102090201040506--