User account creation filtered due to spam.

Bug 36753 - [4.3/4.4 Regression] Forward propagation interacts badly with global register variable
Summary: [4.3/4.4 Regression] Forward propagation interacts badly with global register...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.3.1
: P1 normal
Target Milestone: 4.3.2
Assignee: Paolo Bonzini
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2008-07-07 19:22 UTC by Slava Pestov
Modified: 2008-07-17 09:19 UTC (History)
3 users (show)

See Also:
Host: x86_64-unknown-linux-gnu
Target: x86_64-unknown-linux-gnu
Build: x86_64-unknown-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2008-07-14 16:18:24


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Slava Pestov 2008-07-07 19:22:55 UTC
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);
}
===========================================
Comment 1 Andrew Pinski 2008-07-07 19:26:37 UTC
global register should really not be used anyways :).  They are an example of a bad extension.
Comment 2 William Schlieper 2008-07-07 22:38:30 UTC
If it's a bad language extension, why is it implemented?
Comment 3 Slava Pestov 2008-07-07 22:41:36 UTC
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?
Comment 4 Andrew Pinski 2008-07-07 22:42:42 UTC
(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.
Comment 5 Richard Biener 2008-07-08 13:23:39 UTC
Confirmed.
Comment 6 Jakub Jelinek 2008-07-10 13:34:26 UTC
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...
Comment 7 Paolo Bonzini 2008-07-11 11:58:35 UTC
> 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).
Comment 8 Paolo Bonzini 2008-07-14 16:18:24 UTC
mine
Comment 9 Paolo Bonzini 2008-07-14 16:29:26 UTC
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.  */
Comment 10 Paolo Bonzini 2008-07-17 09:08:19 UTC
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

Comment 11 Paolo Bonzini 2008-07-17 09:19:38 UTC
committed to trunk and 4.3.
Comment 12 Paolo Bonzini 2008-07-17 09:20:16 UTC
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