-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Description
In some situations it seems that the "has value" flag of a std::expected gets overwritten when the value is constructed. Example:
#undef NDEBUG
#include <cassert>
#include <expected>
#include <optional>
std::expected<std::optional<int>, long> f1()
{
return 0;
}
std::expected<std::optional<int>, int> f2()
{
return f1().transform_error([](auto) { return 0; });
}
int main()
{
auto r = f2();
assert(r.has_value());Godbolt: https://godbolt.org/z/nbcPr9e5f
I would expect that the assert is not triggered, but it is. I'm testing with commit d408770.
I strongly suspect code like this, i.e. where the call to std::construct_at happens after setting the "has value" flag:
llvm-project/libcxx/include/__expected/expected.h
Lines 252 to 258 in 7cc1bfa
| template <class... _Args> | |
| requires is_constructible_v<_Tp, _Args...> | |
| _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t, _Args&&... __args) | |
| noexcept(is_nothrow_constructible_v<_Tp, _Args...>) // strengthened | |
| : __has_val_(true) { | |
| std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...); | |
| } |
In the above example, __has_val_ (a bool) and __union_.__val_ (the std::optional<int>) overlap. I think std::construct_at is allowed to overwrite __has_val_ in this case by the language rules as the std::construct_at has no idea that the __has_val_ is there. It just assumes it has full 8 bytes to work with to construct the std::optional<int>.
The data layout of the problematic std::expected<std::optional<int>, int> looks like this:
+-- optional "has value" flag
| +--+--padding
/---int---\ | | |
00 00 00 00 01 00 00 00
|
|
+- expected "has value" flag
The expected's "has value" flag reuses one byte of the padding. It should be "1" in this case but is "0".
Suggested fix: move all assignments to the "has value" flag after the std::construct_at calls like this:
template <class... _Args>
requires is_constructible_v<_Tp, _Args...>
_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t, _Args&&... __args)
noexcept(is_nothrow_constructible_v<_Tp, _Args...>) // strengthened
{
std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);
__has_val = true;
}I wonder if this analysis is correct or if I'm missing some subtleties with std::construct_at.