User account creation filtered due to spam.

Bug 16721 - [4.0 Regression] Accesses to volatile objects optimized away
Summary: [4.0 Regression] Accesses to volatile objects optimized away
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.0.0
: P3 critical
Target Milestone: 4.0.0
Assignee: Diego Novillo
URL:
Keywords: alias, wrong-code
Depends on:
Blocks:
 
Reported: 2004-07-26 18:11 UTC by Maciej W. Rozycki
Modified: 2004-09-22 23:36 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 3.3.4
Known to fail: 4.0.0
Last reconfirmed: 2004-09-22 17:23:55


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej W. Rozycki 2004-07-26 18:11:56 UTC
 Certain read accesses to objects marked as "volatile" are optimized away.  
This happens when both "-O2" and "-fno-strict-aliasing" are specified.  
This has been observed with the Linux kernel which is unusable in certain
configurations due to this problem.  This used to work with 3.4.0.

Environment:
System: Linux macro 2.4.26 #3 Thu May 6 20:57:18 CEST 2004 i686 unknown unknown GNU/Linux
Architecture: i686

host: i686-pc-linux-gnu
build: i686-pc-linux-gnu
target: mips64el-unknown-linux-gnu
configured with: ../configure --prefix=/usr --enable-languages=c --disable-libmudflap --disable-shared --target=mips64el-linux

How-To-Repeat:
 The following program can be used:

$ cat volatile.c
struct data {
	volatile unsigned long *addr;
} *p;

int test()
{
	*p->addr;
	return 0;
}
$ mips64el-linux-gcc -mabi=32 -O2 -fno-strict-aliasing -mno-abicalls -S
volatile.c -o volatile.s
$ cat volatile.s
	.file	1 "volatile.c"
	.section .mdebug.abi32
	.previous
	.text
	.align	2
	.align	2
	.globl	test
	.ent	test
	.type	test, @function
test:
	.frame	$sp,0,$31		# vars= 0, regs= 0/0, args= 0, gp= 0
	.mask	0x00000000,0
	.fmask	0x00000000,0
	.set	noreorder
	.set	nomacro
	
	j	$31
	move	$2,$0

	.set	macro
	.set	reorder
	.end	test

	.comm	p,4,4
	.ident	"GCC: (GNU) 3.5.0 20040726 (experimental)"

After removing the "-fno-strict-aliasing" option, the output is correct:

$ mips64el-linux-gcc -mabi=32 -O2 -mno-abicalls -S volatile.c -o 
volatile.s
$ cat volatile.s
	.file	1 "volatile.c"
	.section .mdebug.abi32
	.previous
	.text
	.align	2
	.align	2
	.globl	test
	.ent	test
	.type	test, @function
test:
	.frame	$sp,0,$31		# vars= 0, regs= 0/0, args= 0, gp= 0
	.mask	0x00000000,0
	.fmask	0x00000000,0
	.set	noreorder
	.set	nomacro
	
	lui	$2,%hi(p)
	lw	$4,%lo(p)($2)
	move	$2,$0
	lw	$3,0($4)
	nop
	lw	$5,0($3)
	j	$31
	nop

	.set	macro
	.set	reorder
	.end	test

	.comm	p,4,4
	.ident	"GCC: (GNU) 3.5.0 20040726 (experimental)"
Comment 1 Maciej W. Rozycki 2004-07-26 18:11:56 UTC
Fix:
 None known.  Using "-fstrict-aliasing" may be a workaround, but it is not 
an option for Linux.
Comment 2 Falk Hueffner 2004-07-26 19:04:29 UTC
I can reproduce this on alphaev68-linux.

In the test case you gave, removing the access seems entirely appropriate
to me, since addr is not volatile.

However, the same happens if you mark it volatile, so this is really a bug.
-fno-strict-aliasing is a red herring; the access also gets optimized away for

struct data { volatile unsigned long addr; } p;
void test() { p.addr; }

