[libcxx-commits] [libcxx] [libc++] Fix UB in <expected> related to "has value" flag (#68552) (PR #68733)

[Louis Dionne via libcxx-commits]

Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>
Message-ID:
In-Reply-To: <llvm/llvm-project/pull/68733/libcxx at github.com>


https://github.com/ldionne commented:

Thanks a lot for chiming in @jwakely! IMO this is the most reasonable way to resolve this given the tradeoffs (although a LWG issue would be needed to clarify).

So just to try and summarize the rather complex situation:

1. Our current understanding (shared by @jwakely) is that the Standard doesn't intend to prevent us from using `[[no_unique_address]]`. A LWG issue is needed to officialize this (or else WG21 has bad news for us). For the sake of this conversation, let's assume that LWG officializes what @jwakely said.
2. No ABI break is required on our part, we can keep `[[no_unique_address]]` and our optimization.
3. We ensure that we initialize `expected`'s members in the right order to avoid overwriting our own `__has_val_` member.
4. We cherry-pick this bugfix to LLVM 17.
5. Regarding the following case:
    ```
    struct Foo {
     [[no_unique_address]] std::expected<std::optional<int>, int> x_;
     [[no_unique_address]] bool y_;
    };
    ```
    Here it is possible for the `y_` to be overwritten by operations in `x_`. However, I would argue this is expected by the user since they used `[[no_unique_address]]` on `y_`, meaning it can overlap with another subobject. I don't think there's anything for us to fix here.

@jiixyj The current state of the patch looks quite good -- thanks for sticking around with us for this wild ride. I have a few comments but they are rather minor. I think the biggest thing is that I'd like us to add a few more tests for the other operations that were fixed in this patch. Does that sound reasonable?

https://github.com/llvm/llvm-project/pull/68733