[Bug tree-optimization/60770] New: disappearing clobbers

[Bug tree-optimization/60770] New: disappearing clobbers

[glisse at gcc dot gnu.org]

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

            Bug ID: 60770
           Summary: disappearing clobbers
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: glisse at gcc dot gnu.org

Hello,

looking at Manuel's testcase from PR 60517, I notice that EINLINE changes:

  D.2253 = A::getB (&a);

to:

  D.2264 = a.b;
  D.2263 = D.2264;
  D.2253 = D.2263;

(several copies, but only the original D.2253 has a clobber)
and ESRA changes:

  D.2253 = D.2263;
  D.2253 ={v} {CLOBBER};
  _5 = MEM[(double *)&D.2253];

to:

  D.2253 = D.2263;
  SR.1_3 = MEM[(struct B *)&D.2263];
  D.2253 ={v} {CLOBBER};
  _5 = SR.1_3;

The clobber then disappears in release_ssa.

It is correct, but not so helpful, because it hides the fact that we are
reading from dead memory. If I disable ESRA, the clobber and the memory read
are still present in the right order in the .optimized dump at -O3.

Would it be possible to keep the memory read after the clobber, without
affecting performance?


class B {
public:
    double x[2];
};
class A {
    B b;
public:
    B getB(void) { return b; }
};

double foo(A a) {
    double * x = &(a.getB().x[0]);
    return x[0];
}

[Bug tree-optimization/60770] disappearing clobbers

[manu at gcc dot gnu.org]

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

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

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

--- Comment #2 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #1)
> I guess relying on clobbers in the middle-end for warnings is a bit
> optimistic...

If I interpret the dump correctly, this is a valid optimization because the
original code is undefined, but it is hiding the fact that the original code is
buggy. CCP does this kind of thing for uninitialized warnings (PR18501), and it
would be nice to find a way to warn for it.
>From gcc-bugs-return-448392-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Sun Apr 06 09:00:34 2014
Return-Path: <gcc-bugs-return-448392-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 19480 invoked by alias); 6 Apr 2014 09:00:33 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 19414 invoked by uid 48); 6 Apr 2014 09:00:25 -0000
From: "manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug c++/60517] warning/error for taking address of member of a temporary object
Date: Sun, 06 Apr 2014 09:00:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: c++
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: manu at gcc dot gnu.org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-60517-4-KYH2JAY1Qx@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-60517-4@http.gcc.gnu.org/bugzilla/>
References: <bug-60517-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-04/txt/msg00412.txt.bz2
Content-length: 681

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

--- Comment #12 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #11)
> Created attachment 32551 [details]
> first try
> 
> With clobber removal now.

Why do you want to remove the clobber? 

I understood your idea to avoid duplicated warnings was to add
__builtin_unreachable and replace the value.

Could be possible to have a special var/value that is marked as artificial such
that the warning code never warns for statements containing this variable? We
already have DECL_ARTIFICIAL in the FE but I am not sure whether that survives
to the middle-end.
>From gcc-bugs-return-448394-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Sun Apr 06 09:04:35 2014
Return-Path: <gcc-bugs-return-448394-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 21618 invoked by alias); 6 Apr 2014 09:04:35 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 21197 invoked by uid 48); 6 Apr 2014 09:04:29 -0000
From: "dominiq at lps dot ens.fr" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug fortran/56674] [4.7/4.8/4.9 Regression] ICE in check_sym_interfaces
Date: Sun, 06 Apr 2014 09:04:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: fortran
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords: error-recovery, ice-on-invalid-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: dominiq at lps dot ens.fr
X-Bugzilla-Status: NEW
X-Bugzilla-Priority: P4
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 4.7.4
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-56674-4-McpGIX0SaP@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-56674-4@http.gcc.gnu.org/bugzilla/>
References: <bug-56674-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-04/txt/msg00414.txt.bz2
Content-length: 594

http://gcc.gnu.org/bugzilla/show_bug.cgi?idV674

--- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
Slightly reduced test

MODULE realspace_grid_types
  PUBLIC :: realspace_grid_input_type
CONTAINS
  SUBROUTINE rs_grid_create_descriptor ( )
    TYPE(realspace_grid_input_type),
  END SUBROUTINE
END MODULE realspace_grid_types

pr56674_red.f90:5.35:

    TYPE(realspace_grid_input_type),
                                   1
Error: Derived type 'realspace_grid_input_type' at (1) is being used before it
is defined
f951: internal compiler error: Segmentation fault: 11

[Bug tree-optimization/60770] disappearing clobbers

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Yeah, you simply need to catch this earlier ...

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #4 from Marc Glisse <glisse at gcc dot gnu.org> ---
Catching it earlier may be hard, even for these trivial examples we only have
from einline (18) to esra (24) or from eh (10) to ccp1 (21) and in more
complicated examples I fear the interval will be empty, but we can try... CCP
looks like the most natural choice in the interval 18-21.

Manuel, for the CCP thing, it looks a bit different from PR 18501. There, you
had something that was either 42 or undefined, so we picked 42. Here, we have
something that must be undefined, and we are still picking whatever value it
might have had before becoming undefined (but the write of that old value may
have already been removed because it sees the value becomes undefined
later...). Not that it is necessarily easier to do anything about it.

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #5 from Marc Glisse <glisse at gcc dot gnu.org> ---
Looking at comment #1 more closely, the problem is not with CCP, which
produces:

q = n_2(D);
q ={v} {CLOBBER};
_6 = q;
return _6;

The problem is during the TODO, in execute_update_addresses_taken, which finds
out that q does not need its address taken anymore, and

                /* For var ={v} {CLOBBER}; where var lost
                   TREE_ADDRESSABLE just remove the stmt.  */

Maybe it could instead create an uninitialized ssa_name for var.

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #6 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #5)
> The problem is during the TODO, in execute_update_addresses_taken, which
> finds out that q does not need its address taken anymore, and
> 
>                 /* For var ={v} {CLOBBER}; where var lost
>                    TREE_ADDRESSABLE just remove the stmt.  */

Added by Jakub for PR 50317.

> Maybe it could instead create an uninitialized ssa_name for var.

In r190200, Richard added to rewrite_stmt:

            /* If we rewrite a DECL into SSA form then drop its
               clobber stmts and replace uses with a new default def.  */

rewrite_update_stmt doesn't do the same, but I think it would make sense if it
did. I am having some difficulty finding the right place to do it so the
bookkeeping works properly (the clobber has a vdef, so there are vuses to
update at least), so I can't easily test the impact it would have.

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #7 from Marc Glisse <glisse at gcc dot gnu.org> ---
Created attachment 33024
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33024&action=edit
replace clobber with default def

This passes bootstrap+testsuite with all default languages, but it breaks ada,
I get plenty of warnings (as if it was reading objects after clobbering them)
and 2 errors about "Unable to coalesce ssa_names [...] which are marked as MUST
COALESCE."

I am probably doing something wrong in the patch, but ada seems to be using
clobbers differently than others since it is the only one that notices (well, I
didn't have a chance to check go since ada broke early).

It would also be nice to save the info somewhere that this default def actually
comes from a clobber, so the uninit pass can give a different warning message.
Maybe there is a bit available in the gimple_nop...

[Bug tree-optimization/60770] disappearing clobbers

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

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

--- Comment #8 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I am probably doing something wrong in the patch, but ada seems to be using
> clobbers differently than others since it is the only one that notices
> (well, I didn't have a chance to check go since ada broke early).

The Ada front-end is the only serious user of abnormal call edges and the like
(for example SSA_NAME_OCCURS_IN_ABNORMAL_PHI) so this is not very surprising.

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #9 from Marc Glisse <glisse at gcc dot gnu.org> ---
The warnings are "normal", they also appear in a non-patched build, so that
leaves only the coalesce errors.

(In reply to Eric Botcazou from comment #8)
> The Ada front-end is the only serious user of abnormal call edges and the
> like (for example SSA_NAME_OCCURS_IN_ABNORMAL_PHI) so this is not very
> surprising.

Indeed there are setjmp related things nearby in the dump and it is most likely
related to this error.

Though I am not sure what coalesce is unhappy about:

Unable to coalesce ssa_names 3329 and 3388 which are marked as MUST COALESCE.
r_3329(ab) and  r_3388(D)(ab)

That's likely:
  # r_3329(ab) = PHI <r_3388(D)(ab)(3), ... (super long)
  # r_3330(ab) = PHI <r_3388(D)(ab)(6), r_3329(ab)(7)>

but r_3388 only ever appears on the rhs of PHI nodes, always uninitialized, so
it can essentially be ignored. Well, I'll keep looking when I can. (tools like
delta have a hard time removing much more than the comments there, the size of
the testcase (ali.adb) doesn't help)

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #10 from Marc Glisse <glisse at gcc dot gnu.org> ---
Author: glisse
Date: Mon Nov  3 10:47:04 2014
New Revision: 217034

URL: https://gcc.gnu.org/viewcvs?rev=217034&root=gcc&view=rev
Log:
2014-11-03  Marc Glisse  <marc.glisse@inria.fr>

    PR tree-optimization/60770
gcc/
    * tree-into-ssa.c (rewrite_update_stmt): Return whether the
    statement should be removed.
    (maybe_register_def): Likewise. Replace clobbers with default
    definitions.
    (rewrite_dom_walker::before_dom_children): Remove statement if
    rewrite_update_stmt says so.
    * tree-ssa-live.c: Include tree-ssa.h.
    (set_var_live_on_entry): Do not mark undefined variables as live.
    (verify_live_on_entry): Do not check undefined variables.
    * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
    of partially undefined variables.
    * tree-ssa.c (ssa_undefined_value_p): Likewise.
    (execute_update_addresses_taken): Do not drop clobbers.

gcc/testsuite/
    * gcc.dg/tree-ssa/pr60770-1.c: New file.


Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-into-ssa.c
    trunk/gcc/tree-ssa-live.c
    trunk/gcc/tree-ssa.c
    trunk/gcc/tree-ssa.h

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #11 from Marc Glisse <glisse at gcc dot gnu.org> ---
Author: glisse
Date: Sat Nov 22 14:28:19 2014
New Revision: 217967

URL: https://gcc.gnu.org/viewcvs?rev=217967&root=gcc&view=rev
Log:
2014-11-22  Marc Glisse  <marc.glisse@inria.fr>

    PR tree-optimization/60770
    * tree-sra.c (clobber_subtree): New function.
    (sra_modify_constructor_assign): Call it.


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

[Bug tree-optimization/60770] disappearing clobbers

[orgads at gmail dot com]

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

Orgad Shaneh <orgads at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |orgads at gmail dot com

--- Comment #13 from Orgad Shaneh <orgads at gmail dot com> ---
The case described in comment 1 doesn't issue a warning with GCC 10.

Looks like it's a different case than bug 60517.

[Bug tree-optimization/60770] disappearing clobbers

[glisse at gcc dot gnu.org]

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

--- Comment #14 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Orgad Shaneh from comment #13)
> The case described in comment 1 doesn't issue a warning with GCC 10.

It does for me with -Wall -O (you need at least some optimization). If there is
still a problem, you need to open a new issue.

[Bug tree-optimization/60770] disappearing clobbers

[orgads at gmail dot com]

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

--- Comment #15 from Orgad Shaneh <orgads at gmail dot com> ---
test.cpp: In function ‘int f(int)’:
test.cpp:7:11: warning: ‘q’ is used uninitialized in this function
[-Wuninitialized]
    7 |   return *p;
      |           ^

Is this the intended description? It doesn't refer to the real problem (storing
a pointer to a variable that is out of scope).