Bug 48814 - [4.5/4.6/4.7 Regression] Incorrect scalar increment result
Summary: [4.5/4.6/4.7 Regression] Incorrect scalar increment result
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.6.0
: P2 normal
Target Milestone: 4.8.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
: 53656 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-29 06:26 UTC by Johannes Schaub
Modified: 2018-03-24 19:20 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.8.0
Known to fail: 4.7.1
Last reconfirmed: 2011-05-02 14:53:42


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Johannes Schaub 2011-04-29 06:26:27 UTC
The following test-case should output "2 3", but GCC outputs "1 2":

#include <stdio.h>

int arr[] = {1,2,3,4};
int count = 0;

int incr(){
 return ++count;
}

int main(){  
   arr[count++]=incr();
   printf("%d %d",count,arr[count]);    
   return 0;
}

(it may not be obvious; see http://stackoverflow.com/questions/5827707/why-this-output/5828578#5828578 ). Clang correctly handles this case.
Comment 1 Richard Biener 2011-04-29 09:56:08 UTC
6.5.16/4

"The order of evaluation of the operands is unspecified."


The gimplifier is responsible for this semantic detail of GENERIC (that
matches C for its sequence point rules).

Can you explain the rationale why the behavior is not simply undefined?
The sequence point before the call does not make the evaluation order
of the assignment operands defined.
Comment 2 Johannes Schaub 2011-04-29 10:42:12 UTC
Since the order of evaluation is undefined it may evaluate "count++" and "incr()" in any order, as it pleases. 

Since there is a sequence point before entering a function, and before leaving a function, and since function invocations do not interleave, we know that no matter whether we evaluate "count++" before or after executing "incr", the side effects of both "count++" and "++count" do not happen without an intervening sequence point.

Sequence points have that effect of guaranteeing that side effects of previous evaluations have been finished, and side effects of subsequent evaluations have not yet occured.

Undefined behavior would occur if the two side effects would occur without an intervening sequence point. For example, if we were to replace "incr()" by a plain "++count".
Comment 3 joseph@codesourcery.com 2011-04-29 12:03:14 UTC
This may well be a bug, but it's the sort of case where you want an 
analysis not in terms of sequence points but in terms of the more 
precisely defined sequencing model in C++0x and C1x.  Do you have such an 
analysis?
Comment 4 joseph@codesourcery.com 2011-04-29 12:13:46 UTC
I think the relevant wording in the C1X DIS is "With respect to an 
indeterminately-sequenced function call, the operation of postfix ++ is a 
single evaluation."; C++ N3291 has the same wording.
Comment 5 Johannes Schaub 2011-04-29 16:20:44 UTC
(In reply to comment #4)
> I think the relevant wording in the C1X DIS is "With respect to an 
> indeterminately-sequenced function call, the operation of postfix ++ is a 
> single evaluation."; C++ N3291 has the same wording.

Yes, I agree. This makes it clearer than my C++03 description using sequence points that GCC is in error.
Comment 6 Joseph S. Myers 2011-05-02 14:53:42 UTC
Confirming as a gimplifier bug, a regression from 4.0 onwards (presumably introduced with tree-ssa).
Comment 7 Richard Biener 2011-05-02 15:35:01 UTC
(In reply to comment #2)
> Since the order of evaluation is undefined it may evaluate "count++" and
> "incr()" in any order, as it pleases. 
> 
> Since there is a sequence point before entering a function, and before leaving
> a function, and since function invocations do not interleave, we know that no
> matter whether we evaluate "count++" before or after executing "incr", the side
> effects of both "count++" and "++count" do not happen without an intervening
> sequence point.

The side-effects yes, but the read in count++ happens at either before
or after incr is executed.  At least that is my reading of undefined
evaluation order.  Does the sequence point before entering a function
make that evaluation order suddenly defined?

Of course since Joseph seems to agree I won't object to fixing it.
Comment 8 joseph@codesourcery.com 2011-05-02 16:24:01 UTC
On Mon, 2 May 2011, rguenth at gcc dot gnu.org wrote:

> The side-effects yes, but the read in count++ happens at either before
> or after incr is executed.  At least that is my reading of undefined
> evaluation order.  Does the sequence point before entering a function
> make that evaluation order suddenly defined?

The sequence point wording in C99 and C++03 is inherently confusing and 
doesn't go into much detail about what may or may not interleave, but what 
C1x and C++0x make explicit is that the evaluation of ++, -- and compound 
assignment do not interleave with an indeterminately-sequenced function 
call (and so they may not act by calling the indeterminately sequenced 
function between the read and the write of the operand being updated).  
You can argue about what exactly C99 and C++03 require (and so about 
whether there is a regression), but there is quite clearly now a bug to be 
fixed (and the fix should not depend on the language version).
Comment 9 Richard Biener 2011-06-27 12:14:19 UTC
4.3 branch is being closed, moving to 4.4.7 target.
Comment 10 Richard Biener 2012-02-09 13:03:25 UTC
For

extern void abort (void);
struct S { int i; };
struct S arr[32];
volatile int count = 0;

struct S __attribute__((noinline))
incr (void)
{
  ++count;
}

int main()
{
  arr[count++] = incr ();
  if (count != 2)
    abort ();
  return 0;
}

we can only avoid reading 'count' too many times (once to get at the
index for the store and once for updating its value) if we can
insert a statement inbetween the call and the store to arr[].  Which
we can do only if we are introducing an aggregate temporary - which
might work in C, but does not work in C++ when we require return-slot-optimization.  Thus, in the face of volatiles and required RSO
this is unfixable.
Comment 11 Jakub Jelinek 2012-03-13 12:47:46 UTC
4.4 branch is being closed, moving to 4.5.4 target.
Comment 12 Richard Biener 2012-03-16 11:48:54 UTC
Author: rguenth
Date: Fri Mar 16 11:48:48 2012
New Revision: 185465

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185465
Log:
2012-03-16  Richard Guenther  <rguenther@suse.de>
	Kai Tietz  <ktietz@redhat.com>

	PR middle-end/48814
	* gimplify.c (gimplify_self_mod_expr): Evaluate postfix
	side-effects completely in the pre-queue and use a temporary
	for the result.

	* gcc.c-torture/execute/pr48814-1.c: New test.
	* gcc.c-torture/execute/pr48814-2.c: New test.
	* gcc.dg/tree-ssa/assign-1.c: New test.
	* gcc.dg/tree-ssa/assign-2.c: New test.
	* gcc.dg/tree-ssa/assign-3.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/assign-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimplify.c
    trunk/gcc/testsuite/ChangeLog
Comment 13 Richard Biener 2012-03-16 11:49:39 UTC
Fixed.  Deliberately not backporting.
Comment 14 Andrew Pinski 2012-06-14 17:55:29 UTC
*** Bug 53656 has been marked as a duplicate of this bug. ***
Comment 15 Nick Maclaren 2013-03-15 23:20:56 UTC
I must correct some of the statements here.  This is not a bug, and the
intent of WG14 during the standardisation of C90 was that this should be
undefined behaviour.  This point was raised explicitly - unfortunately,
those of us who wanted this area to be better specified did not
prevail:-(

This is a CHANGE in the C language introduced by C 2011.  gcc may wish
to adopt that, but that is not the same as saying this is a bug.  As
it merely degrades optimisation and does not break any working programs,
it is not a major matter.

The only reason that I am posting this after the decision has been taken
is that there are other changes introduced by C11 that may not be so
benign.  There are many less than in C99, but still quite a lot.  This
should not be used as precedent.