Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inline asm!: ban OpReturn/OpReturnValue (they're always UB). #1006

Merged
merged 5 commits into from
Mar 18, 2023

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Mar 17, 2023

This PR fixes #1002 by banning the misuse of inline asm! (see the issue for more details).

We used to get away with the UB, but now MIR inlining is wreaking havoc (e.g. inlining asm!("ret") will return from the caller it was inlined into - one of the many ways in which the UB can manifest).

Most of this PR is actually making progress towards:

However, I'm not considering #981 as being fixed just yet, because the original usecase involves arrays, which have their own complications, and I focused on only opaque handles (as AccelerationStructure constructors need it).

I've also not changed any unaffected use of asm!, leaving the migration to MaybeUninit for an issue:

Each commit should be reviewed separately (or maybe I should've split this into two PRs?).

@eddyb eddyb marked this pull request as ready for review March 17, 2023 20:14
@eddyb eddyb requested a review from oisyn as a code owner March 17, 2023 20:14
@eddyb eddyb mentioned this pull request Mar 17, 2023
@eddyb eddyb enabled auto-merge (rebase) March 17, 2023 20:35
@eddyb eddyb merged commit 6bbd34b into EmbarkStudios:main Mar 18, 2023
@eddyb eddyb deleted the no-asm-ret branch March 18, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning from inline asm! is unsound (always UB, and made worse by MIR inlining).
2 participants