[Bug c++/58950] New: [4.9 Regression] Missing "statement has no effect"

[Bug c++/58950] New: [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

            Bug ID: 58950
           Summary: [4.9 Regression] Missing "statement has no effect"
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: diagnostic
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: glisse at gcc dot gnu.org

void f(){
  int i __attribute__((vector_size(2*sizeof(int)))) = { 2, 3 };
  __builtin_shuffle (i, i);
}

g++-4.8:
x.c:3:3: warning: statement has no effect [-Wunused-value]

gcc-4.9:
x.c:3:3: warning: value computed is not used [-Wunused-value]

g++-4.9: nothing :-(
We do warn for i+i, but not for (i+i) (again this is missing only in C++).

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.9.0

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-11-19
     Ever confirmed|0                           |1

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

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #2 from Marc Glisse <glisse at gcc dot gnu.org> ---
For __builtin_shuffle, the issue is that we now call save_expr, which always
sets TREE_SIDE_EFFECTS to 1. I don't know if it would make sense to introduce a
maybe_save_expr that is equivalent to save_expr but does not set
TREE_SIDE_EFFECTS if its argument doesn't have it. But in any case I think we
still want to warn for an unused result in __builtin_shuffle(x,++m) so that's
not the solution. In C we also have the side_effects_flag but we still warn in
warn_if_unused_value (the default for unknown trees), whereas in C++ (near the
end of convert_to_void in cvt.c) only some tcc_comparison, tcc_unary and
tcc_binary can warn when they have side effects. It would be easy to add
VEC_PERM_EXPR to the list and get a "value computed is not used", I just don't
know if something more general is possible.

For (i+i), the PLUS_EXPR ends up with nowarning_flag = 1 somehow.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #4 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Manuel López-Ibáñez from comment #3)
> You could decorate __builtin_shuffle with attribute "warn_unused_result". It
> is not the same, but it probably more precise. It would also warn for:
> __builtin_shuffle(x,++m), while -Wunused-value would not.

Would it? The front-ends replace __builtin_shuffle with VEC_PERM_EXPR quite
early. I am not even sure we have a function type to which we could attach the
attribute.
>From gcc-bugs-return-435858-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Tue Nov 26 06:56:20 2013
Return-Path: <gcc-bugs-return-435858-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 30150 invoked by alias); 26 Nov 2013 06:56:20 -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 30087 invoked by uid 48); 26 Nov 2013 06:56:15 -0000
From: "kcc at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug sanitizer/59286] segfault in  __sanitizer::StackDepotGet
Date: Tue, 26 Nov 2013 06:56:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: sanitizer
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: kcc 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-59286-4-qMp6VIFAjh@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-59286-4@http.gcc.gnu.org/bugzilla/>
References: <bug-59286-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: 2013-11/txt/msg02635.txt.bz2
Content-length: 155

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

--- Comment #1 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
does this happen with clang's tsan?

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[ebotcazou at gcc dot gnu.org]

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

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

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> For __builtin_shuffle, the issue is that we now call save_expr, which always
> sets TREE_SIDE_EFFECTS to 1. I don't know if it would make sense to
> introduce a maybe_save_expr that is equivalent to save_expr but does not set
> TREE_SIDE_EFFECTS if its argument doesn't have it.

No, this would defeat the purpose of the SAVE_EXPR, since you could duplicate
the expression or move it at will, leading to nasty order of evaluation issues.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #6 from Marc Glisse <glisse at gcc dot gnu.org> ---
For:

void f(){
  int i = 2;
  (i+i);
}

we always set TREE_NOWARNING in finish_parenthesized_expr. The comment next to
it says this is to avoid warning in c_common_truthvalue_conversion. If that's
the only reason, the hammer seems a bit heavy, in C we only set the bit for
MODIFY_EXPR.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #7 from Marc Glisse <glisse at gcc dot gnu.org> ---
Created attachment 31342
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31342&action=edit
patch

The __builtin_shuffle part of the patch seems fine.

For (i+i), I first tried not setting TREE_NOWARNING on things that are not
MODIFY_EXPR (as in the C front-end), but apparently that's how the warning for
logical ops is disabled for a || (b && c), so that's not ok. The current patch
breaks g++.dg/ext/vla13.C (PR 54583), but nothing else covered by the
testsuite, so it is tempting to see if there are other ways to fix PR 54583.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #8 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #5)
> > For __builtin_shuffle, the issue is that we now call save_expr, which always
> > sets TREE_SIDE_EFFECTS to 1. I don't know if it would make sense to
> > introduce a maybe_save_expr that is equivalent to save_expr but does not set
> > TREE_SIDE_EFFECTS if its argument doesn't have it.
> 
> No, this would defeat the purpose of the SAVE_EXPR, since you could
> duplicate the expression or move it at will, leading to nasty order of
> evaluation issues.

What I meant was, there are many places in the front-end that do:

if (TREE_SIDE_EFFECTS (op0)) op0 = save_expr (op0);

and I'd like to replace it with

op0 = maybe_save_expr (op0);

that would do the same thing to begin with. Now, if op0 is a long expression
without side-effects, the code I quoted will not call save_expr, it will
duplicate the expression and count on CSE for dedup, whereas we could imagine
having a single version to start with, that optimizations would still be
allowed to duplicate as usual if they consider it worth it.

Of course the uses of SAVE_EXPR that require the stronger protection would be
left alone.

I may still not be making sense, but at least I think I was a little more
precise about what I meant ;-)

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[ebotcazou at gcc dot gnu.org]

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

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> What I meant was, there are many places in the front-end that do:
> 
> if (TREE_SIDE_EFFECTS (op0)) op0 = save_expr (op0);
> 
> and I'd like to replace it with
> 
> op0 = maybe_save_expr (op0);
> 
> that would do the same thing to begin with. Now, if op0 is a long expression
> without side-effects, the code I quoted will not call save_expr, it will
> duplicate the expression and count on CSE for dedup, whereas we could
> imagine having a single version to start with, that optimizations would
> still be allowed to duplicate as usual if they consider it worth it.

Yes, we do something like that in the Ada front-end, i.e. we call save_expr for
complex expressions without side-effects, see e.g. utils2.c:gnat_protect_expr.
But once the SAVE_EXPR is built, you *cannot* remove TREE_SIDE_EFFECTS on it,
or else you'll run into nasty order of evaluation issues.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

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

--- Comment #10 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #7)
> The __builtin_shuffle part of the patch seems fine.

Yes, that looks right.  That fixes the bug, yes?

The maybe_save_expr idea also makes sense to me, but doesn't seem necessary to
fix this.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #11 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Jason Merrill from comment #10)
> (In reply to Marc Glisse from comment #7)
> > The __builtin_shuffle part of the patch seems fine.
> 
> Yes, that looks right.  That fixes the bug, yes?

It fixes this half of the bug, yes. I'll regtest it separately.

[Bug c++/58950] [4.9 Regression] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #12 from Marc Glisse <glisse at gcc dot gnu.org> ---
Author: glisse
Date: Fri Jan  3 21:12:48 2014
New Revision: 206325

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

    PR c++/58950
gcc/cp/
    * cvt.c (convert_to_void): Handle VEC_PERM_EXPR and VEC_COND_EXPR.
gcc/testsuite/
    * g++.dg/pr58950.C: New file.

Added:
    trunk/gcc/testsuite/g++.dg/pr58950.C   (with props)
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cvt.c
    trunk/gcc/testsuite/ChangeLog

Propchange: trunk/gcc/testsuite/g++.dg/pr58950.C
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/g++.dg/pr58950.C
            ('svn:keywords' added)

[Bug c++/58950] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

Marc Glisse <glisse at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[4.9 Regression] Missing    |Missing "statement has no
                   |"statement has no effect"   |effect"

--- Comment #13 from Marc Glisse <glisse at gcc dot gnu.org> ---
Testcase for the remaining part of the PR:

void f(){
  int i=2;
  (i+i); // should warn with -Wall
}

[Bug c++/58950] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

Marc Glisse <glisse at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |paolo.carlini at oracle dot com

--- Comment #14 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #7)
> The current
> patch breaks g++.dg/ext/vla13.C (PR 54583), but nothing else covered by the
> testsuite, so it is tempting to see if there are other ways to fix PR 54583.

Paolo, do you have an opinion on this PR?

[Bug c++/58950] Missing "statement has no effect"

[paolo.carlini at oracle dot com]

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

--- Comment #15 from Paolo Carlini <paolo.carlini at oracle dot com> ---
I don't think you simply want a better fix for 54583, because for the testcase
in #Comment 13 the new conditional setting TREE_NO_WARNING isn't used.
Otherwise, I think it would be easy to tighten it via array_of_runtime_bound_p.

[Bug c++/58950] Missing "statement has no effect"

[glisse at gcc dot gnu.org]

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

--- Comment #16 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Paolo Carlini from comment #15)
> I don't think you simply want a better fix for 54583, because for the
> testcase in #Comment 13 the new conditional setting TREE_NO_WARNING isn't
> used. Otherwise, I think it would be easy to tighten it via
> array_of_runtime_bound_p.

The issue isn't with setting the bit but reading it. If you look at the patch,
I remove a test for TREE_NO_WARNING (expr). This breaks 54583 because the
TREE_NO_WARNING bit is then ignored.

[Bug c++/58950] Missing "statement has no effect"

[paolo.carlini at oracle dot com]

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

--- Comment #17 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Yes, I know that. What I'm saying is that other code may want to see that
TREE_NO_WARNING honored, the issue doesn't have much to do with 54583 per se.
In my personal opinion removing a TREE_NO_WARNING check is in general a pretty
risky thing to do, because unfortunately we have only that generic bit and we
use it in many different circumstances.

[Bug c++/58950] Missing "statement has no effect"

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.0                       |4.9.1

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

[Bug c++/58950] Missing "statement has no effect"

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.1                       |4.9.2

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

[Bug c++/58950] Missing "statement has no effect"

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.2                       |4.9.3

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

[Bug c++/58950] Missing "statement has no effect"

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

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

--- Comment #21 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Should this be P1?

[Bug c++/58950] Missing "statement has no effect"

[jakub at gcc dot gnu.org]

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

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

[Bug c++/58950] Missing "statement has no effect"

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.3                       |4.9.4

[Bug c++/58950] Missing "statement has no effect"

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.4                       |---

[Bug c++/58950] Missing "statement has no effect"

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection

--- Comment #25 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #6)
> For:
> 
> void f(){
>   int i = 2;
>   (i+i);
> }

This case is fixed on the trunk:

<source>: In function 'void f1()':
<source>:9:5: warning: statement has no effect [-Wunused-value]
    9 |   (i+i);
      |   ~~^~~

[Bug c++/58950] Missing "statement has no effect"

[pinskia at gcc dot gnu.org]

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

--- Comment #26 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #25)
> (In reply to Marc Glisse from comment #6)
> > For:
> > 
> > void f(){
> >   int i = 2;
> >   (i+i);
> > }
> 
> This case is fixed on the trunk:
> 
> <source>: In function 'void f1()':
> <source>:9:5: warning: statement has no effect [-Wunused-value]
>     9 |   (i+i);
>       |   ~~^~~

I think it was fixed via r12-1804-g65870e75616ee4359d1c13b99 .