[Bug c++/65786] New: Wrong code when using decltype to specify the return type

[Bug c++/65786] New: Wrong code when using decltype to specify the return type

[josopait at goopax dot com]

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

            Bug ID: 65786
           Summary: Wrong code when using decltype to specify the return
                    type
           Product: gcc
           Version: 4.9.2
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: josopait at goopax dot com

In the program below, the assignment of the return value is messed up. While
the C++-14 style of fully automatic type deduction works fine, the mymax11
function call produces some random numbers.

The output is:

1
2
-2100190336

or similar. The last line is different for every program execution.
I am using gcc 4.9.2 on x86_64 Linux. I don't get this bug if I use -m32.




#include <iostream>
using namespace std;


struct testclass
{
  int data;
  inline operator const int&() const
  {
    return data; 
  }

  testclass& operator = (const int& in)
  {
    data = in;
    return *this;
  }
};


template <typename A, typename B> auto mymax14(const A& a, const B& b)
{
  return std::max((int)a, (int)b);
}

template <typename A, typename B> auto mymax11(const A& a, const B& b) ->
decltype(std::max((int)a, (int)b))
{
  return std::max((int)a, (int)b);
}



int main()
{

  testclass d;

  d = 1;
  cout << d.data << endl;   // ok, d.data==1

  d = mymax14(d, 2);
  cout << d.data << endl;   // ok, d.data==2

  d = mymax11(d, 2);
  cout << d.data << endl;   // bad: d.data == some random number.

  return 0;
}

[Bug c++/65786] Wrong code when using decltype to specify the return type

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|critical                    |normal

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think this is undefined code as you are std::max returns a reference so you
are returning a reference to a temp variable which went out of scope.

[Bug c++/65786] Wrong code when using decltype to specify the return type

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Undefined behaviour, for the reason Andrew gave.

(People seem to like blaming new C++11 or C++14 features for their mistakes
when using std::max incorrectly, Bug 61769 is another example.)

[Bug c++/65786] Wrong code when using decltype to specify the return type

[josopait at goopax dot com]

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

--- Comment #3 from Ingo Josopait <josopait at goopax dot com> ---
Yes, you are right. Thanks.

[Bug c++/65786] Wrong code when using decltype to specify the return type

[glisse at gcc dot gnu.org]

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

--- Comment #4 from Marc Glisse <glisse at gcc dot gnu.org> ---
Compiling with -Wall -O prints:

w.cc: In function ‘int main()’:
w.cc:45:13: warning: ‘<anonymous>’ is used uninitialized in this function
[-Wuninitialized]
   cout << d.data << endl;   // bad: d.data == some random number.
             ^
>From gcc-bugs-return-483770-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Apr 16 10:10:20 2015
Return-Path: <gcc-bugs-return-483770-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 3465 invoked by alias); 16 Apr 2015 10:10: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 3376 invoked by uid 48); 16 Apr 2015 10:10:15 -0000
From: "mwahab at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug target/65697] __atomic memory barriers not strong enough for __sync builtins
Date: Thu, 16 Apr 2015 10:10:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: target
X-Bugzilla-Version: 5.0
X-Bugzilla-Keywords: wrong-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: mwahab at gcc dot gnu.org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: mwahab at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-65697-4-vwLYcpiGUo@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-65697-4@http.gcc.gnu.org/bugzilla/>
References: <bug-65697-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: 2015-04/txt/msg01322.txt.bz2
Content-length: 3957

https://gcc.gnu.org/bugzilla/show_bug.cgi?ide697

--- Comment #30 from mwahab at gcc dot gnu.org ---
(In reply to James Greenhalgh from comment #28)

> Which leaves 3). From Andrew's two proposed solutions:
>
> > a) introduce an additional memory model... MEMMODEL_SYNC or something
> > which is even stronger than SEQ_CST.  This would involve fixing any places
> > that assume SEQ_CST is the highest.  And then we use this from the places
> > that expand sync calls as the memory model.
>
> This seems a sensible approach and leads to a nicer design, but I worry that
> it might be overkill for primitives which we ultimately want to reduce
> support for and eventually deprecate.

I don't understand why it would be overkill. Its already expected that not all
barriers will be meaningful for all targets and target code to handle redundant
barriers usually comes down to a few clauses in a conditional statement.

> > (b) may be easier to implement, but puts more onus on the target..
> > probably involves more complex patterns since you need both atomic and
> > sync patterns. My guess is some duplication of code will occur here.  But
> > the impact is only to specific targets.
>
> When I looked at this problem internally a few weeks back, this is exactly
> how I expected things to work (we can add the documentation for the pattern
> names to the list of things which need fixing, as it took me a while to
> figure out why my new patterns were never expanded!).
>
> I don't think this is particularly onerous for a target. The tough part in
> all of this is figuring out the minimal cost instructions at the ISA level
> to use for the various __atomic primitives. Extending support to a stronger
> model, should be straightforward given explicit documentation of the
> stronger ordering requirements.
>

My objection to using sync patterns is that it does take more work, both for
the initial implementation and for continuing maintenance. It also means adding
sync patterns to targets that would not otherwise need them and preserving a
part of the gcc infrastructure that is only needed for a legacy feature and
could otherwise be targeted for removal.

> Certainly, a target would have to do the same legwork for a) regardless, and
> would have to pollute their atomic patterns with checks and code paths for
> MEMMODEL_SYNC.

Actually, the changes needed for (a) are very simple. The checks and code-paths
for handling barriers are already there, reusing them for MEMMODEL_SYNC is
trivial. The resulting code is much easier to understand, and safely fix,
because it is all in the same place, than if it was spread out across patterns
and functions.

> This also gives us an easier route to fixing any issues with the
> acquire/release __sync primitives (__sync_lock_test_and_set and
> __sync_lock_release) if we decide that these also need to be stronger than
> their C++11 equivalents.

This seems like a chunky workaround to avoid having to add a barrier
representation to gcc.  It's a, not necessarily straightforward, reworking of
the middle-end to use patterns that would need to be added to architectures
that don't currently have them and at least checked in the architectures that
do.

This seems to come down to what enum memmodel is supposed to be for. If it is
intended to represent the barriers provided by C11/C+11 atomics and nothing
else than a workaround seems unavoidable. If it is meant to represent barriers
needed by gcc to compile user code than, it seems to me, that it would be
better to just add the barrier to the enum and update code where necessary.

Since memmodel is relatively recent, was merged from what looks like the C++
memory model branch (cxx-mem-model), and doesn't seem to have changed since
then, it's maybe not surprising that it doesn't already include every thing
needed by gcc. I don't see that adding to it should be prohibited, provided the
additions can be show to be strictly required.