the following function does not setup a stackframe. It is only visible in the generated assembler code. good: 0: 55 push %ebp 1: 89 e5 mov %esp,%ebp 3: 83 ec 10 sub $0x10,%esp 6: 8b 45 08 mov 0x8(%ebp),%eax 9: 8d 55 fc lea 0xfffffffc(%ebp),%edx ... currently: 0: 55 push %ebp 1: 89 e5 mov %esp,%ebp 3: 8b 45 08 mov 0x8(%ebp),%eax 6: 8d 55 fc lea 0xfffffffc(%ebp),%edx 9: 89 45 fc mov %eax,0xfffffffc(%ebp) ... Note the lack of the sub $0x10,$esp This seems to have been caused by the dataflow merge, my regression hunt against TRUNK is currently between 125600 and 125612.
Created attachment 13769 [details] preloader.i gcc -O2 -c preloader.i (on i*86), then disassemble.
The target might have forgot a barrior. the RTL is correct after pro_epilogue.
dse2 removes the decrement: (insn/f 24 23 25 2 t2.c:2 (parallel [ (set (reg/f:SI 7 sp) (plus:SI (reg/f:SI 7 sp) (const_int -16 [0xfffffffffffffff0]))) (clobber (reg:CC 17 flags)) (clobber (mem:BLK (scratch) [0 A8])) ]) -1 (nil)) Even though it is frame related.
I think Kenny's last patch for PR32437 fixes this as well.
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame spark at gcc dot gnu dot org wrote: > ------- Comment #4 from spark at gcc dot gnu dot org 2007-06-24 01:05 ------- > I think Kenny's last patch for PR32437 fixes this as well. > > > I have not had a chance to play with this. i lost my last x86-32 system and i have been futzing around with richi trying to build one on my x86-64 machines. So far without success. it would be good if it went away. if it has not, i was going to try -fno-dse. Kenny
(In reply to comment #5) > Subject: Re: [4.3 Regression] function with asm() > does not setup stack frame > > spark at gcc dot gnu dot org wrote: > > ------- Comment #4 from spark at gcc dot gnu dot org 2007-06-24 01:05 ------- > > I think Kenny's last patch for PR32437 fixes this as well. > > > > > > > I have not had a chance to play with this. i lost my last x86-32 system > and i have been futzing around with richi trying to build one on my > x86-64 machines. So far without success. > > it would be good if it went away. if it has not, i was going to try > -fno-dse. > > Kenny I'm positive your patch fixes this, as the latest trunk doesn't have this problem, and I've checked out the revision just before your last patch, and it showed the problem (on i686-unknown-linux-gnu).
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame spark at gcc dot gnu dot org wrote: > ------- Comment #6 from spark at gcc dot gnu dot org 2007-06-24 02:01 ------- > (In reply to comment #5) > >> Subject: Re: [4.3 Regression] function with asm() >> does not setup stack frame >> >> spark at gcc dot gnu dot org wrote: >> >>> ------- Comment #4 from spark at gcc dot gnu dot org 2007-06-24 01:05 ------- >>> I think Kenny's last patch for PR32437 fixes this as well. >>> >>> >>> >>> >> I have not had a chance to play with this. i lost my last x86-32 system >> and i have been futzing around with richi trying to build one on my >> x86-64 machines. So far without success. >> >> it would be good if it went away. if it has not, i was going to try >> -fno-dse. >> >> Kenny >> > > I'm positive your patch fixes this, > as the latest trunk doesn't have this problem, > and I've checked out the revision just before your last patch, > and it showed the problem (on i686-unknown-linux-gnu). > > > great, then close it and be done with it. kenny
is fixed in current SVN.
This bug has resurfaced in the last two days. r126139 is affected.
Richard, Could you check to see if this bug is collateral damage from your latest fix to deletable_insn_p. It's appearance has been tied to that function in the past. kenny
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame "zadeck at naturalbridge dot com" <gcc-bugzilla@gcc.gnu.org> writes: > Could you check to see if this bug is collateral damage from your > latest fix to deletable_insn_p. It's appearance has been tied to that > function in the past. _My_ latest fix to deletable_insn_p? ;) All I did was rephrase your patch in terms of deletable_insn_p_1. I won't have time to work on this in the near future, so I'll just revert the patch for now. Richard
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame "richard at codesourcery dot com" <gcc-bugzilla@gcc.gnu.org> writes: > "zadeck at naturalbridge dot com" <gcc-bugzilla@gcc.gnu.org> writes: >> Could you check to see if this bug is collateral damage from your >> latest fix to deletable_insn_p. It's appearance has been tied to that >> function in the past. > > _My_ latest fix to deletable_insn_p? ;) All I did was rephrase > your patch in terms of deletable_insn_p_1. Sorry, I shouldn't have said that. Kenny's original patch was basically an implementation of something I'd suggested on IRC, so the ultimate blame does lie with me. I think the upshot is still the same: I don't really have time to work on this, and given that what I suggested was either wrong, or exposed some other latent problem, I think reverting the patch is the correct thing to do under the circumstances. I suggest that someone else looks at the problem that the patch was trying to solve (which AIUI was a pessimisation rather than a wrong-code bug). Richard
The problem here is that although the stack pointer is not used in the function, adjusting it does reserve space on the stack for the local variables which are used. The local variables are accessed via the frame pointer with a negative offset. Deleting the adjustment of the stack pointer is causing that the reference to be implicitly invalid. This test case makes the problem obvious via an asm which essentially does a function call which the compiler doesn't know about. If the asm weren't there, though, there would still be a difficult to diagnose problem in that an interrupt at the wrong time would corrupt the value of the local variables. One possible approach to this is that DF should note that any reference to a local variable via the frame pointer is implicitly a use of the stack pointer. Note that this doesn't necessarily mean a negative offset from the frame pointer, although it does in this case. On processors like the Thumb even local variables are accessed as positive offsets from the frame pointer, and parameters are accessed by larger positive offsets. We also need to consider whether any asm statement is a use of the stack pointer. It may be that DF should treat an asm statement however it treats a function call with regard to uses of the stack pointer. A non-volatile asm would be a const call, and a volatile asm would be an ordinary call. I'm not completely sure about that, but it seems worth considering. A simpler approach would be for DCE to simply not remove adjustments to the stack pointer. I believe this would have to be done whether the adjustment was frame related or not. I think the fact that the instruction is frame related is probably a red herring here.
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame ian at airs dot com wrote: > ------- Comment #13 from ian at airs dot com 2007-06-30 18:08 ------- > The problem here is that although the stack pointer is not used in the > function, adjusting it does reserve space on the stack for the local variables > which are used. The local variables are accessed via the frame pointer with a > negative offset. Deleting the adjustment of the stack pointer is causing that > the reference to be implicitly invalid. This test case makes the problem > obvious via an asm which essentially does a function call which the compiler > doesn't know about. If the asm weren't there, though, there would still be a > difficult to diagnose problem in that an interrupt at the wrong time would > corrupt the value of the local variables. > > One possible approach to this is that DF should note that any reference to a > local variable via the frame pointer is implicitly a use of the stack pointer. > Note that this doesn't necessarily mean a negative offset from the frame > pointer, although it does in this case. On processors like the Thumb even > local variables are accessed as positive offsets from the frame pointer, and > parameters are accessed by larger positive offsets. We also need to consider > whether any asm statement is a use of the stack pointer. It may be that DF > should treat an asm statement however it treats a function call with regard to > uses of the stack pointer. A non-volatile asm would be a const call, and a > volatile asm would be an ordinary call. I'm not completely sure about that, > but it seems worth considering. > > A simpler approach would be for DCE to simply not remove adjustments to the > stack pointer. I believe this would have to be done whether the adjustment was > frame related or not. I think the fact that the instruction is frame related > is probably a red herring here. > > > it is interesting to read the comments as a historical record to what we do. Looking at it from the point of view that we should treat asms more like function calls, there is a disturbing comment in the code that scans the asm inputs: case ASM_INPUT: { /* Traditional and volatile asm instructions must be considered to use and clobber all hard registers, all pseudo-registers and all of memory. So must TRAP_IF and UNSPEC_VOLATILE operations. Consider for instance a volatile asm that changes the fpu rounding mode. An insn should not be moved across this even if it only uses pseudo-regs because it might give an incorrectly rounded result. However, flow.c's liveness computation did *not* do this, giving the reasoning as " ?!? Unfortunately, marking all hard registers as live causes massive problems for the register allocator and marking all pseudos as live creates mountains of uninitialized variable warnings." In order to maintain the status quo with regard to liveness and uses, we do what flow.c did and just mark any regs we can find in ASM_OPERANDS as used. In global asm insns are scanned and regs_asm_clobbered is filled out. For all ASM_OPERANDS, we must traverse the vector of input operands. We can not just fall through here since then we would be confused by the ASM_INPUT rtx inside ASM_OPERANDS, which do not indicate traditional asms unlike their normal usage. */ if (code == ASM_OPERANDS) { int j; for (j = 0; j < ASM_OPERANDS_INPUT_LENGTH (x); j++) df_uses_record (collection_rec, &ASM_OPERANDS_INPUT (x, j), DF_REF_REG_USE, bb, insn, flags); return; } break; } Asside from the fallout in the register allocator or uninitialized warnings, there really is not problem adding the stack_pointer. Flow could get away with it because it could assume that the rest of the optimizations were stupid or had been neutered. I do not think that is the way to go. We add the stack pointer not matter if the function call is a const call or not. Note that const calls do generally read the stack. Adding the stack pointer for asms is certainly the easiest thing to do. If you think that we want to view mem refs off the frame pointer as if they also touch the stack pointer, we can do that, but i would ask the question as to "why just in dce?"
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame > Adding the stack pointer for asms is certainly the easiest thing to do. I don't know if that is enough. Maybe it is, maybe it isn't. You can't delete the subtraction of the stack pointer if there is any use of any local variable on the frame in any way. If an interrupt occurs, and the stack pointer has not been decremented to be below the local variables, then the local variables will be corrupted by the interrupt handler. > If you think that we want to view mem refs off the frame pointer as if > they also touch the stack pointer, we can do that, but i would ask the > question as to "why just in dce?" I meant it should be done when doing dataflow scanning, not in DCE. I agree there is nothing special about DCE here.
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame Ian Lance Taylor wrote: >> Adding the stack pointer for asms is certainly the easiest thing to do. >> > > I don't know if that is enough. Maybe it is, maybe it isn't. You > can't delete the subtraction of the stack pointer if there is any use > of any local variable on the frame in any way. If an interrupt > occurs, and the stack pointer has not been decremented to be below the > local variables, then the local variables will be corrupted by the > interrupt handler. > > >> If you think that we want to view mem refs off the frame pointer as if >> they also touch the stack pointer, we can do that, but i would ask the >> question as to "why just in dce?" >> > > I meant it should be done when doing dataflow scanning, not in DCE. I > agree there is nothing special about DCE here. > ian, a set of specific questions: 1) Do i do this for both the frame_pointer and hard_frame_pointer? 2) There is currently code in the scanning for adding the stack pointer for call insns. This code marks that ref so that it is not considered when building log links for combine. a) Should I mark the stack pointer ref added for asm's this way? b) Should I mark the stack pointer refs put in for the mems in this way? Kenny
Before I tackle the specific questions, let me try to explain the issue that I see. The issue is that during a function the value of the stack pointer register must always be at or below the local variables. An interrupt can occur at any time. On a processor which does not use a separate interrupt stack, the interrupt will push values on the stack and pass control to the kernel. The stack pointer must be set such that when an interrupt pushes values on the stack the local variables are not corrupted. Does that make sense? My hope is that understanding how this has to work will make clear what must be done. > 1) Do i do this for both the frame_pointer and hard_frame_pointer? I think this kind of dependency will only be relevant after the prologue has set up the stack. Therefore, it is only necessary to consider the hard frame pointer. > 2) There is currently code in the scanning for adding the stack pointer > for call insns. Sure, makes sense. > This code marks that ref so that it is not considered when building log > links for combine. That makes sense too. It doesn't make sense to build log links for the stack pointer for a call instruction; there isn't going to be anything combine. > a) Should I mark the stack pointer ref added for asm's this way? It's not obvious to me that asm's should have an implicit stack pointer ref. But maybe they should, I don't know. If asm's should have an explicit stack pointer ref, then regarding log links you should do whatever is simplest. asm's can not be combined anyhow. > b) Should I mark the stack pointer refs put in for the mems in this way? Yes, I would think so.
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame ian at airs dot com wrote: > ------- Comment #17 from ian at airs dot com 2007-07-02 01:45 ------- > Before I tackle the specific questions, let me try to explain the issue that I > see. The issue is that during a function the value of the stack pointer > register must always be at or below the local variables. An interrupt can > occur at any time. On a processor which does not use a separate interrupt > stack, the interrupt will push values on the stack and pass control to the > kernel. The stack pointer must be set such that when an interrupt pushes > values on the stack the local variables are not corrupted. Does that make > sense? My hope is that understanding how this has to work will make clear what > must be done. > > >> 1) Do i do this for both the frame_pointer and hard_frame_pointer? >> > > I think this kind of dependency will only be relevant after the prologue has > set up the stack. Therefore, it is only necessary to consider the hard frame > pointer. > > >> 2) There is currently code in the scanning for adding the stack pointer >> for call insns. >> > > Sure, makes sense. > > >> This code marks that ref so that it is not considered when building log >> links for combine. >> > > That makes sense too. It doesn't make sense to build log links for the stack > pointer for a call instruction; there isn't going to be anything combine. > > >> a) Should I mark the stack pointer ref added for asm's this way? >> > > It's not obvious to me that asm's should have an implicit stack pointer ref. > But maybe they should, I don't know. > > If asm's should have an explicit stack pointer ref, then regarding log links > you should do whatever is simplest. asm's can not be combined anyhow. > > >> b) Should I mark the stack pointer refs put in for the mems in this way? >> > > Yes, I would think so. > > > there is an inconsistency in your answers. Because if i am only marking the hard frame pointer and this is not set up until after reload, then the questions about the log_links and combine are moot since combine runs long before reload.
I don't see an inconsistency in my answers. If you mark local variables as having a reference to the stack pointer before combine, then you should handle log_links as appropriate. I am explicitly not telling you what to do. I am telling you what the problem is, and leaving the solution up to you. The correct solution is in the dataflow code, which I don't really know.
We already put stack pointer in the exit block use set always. However, after epilogue is generated, we see the sp restoration code: 1 NOTE_INSN_DELETED 4 NOTE_INSN_BASIC_BLOCK 22 [--sp:SI]=bp:SI 23 bp:SI=sp:SI 24 {sp:SI=sp:SI-0x10;clobber flags:CC;clobber [scratch];} 25 NOTE_INSN_PROLOGUE_END 3 NOTE_INSN_FUNCTION_BEG 19 ax:SI=[bp:SI+0x8] REG_EQUIV: [bp:SI+0x8] 6 [bp:SI-0x4]=ax:SI 21 dx:SI=bp:SI-0x4 18 ax:SI=0x5a REG_EQUIV: 0x5a 9 {ax:SI=asm_operands;clobber fpsr:QI;clobber flags:QI;clobber [scratch];} 26 NOTE_INSN_EPILOGUE_BEG 27 {sp:SI=bp:SI+0x4;bp:SI=[bp:SI];clobber [scratch];} 28 return i 29: barrier 20 NOTE_INSN_DELETED Insn 27, which restores the sp for the caller, naturally writes to %sp, and this makes %sp dead at insn 24. I think the only correct solution is somehow to mark sp as used by insn 27, or some similar effect. So, although this is a scanning problem, we may want to consider adding a use of %sp inside 27 or just before insn 27, *if* sp is indeed necessary. It is not possible for df to figure this out alone, as its epilogue generation code that determines whether we want sp or not.
Subject: Re: [4.3 Regression] function with asm() does not setup stack frame ian at airs dot com wrote: > ------- Comment #13 from ian at airs dot com 2007-06-30 18:08 ------- > The problem here is that although the stack pointer is not used in the > function, adjusting it does reserve space on the stack for the local variables > which are used. The local variables are accessed via the frame pointer with a > negative offset. Deleting the adjustment of the stack pointer is causing that > the reference to be implicitly invalid. This test case makes the problem > obvious via an asm which essentially does a function call which the compiler > doesn't know about. If the asm weren't there, though, there would still be a > difficult to diagnose problem in that an interrupt at the wrong time would > corrupt the value of the local variables. > > One possible approach to this is that DF should note that any reference to a > local variable via the frame pointer is implicitly a use of the stack pointer. > Note that this doesn't necessarily mean a negative offset from the frame > pointer, although it does in this case. On processors like the Thumb even > local variables are accessed as positive offsets from the frame pointer, and > parameters are accessed by larger positive offsets. We also need to consider > whether any asm statement is a use of the stack pointer. It may be that DF > should treat an asm statement however it treats a function call with regard to > uses of the stack pointer. A non-volatile asm would be a const call, and a > volatile asm would be an ordinary call. I'm not completely sure about that, > but it seems worth considering. > > A simpler approach would be for DCE to simply not remove adjustments to the > stack pointer. I believe this would have to be done whether the adjustment was > frame related or not. I think the fact that the instruction is frame related > is probably a red herring here. > > > 2007-07-02 Kenneth Zadeck <zadeck@naturalbridge.com> PR middle-end/32475 * df-scan.c (df_ref_record): Add ref for stack_ptr whenever mem is referenced thru hard_frame_pointer. (df_uses_record): Add ref for stack_ptr for asm insns since they may implicitly use the stack. This patch does what iant suggested. It fixes this pr and bootstraps and regression tests on x86-64. Ok for trunk? Kenny Index: df-scan.c =================================================================== --- df-scan.c (revision 126167) +++ df-scan.c (working copy) @@ -2649,7 +2649,19 @@ df_ref_record (struct df_collection_rec endregno = regno + subreg_nregs (reg); } else - endregno = END_HARD_REGNO (reg); + { + endregno = END_HARD_REGNO (reg); + + /* If we have a mem reference that is relative to the + hard_frame_pointer, add a reference to the stack_pointer. + This keeps the stack pointer alive when there may not be + any other obvious reason. */ + if ((ref_type == DF_REF_REG_MEM_STORE + || ref_type == DF_REF_REG_MEM_LOAD) + && regno == HARD_FRAME_POINTER_REGNUM) + df_ref_record (collection_rec, regno_reg_rtx[STACK_POINTER_REGNUM], + NULL, bb, insn, ref_type, ref_flags); + } /* If this is a multiword hardreg, we create some extra datastructures that will enable us to easily build REG_DEAD @@ -2955,6 +2967,11 @@ df_uses_record (struct df_collection_rec for (j = 0; j < ASM_OPERANDS_INPUT_LENGTH (x); j++) df_uses_record (collection_rec, &ASM_OPERANDS_INPUT (x, j), DF_REF_REG_USE, bb, insn, flags); + + /* The stack ptr may be used (honorarily) by an ASM, + especially if the asm makes a function call. */ + df_ref_record (collection_rec, regno_reg_rtx[STACK_POINTER_REGNUM], + NULL, bb, insn, DF_REF_REG_USE, flags); return; } break;
(In reply to comment #21) I don't think this patch is quite enough. IIUC, it is still possible (although very unlikely) for stack space to be required without any MEM_LOAD or MEM_STORE present, and without any direct reference to sp. Even if such a case is not possible, we still want to avoid sp looking as if dead for any point at function that it shouldn't be - as I pointed out earlier, after epilogue generation, because of the sp restoration, sp looks as if it's dead. This can cause a problem, e.g. if we implement any hard register renaming pass after register alloc/reload - because from df's point of view, %sp will look available between the last MEM_LOAD/MEM_STORE w/ hard fp reference, and the sp restoration code in the epilogue.
This patch: diff -r 29a7055e69fc gcc/df-scan.c --- a/gcc/df-scan.c Sun Jun 24 00:16:55 2007 +0000 +++ b/gcc/df-scan.c Tue Jul 03 12:38:41 2007 -0700 @@ -2763,6 +2763,12 @@ df_def_record_1 (struct df_collection_re || (GET_CODE (dst) == SUBREG && REG_P (SUBREG_REG (dst)))) df_ref_record (collection_rec, dst, loc, bb, insn, DF_REF_REG_DEF, flags); + + /* We want to keep sp alive everywhere - by making all + writes to sp also use of sp. */ + if (REG_P (dst) && REGNO (dst) == STACK_POINTER_REGNUM) + df_ref_record (collection_rec, + dst, NULL, bb, insn, DF_REF_REG_USE, flags); } also fixes the problem, by treating all defs of SP as a use of SP. This patch essentially makes SP alive everywhere. Also, this would create less number of unnecessary df_ref's (since presumably there are less number of stack defs than MEM_LOAD/STORE w/ hard frame pointer reference).
Subject: Bug 32475 Author: spark Date: Thu Jul 5 23:44:44 2007 New Revision: 126391 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=126391 Log: 2007-07-05 Seongbae Park <seongbae.park@gmail.com> PR rtl-optimization/32475 * df-scan.c (df_def_record_1): Add a use of the stack pointer for every definition of the stack pointer. Modified: trunk/gcc/ChangeLog trunk/gcc/df-scan.c
Fixed.