| Message ID | 20111018174145.4d7cf3bc@rex.config |
|---|---|
| State | New |
| Headers | show |
Julian Brown writes: > Hi, > > This patch fixes computed gotos on m68k, and probably other targets too > (the fix is in the middle end). Several tests fail at present with "-O3 > -fomit-frame-pointer". This is the same bug as PR47918. As described in that PR trail, the bug can be triggered also on i386. Thanks for debugging this very nasty issue! > [...] /Mikael
> This has been broken because since mainline revision r160124, which > (correctly) causes 'y' to be flagged as noreturn -- y never returns via > the normal function return route. If ipa-pure-const.c is hacked to > check that a function does not perform non-local gotos before marking > it noreturn, the original (correct) offset is restored -- but that is > simply papering over the problem. Agreed. > So, on to the attached fix. As mentioned previously, GCC tracks > elimination offsets throughout each function, which can vary as e.g. > other functions are called. There is a concept of an "initial" offset: > I believe that when nonlocal gotos are executed, and hence at all the > labels a nonlocal goto may reach, the elimination offsets must have > their "initial" values. Agreed. > We can find out if a label is the target of a nonlocal goto (and hence > force the use of initial offsets) in a somewhat roundabout way, by > looking up its containing basic block, seeing if that BB is a nonlocal > goto target, then seeing if the label is the first instruction in the > block. This seems slightly clumsy, but I didn't find another way of > doing it. These labels are on the nonlocal_goto_handler_labels chain. You presumably just need to apply the same treatment to them in set_initial_label_offsets as the one applied to forced labels. > Any comments, or OK to apply? OK for the adjusted patch if it works, mainline and 4.6 branch once it reopens. Please mention PR rtl-optimization/47918 in the ChangeLog. Thanks for fixing this.
Index: gcc/reload1.c =================================================================== --- gcc/reload1.c (revision 179967) +++ gcc/reload1.c (working copy) @@ -2371,6 +2371,19 @@ set_label_offsets (rtx x, rtx insn, int if (! offsets_known_at[CODE_LABEL_NUMBER (x) - first_label_num]) { + if (x == insn) + { + basic_block bb; + + bb = BLOCK_FOR_INSN (insn); + + /* If the label is the target of a non-local GOTO, we must use + the initial elimination offsets. */ + if (bb && BB_HEAD (bb) == insn + && (bb->flags & BB_NON_LOCAL_GOTO_TARGET)) + initial_p = true; + } + for (i = 0; i < NUM_ELIMINABLE_REGS; i++) offsets_at[CODE_LABEL_NUMBER (x) - first_label_num][i] = (initial_p ? reg_eliminate[i].initial_offset
Hi, This patch fixes computed gotos on m68k, and probably other targets too (the fix is in the middle end). Several tests fail at present with "-O3 -fomit-frame-pointer". One example of erroneous behaviour is as follows: the function 'x' in comp-goto-1.c compiles to: x: lea (-104,%sp),%sp lea (104,%sp),%a0 move.l %a0,92(%sp) fmovem #63,44(%sp) movem.l #31996,(%sp) move.l %sp,96(%sp) move.l 108(%sp),-(%sp) lea (96,%sp),%a0 jsr y.1217 .L3: .L12: move.l 112(%sp),%d0 <--- wrong offset, should be 108. movem.l (%sp),#31996 fmovem 44(%sp),#63 lea (104,%sp),%sp rts Where the source code looks like: int x(a) { __label__ xlab; void y(a) { void *x = &&llab; if (a==-1) goto *x; if (a==0) goto xlab; llab: y (a-1); } y (a); xlab:; return a; } The offset is intended to load the argument value 'a' for the "return a;" statement, but actually loads one word beyond that. This has been broken because since mainline revision r160124, which (correctly) causes 'y' to be flagged as noreturn -- y never returns via the normal function return route. If ipa-pure-const.c is hacked to check that a function does not perform non-local gotos before marking it noreturn, the original (correct) offset is restored -- but that is simply papering over the problem. Prior to r160124, or with the aforementioned patch to inhibit the "noreturn" attribute for the function y, the call to y from x (just before "xlab:") has a goto to the following label emitted at expand time. Before/after that patch is applied, the diff of 148r.expand around the relevant part looks like: # BLOCK 2 freq:10000 # PRED: ENTRY [100.0%] (fallthru,exec) y (a_1(D)); [static-chain: &FRAME.0] - # SUCC: 3 [100.0%] (ab,exec) + goto <bb 4> (xlab); + # SUCC: 3 [61.0%] (ab,exec) 4 [39.0%] (fallthru,exec) - # BLOCK 3 freq:10000 - # PRED: 2 [100.0%] (ab,exec) + # BLOCK 3 freq:6100 + # PRED: 2 [61.0%] (ab,exec) <L0>: [non-local] # SUCC: 4 [100.0%] (fallthru,exec) # BLOCK 4 freq:10000 - # PRED: 3 [100.0%] (fallthru,exec) + # PRED: 2 [39.0%] (fallthru,exec) 3 [100.0%] (fallthru,exec) xlab: Now, this is important because when the "goto <bb 4>" is emitted, a stack adjustment is also emitted to pop y's arguments (via dojump.c:do_pending_stack_adjust). Without that (i.e. the present state with no patches), when later passes try to figure out elimination offsets (from arg pointer to frame pointer, frame pointer to stack pointer and so on) at the labels L0/xlab, they get the wrong answer. (This happens in reload, called from ira.) So, on to the attached fix. As mentioned previously, GCC tracks elimination offsets throughout each function, which can vary as e.g. other functions are called. There is a concept of an "initial" offset: I believe that when nonlocal gotos are executed, and hence at all the labels a nonlocal goto may reach, the elimination offsets must have their "initial" values. We can find out if a label is the target of a nonlocal goto (and hence force the use of initial offsets) in a somewhat roundabout way, by looking up its containing basic block, seeing if that BB is a nonlocal goto target, then seeing if the label is the first instruction in the block. This seems slightly clumsy, but I didn't find another way of doing it. Interestingly perhaps, the comment near the fix in reload1.c:set_label_offsets hints at a possible issue which may be related: case CODE_LABEL: /* If we know nothing about this label, set the desired offsets. Note that this sets the offset at a label to be the offset before a label if we don't know anything about the label. This is not correct for the label after a BARRIER, but is the best guess we can make. If we guessed wrong, we will suppress an elimination that might have been possible had we been able to guess correctly. */ But obviously, in our case, rather than just failing to make an elimination which may have been possible, we get wrong code instead (there is a barrier before our non-local-goto-receiving label). Anyway. The attached patch appears to work fine, and we get the following changes in test results (cross-building and testing to ColdFire Linux): FAIL -> PASS: gcc.c-torture/execute/920501-7.c execution, -O3 -fomit-frame-pointer FAIL -> PASS: gcc.c-torture/execute/comp-goto-2.c execution, -O3 -fomit-frame-pointer FAIL -> PASS: gcc.dg/torture/stackalign/comp-goto-1.c -O3 -fomit-frame-pointer execution test FAIL -> PASS: gcc.dg/torture/stackalign/comp-goto-1.c -O3 -fomit-frame-pointer execution test [#2] FAIL -> PASS: gcc.dg/torture/stackalign/non-local-goto-4.c -O3 -fomit-frame-pointer execution test FAIL -> PASS: gcc.dg/torture/stackalign/non-local-goto-4.c -O3 -fomit-frame-pointer execution test [#2] Any comments, or OK to apply? Thanks, Julian ChangeLog gcc/ * reload1.c (set_label_offsets): Force initial_p for labels receiving non-local gotos.