ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
704 stars 62 forks source link

TRY/TRYX has dangling reference if xvalues are emitted from tried expression #244

Closed vasama closed 3 years ago

vasama commented 3 years ago

When an xvalue reference to temporary is passed to TRYX it creates a dangling reference at auto&& unique = (__VA_ARGS__); as the temporary is destroyed at the semicolon and lifetime extension does not apply.

ned14 commented 3 years ago

I'm not seeing the temporary being destroyed at the semicolon in https://godbolt.org/z/eGnKn3. Can you supply a repro demonstrating the issue?

vasama commented 3 years ago

You're passing a prvalue which' lifetime is extended. https://godbolt.org/z/zbo4q3 here I pass an xvalue reference to temporary instead.

The temporary is destroyed before being moved-from:

        lea     rdi, [rsp+16]
        call    Foo::~Foo() [complete object destructor]
        ; ...
        lea     rsi, [rsp+16]
        lea     rdi, [rsp+12]
        call    Foo::Foo(Foo&&)
ned14 commented 3 years ago

Yes I see it now. It actually affects all TRY operations, not just TRYX. Most interesting that nobody has ever reported this before, so I thank you for that.

In terms of fixing it, it seems to me that one needs to create lifetime for whatever is returned by the expression. The obvious fix is to replace any rvalue refs returned by the expression with values, so:

https://godbolt.org/z/hcve9s

Obviously this will change the semantics of code which does something like:

auto s = expr();
OUTCOME_TRY(std::move(s));

... which is a common pattern, and source code compatibility would now break if s is neither copyable nor movable.

I don't suppose you can think of any way where we can preserve strict copy elision such that non-copyable non-movable types still work, but early lifetime end for xvalues is also prevented?

vasama commented 3 years ago

There doesn't seem to be any way to implement this. It requires referencing the same value twice in a single full expression, and a lambda cannot be used because of the return. Even if that were possible, the return would still require a GNU statement expression, even in TRY.

ned14 commented 3 years ago

There doesn't seem to be any way to implement this. It requires referencing the same value twice in a single full expression, and a lambda cannot be used because of the return. Even if that were possible, the return would still require a GNU statement expression, even in TRY.

Perhaps I should explain more.

If a user writes:

OUTCOME_TRY(auto &&var, filter(func()));

... then dangling references are to be expected if filter() returns xvalues, because the user wrote auto &&. But if the user writes:

OUTCOME_TRY(auto var, filter(func()));

... then I think it improperly surprising that var gets move constructed from a destructed object. What would be ideal here is if TRY internally used the same value semantics as the result, but I'm not sure if the C preprocessor is capable.

Re: TRYX, in some ways it's impossible to properly fix without breaking something else. The xvalue/prvalue semantics on values emitted from it are buggy across compilers in any case, you're kinda on your own if you use TRYX. All that said, I'd prefer it to fail to compile than silently do UB, so I'd say for that we'll just make unique either a lvalue ref or a value, and if the user passes in some type which can't move, it won't compile and then the user has to use TRY instead of TRYX.

ned14 commented 3 years ago

I asked how to get the preprocessor to do what would be ideal at https://stackoverflow.com/questions/66069152/token-detection-within-a-c-preprocessor-macro-argument

ned14 commented 3 years ago

Ok, I think we have a solution. We now deduce xvalues to valued uniques to ensure lifetime extension. If you wish to pass through rvalues, one can now override the unique specification using the new syntax:

OUTCOME_TRY((auto &&,v), expr);

... which initialises the unique as a auto &&, and the v as auto &&, and thus rvalue ref passthrough works. If you examine the new unit test issue0244.cpp, you'll see lots of use examples.

Can you check the fix to ensure it meets your approval? My thanks in advance.

vasama commented 3 years ago

The macro expansion seems to be broken. The output still contains OUTCOME_TRY_CALL_OVERLOAD. https://godbolt.org/z/5sGGEv

ned14 commented 3 years ago

I just merged it to develop branch. Try it now.

vasama commented 3 years ago

L-value references can refer to temporaries as well. E.g. https://godbolt.org/z/3Kbboo still uses a destroyed object. For arbitrary expressions the only safe deduction for the hidden variable is auto __result. The visible variable can refer to the value within that result without a move though: auto&& x = __result.value();. If the argument expression is known not to produce references to temporaries, only then can the hidden variable be deduced as a reference. This can only be known if the user explicitly guarantees it.

OUTCOME_TRY(auto&& x, expr) deducing auto, auto&& here is safe. OUTCOME_TRY(auto&&, auto&& x, expr) might let the user decide the hidden deduction.

ned14 commented 3 years ago

Ok, develop branch now uses a valued unique if the input expression is a lvalue ref. Thanks for helping me catch that oversight. Can you confirm it now all works well for you?

vasama commented 3 years ago

Oh I never actually hit this issue. I just saw it while looking at how TRYX was implemented.