Skip to content

Prefer asm! in std - all in sgx module - #77292

Merged
bors merged 1 commit into
rust-lang:masterfrom
tesuji:std_asm
Sep 30, 2020
Merged

Prefer asm! in std - all in sgx module#77292
bors merged 1 commit into
rust-lang:masterfrom
tesuji:std_asm

Conversation

@tesuji

@tesujitesuji commented Sep 28, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Similar to the change in #76669 but all llvm_asm! is gate in x86/x86_64 target.
Godbolt:

@tesuji

tesuji commented Sep 28, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

cc @jethrogb
r? @Amanieu

jethrogb
jethrogb reviewed Sep 28, 2020
View reviewed changes
Comment threadlibrary/std/src/sys/sgx/abi/mem.rs Outdated

@jethrogbjethrogbSep 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't require a FIXME (same for other similar comments throughout the PR)

@tesujitesujiSep 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fellow contributors may not know this and try to convert to intel syntax.

@jethrogbjethrogbSep 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but FIXME implies this needs to be changed, which is not the case.

@tesujitesujiSep 28, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be change in the future when #76738 resolved.
But I could change it to a NOTE if you prefer.

Comment threadlibrary/std/src/sys/sgx/abi/mem.rs Outdated

@jethrogbjethrogbSep 28, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: preserves_flags, nomem.

Does nomem imply nostack?

@AmanieuAmanieuSep 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nostack is separate from nomem: you can still allocate and use stack memory with nomem as long as you don't access any memory outside the asm.

Comment threadlibrary/std/src/sys/sgx/ext/arch.rs Outdated

@jethrogbjethrogbSep 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly not preserves_flags. But you can use nostack.

Comment threadlibrary/std/src/sys/sgx/ext/arch.rs Outdated
Comment threadlibrary/std/src/sys/sgx/ext/arch.rs Outdated

@jethrogbjethrogbSep 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: preserves_flags, nostack.

@tesuji

tesuji commented Sep 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Passed CI, just fall at upload state, which it should be.

Amanieu
Amanieu reviewed Sep 28, 2020
View reviewed changes
Comment threadlibrary/std/src/sys/sgx/abi/mem.rs Outdated
@Amanieu

Amanieu commented Sep 28, 2020

Copy link
Copy Markdown
Member

LGTM

@tesuji
tesuji marked this pull request as ready for review September 28, 2020 13:09
@Amanieu

Amanieu commented Sep 28, 2020

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit d477201 has been approved by Amanieu

@borsbors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 28, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 28, 2020
@Dylan-DPC
Rollup merge ofrust-lang#77292- lzutao:std_asm, r=Amanieu
f150083
Prefer asm! in std - all in sgx module
Similar to the change in rust-lang#76669 but all `llvm_asm!` is gate in x86/x86_64 target.
Godbolt:
- https://rust.godbolt.org/z/h7nG1h
- https://rust.godbolt.org/z/xx39hW
r? @ghost
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Sep 28, 2020
Closed
@bors

bors commented Sep 30, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit d477201 with merge c0127e4...

@bors

bors commented Sep 30, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing c0127e4 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2020
@bors
bors merged commit c0127e4 into rust-lang:masterSep 30, 2020
@rustbotrustbot added this to the 1.48.0 milestone Sep 30, 2020
@tesuji
tesuji deleted the std_asm branch September 30, 2020 08:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AmanieuAmanieuAmanieu left review comments
+1 more reviewer
@jethrogbjethrogbjethrogb left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@AmanieuAmanieu

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

5 participants

@tesuji@Amanieu@bors@jethrogb@rustbot