| Message ID | 87h9dyt7m3.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/16/2016 08:12 AM, Nick Clifton wrote: > Hi Guys, > > Currently dwarf2out.c:mem_loc_descriptor() has some special case > code to handle the situation where an address is held in a register > whose mode is not of type MODE_INT. It generates a > DW_OP_GNU_regval_type expression which may later on be converted into > a frame pointer based expression. This is a problem for targets which > use a partial integer mode for their pointers (eg the msp430). In > such cases the conversion to a frame pointer based expression could > be wrong if the frame pointer is not being used. > > For example the GDB testfile gdb/testsuite/gdb.base/advance.c contains > this code fragment: > > int > main () > { > int result; > int b, c; > c = 5; > b = 3; /* advance this location */ > > func (c); /* stop here after leaving current frame */ > > which compiles to these instructions: > > suba #6, r1 > mov #5, 4(r1) > mov #3, 2(r1) > mov 4(r1), r12 > calla #0 ;<func> > > (Note that only r1 - the stack pointer - is used. r4 - the frame > pointer - is not). > > The debug information produced for the "c" local variable looks like > this: > > Abbrev Number: 3 (DW_TAG_variable) > DW_AT_name : c > DW_AT_decl_file : 1 > DW_AT_decl_line : 40 > DW_AT_type : <0x37> > DW_AT_location : 5 byte block: f5 4 21 32 1c (DW_OP_GNU_regval_type: 4 (r4) <0x21>; DW_OP_lit2; DW_OP_minus) > > ie it says that "c" is stored in memory location "r4 - 2", which is > wrong since register r4 is not even used in this function. > > The patch below addresses this problem by allowing the normal, > register based descriptor to be produced when the mode is Pmode. > > With this patch applied the unexpected failure count in the GDB > testsuite for the MSP430's -mlarge multilib changes from 2253 to 367. > There are no regressions, for MSP430 or x86_64, and no changes to > the GCC testsuite results for either target. > > OK to apply ? > > Cheers > Nick > > gcc/ChangeLog > 2016-05-16 Nick Clifton <nickc@redhat.com> > > * dwarf2out.c (mem_loc_descriptor): Convert REG based addresses > whose mode is Pmode into basereg descriptors even if Pmode is > not an integer mode. I'm not real familiar with dwarf, so if one of other maintainers steps in and says this is OK, then ignore my comments/questions. I may be missing something, but isn't it the transition to an FP relative address rather than a SP relative address that's the problem here? Where does that happen? Is it possible we've got the wrong DECL_RTL or somesuch? Jeff
Hi Jeff, >> Currently dwarf2out.c:mem_loc_descriptor() has some special case >> code to handle the situation where an address is held in a register >> whose mode is not of type MODE_INT. It generates a >> DW_OP_GNU_regval_type expression which may later on be converted into >> a frame pointer based expression. This is a problem for targets which >> use a partial integer mode for their pointers (eg the msp430). In >> such cases the conversion to a frame pointer based expression could >> be wrong if the frame pointer is not being used. > I may be missing something, but isn't it the transition to an FP > relative address rather than a SP relative address that's the problem > here? Yes, I believe so. > Where does that happen? I did not track it down. But whilst I was searching for the cause I came across the code that is modified by the patch. Reading the code it seemed obvious to me that the special case for handling non INT_MODE register modes was not intended for pointers, and when I tried out a small patch it worked. > Is it possible we've got the wrong DECL_RTL or somesuch? I don't think so. I am not familiar with this code myself, but the dump from the dwarf2 pass shows: (insn 5 2 6 (set (mem/c:HI (plus:PSI (reg/f:PSI 1 R1) (const_int 4 [0x4])) [1 c+0 S2 A16]) (const_int 5 [0x5])) /work/sources/binutils/current/gdb/testsuite/gdb.base/advance.c:41 12 {movhi} (nil)) which to me pretty clearly shows that "c" is being stored at R1+4. Cheers Nick
On 05/17/2016 06:37 AM, Nick Clifton wrote: > Hi Jeff, > >>> Currently dwarf2out.c:mem_loc_descriptor() has some special case >>> code to handle the situation where an address is held in a register >>> whose mode is not of type MODE_INT. It generates a >>> DW_OP_GNU_regval_type expression which may later on be converted into >>> a frame pointer based expression. This is a problem for targets which >>> use a partial integer mode for their pointers (eg the msp430). In >>> such cases the conversion to a frame pointer based expression could >>> be wrong if the frame pointer is not being used. > >> I may be missing something, but isn't it the transition to an FP >> relative address rather than a SP relative address that's the problem >> here? > > Yes, I believe so. > >> Where does that happen? > > I did not track it down. But whilst I was searching for the cause I came > across the code that is modified by the patch. Reading the code it seemed > obvious to me that the special case for handling non INT_MODE register modes > was not intended for pointers, and when I tried out a small patch it worked. Maybe rather than tweaking behaviour based on whether or not it's a pointer type, we should look at whether or not the object has an integer mode (ie, test for MODE_INT or MODE_PARTIAL_INT). > >> Is it possible we've got the wrong DECL_RTL or somesuch? > > I don't think so. I am not familiar with this code myself, but the dump from > the dwarf2 pass shows: > > (insn 5 2 6 (set (mem/c:HI (plus:PSI (reg/f:PSI 1 R1) > (const_int 4 [0x4])) [1 c+0 S2 A16]) > (const_int 5 [0x5])) /work/sources/binutils/current/gdb/testsuite/gdb.base/advance.c:41 12 {movhi} > (nil)) > > which to me pretty clearly shows that "c" is being stored at R1+4. Right, but I believe a fair amount of the dwarf stuff goes back to trees, which have things like DECL_RTL/DECL_INCOMING_RTL and friends embedded inside them. I wouldn't be terribly surprised to find that it's looking at some stale hunk of RTL that wasn't updated for register eliminations or something of that nature. I think we should dig further into why the base register (and offset) is wrong and fix that. We may independently want to tweak the code in mem_loc_descriptor to better handle partial integers. jeff
Hi Jeff, >>> I may be missing something, but isn't it the transition to an FP >>> relative address rather than a SP relative address that's the problem >>> here? >> >> Yes, I believe so. >> >>> Where does that happen? I think that it happens in dwarf2out.c:based_loc_descr() which detects the use of the frame pointer and works out that it is going to be eliminated to the stack pointer: /* We only use "frame base" when we're sure we're talking about the post-prologue local stack frame. We do this by *not* running register elimination until this point, and recognizing the special argument pointer and soft frame pointer rtx's. */ if (reg == arg_pointer_rtx || reg == frame_pointer_rtx) { rtx elim = (ira_use_lra_p ? lra_eliminate_regs (reg, VOIDmode, NULL_RTX) : eliminate_regs (reg, VOIDmode, NULL_RTX)); if (elim != reg) ..... The problem, I believe, is that based_loc_descr() is only called from mem_loc_descriptor when the mode of the rtl concerned is an MODE_INT. For example: case REG: if (GET_MODE_CLASS (mode) != MODE_INT [...] else if (REGNO (rtl) < FIRST_PSEUDO_REGISTER) mem_loc_result = based_loc_descr (rtl, 0, VAR_INIT_STATUS_INITIALIZED); or, (this is another one that I found whilst investigating this problem further): case PLUS: plus: if (is_based_loc (rtl) && (GET_MODE_SIZE (mode) <= DWARF2_ADDR_SIZE || XEXP (rtl, 0) == arg_pointer_rtx || XEXP (rtl, 0) == frame_pointer_rtx) && GET_MODE_CLASS (mode) == MODE_INT) mem_loc_result = based_loc_descr (XEXP (rtl, 0), INTVAL (XEXP (rtl, 1)), VAR_INIT_STATUS_INITIALIZED); else There are quite a few places in mem_loc_descriptor where the code checks for the mode being in the MODE_INT class. I am not exactly sure why. I think that it might be that the programmer thought that any expression that does not involve integer based arithmetic cannot be expressed in DWARF CFA notation and so would have to be handled specially. If I am correct, then it seems to me that the proper fix would be to use SCALAR_INT_MODE_P instead. I tried out the extended patch (attached) and it gave even better GDB results for the MSP430 and still no regressions (GCC or GDB) for MSP430 or x86_64. Is this enough justification ? Cheers Nick
On 05/26/2016 10:16 AM, Nick Clifton wrote: > Hi Jeff, > >>>> I may be missing something, but isn't it the transition to an FP >>>> relative address rather than a SP relative address that's the problem >>>> here? >>> >>> Yes, I believe so. >>> >>>> Where does that happen? > > I think that it happens in dwarf2out.c:based_loc_descr() which > detects the use of the frame pointer and works out that it is going > to be eliminated to the stack pointer: > > /* We only use "frame base" when we're sure we're talking about the > post-prologue local stack frame. We do this by *not* running > register elimination until this point, and recognizing the special > argument pointer and soft frame pointer rtx's. */ > if (reg == arg_pointer_rtx || reg == frame_pointer_rtx) > { > rtx elim = (ira_use_lra_p > ? lra_eliminate_regs (reg, VOIDmode, NULL_RTX) > : eliminate_regs (reg, VOIDmode, NULL_RTX)); > > if (elim != reg) > ..... > > The problem, I believe, is that based_loc_descr() is only called > from mem_loc_descriptor when the mode of the rtl concerned is an > MODE_INT. For example: > > case REG: > if (GET_MODE_CLASS (mode) != MODE_INT > [...] > else > if (REGNO (rtl) < FIRST_PSEUDO_REGISTER) > mem_loc_result = based_loc_descr (rtl, 0, VAR_INIT_STATUS_INITIALIZED); > > or, (this is another one that I found whilst investigating this > problem further): > > case PLUS: > plus: > if (is_based_loc (rtl) > && (GET_MODE_SIZE (mode) <= DWARF2_ADDR_SIZE > || XEXP (rtl, 0) == arg_pointer_rtx > || XEXP (rtl, 0) == frame_pointer_rtx) > && GET_MODE_CLASS (mode) == MODE_INT) > mem_loc_result = based_loc_descr (XEXP (rtl, 0), > INTVAL (XEXP (rtl, 1)), > VAR_INIT_STATUS_INITIALIZED); > else > > > There are quite a few places in mem_loc_descriptor where the code checks > for the mode being in the MODE_INT class. I am not exactly sure why. I > think that it might be that the programmer thought that any expression that > does not involve integer based arithmetic cannot be expressed in DWARF CFA > notation and so would have to be handled specially. If I am correct, > then it seems to me that the proper fix would be to use SCALAR_INT_MODE_P > instead. > > I tried out the extended patch (attached) and it gave even better GDB > results for the MSP430 and still no regressions (GCC or GDB) for MSP430 or > x86_64. > > Is this enough justification ? So the argument is that MODE_INT was likely intended to filter out things like FP modes and such that might somehow be bogusly used as addresses. As written those tests are also filtering out partial integer modes which we can support? And that many (most?) of the things that filter on MODE_INT should really be using MODE_INT || MODE_PARTIAL_INT (aka SCALAR_INT_MODE_P). I can buy that ;-) OK with a suitable ChangeLog entry. jeff
Hi Jeff,
> I can buy that ;-) OK with a suitable ChangeLog entry.
Thanks! Checked in with this changelog entry.
Cheers
Nick
gcc/ChangeLog
2016-06-22 Nick Clifton <nickc@redhat.com>
* dwarf2out.c (scompare_loc_descriptor): Use SCALAR_INT_MODE_P() in
place of GET_MODE_CLASS() == MODE_INT, so that partial integer
modes are accepted as well.
(ucompare_loc_descriptor): Likewise.
(minmax_loc_descriptor): Likewise.
(clz_loc_descriptor): Likewise.
(popcount_loc_descriptor): Likewise.
(bswap_loc_descriptor): Likewise.
(rotate_loc_descriptor): Likewise.
(mem_loc_descriptor): Likewise.
(loc_descriptor): Likewise.
Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 236283) +++ gcc/dwarf2out.c (working copy) @@ -13396,7 +13396,11 @@ break; case REG: - if (GET_MODE_CLASS (mode) != MODE_INT + if ((GET_MODE_CLASS (mode) != MODE_INT + /* Targets which have pointers that use a partial integer mode + (eg the msp430x) still want their debug information to be + based on the normal DWARF base register notation. */ + && mode != Pmode) || (GET_MODE_SIZE (mode) > DWARF2_ADDR_SIZE && rtl != arg_pointer_rtx && rtl != frame_pointer_rtx
Hi Guys, Currently dwarf2out.c:mem_loc_descriptor() has some special case code to handle the situation where an address is held in a register whose mode is not of type MODE_INT. It generates a DW_OP_GNU_regval_type expression which may later on be converted into a frame pointer based expression. This is a problem for targets which use a partial integer mode for their pointers (eg the msp430). In such cases the conversion to a frame pointer based expression could be wrong if the frame pointer is not being used. For example the GDB testfile gdb/testsuite/gdb.base/advance.c contains this code fragment: int main () { int result; int b, c; c = 5; b = 3; /* advance this location */ func (c); /* stop here after leaving current frame */ which compiles to these instructions: suba #6, r1 mov #5, 4(r1) mov #3, 2(r1) mov 4(r1), r12 calla #0 ;<func> (Note that only r1 - the stack pointer - is used. r4 - the frame pointer - is not). The debug information produced for the "c" local variable looks like this: Abbrev Number: 3 (DW_TAG_variable) DW_AT_name : c DW_AT_decl_file : 1 DW_AT_decl_line : 40 DW_AT_type : <0x37> DW_AT_location : 5 byte block: f5 4 21 32 1c (DW_OP_GNU_regval_type: 4 (r4) <0x21>; DW_OP_lit2; DW_OP_minus) ie it says that "c" is stored in memory location "r4 - 2", which is wrong since register r4 is not even used in this function. The patch below addresses this problem by allowing the normal, register based descriptor to be produced when the mode is Pmode. With this patch applied the unexpected failure count in the GDB testsuite for the MSP430's -mlarge multilib changes from 2253 to 367. There are no regressions, for MSP430 or x86_64, and no changes to the GCC testsuite results for either target. OK to apply ? Cheers Nick gcc/ChangeLog 2016-05-16 Nick Clifton <nickc@redhat.com> * dwarf2out.c (mem_loc_descriptor): Convert REG based addresses whose mode is Pmode into basereg descriptors even if Pmode is not an integer mode.