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.
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
Add SGX target to std and dependencies #56066
Add SGX target to std and dependencies #56066
Changes from all commits
030b1ed
22c4368
6c03640
c559216
4a35056
39f9751
1e44e2d
8d6edc9
1a894f1
59b79f7
6650f43
7bea6a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this cropping up in a few places for a few APIs, but how come this was necessary? (should be fine in any case, just curious)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ultimately comes down to the various
const fn new
implementations inwaitqueue.rs
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ok, that seems suspicious but it seems fine anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe someone with more experience with
min_const_fn
can suggest an alternative solution? @oli-obk ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't call an unstable const fn from a stable one. You might be able to make all of the functions called from this one stable and then you won't need the unstable anymore. note that as of right now, you cannot call unsafe const fns from any const fn.
If you want, just merge the PR as is and open an issue about it and assign it to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe at the time this was added this was an optimization for wasm specifically in LTO mode to ensure that lots of panicking infrastructure csan be removed because in this case the payload is never actually used.
I haven't finished reading this patch yet, but it may be worthwhile running the wasm tests as they should in theory catch this if it's still a problem!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made sure the changes should have the same functionality, and I also checked that
set_payload
isn't really doing much so it's ok to do unconditionally. But yeah I suppose some form of LTO might no longer work.The crux is that currently in this target, checking whether there is a panic output mechanism prevents future use of the same (it's an atomic “take”). I can think if there's another way to restructure all this tomorrow, but this was the best I could come up with a few months ago :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, with regards to this and #56066 (comment), I conflated two things a little bit.
In the SGX target, whether panicking can ever output anything at all is determined post-link time by setting/unsetting the
DEBUG
global flag. This is because panic message may contain sensitive information, so it's opt-in. This requirement leads to the function signature:fn panic_output() -> Option<PanicOutput>
. Ideally havingDEBUG = false
leads to the same fast-path as wasm without a panic implementation.The second requirement that leads to the "writer factory" in
PanicOutput::Custom
is that in this target the panic writer can only be constructed once.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this a bunch to use types instead of bare functions. I think it should be LTOable. Please take a look.