[Bug c/58943] New: wrong calculation of indirect structure member arithmetic via function call

[Bug c/58943] New: wrong calculation of indirect structure member arithmetic via function call

[Dimitar.Peikov at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

            Bug ID: 58943
           Summary: wrong calculation of indirect structure member
                    arithmetic via function call
           Product: gcc
           Version: 4.8.1
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: Dimitar.Peikov at gmail dot com

Created attachment 31124
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31124&action=edit
example for failed structure propagation

In attached file there is an example of invalid calculation where, a structure
with integer is manipulated inside a function call which takes struct as
pointer and same integer member is part of arithmetic result operation.
The issue was tested on several compilers 4.6.3, 4.7.2, 4.8.1 on different
platforms Linux and Windows with same result.

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-11-04
      Known to work|                            |3.3.6
   Target Milestone|---                         |4.7.4
            Summary|wrong calculation of        |[4.7/4.8/4.9 Regression]
                   |indirect structure member   |wrong calculation of
                   |arithmetic via function     |indirect structure member
                   |call                        |arithmetic via function
                   |                            |call
     Ever confirmed|0                           |1

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
           Severity|critical                    |normal

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If anything, this would be a C FE request for a change like PR45437 that made
into the C++ FE.  The question is if we want to do this only for flag_isoc11,
or also older.  Handling it during gimplification or later is impossible,
because
the difference between x[0] |= x[0] | f (); and x[0] = x[0] | f (); is already
lost there.

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

--- Comment #4 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I think it should be fixed for all C standard versions, not just C11 (that 
is, the front end should produce something like (save (rhs), save (lhs) = 
save (lhs) op save (rhs)) to get the right order of evaluation).

As it's reported for at least 4.6 onwards and earlier versions didn't 
claim any C11 support and only C11 defined things sufficiently precisely 
for this to be a bug, I don't see this as a regression.

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So something like:
--- gcc/c/c-typeck.c.jj    2013-12-03 08:27:22.000000000 +0100
+++ gcc/c/c-typeck.c    2013-12-04 11:53:57.160894632 +0100
@@ -5192,6 +5192,7 @@ build_modify_expr (location_t location,
 {
   tree result;
   tree newrhs;
+  tree rhseval = NULL_TREE;
   tree rhs_semantic_type = NULL_TREE;
   tree lhstype = TREE_TYPE (lhs);
   tree olhstype = lhstype;
@@ -5253,8 +5254,17 @@ build_modify_expr (location_t location,
       /* Construct the RHS for any non-atomic compound assignemnt. */
       if (!is_atomic_op)
         {
+      /* If in LHS op= RHS the RHS has side-effects, ensure they
+         are preevaluated before the rest of the assignment expression's
+         side-effects, because RHS could contain e.g. function calls
+         that modify LHS.  */
+      if (TREE_SIDE_EFFECTS (rhs))
+        {
+          newrhs = c_save_expr (rhs);
+          rhseval = newrhs;
+        }
       newrhs = build_binary_op (location,
-                    modifycode, lhs, rhs, 1);
+                    modifycode, lhs, newrhs, 1);

       /* The original type of the right hand side is no longer
          meaningful.  */
@@ -5268,7 +5278,7 @@ build_modify_expr (location_t location,
      if so, we need to generate setter calls.  */
       result = objc_maybe_build_modify_expr (lhs, newrhs);
       if (result)
-    return result;
+    goto return_result;

       /* Else, do the check that we postponed for Objective-C.  */
       if (!lvalue_or_else (location, lhs, lv_assign))
@@ -5362,7 +5372,7 @@ build_modify_expr (location_t location,
       if (result)
     {
       protected_set_expr_location (result, location);
-      return result;
+      goto return_result;
     }
     }

@@ -5383,11 +5393,15 @@ build_modify_expr (location_t location,
      as the LHS argument.  */

   if (olhstype == TREE_TYPE (result))
-    return result;
+    goto return_result;

   result = convert_for_assignment (location, olhstype, result, rhs_origtype,
                    ic_assign, false, NULL_TREE, NULL_TREE, 0);
   protected_set_expr_location (result, location);
+
+return_result:
+  if (rhseval)
+    result = build2 (COMPOUND_EXPR, TREE_TYPE (result), rhseval, result);
   return result;
 }


?  In any case, it will have various consequences on OpenMP and perhaps
vectorization.

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
Yes, something like that.

[Bug c/58943] [4.7/4.8/4.9 Regression] wrong calculation of indirect structure member arithmetic via function call

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58943

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Wed Jan 15 08:03:53 2014
New Revision: 206620

URL: http://gcc.gnu.org/viewcvs?rev=206620&root=gcc&view=rev
Log:
    PR c/58943
    * c-typeck.c (build_modify_expr): For lhs op= rhs, if rhs has side
    effects, preevaluate rhs using SAVE_EXPR first.

    * c-omp.c (c_finish_omp_atomic): Set in_late_binary_op around
    build_modify_expr with non-NOP_EXPR opcode.  Handle return from it
    being COMPOUND_EXPR.
    (c_finish_omp_for): Handle incr being COMPOUND_EXPR with first
    operand a SAVE_EXPR and second MODIFY_EXPR.

    * gcc.c-torture/execute/pr58943.c: New test.
    * gcc.dg/tree-ssa/ssa-fre-33.c (main): Avoid using += in the test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr58943.c
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-omp.c
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-typeck.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-33.c

[Bug c/58943] [4.7/4.8 Regression] wrong calculation of indirect structure member arithmetic via function call

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.7.4                       |4.8.4

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
The 4.7 branch is being closed, moving target milestone to 4.8.4.

[Bug c/58943] [4.8 Regression] wrong calculation of indirect structure member arithmetic via function call

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.4                       |4.8.5

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.4 has been released.

[Bug c/58943] [4.8 Regression] wrong calculation of indirect structure member arithmetic via function call

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|4.8.5                       |4.9.0
      Known to fail|                            |4.8.5

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed for 4.9.0.