[Bug libstdc++/54388] New: std::array.at() const results in undefined behaviour

[Bug libstdc++/54388] New: std::array.at() const results in undefined behaviour

[lukeallardyce at yahoo dot co.uk]

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

             Bug #: 54388
           Summary: std::array.at() const results in undefined behaviour
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: lukeallardyce@yahoo.co.uk


array line 166:

#ifdef __EXCEPTIONS
      constexpr const_reference
      at(size_type __n) const
      {
    return __n < _Nm ? 
           _M_instance[__n] : throw out_of_range(__N("array::at"));
      }
#else
      const_reference
      at(size_type __n) const
      {
    return __n < _Nm ?
           _M_instance[__n] : __throw_out_of_range(__N("array::at"));
      }
#endif

Refer to

http://stackoverflow.com/questions/12136394/issue-with-bounds-checking-a-member-stdarray-inside-a-const-function

A conditional with a throw operand evaluates to a prvalue, which expires at the
end of the return statement.

[Bug libstdc++/54388] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-28
     Ever Confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-28 08:19:25 UTC ---
Confirmed

#include <array>
#include <cassert>

struct A
{
  bool valid = true;
  ~A() { valid = false; }
};

int main()
{
  const std::array<A, 1> a;
  const A& aa = a.at(0);
  assert(aa.valid);
}

[Bug libstdc++/54388] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-28 08:21:21 UTC ---
The fix is to remove the constexpr from array::at, which isn't required by the
standard anyway:

diff --git a/libstdc++-v3/include/std/array b/libstdc++-v3/include/std/array
index 58263ce..88595c0 100644
--- a/libstdc++-v3/include/std/array
+++ b/libstdc++-v3/include/std/array
@@ -164,14 +164,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        return _M_instance[__n];
       }

-#ifdef __EXCEPTIONS
-      constexpr const_reference
-      at(size_type __n) const
-      {
-       return __n < _Nm ? 
-              _M_instance[__n] : throw out_of_range(__N("array::at"));
-      }
-#else
       const_reference
       at(size_type __n) const
       {
@@ -179,7 +171,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
          std::__throw_out_of_range(__N("array::at"));
        return _M_instance[__n];
       }
-#endif

       reference 
       front()

[Bug libstdc++/54388] std::array.at() const results in undefined behaviour

[daniel.kruegler at googlemail dot com]

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

--- Comment #3 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-08-28 13:43:42 UTC ---
(In reply to comment #2)
> The fix is to remove the constexpr from array::at, which isn't required by the
> standard anyway:

It's not required, but I would like to encourage you to keep it if possible,
because otherwise it would like as if the constexpr extension paper 

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3304.html

would have no implementation experience. Here is my suggested replacement
instead (based on comma operator semantics):

Replace

    return __n < _Nm ? 
           _M_instance[__n] : throw out_of_range(__N("array::at"));

by

    return __n < _Nm ? 0 : throw out_of_range(__N("array::at")),  
_M_instance[__n];

This should work, because the right argument of comma operator determines type
and value category

[Bug libstdc++/54388] std::array.at() const results in undefined behaviour

[daniel.kruegler at googlemail dot com]

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

--- Comment #4 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-08-28 13:53:18 UTC ---
(In reply to comment #3)
> (In reply to comment #2)
> > The fix is to remove the constexpr from array::at, which isn't required by the
> > standard anyway:
> 
> It's not required, but I would like to encourage you to keep it if possible,
> because otherwise it would like as if the constexpr extension paper 

Please read that as: "it would *look* like"

> Replace
> 
>     return __n < _Nm ? 
>            _M_instance[__n] : throw out_of_range(__N("array::at"));
> 
> by
> 
>     return __n < _Nm ? 0 : throw out_of_range(__N("array::at")),  
> _M_instance[__n];

Alternatively prevent any value evaluation at all:

return __n < _Nm ? void() : throw out_of_range(__N("array::at")),  
 _M_instance[__n];

[Bug libstdc++/54388] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-28 13:55:36 UTC ---
The paper does have implementation experience, and experience shows we got it
wrong ;)

We could do:

  return __n < _Nm ? _M_instance[__n]
         : (std::__throw out_of_range(__N("array::at")), _M_instance[0]);

and still get rid of the code that's conditionally compiled on __EXCEPTIONS.

N.B. the second branch uses _M_instance[0] to avoid possible warnings about
out-of-bounds accesses.

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[daniel.kruegler at googlemail dot com]

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

--- Comment #6 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-08-28 14:07:11 UTC ---
(In reply to comment #5)
> The paper does have implementation experience, and experience shows we got it
> wrong ;)

We implemented it wrong is a different message than "we couldn't implement it"

> We could do:
> 
>   return __n < _Nm ? _M_instance[__n]
>          : (std::__throw out_of_range(__N("array::at")), _M_instance[0]);
> 
> and still get rid of the code that's conditionally compiled on __EXCEPTIONS.
> 
> N.B. the second branch uses _M_instance[0] to avoid possible warnings about
> out-of-bounds accesses.

Yes, this should also work (I noticed that gcc gets an ICE in any of my
suggested forms. I will investigate this separately)

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[bkoz at gcc dot gnu.org]

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

Benjamin Kosnik <bkoz at gcc dot gnu.org> changed:

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

--- Comment #7 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-08-28 17:52:45 UTC ---

I like #5 Jonathan, thank you. 

It's ok if the initial implementation got things wrong and we have to correct
them later. No sweat. We can just update the paper anyway.

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[daniel.kruegler at googlemail dot com]

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

--- Comment #8 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-08-28 20:34:34 UTC ---
(In reply to comment #4)
> > Replace
> > 
> >     return __n < _Nm ? 
> >            _M_instance[__n] : throw out_of_range(__N("array::at"));
> > 
> > by
> > 
> >     return __n < _Nm ? 0 : throw out_of_range(__N("array::at")),  
> > _M_instance[__n];
> 
> Alternatively prevent any value evaluation at all:
> 
> return __n < _Nm ? void() : throw out_of_range(__N("array::at")),  
>  _M_instance[__n];

Upon 2nd reflection I see now that neither of my proposals should work, so
Jonathan's solution is the only correct one presented here. While looking at
5.19 p2 I checked all bullets and couldn't find a problem - until finally I saw
that the initial wording of p2 says "A conditional-expression is a core
constant expression". Well yes, a comma operator does not satisfy this. Thanks
Jonathan for your correct form - I hope we can get this in as replacement for
the current implementation.

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[bkoz at gcc dot gnu.org]

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

--- Comment #9 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-08-30 15:36:18 UTC ---

BTW, we definitely need a comment on  why this particular code is so tricky. 

// NB: Interesting use of comma operator semantics.

at the very least...

;)

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[fweimer at redhat dot com]

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

--- Comment #10 from Florian Weimer <fweimer at redhat dot com> 2012-08-30 15:39:21 UTC ---
(In reply to comment #9)
> BTW, we definitely need a comment on  why this particular code is so tricky. 
> 
> // NB: Interesting use of comma operator semantics.
> 
> at the very least...
> 
> ;)

Can the compiler warn about the original (buggy) code?

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.7.2

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[paolo.carlini at oracle dot com]

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

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely.gcc at gmail dot
                   |                            |com

--- Comment #11 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-09-07 09:24:21 UTC ---
Jon, can you handle this?

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|jwakely.gcc at gmail dot    |
                   |com                         |
         AssignedTo|unassigned at gcc dot       |redi at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-09-07 11:35:31 UTC ---
mine

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-09-09 17:56:15 UTC ---
(In reply to comment #10)
> Can the compiler warn about the original (buggy) code?

It already does:

c.cc: In function ‘A& get(int)’:
c.cc:3:43: error: invalid initialization of non-const reference of type ‘A&’
from an rvalue of type ‘A’
 A& get(int i) { return i == 0 ? a : throw 1; }
                                           ^
But warnings are suppressed in system headers.

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-09-09 17:56:15 UTC ---
(In reply to comment #10)
> Can the compiler warn about the original (buggy) code?

It already does:

c.cc: In function ‘A& get(int)’:
c.cc:3:43: error: invalid initialization of non-const reference of type ‘A&’
from an rvalue of type ‘A’
 A& get(int i) { return i == 0 ? a : throw 1; }
                                           ^
But warnings are suppressed in system headers.

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-09-09 17:56:56 UTC ---
Author: redi
Date: Sun Sep  9 17:56:51 2012
New Revision: 191114

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191114
Log:
    PR libstdc++/54388
    * include/std/array (array::at() const): Ensure lvalue result.
    * testsuite/23_containers/array/element_access/54388.cc: New.
    * testsuite/23_containers/array/tuple_interface/get_neg.cc: Adjust
    dg-error line numbers.
    * testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc:
    Likewise.

Added:
    trunk/libstdc++-v3/testsuite/23_containers/array/element_access/54388.cc
      - copied, changed from r191113,
trunk/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/array
    trunk/libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc
   
trunk/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

--- Comment #15 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-09-09 18:40:50 UTC ---
Author: redi
Date: Sun Sep  9 18:40:46 2012
New Revision: 191117

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191117
Log:
    PR libstdc++/54388
    * include/std/array (array::at() const): Ensure lvalue result.
    * testsuite/23_containers/array/element_access/54388.cc: New.

Added:
   
branches/gcc-4_7-branch/libstdc++-v3/testsuite/23_containers/array/element_access/54388.cc
Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/include/std/array

[Bug libstdc++/54388] [4.7/4.8 Regression] std::array.at() const results in undefined behaviour

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-09-09 18:44:59 UTC ---
fixed