[Bug rtl-optimization/63375] New: reordering of reads across fences

[Bug rtl-optimization/63375] New: reordering of reads across fences

[bobby.prani at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

            Bug ID: 63375
           Summary: reordering of reads across fences
           Product: gcc
           Version: 4.9.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bobby.prani at gmail dot com

A read within the region of code enclosed by barriers is being moved out of the
regions. Test case follows:

#include<stdio.h>

typedef struct {
    int counter;
} atomic_t;

static inline int atomic_read(atomic_t *v)
{
    return (*(volatile int *)&(v)->counter);
}

#define smp_mb() asm volatile ("mfence":::"memory")

int main()
{
    atomic_t val = {1};
    int p;

    smp_mb();
    p = atomic_read(&val);
    smp_mb();

    printf("%d\n", p);

    return 0;
}

Generated assembly:

  15:    mfence
  18:    mfence
  1b:    mov    $0x1,%esi
  20:    mov    $0x0,%rdi
  27:    xor    %eax,%eax
  29:    callq  2e <init_module+0x2e>

[Bug rtl-optimization/63375] reordering of reads across fences

[bobby.prani at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

Pranith Kumar <bobby.prani at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bobby.prani at gmail dot com

--- Comment #1 from Pranith Kumar <bobby.prani at gmail dot com> ---
Created attachment 33576
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33576&action=edit
test case

$ gcc -S -O2 test.c

[Bug tree-optimization/63375] reordering of reads across fences

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-09-26
          Component|rtl-optimization            |tree-optimization
     Ever confirmed|0                           |1

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>A read within the region of code enclosed by barriers is being moved out of the regions. 

Actually no.  What is happening is a volatile read is being changed into a non
volatile one.  This is happening during early SRA.

Also this does not happen if val was a global variable rather than a stack
variable.

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
                 CC|                            |jamborm at gcc dot gnu.org
      Known to work|                            |4.3.4
   Target Milestone|---                         |4.8.4
            Summary|reordering of reads across  |[4.8/4.9/5 Regression]
                   |fences                      |reordering of reads across
                   |                            |fences
      Known to fail|                            |4.8.3, 4.9.1, 5.0

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed the SRA issue.  Martin, can you have a look please?

4.3 works (because we keep the variable addressable due to the lack of
MEM_REFs)

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I'v posted the SRA patch to the mailing list:

https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00062.html

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #6 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Author: jamborm
Date: Thu Oct  2 16:49:14 2014
New Revision: 215804

URL: https://gcc.gnu.org/viewcvs?rev=215804&root=gcc&view=rev
Log:
2014-10-02  Martin Jambor  <mjambor@suse.cz>

    PR tree-optimization/63375
    * tree-sra.c (build_access_from_expr_1): Disqualify volatile
    references.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-sra.c

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #7 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Author: jamborm
Date: Thu Oct  2 17:11:24 2014
New Revision: 215806

URL: https://gcc.gnu.org/viewcvs?rev=215806&root=gcc&view=rev
Log:
2014-10-02  Martin Jambor  <mjambor@suse.cz>

    PR tree-optimization/63375
    * tree-sra.c (build_access_from_expr_1): Disqualify volatile
    references.


Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/tree-sra.c

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #8 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Author: jamborm
Date: Thu Oct  2 17:13:30 2014
New Revision: 215807

URL: https://gcc.gnu.org/viewcvs?rev=215807&root=gcc&view=rev
Log:
2014-10-02  Martin Jambor  <mjambor@suse.cz>

    PR tree-optimization/63375
    * tree-sra.c (build_access_from_expr_1): Disqualify volatile
    references.


Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/tree-sra.c

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So fixed?

[Bug tree-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
No, RTL forwprop1 still breaks the testcase according to comment #4.

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[bobby.prani at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #11 from Pranith Kumar <bobby.prani at gmail dot com> ---
Is there any work around for this in the mean time? Thanks!

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
I believe that the testcase is "invalid" - the atomic is an automatic variable
which can never be involved in data races given the stack is not shared amongst
threads and you do not take the address of the variable and let it escape
to other threads.

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[bobby.prani at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #13 from Pranith Kumar <bobby.prani at gmail dot com> ---
The main concern here is moving the read past the fence instruction
irrespective of volatile semantics. The fence instruction guarantees that
accesses before the fence will complete before the accesses coming after the
fence. Consider the following case:

#include<stdio.h>

typedef struct {
    int counter;
} atomic_t;

static inline int atomic_read(atomic_t *v)
{
    return (*(volatile int *)&(v)->counter);
}

static inline void atomic_write(atomic_t *v, int val)
{
    v->counter = val;
}

#define smp_mb() asm volatile ("mfence":::"memory")

atomic_t val2 = {1};
int main()
{
    atomic_t val1 = {1};
    int p, q;

    smp_mb();
    p = atomic_read(&val1);
    smp_mb();
    atomic_write(&val2, 2);
    smp_mb();
    q = atomic_read(&val2);

    printf("%d %d\n", p, q);

    return 0;
}

Here, because of the bug the read from val1 is being generated after the write
to val2 breaking the semantics of memory fences. What am I missing?

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
How is that observable (other than looking at the assembly)?
val1 is an automatic variable whose address doesn't escape to other threads,
and isn't volatile either.  Why do you care?

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
Note that the technical issue here is that the memory is considered as not
aliased and thus all barriers would need to explicitely reference it as
used and clobbered.

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[bobby.prani at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #16 from Pranith Kumar <bobby.prani at gmail dot com> ---
This is not a good use case(it is a bit twisted, invalid too maybe?), but when
I try to read the stack without aliasing, the volatile write which I performed
is not visible.

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>

typedef struct {
    int counter;
} atomic_t;

static inline int atomic_read(atomic_t *v)
{
    return (*(volatile int *)&(v)->counter);
}

static inline void atomic_write(atomic_t *v, int val)
{
    v->counter = val;
}

#define smp_mb() asm volatile ("mfence":::"memory")

int i = 0;

int main(int argc, char *argv[])
{
    atomic_t val1 = {2};
    atomic_t val2 = {5};
    int p = 0;

    printf("%p %p\n", &val1, &val2); // try commenting this
    smp_mb();
    p = atomic_read(&val1);
    smp_mb();
    atomic_write(&val2, 9);
    smp_mb();

    printf("%d\n", *(int *)(&val1+4));

    return 0;
}

In the above program, if I comment the first printf() there is no alias for
val2 and the atomic_write() is optimized out and I see 0 printed out in the
second printf(). But without commenting the first printf() the write is issued
because of which I see 9 printed in the second printf.

[Bug rtl-optimization/63375] [4.8/4.9/5 Regression] reordering of reads across fences

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63375

--- Comment #17 from Mikael Pettersson <mikpelinux at gmail dot com> ---
(In reply to Pranith Kumar from comment #16)
>     printf("%d\n", *(int *)(&val1+4));

This is broken, you're invoking undefined behaviour.