The attached test case is supposed to print '31337' when run. Correct output: [slava@imac factor]$ gcc -O1 testcase.c [slava@imac factor]$ ./a.out 31337 Correct output: [slava@imac factor]$ gcc -O2 -fno-forward-propagate testcase.c [slava@imac factor]$ ./a.out 31337 *Incorrect* output: [slava@imac factor]$ gcc -O2 testcase.c [slava@imac factor]$ ./a.out 0 We can plainly see the problem in the disassemble for the broken() function. With -O2 -fno-forward-propagate, broken: leaq 8(%r14), %rax movq %rax, %r14 movq $31337, (%rax) ret With -O2: broken: addq $8, %r14 movq $31337, 8(%r14) ret =========================================== #include <stdbool.h> #include <stdio.h> register unsigned long *ds asm("r14"); __attribute__((noinline)) void broken(bool value) { *++ds = 31337; } int main() { unsigned long stack[2]; stack[0] = 0; stack[1] = 0; ds = stack; broken(true); printf("%ld\n",*ds); } ===========================================
global register should really not be used anyways :). They are an example of a bad extension.
If it's a bad language extension, why is it implemented?
So is this extension bad/deprecated or not? I'd appreciate a clarification from the gcc team. If the resolution is that this feature should simply not be used, I would strongly suggest removing it. Otherwise I'm not sure what to think; do I invest the effort in refactoring my code to not rely on this extension, or can I count on it being supported going forward?
(In reply to comment #2) > If it's a bad language extension, why is it implemented? Because someone thought it was nice but it really hard to get correct as it changes the ABI.
Confirmed.
More self-contained testcase: register unsigned long *r14 asm ("r14"); extern void abort (void); __attribute__ ((noinline)) void test (void) { *++r14 = 31337; } int main () { unsigned long stack[2]; stack[0] = 0; stack[1] = 0; r14 = stack; test (); if (r14 != stack + 1 || *r14 != 31337) abort (); return 0; } We have: (insn 5 2 6 2 pr36753.c:7 (parallel [ (set (reg/f:DI 58 [ r14.1 ]) (plus:DI (reg/v:DI 43 r14 [ r14 ]) (const_int 8 [0x8]))) (clobber (reg:CC 17 flags)) ]) 279 {*adddi_1_rex64} (expr_list:REG_DEAD (reg/v:DI 43 r14 [ r14 ]) (expr_list:REG_UNUSED (reg:CC 17 flags) (nil)))) (insn 6 5 7 2 pr36753.c:7 (set (reg/v:DI 43 r14 [ r14 ]) (reg/f:DI 58 [ r14.1 ])) 89 {*movdi_1_rex64} (nil)) (insn 7 6 0 2 pr36753.c:7 (set (mem:DI (reg/f:DI 58 [ r14.1 ]) [2 S8 A64]) (const_int 31337 [0x7a69])) 89 {*movdi_1_rex64} (expr_list:REG_DEAD (reg/f:DI 58 [ r14.1 ]) (nil))) before fwprop2, and: In insn 7, replacing (mem:DI (reg/f:DI 58 [ r14.1 ]) [2 S8 A64]) with (mem:DI (plus:DI (reg/v:DI 43 r14 [ r14 ]) (const_int 8 [0x8])) [2 S8 A64]) Changed insn 7 Wonder why nothing in fwprop.c checks modified_between_p, that would clearly say that reg:DI 43 r14 has been modified between the setter and use and thus the propagation is invalid. To Andrew: many ABIs have one or more fixed registers reserved, so you can use those as global register vars, or you can e.g. build with -ffixed-* to make sure even when you don't have the global register var declaration in some CU, it isn't randomly clobbered. Many projects rely heavily on global register vars, including the Linux kernel or e.g. glibc (in the latter it is used in libpthread for all thread specific stuff). From what I see, this fwprop bug isn't specific to global register vars, any REG, be it hard or pseudo, can be modified between the setter and user...
> Wonder why nothing in fwprop.c checks modified_between_p There is code similar to modified_between_p in fwprop.c, that uses def info from the dataflow pass. Part of the fwprop work was to try using the df-scan.c info more than the recursive walks provided by rtlanal.c. That said, the bug should be trivial to fix. I'll try (but not today unfortunately).
mine
In fact, using modified_between_p would not fix the bug. The problem is that use_killed_between gives the wrong answer *before* it reaches the code that "redoes" modified_between_p using df data structures. I'll regtest this patch tomorrow because the machine is right now too busy. But if anyone wants to beat me, feel free. Index: ../../peak-gcc-src/gcc/fwprop.c =================================================================== --- ../../peak-gcc-src/gcc/fwprop.c (revision 136756) +++ ../../peak-gcc-src/gcc/fwprop.c (working copy) @@ -480,10 +480,15 @@ use_killed_between (struct df_ref *use, return true; /* Check if the reg in USE has only one definition. We already - know that this definition reaches use, or we wouldn't be here. */ + know that this definition reaches use, or we wouldn't be here. + However, this is not true for hard registers, because if they are + live at the beginning of the function it does not mean that we + have an uninitialized access. */ regno = DF_REF_REGNO (use); def = DF_REG_DEF_CHAIN (regno); - if (def && (def->next_reg == NULL)) + if (def + && (def->next_reg == NULL) + && regno >= FIRST_PSEUDO_REGISTER) return false; /* Check locally if we are in the same basic block. */
Subject: Bug 36753 Author: bonzini Date: Thu Jul 17 09:07:31 2008 New Revision: 137913 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=137913 Log: gcc: 2008-07-17 Paolo Bonzini <bonzini@gnu.org> PR rtl-optimization/36753 * fwprop.c (use_killed_between): Don't shortcut single-definition global registers. gcc/testsuite: 2008-07-17 Paolo Bonzini <bonzini@gnu.org> PR rtl-optimization/36753 * gcc.target/i386/pr36753.c: New. Added: trunk/gcc/testsuite/gcc.target/i386/pr36753.c Modified: trunk/gcc/ChangeLog trunk/gcc/fwprop.c trunk/gcc/testsuite/ChangeLog
committed to trunk and 4.3.
Subject: Bug 36753 Author: bonzini Date: Thu Jul 17 09:19:31 2008 New Revision: 137915 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=137915 Log: gcc: 2008-07-17 Paolo Bonzini <bonzini@gnu.org> PR rtl-optimization/36753 * fwprop.c (use_killed_between): Don't shortcut single-definition global registers. gcc/testsuite: 2008-07-17 Paolo Bonzini <bonzini@gnu.org> PR rtl-optimization/36753 * gcc.target/i386/pr36753.c: New. Added: branches/gcc-4_3-branch/gcc/testsuite/gcc.target/i386/pr36753.c - copied unchanged from r137913, trunk/gcc/testsuite/gcc.target/i386/pr36753.c Modified: branches/gcc-4_3-branch/gcc/ChangeLog branches/gcc-4_3-branch/gcc/fwprop.c branches/gcc-4_3-branch/gcc/testsuite/ChangeLog