at -O2.
Comment 3 Andrew Pinski 2004-07-26 19:24:37 UTC
In my view the defintion of volatile just means that the value can be changed from beneath you but you 
can still get rid of the access so it looks like this might not be a bug but rather a bug in the Linux 
Kernel thinking otherwise.
Comment 4 Falk Hueffner 2004-07-26 19:30:14 UTC
I don't think that's what the standard says. Even if it did, it is not
what people expect. Read accesses can have effects on magic hardware 
addresses, and you need volatile to keep the compiler from optimizing
these away.

So I still think this is an important bug.
Comment 5 Maciej W. Rozycki 2004-07-27 12:25:30 UTC
(In reply to comment #2)
> I can reproduce this on alphaev68-linux.
> 
> In the test case you gave, removing the access seems entirely appropriate
> to me, since addr is not volatile.
> 
> However, the same happens if you mark it volatile, so this is really a bug.
> -fno-strict-aliasing is a red herring; the access also gets optimized away for
> 
> struct data { volatile unsigned long addr; } p;
> void test() { p.addr; }
> 
> at -O2.
> 

p->addr is not volatile, but *p->addr is.  Threfore the original test case is valid.
Comment 6 Maciej W. Rozycki 2004-07-27 12:50:32 UTC
(In reply to comment #3)

`info "(gcc)"Volatiles' disagrees -- despite it being mostly about C++, it
refers to GCC's behavior for C code, too.  Specifically:

"[...]  Less obvious expressions are where something which looks like an
access is used in a void context.  An example would be,

     volatile int *src = SOMEVALUE;
     *src;

   With C, such expressions are rvalues, and as rvalues cause a read of
the object, GCC interprets this as a read of the volatile being pointed
to. [...]"

Otherwise, what is your proposal about coding reads that must not be optimized
away due to side effects they have?  The reads missing from Linux binaries due
to this bug are accesses to a DMA controller register to reset the logic at the
end of a transfer.  This is the lone need for them in this context -- the value
read is not needed there for anything.  There are more cases like this -- it's
just that this one was the first one I've spotted, and actually quite a critical
one as the DMA controller is used for CoW by the paging subsystem on this
platform.  Others may stay hidden as the may only trigger under less common
circumstances only.
Comment 7 Falk Hueffner 2004-07-27 14:48:48 UTC
Setting severity to critical, since gcc is not behaving as documented
Comment 8 Andrew Pinski 2004-07-29 02:33:22 UTC
Well we do not document what we do for volatiles so ...
http://gcc.gnu.org/onlinedocs/gcc/Qualifiers-implementation.html#Qualifiers-implementation

But we do document the extension to c++ because in C++ there cannot happen:
http://gcc.gnu.org/onlinedocs/gcc/Volatiles.html#Volatiles
Comment 9 Maciej W. Rozycki 2004-07-29 11:26:51 UTC
(In reply to comment #8)

 Well, I've missed the section about standard conformance -- thanks for pointing
that out.  Anyway, the note about C in the C++ section hints about the expected
behavior of GCC for read accesses to volatile objects and I am pretty sure it's
actually what's widely expected by developers as well.  The use of the
"volatile" keyword is presently two-fold, specifically:

1. Marking objects referring to hardware that does not behave as plain memory.

2. Marking objects in ordinary memory that can change unexpectedly, e.g. by
   another CPU or by an interrupt handler.

For the latter eliminating dead reads is safe.  For the former it is not.  Thus
one could argue it would be beneficial to differentiate between the two cases,
e.g. by defining an additional variable attribute to enable or inhibit such
elimination.  I don't think it is really necessary -- the performance penalty
from a dead read from a memory variable is not that high and volatile objects
are special and require special attention from programmers anyway.  If there is
a possibility of a dead read to happen, then most likely it can be handled
explicitly with a conditional statement.  And older GCC versions used not to
optimize these reads away, so programmers should be prepared for the penalty
already.

 Therefore I propose to keep the old behavior and document it in the standard
conformance section to avoid confusion in the future.
Comment 10 Andrew Pinski 2004-08-03 05:58:57 UTC
It is now documented in the new spot.
This is a bug in DCE.
Comment 11 Steven Bosscher 2004-09-19 13:57:00 UTC
Fails at -O -fno-strict-aliasing, works with -O -fstrict-aliasing. 
This looks like a may-alias bug in tree-ssa-alias. 
Comment 12 Wolfgang Bangerth 2004-09-19 15:42:36 UTC
This would be the first bug where -fstrict-aliasing produces correct 
code while -fno-strict-aliasing doesn't :-) 
 
W. 
Comment 13 Diego Novillo 2004-09-22 16:13:52 UTC
I still see the load from p->addr in the .optimized dump.  And the load seems to
be there in the assembly output.

Is this still a problem?
$ gcc/cc1 -O2 -fdump-tree-all-vops 16721.c
$ diff -y -W 80 16721.c.t03.generic 16721.c.t62.vars
test ()                                 test ()
{                                       {
  struct data * p.0;                  <
  volatile long unsigned int * D.1470 <
  long unsigned int vol.1;                long unsigned int vol.1;
  int D.1472;                         |   volatile long unsigned int * D.1470

  p.0 = p;                            | <bb 0>:
  D.1470 = p.0->addr;                 |   #   VUSE <TMT.2_7>;
                                      >   D.1470 = p->addr;
  vol.1 = *D.1470;                        vol.1 = *D.1470;
  D.1472 = 0;                         |   return 0;
  return D.1472;                      |
}                                       }

$ gcc/cc1 --version
GNU C version 4.0.0 20040920 (experimental) (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.0.0 20040920 (experimental).
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
Comment 14 Steven Bosscher 2004-09-22 16:26:31 UTC
Subject: Re:  [4.0 Regression] Accesses to volatile objects optimized away

On Wednesday 22 September 2004 18:13, dnovillo at gcc dot gnu dot org wrote:
> ------- Additional Comments From dnovillo at gcc dot gnu dot org 
> 2004-09-22 16:13 -------
>
> I still see the load from p->addr in the .optimized dump.  And the load
> seems to be there in the assembly output.
>
> Is this still a problem?
> $ gcc/cc1 -O2 -fdump-tree-all-vops 16721.c

Add -fno-strict-aliasing

Comment 15 Diego Novillo 2004-09-22 16:41:22 UTC
Subject: Re:  [4.0 Regression] Accesses to
	volatile objects optimized away

On Wed, 2004-09-22 at 12:26, stevenb at suse dot de wrote:

> Add -fno-strict-aliasing
> 
Ah.  Thanks.

Comment 16 Diego Novillo 2004-09-22 17:23:11 UTC
Testing patch.
Comment 17 CVS Commits 2004-09-22 23:33:24 UTC
Subject: Bug 16721

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	dnovillo@gcc.gnu.org	2004-09-22 23:33:20

Modified files:
	gcc            : ChangeLog tree-dfa.c tree-ssa-alias.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg/tree-ssa: pr16721.c 

Log message:
	PR tree-optimization/16721
	* tree-dfa.c (dump_variable): Show TREE_THIS_VOLATILE.
	* tree-ssa-alias.c (create_memory_tag): Move setting of
	TREE_THIS_VOLATILE ...
	(get_tmt_for): ... here.
	
	testsuite/ChangeLog
	
	PR tree-optimization/16721
	* testsuite/gcc.dg/tree-ssa/pr16721.c: New test.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.5570&r2=2.5571
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-dfa.c.diff?cvsroot=gcc&r1=2.33&r2=2.34
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-ssa-alias.c.diff?cvsroot=gcc&r1=2.41&r2=2.42
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.4331&r2=1.4332
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/tree-ssa/pr16721.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 18 Diego Novillo 2004-09-22 23:36:44 UTC
Fix: http://gcc.gnu.org/ml/gcc-patches/2004-09/msg02282.html