Prevent clobbering of data in Result's tail padding internally and externally #407
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danakj
force-pushed
the
expected-padding
branch
from
0428ef1 to
a344f45
Compare
danakj
force-pushed
the
expected-padding
branch
6 times, most recently
from
e74ff33 to
817666b
Compare
…ternally llvm/llvm-project#70494 points out that `[[no_unique_address]]` on a union member can cause its placement-new construction to clobber data in its tail padding, as ctors can and do write their entire `sizeof()` size, not just their data-size. So we do some wild things to prevent this from occurring: The `T` and `E` union are grouped with the state into a struct, which we call `Inner`. This structure is always constructed together as a group when changing the state to `Ok` or `Err`. This ensures that we construct `T` or `E` and then set the state (which follows in the struct) thus avoiding clobbering of the state. Second, the `Inner` struct needs to be protected from clobbering other things in its tail padding. So it is wrapped in another struct that conditionally applies `[[no_unique_address]]` to it, which we call `MaybeNoUniqueAddress`. We mark `Inner` as `[[no_unique_address]]` only when there is no tail padding in `T` or `E`. Then the tail padding of `Inner` which is introduced by the state flag can be used by things external to the Result if `T` are without tail padding. All of this is put into the outermost structure `StorageVoid` or `StorageNonVoid` for a void `T` or not, respectively. Everything is marked `[[no_unique_address]]` except `Inner` which is conditionally marked as mentioned above. In order to do this, we've rewritten the entire implementation of `Result`'s storage. In the process, copy/move operations and destructors are all trivial if the same for both `T` and `E` are trivial, which fixes chromium#403.
The constexpr keyword causes MSVC to miscompile and require the method to be able to be instantiated even though it is removed by a requires clause.
danakj
force-pushed
the
expected-padding
branch
from
4354adc to
83a34b0
Compare
Update README to say so. Older MSVC crashes in Result now also, so drop the older version.
danakj
force-pushed
the
expected-padding
branch
from
83a34b0 to
242041b
Compare
MSVC has fixed the bug where [[msvc::no_unique_address]] on an array would break its alignment. Previous MSVC versions can't even compile Subspace so we don't need to worry about supporting them here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
llvm/llvm-project#70494 points out that
[[no_unique_address]]on a union member can cause its placement-new construction to clobber data in its tail padding, as ctors can and do write their entiresizeof()size, not just their data-size.So we do some wild things to prevent this from occurring:
The
TandEunion are grouped with the state into a struct, which we callInner. This structure is always constructed together as a group when changing the state to Ok or Err. This ensures that we constructTorEand then set the state (which follows in the struct) thus avoiding clobbering of the state.Second, the Inner struct needs to be protected from clobbering other things in its tail padding. So it is wrapped in another struct that conditionally applies
[[no_unique_address]]to it, which we callMaybeNoUniqueAddress.We mark
Inneras[[no_unique_address]]only when there is no tail padding inTorE. Then the tail padding ofInnerwhich is introduced by the state flag can be used by things external to theResult.All of this is put into the outermost structure
StorageVoidorStorageNonVoidfor a voidTor not, respectively. Everything is marked[[no_unique_address]]exceptInnerwhich is conditionally marked as mentioned above.In order to do this, we've rewritten the entire implementation of Result's storage. In the process, copy/move operations and destructors are all trivial if the same for both
TandEare trivial, which fixes #403.The Clang 18 and GCC builds are failing due to the Clang-18 apt package not including libclang: llvm/llvm-project#73402