Bug 22000 - [4.1 Regression] Read from volatile member of struct is optimized away
Summary: [4.1 Regression] Read from volatile member of struct is optimized away
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.0.0
: P2 normal
Target Milestone: 4.1.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2005-06-10 13:59 UTC by rpedersen
Modified: 2005-06-24 03:40 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.1.0 3.4.0
Known to fail: 4.0.0 4.0.1
Last reconfirmed: 2005-06-10 14:16:37


Attachments
Preprocessed testcase. (158 bytes, text/plain)
2005-06-10 14:00 UTC, rpedersen
Details
Proposed patch (476 bytes, patch)
2005-06-23 16:06 UTC, Mark Mitchell
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rpedersen 2005-06-10 13:59:25 UTC
In the code below the read "(int)ptr->b;" gets optimized away.
It appears to be optimized away before the RTL stages, so I guess
it must be because of the tree optimalisation.

I think that the C standard says C that the "name" of variable 
becomes an rvalue (the actual value) thus requires that the variable 
is accessed.

Code:

struct test {
  volatile int a;
  volatile int b;
};

int main(int argc, char *argv[]){

 struct test *ptr = (struct test *)0x10000000;

 ptr->b = 10;
 (int)ptr->b;

 return 1;
}
Comment 1 rpedersen 2005-06-10 14:00:32 UTC
Created attachment 9059 [details]
Preprocessed testcase.
Comment 2 Andrew Pinski 2005-06-10 14:07:42 UTC
This works on the mainline, it might work on the 4.0.x branch too but I did not check yet.
Comment 3 Andrew Pinski 2005-06-10 14:16:37 UTC
Still fails in 4.0.1.

Confirmed.
Comment 4 rpedersen 2005-06-13 06:42:06 UTC
Is it posible find out why it is woking in the mainline and generate
a patch for 4.0.0?
Comment 5 Paul Schlie 2005-06-13 11:17:13 UTC
(In reply to comment #0)
> I think that the C standard says C that the "name" of variable 
> becomes an rvalue (the actual value) thus requires that the variable 
> is accessed.

It actually says that accessing a volatile qualified object through a nonvolatile
qualified lvalue (as would be the case if referenced via a non-volatile qualified
cast expression) is implementation defined.

Personally, it seems more consistent to abide by the semantics specified
by a programmer's explicit cast expression, thereby if given:

int x, y;
const int *z;

x = (volatile int)y; // specifies a reference to a volatile int object.

z = &(const int)x;   // specifies a reference to a const int object.

And correspondingly:

int x;
volatile int y;
const int z;

x = (int)y; // cast's away volatile (therefore may be optimized away),
            // therefore if not desired, shouldn't be "cast away".

(int)z = x; // cast's away const (although may generate a run-time
            // exception if referencing a READONLY allocated object),
            // therefore if not desired, shouldn't be "cast away".
Comment 6 rpedersen 2005-06-13 11:56:01 UTC
(In reply to comment #5)
> (In reply to comment #0)
> > I think that the C standard says C that the "name" of variable 
> > becomes an rvalue (the actual value) thus requires that the variable 
> > is accessed.
> 
> It actually says that accessing a volatile qualified object through a 
nonvolatile
> qualified lvalue (as would be the case if referenced via a non-volatile 
qualified
> cast expression) is implementation defined.
> 

If you are correct, the proposed testcase might be misleading. 

Changing the expression for accessing the volatile member
of the struct to:
 "(volatile int)ptr->b;"
or just:
 "ptr->b;"
still leads to the access being optimized away.

> Personally, it seems more consistent to abide by the semantics specified
> by a programmer's explicit cast expression, thereby if given:
> 
> int x, y;
> const int *z;
> 
> x = (volatile int)y; // specifies a reference to a volatile int object.
> 
> z = &(const int)x;   // specifies a reference to a const int object.
> 
> And correspondingly:
> 
> int x;
> volatile int y;
> const int z;
> 
> x = (int)y; // cast's away volatile (therefore may be optimized away),
>             // therefore if not desired, shouldn't be "cast away".
> 
> (int)z = x; // cast's away const (although may generate a run-time
>             // exception if referencing a READONLY allocated object),
>             // therefore if not desired, shouldn't be "cast away".
> 
I agree with you on this.

Comment 7 Paul Schlie 2005-06-13 14:12:17 UTC
(In reply to comment #6)
> Changing the expression for accessing the volatile member
> of the struct to:
>  "(volatile int)ptr->b;"
> or just:
>  "ptr->b;"
> still leads to the access being optimized away.

Which seems like the real problem, as volatile access
sematics should clearly apply in both instances.

Comment 8 Andrew Pinski 2005-06-23 14:42:01 UTC
This looks like a latent bug on the mainline too, just harder to reproduce and here is a testcase:
struct test {
  struct tt
  {
  volatile int a;
  volatile int b;
  } t;
};

int main(int argc, char *argv[]){

 struct test *p = (struct test *)0x10000000;
 struct tt *ptr = &p->t;

 ptr->b = 10;
 (int)ptr->b;

 return 1;
}
Comment 9 Mark Mitchell 2005-06-23 15:11:51 UTC
I somewhat suspect this hunk of code in c-typeck.c:build_component_ref:

	  if (TREE_THIS_VOLATILE (datum) || TREE_THIS_VOLATILE (subdatum))
	    TREE_THIS_VOLATILE (ref) = 1;

I don't see anything that sets TREE_SIDE_EFFECTS.

However, from tree.h:

   If this bit is set in an expression, so is TREE_SIDE_EFFECTS.  */
   #define TREE_THIS_VOLATILE(NODE) ((NODE)->common.volatile_flag)

However, I'm having a hard time getting good connectivity back to my build
machine to experiment with this.

Joseph, any ideas about this?
Comment 10 Mark Mitchell 2005-06-23 15:28:16 UTC
Hmm, no, TREE_SIDE_EFFECTS does seem to be getting set.
Comment 11 Mark Mitchell 2005-06-23 16:06:47 UTC
Created attachment 9134 [details]
Proposed patch

This patch seems to fix the bug.  I'll kick off a test cycle.
Comment 12 joseph@codesourcery.com 2005-06-23 16:59:33 UTC
Subject: Re:  [4.0/4.1 Regression] Read from
 volatile member of struct is optimized away

On Thu, 23 Jun 2005, mmitchel at gcc dot gnu dot org wrote:

> I somewhat suspect this hunk of code in c-typeck.c:build_component_ref:
> 
> 	  if (TREE_THIS_VOLATILE (datum) || TREE_THIS_VOLATILE (subdatum))
> 	    TREE_THIS_VOLATILE (ref) = 1;
> 
> I don't see anything that sets TREE_SIDE_EFFECTS.

In general TREE_SIDE_EFFECTS is set by the low-level tree-building code in 
tree.c (if set for the operands of a tree) so front ends need only change 
it if the defaults are wrong.

>    If this bit is set in an expression, so is TREE_SIDE_EFFECTS.  */
>    #define TREE_THIS_VOLATILE(NODE) ((NODE)->common.volatile_flag)

This does seem like something tree checking could usefully be added for.

Comment 13 CVS Commits 2005-06-23 20:15:41 UTC
Subject: Bug 22000

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	mmitchel@gcc.gnu.org	2005-06-23 20:15:29

Modified files:
	gcc            : ChangeLog tree-ssa-operands.c 

Log message:
	PR 22000
	* tree-ssa-operands.c (get_expr_operands): Check the volatility of
	the FIELD_DECL and set s_ann->has_volatile_ops accordingly.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.290&r2=2.7592.2.291
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-ssa-operands.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.63&r2=2.63.2.1

Comment 14 Mark Mitchell 2005-06-23 20:23:33 UTC
Fixed in 4.0.1.
Comment 15 CVS Commits 2005-06-24 03:38:11 UTC
Subject: Bug 22000

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	mmitchel@gcc.gnu.org	2005-06-24 03:38:06

Modified files:
	gcc            : ChangeLog tree-ssa-operands.c 

Log message:
	PR 22000
	* tree-ssa-operands.c (get_expr_operands): Check the volatility of
	the FIELD_DECL and set s_ann->has_volatile_ops accordingly.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9215&r2=2.9216
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-ssa-operands.c.diff?cvsroot=gcc&r1=2.87&r2=2.88

Comment 16 Mark Mitchell 2005-06-24 03:40:02 UTC
Fixed in 4.0.1.