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

Be stricter about rejecting LLVM reserved registers in asm! #84658

Merged
merged 3 commits into from
May 1, 2021

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Apr 28, 2021

LLVM will silently produce incorrect code if these registers are used as operands.

cc @rust-lang/wg-inline-asm

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 28, 2021

📌 Commit e6a731e has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 29, 2021
Be stricter about rejecting LLVM reserved registers in asm!

LLVM will silently produce incorrect code if these registers are used as operands.

cc `@rust-lang/wg-inline-asm`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 29, 2021
Be stricter about rejecting LLVM reserved registers in asm!

LLVM will silently produce incorrect code if these registers are used as operands.

cc ``@rust-lang/wg-inline-asm``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 29, 2021
Be stricter about rejecting LLVM reserved registers in asm!

LLVM will silently produce incorrect code if these registers are used as operands.

cc ```@rust-lang/wg-inline-asm```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 30, 2021
Be stricter about rejecting LLVM reserved registers in asm!

LLVM will silently produce incorrect code if these registers are used as operands.

cc ````@rust-lang/wg-inline-asm````
@jackh726
Copy link
Member

Possibly failed in #84739 (comment)

@bors rollup=iffy

@Amanieu
Copy link
Member Author

Amanieu commented Apr 30, 2021

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Apr 30, 2021

📌 Commit 2fb751c1edd59b7199fe33207767c01f1cd98487 has been approved by petrochenkov

@bors bors merged commit 603a42e into rust-lang:master May 1, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 1, 2021
@haraldh
Copy link
Contributor

haraldh commented May 3, 2021

The enclu operation takes rbx as an input.
Do you suggest to change:

        asm!(
            "enclu",

            in("rax") EREPORT,
            in("rbx") ti.as_ptr(),
            in("rcx") data.0.as_ptr(),
            in("rdx") report.as_mut_ptr(),
        );

to

        asm!(
            "xchg rsi, rbx",
            "enclu",
            "xchg rsi, rbx",

            in("rax") EREPORT,
            in("rsi") ti.as_ptr(),
            in("rcx") data.0.as_ptr(),
            in("rdx") report.as_mut_ptr(),
        );

@Amanieu
Copy link
Member Author

Amanieu commented May 3, 2021

Yes. See the changes here.

@haraldh
Copy link
Contributor

haraldh commented May 3, 2021

@Amanieu thanks!

haraldh added a commit to haraldh/sgx that referenced this pull request May 3, 2021
haraldh added a commit to haraldh/sgx that referenced this pull request May 3, 2021
haraldh added a commit to haraldh/enarx-keepldr that referenced this pull request May 3, 2021
error: invalid register rbx: rbx is used internally by LLVM and
cannot be used as an operand for inline asm

See:
rust-lang/rust#84658 (comment)

and:
https://github.com/rust-lang/rust/pull/84658/files#diff-d7283132d97a993fad4e2d491ac883dbce4e17fe248cdf37fa3f9334e2a5a115

Signed-off-by: Harald Hoyer <harald@redhat.com>
enarxbot pushed a commit to enarx/sgx that referenced this pull request May 3, 2021
match arch {
InlineAsmArch::X86 => Ok(()),
InlineAsmArch::X86_64 => {
Err("rbx is used internally by LLVM and cannot be used as an operand for inline asm")
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a problematic restriction. Many x86 instructions that you might want to run with asm! take rbx as an operand. Same applies to esi below, really.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the RFC:

The intent is that support for asm! should be independent of the rustc back-end used

Given that, I think backend limitations such as these should be handled by rustc prior to passing the assembly to the backend. It shouldn't be visible to the user. Otherwise, asm! would only be able to support the intersection of all inline assembly implementations across all codegen backends.

Copy link
Member Author

Choose a reason for hiding this comment

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

Handling this in rustc is actually non-trivial (see this long Zulip discussion), which is also why LLVM doesn't just do it for us.

haraldh added a commit to haraldh/enarx-keepldr that referenced this pull request Jul 1, 2021
error: invalid register rbx: rbx is used internally by LLVM and
cannot be used as an operand for inline asm

So, we pull in a newer `sgx` crate version, which has this fixed.

See:
rust-lang/rust#84658 (comment)

and:
https://github.com/rust-lang/rust/pull/84658/files#diff-d7283132d97a993fad4e2d491ac883dbce4e17fe248cdf37fa3f9334e2a5a115

Signed-off-by: Harald Hoyer <harald@redhat.com>
haraldh added a commit to haraldh/enarx-keepldr that referenced this pull request Jul 1, 2021
error: invalid register rbx: rbx is used internally by LLVM and
cannot be used as an operand for inline asm

So, we pull in a newer `sgx` crate version, which has this fixed.

See:
rust-lang/rust#84658 (comment)

and:
https://github.com/rust-lang/rust/pull/84658/files#diff-d7283132d97a993fad4e2d491ac883dbce4e17fe248cdf37fa3f9334e2a5a115

Signed-off-by: Harald Hoyer <harald@redhat.com>
npmccallum pushed a commit to enarx-archive/enarx-keepldr that referenced this pull request Jul 1, 2021
error: invalid register rbx: rbx is used internally by LLVM and
cannot be used as an operand for inline asm

See:
rust-lang/rust#84658 (comment)

and:
https://github.com/rust-lang/rust/pull/84658/files#diff-d7283132d97a993fad4e2d491ac883dbce4e17fe248cdf37fa3f9334e2a5a115

Signed-off-by: Harald Hoyer <harald@redhat.com>
npmccallum pushed a commit to enarx-archive/enarx-keepldr that referenced this pull request Jul 1, 2021
error: invalid register rbx: rbx is used internally by LLVM and
cannot be used as an operand for inline asm

So, we pull in a newer `sgx` crate version, which has this fixed.

See:
rust-lang/rust#84658 (comment)

and:
https://github.com/rust-lang/rust/pull/84658/files#diff-d7283132d97a993fad4e2d491ac883dbce4e17fe248cdf37fa3f9334e2a5a115

Signed-off-by: Harald Hoyer <harald@redhat.com>
crawfxrd added a commit to system76/firmware-smmstore that referenced this pull request Oct 19, 2021
Fix:
- clippy::needless_borrow

Silence:
- clippy::manual_flatten

Not addressed:
- deprecated

`llvm_asm!()` is deprecated, but must be used. The new `asm!()` does not
allow using `ebx` [1], which holds the argument for the SMMSTORE command.

[1]: rust-lang/rust#84658

Signed-off-by: Tim Crawford <tcrawford@system76.com>
crawfxrd added a commit to system76/firmware-smmstore that referenced this pull request Oct 25, 2021
Fix:
- clippy::needless_borrow

Silence:
- clippy::manual_flatten

Not addressed:
- deprecated

`llvm_asm!()` is deprecated, but must be used. The new `asm!()` does not
allow using `ebx` [1], which holds the argument for the SMMSTORE command.

[1]: rust-lang/rust#84658

Signed-off-by: Tim Crawford <tcrawford@system76.com>
crawfxrd added a commit to system76/firmware-smmstore that referenced this pull request Oct 26, 2021
Fix:
- clippy::needless_borrow

Silence:
- clippy::manual_flatten

Not addressed:
- deprecated

`llvm_asm!()` is deprecated, but must be used. The new `asm!()` does not
allow using `ebx` [1], which holds the argument for the SMMSTORE command.

[1]: rust-lang/rust#84658

Signed-off-by: Tim Crawford <tcrawford@system76.com>
crawfxrd added a commit to system76/firmware-smmstore that referenced this pull request Oct 28, 2021
Fix:
- clippy::needless_borrow
- clippy::bool_assert_comparison

Silence:
- clippy::manual_flatten

Not addressed:
- deprecated

`llvm_asm!()` is deprecated, but must be used. The new `asm!()` does not
allow using `ebx` [1], which holds the argument for the SMMSTORE command.

[1]: rust-lang/rust#84658

Signed-off-by: Tim Crawford <tcrawford@system76.com>
crawfxrd added a commit to system76/firmware-smmstore that referenced this pull request Oct 28, 2021
Fix:
- clippy::needless_borrow
- clippy::bool_assert_comparison

Silence:
- clippy::manual_flatten

Not addressed:
- deprecated

`llvm_asm!()` is deprecated, but must be used. The new `asm!()` does not
allow using `ebx` [1], which holds the argument for the SMMSTORE command.

[1]: rust-lang/rust#84658

Signed-off-by: Tim Crawford <tcrawford@system76.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants