[optional] get() misses optional r-value overload in contrast to operator* and value()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Both *value()* and *operator** has an overload to handle if the
*optional<T>* object is an r-value. However *get()* does not have the
r-value overload.

This leads to undefined behavior using *get()*, but not with *operator**
 and *value()*, in the following example:

auto func0() {
  return optional<int>{3};
}
auto func1() {
  const auto& x = func0().get(); // Undefined behavior
  const auto& y = func0().value(); // Correct
  const auto& z = *func0(); // Correct
}

Is this intended, I can't see any reason why the r-value overload of .get()
would be missing?
Otherwise, the following overload of .get() should be added:

reference_type_of_temporary_wrapper get() && {
  BOOST_ASSERT(this->is_initialized());
  return boost::move(this->get_impl());
}


/Viktor Sehr, Software Developer at Toppluva

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Hi Viktor,
Thank you for reporting this. This is an omission (a bug) that I will need
to fix. I have created an issue in GitHub to track it:
https://github.com/boostorg/optional/issues/51.

On the other hand, I do not think that the bug is causing an UB any more
than function `value()` in your example. Unless something has changed in
the way temporaries' life is prolonged in C++17. I tested it for `int`s and
all tests work fine. I tested it on `unique_ptr<int>`:

```
#include <memory>
#include <iostream>
#include <boost/optional.hpp>

using boost::optional;

optional<std::unique_ptr<int>> func() {
  return boost::make_optional(std::unique_ptr<int>(new int{3}));
}

int main()
{
  auto const& p = func().value();
  std::cout << (bool)p << std::endl; // outputs: 0
}
```

And even when I use `value()` I do not get the intuitive result. I would
recommend catching the result by value rather than by reference, and all
surprises will be gone, including the one from your example.

Regards,
&rzej;

2018-03-23 2:04 GMT+01:00 Viktor Sehr via Boost <[hidden email]>:

> Both *value()* and *operator** has an overload to handle if the
> *optional<T>* object is an r-value. However *get()* does not have the
> r-value overload.
>
> This leads to undefined behavior using *get()*, but not with *operator**
>  and *value()*, in the following example:
>
> auto func0() {
>   return optional<int>{3};
> }
> auto func1() {
>   const auto& x = func0().get(); // Undefined behavior
>   const auto& y = func0().value(); // Correct
>   const auto& z = *func0(); // Correct
> }
>
> Is this intended, I can't see any reason why the r-value overload of .get()
> would be missing?
> Otherwise, the following overload of .get() should be added:
>
> reference_type_of_temporary_wrapper get() && {
>   BOOST_ASSERT(this->is_initialized());
>   return boost::move(this->get_impl());
> }
>
>
> /Viktor Sehr, Software Developer at Toppluva
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/
> mailman/listinfo.cgi/boost
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

[optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Hi Andrzej, thanks for your response.

I do not get the same result as you using latest visual studio (as for
today 15.6.4 with std=latest), I'm not sure on what would be the correct
behavior but to me it seems erroneous that the unique_ptr has lost it's
value. Note that using std::optional yields the same result as below.

auto func0() {
  return boost::make_optional(std::make_unique<int>(42));
}
const auto& x = func0().value();
assert(x); // true
assert(*x = 42); // true

I agree that declaring x as a reference rather than a value is sketchy,
this is just how I happened to find the bug, I suppose there are other
cases which might be errorous.
While on the subject I think that boost::optional should add the member
function has_value() in addition to is_initialized() to make it
syntactically compatible with std::optional.

best regards/Viktor


On Fri, Mar 23, 2018 at 3:36 AM Andrzej Krzemienski <[hidden email]>
wrote:

> Hi Viktor,
> Thank you for reporting this. This is an omission (a bug) that I will need
> to fix. I have created an issue in GitHub to track it:
> https://github.com/boostorg/optional/issues/51.
>
> On the other hand, I do not think that the bug is causing an UB any more
> than function `value()` in your example. Unless something has changed in
> the way temporaries' life is prolonged in C++17. I tested it for `int`s and
> all tests work fine. I tested it on `unique_ptr<int>`:
>
> ```
> #include <memory>
> #include <iostream>
> #include <boost/optional.hpp>
>
> using boost::optional;
>
> optional<std::unique_ptr<int>> func() {
>   return boost::make_optional(std::unique_ptr<int>(new int{3}));
> }
>
> int main()
> {
>   auto const& p = func().value();
>   std::cout << (bool)p << std::endl; // outputs: 0
> }
> ```
>
> And even when I use `value()` I do not get the intuitive result. I would
> recommend catching the result by value rather than by reference, and all
> surprises will be gone, including the one from your example.
>
> Regards,
> &rzej;
>
> 2018-03-23 2:04 GMT+01:00 Viktor Sehr via Boost <[hidden email]>:
>
>> Both *value()* and *operator** has an overload to handle if the
>> *optional<T>* object is an r-value. However *get()* does not have the
>> r-value overload.
>>
>> This leads to undefined behavior using *get()*, but not with *operator**
>>  and *value()*, in the following example:
>
>
>>
>> auto func0() {
>>   return optional<int>{3};
>> }
>> auto func1() {
>>   const auto& x = func0().get(); // Undefined behavior
>>   const auto& y = func0().value(); // Correct
>>   const auto& z = *func0(); // Correct
>> }
>>
>> Is this intended, I can't see any reason why the r-value overload of
>> .get()
>> would be missing?
>> Otherwise, the following overload of .get() should be added:
>>
>> reference_type_of_temporary_wrapper get() && {
>>   BOOST_ASSERT(this->is_initialized());
>>   return boost::move(this->get_impl());
>> }
>>
>>
>> /Viktor Sehr, Software Developer at Toppluva
>>
>> _______________________________________________
>> Unsubscribe & other changes:
>> http://lists.boost.org/mailman/listinfo.cgi/boost
>>
>
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
2018-03-23 16:32 GMT+01:00 Viktor Sehr <[hidden email]>:

> Hi Andrzej, thanks for your response.
>
> I do not get the same result as you using latest visual studio (as for
> today 15.6.4 with std=latest), I'm not sure on what would be the correct
> behavior but to me it seems erroneous that the unique_ptr has lost it's
> value. Note that using std::optional yields the same result as below.
>
> auto func0() {
>   return boost::make_optional(std::make_unique<int>(42));
> }
> const auto& x = func0().value();
> assert(x); // true
> assert(*x = 42); // true
>

I am convinced that you are hitting an UB. Temporary returned by func0() is
destroyed in the first line. After that the reference is dangling. One of
the consequences of UB may be the program doing exactly what you expect in
your compiler/platform/version.


>
> I agree that declaring x as a reference rather than a value is sketchy,
> this is just how I happened to find the bug, I suppose there are other
> cases which might be errorous.
> While on the subject I think that boost::optional should add the member
> function has_value() in addition to is_initialized() to make it
> syntactically compatible with std::optional.
>

Thanks. Recorded: https://github.com/boostorg/optional/issues/52

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Andrzej Krzemienski wrote:

> > const auto& x = func0().value();
...
> I am convinced that you are hitting an UB. Temporary returned by func0()
> is destroyed in the first line.

Interesting question. value() && returns T&&. The auto const& reference
can't bind directly to T&&, because that's not an lvalue. So a temporary
value of type T is created, and the auto const& binds to that, extending its
life. That's of course assuming I read http://eel.is/c++draft/dcl.init.ref 
correctly.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
On Fri, Mar 23, 2018 at 12:01 PM, Peter Dimov via Boost
<[hidden email]> wrote:

> Andrzej Krzemienski wrote:
>
>> > const auto& x = func0().value();
>
> ...
>>
>> I am convinced that you are hitting an UB. Temporary returned by func0()
>> is destroyed in the first line.
>
>
> Interesting question. value() && returns T&&. The auto const& reference
> can't bind directly to T&&, because that's not an lvalue.

An lvalue-reference-to-const can bind to an rvalue (it's why you can
pass an rvalue to a function that takes a "const T&").

--
-Matt Calabrese

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Matt Calabrese wrote:
> On Fri, Mar 23, 2018 at 12:01 PM, Peter Dimov via Boost
> <[hidden email]> wrote:
> > Interesting question. value() && returns T&&. The auto const& reference
> > can't bind directly to T&&, because that's not an lvalue.
>
> An lvalue-reference-to-const can bind to an rvalue (it's why you can pass
> an rvalue to a function that takes a "const T&").

You're right, it does bind directly.
http://eel.is/c++draft/dcl.init.ref#5.3.1 


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Clang (6.0) emits "ud2" instructions for both auto&& and const auto&. If
the const auto& can bind directly to an r-value, this must mean Clang is
wrong, or am I missing something?

GCC on the other hand emits code for both functions, although with slight
difference (I'm not skilled enough in assembler to analyze the GCC output
in further detail)

Code example: https://godbolt.org/g/7fjszE <https://godbolt.org/g/uNJoZB>

/Viktor

On Fri, Mar 23, 2018 at 1:06 PM Peter Dimov via Boost <[hidden email]>
wrote:

> Matt Calabrese wrote:
> > On Fri, Mar 23, 2018 at 12:01 PM, Peter Dimov via Boost
> > <[hidden email]> wrote:
> > > Interesting question. value() && returns T&&. The auto const& reference
> > > can't bind directly to T&&, because that's not an lvalue.
> >
> > An lvalue-reference-to-const can bind to an rvalue (it's why you can pass
> > an rvalue to a function that takes a "const T&").
>
> You're right, it does bind directly.
> http://eel.is/c++draft/dcl.init.ref#5.3.1
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Viktor Sehr wrote:

> Clang (6.0) emits "ud2" instructions for both auto&& and const auto&. If
> the const auto& can bind directly to an r-value, this must mean Clang is
> wrong, or am I missing something?

"ud2" is Clang's way of encoding undefined behavior. This is an invalid
instruction that crashes. Both auto&& and auto const& do the same thing -
bind to the rvalue reference returned by value() &&.

> GCC on the other hand emits code for both functions, although with slight
> difference (I'm not skilled enough in assembler to analyze the GCC output
> in further detail)
>
> Code example: https://godbolt.org/g/7fjszE <https://godbolt.org/g/uNJoZB>

GCC crashes too, in another creative way. It returns *p, where the pointer p
is [rsp+8], which is NULL. If you change the functions to

int * func1() {
    const auto& x = func0().value();
    return &*x;
}

you'll see that Clang returns NULL and GCC returns the value in [rsp+8]
which is also NULL.

So both compilers deliberately dereference a NULL pointer because they see
that the reference `x` is no longer valid.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Thanks for the answer, but just so I understand, you said a const-reference
binds directly to a r-value as when passing an r-value to a const reference
argument. Doesn't this mean that both Clang and GCC compiles this
erroneously?

To my understanding, the compiler roughly emits the snippet (2) of the
following possible snippets? (Where the variables named temp_ correspond to
temporaries only existing inside the expression)

1. Defined behavior
auto temp_optional = boost::make_optional(std::make_unique<int>(42));
auto temp_uptr = std::move(temp_optional.value());
const auto& x = temp_uptr;
~temp_optional();
// temp_uptr is alive

2. Undefined behavior
auto temp_optional = boost::make_optional(std::make_unique<int>(42));
auto temp_uptr = std::move(temp_optional.value());
const auto& x = temp_optional.value(); // Whereas value is empty
~temp_uptr()
~temp_optional();




br /Viktor



On Fri, Mar 23, 2018 at 6:21 PM Peter Dimov via Boost <[hidden email]>
wrote:

> Viktor Sehr wrote:
>
> > Clang (6.0) emits "ud2" instructions for both auto&& and const auto&. If
> > the const auto& can bind directly to an r-value, this must mean Clang is
> > wrong, or am I missing something?
>
> "ud2" is Clang's way of encoding undefined behavior. This is an invalid
> instruction that crashes. Both auto&& and auto const& do the same thing -
> bind to the rvalue reference returned by value() &&.
>
> > GCC on the other hand emits code for both functions, although with slight
> > difference (I'm not skilled enough in assembler to analyze the GCC output
> > in further detail)
> >
> > Code example: https://godbolt.org/g/7fjszE <https://godbolt.org/g/uNJoZB
> >
>
> GCC crashes too, in another creative way. It returns *p, where the pointer
> p
> is [rsp+8], which is NULL. If you change the functions to
>
> int * func1() {
>     const auto& x = func0().value();
>     return &*x;
> }
>
> you'll see that Clang returns NULL and GCC returns the value in [rsp+8]
> which is also NULL.
>
> So both compilers deliberately dereference a NULL pointer because they see
> that the reference `x` is no longer valid.
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
Viktor Sehr wrote:

> Thanks for the answer, but just so I understand, you said a
> const-reference binds directly to a r-value as when passing an r-value to
> a const reference argument. Doesn't this mean that both Clang and GCC
> compiles this erroneously?

No, both are correct; optional::value()&& returns an rvalue reference to the
value inside the optional, to which the auto&& or auto const& binds. When
the optional temporary is destroyed, this reference becomes dangling.

> auto temp_uptr = std::move(temp_optional.value());

There is no temp_uptr. For there to have been, value()&& would have needed
to return T, not T&&. But it doesn't.

It's roughly

template<class T> class optional
{
    T t_;

publlic:

    T&& value()&& { return std::move(t_); }
};

so in

auto temp_optional = boost::make_optional(std::make_unique<int>(42));
const auto& x = temp_optional.value();

x binds to temp_optional.t_.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
On 25/03/2018 13:50, Peter Dimov wrote:
> No, both are correct; optional::value()&& returns an rvalue reference to
> the value inside the optional, to which the auto&& or auto const& binds.
> When the optional temporary is destroyed, this reference becomes dangling.

Doesn't this problem go away if value()&& is removed entirely, or at
least changed to return by value instead?

Granted I haven't done much delving into the darker corners of C++11,
but I've usually found that outside of implementing std::move itself,
anything that returns a T&& is probably a bug waiting to happen.

>     T&& value()&& { return std::move(t_); }

In this case, this should be just as efficient:

     T value()&& { return std::move(t_); }

Especially with improved elision guarantees in more recent standards and
compilers.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
On Sun, Mar 25, 2018, 18:12 Gavin Lambert via Boost <[hidden email]>
wrote:

> On 25/03/2018 13:50, Peter Dimov wrote:
> > No, both are correct; optional::value()&& returns an rvalue reference to
> > the value inside the optional, to which the auto&& or auto const& binds.
> > When the optional temporary is destroyed, this reference becomes
> dangling.
>
> Doesn't this problem go away if value()&& is removed entirely, or at
> least changed to return by value instead?
>
> Granted I haven't done much delving into the darker corners of C++11,
> but I've usually found that outside of implementing std::move itself,
> anything that returns a T&& is probably a bug waiting to happen.
>
> >     T&& value()&& { return std::move(t_); }
>
> In this case, this should be just as efficient:
>
>      T value()&& { return std::move(t_); }
>
> Especially with improved elision guarantees in more recent standards and
> compilers.
>

I do not recommend doing that. It has different semantics from the other
overloads and can be a pessimization for certain types. I think the correct
answer here, as harsh as it sounds, is "use the API correctly". I do not
see a defect here.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Gavin Lambert wrote:
> On 25/03/2018 13:50, Peter Dimov wrote:
> > No, both are correct; optional::value()&& returns an rvalue reference to
> > the value inside the optional, to which the auto&& or auto const& binds.
> > When the optional temporary is destroyed, this reference becomes
> > dangling.
>
> Doesn't this problem go away if value()&& is removed entirely,

No...

> or at least changed to return by value instead?

Yes.

> Granted I haven't done much delving into the darker corners of C++11, but
> I've usually found that outside of implementing std::move itself, anything
> that returns a T&& is probably a bug waiting to happen.
>
> >     T&& value()&& { return std::move(t_); }
>
> In this case, this should be just as efficient:
>
>      T value()&& { return std::move(t_); }

I consider it an open question whether T or T&& is "more correct" here.
Return by value is less efficient (one extra move) if you pass the result to
a function taking T&&, and it has the downside of actually moving when you
call std::move(opt).value() without doing anything with the result. But the
lifetime problems are a bit nasty.

Ideally if something like

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0936r0.pdf

goes through the compiler would know to extend the lifetime of the optional
temporary.

Or perhaps we could declare defeat, go back to Cfront days and extend all
temporaries instead. :-)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2018-03-26 0:12 GMT+02:00 Gavin Lambert via Boost <[hidden email]>:

> On 25/03/2018 13:50, Peter Dimov wrote:
>
>> No, both are correct; optional::value()&& returns an rvalue reference to
>> the value inside the optional, to which the auto&& or auto const& binds.
>> When the optional temporary is destroyed, this reference becomes dangling.
>>
>
> Doesn't this problem go away if value()&& is removed entirely, or at least
> changed to return by value instead?
>
> Granted I haven't done much delving into the darker corners of C++11, but
> I've usually found that outside of implementing std::move itself, anything
> that returns a T&& is probably a bug waiting to happen.
>
>     T&& value()&& { return std::move(t_); }
>>
>
> In this case, this should be just as efficient:
>
>     T value()&& { return std::move(t_); }
>
> Especially with improved elision guarantees in more recent standards and
> compilers.


However, note that the only problem it would solve is the problem that does
not exist when you declare objects rather than references in your scope (in
cases you are not interested in object's identity):

```
auto p = func().value();
```

And everything is correct, shorter, an reflects you intentions more clearly
("I am interested in value, not in address").

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] get() misses optional r-value overload in contrast to operator* and value()

Boost - Dev mailing list
>
>
> ("I am interested in value, not in address").
>
>

Hi,
I'm going a little bit off-topic here, but another possible idealistic
interpretations could be as following:
As a const reference extends the lifetime of an object returned by value,
I'd like to (once again ideally) interpret
*const auto&* as "give me the value as effective as possible", and *const
auto *as "I require the value to be a copy".

That trail of thought is what got me here in the beginning, and
unfortunately reality does not match :/

/Viktor

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost