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

Deduplication of functions containing inline assembly contradicts Reference's documentation about code-patching. #116857

Open
zachs18 opened this issue Oct 17, 2023 · 13 comments · May be fixed by rust-lang/reference#1441
Labels
A-codegen Area: Code generation A-inline-assembly Area: inline asm!(..) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team

Comments

@zachs18
Copy link
Contributor

zachs18 commented Oct 17, 2023

(I'm not sure if this should be considered a codegen bug (i.e. the Reference is correct) or a documentation bug (i.e. the codegen is correct). This issue assumes it is a codegen bug. If this is determined to be a documentation bug, then I can instead report this on the rust-lang/reference repo).

I tried this code:

#![no_std]
use core::arch::asm;
pub fn foo() {
    unsafe { asm!("nop"); }
}
pub fn bar() {
    unsafe { asm!("nop"); }
}

View the resulting assembly using either godbolt.org or cargo-show-asm.

I expected to see this happen: foo and bar are not deduplicated, due to the Rust Reference's documentation that

The compiler cannot assume that the instructions in the asm are the ones that will actually end up executed.
This effectively means that the compiler must treat the asm! as a black box and only take the interface specification into account, not the instructions themselves.
Runtime code patching is allowed, via target-specific mechanisms.

# expected output for --target x86_64-unknown-linux-gnu
example::foo:
  push rax
  
  nop
  
  pop rax
  ret

example::bar:
  push rax
  
  nop
  
  pop rax
  ret

Instead, this happened: When compiled with optimizations (-Copt-level=2 or above), foo and bar are deduplicated (godbolt link)

# actual output for --target x86_64-unknown-linux-gnu
example::bar:
  push rax
  
  nop
  
  pop rax
  ret

Notes:

  • This only happens when the asm invocations are exactly the same, thus a workaround is to add spaces to the asm template string to make them different.
  • Different extern "ABI" and #[no_mangle] do not inherently prevent deduplication.
  • Different #[link_section = "..."]s work to prevent deduplication (if the functions are given the same #[link_section = "..."] then they are still deduplicated).

Meta

Happens on nightly (2023-10-16), stable (1.73.0), and 1.59.0 (when inline asm was stabilized) (and probably all versions between).

rustc --version --verbose:

rustc 1.75.0-nightly (49691b1f7 2023-10-16)
binary: rustc
commit-hash: 49691b1f70d71dd7b8349c332b7f277ee527bf08
commit-date: 2023-10-16
host: x86_64-unknown-linux-gnu
release: 1.75.0-nightly
LLVM version: 17.0.2

rustc 1.73.0 (cc66ad468 2023-10-03)
binary: rustc
commit-hash: cc66ad468955717ab92600c770da8c1601a4ff33
commit-date: 2023-10-03
host: x86_64-unknown-linux-gnu
release: 1.73.0
LLVM version: 17.0.2

(no backtrace)

@rustbot label +A-inline-assembly

@zachs18 zachs18 added the C-bug Category: This is a bug. label Oct 17, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-inline-assembly Area: inline asm!(..) labels Oct 17, 2023
@workingjubilee
Copy link
Contributor

@zachs18 can you test specifically with -Zmerge-functions?

@the8472
Copy link
Member

the8472 commented Oct 17, 2023

The compiler cannot assume that the instructions in the asm are the ones that will actually end up executed.
This effectively means that the compiler must treat the asm! as a black box and only take the interface specification into account, not the instructions themselves.
Runtime code patching is allowed, via target-specific mechanisms.

This language could be interpreted as the compiler cannot make assumptions about the assembly because they might get patched, without actually promising that all assembly blocks can be patched in useful ways.

It might some annotation to opt out of unnamed_addr in the IR.

@zachs18
Copy link
Contributor Author

zachs18 commented Oct 17, 2023

@workingjubilee Yes, -Zmerge-functions=disabled does make the functions not be merged (godbolt link).

@workingjubilee
Copy link
Contributor

This language could be interpreted as the compiler cannot make assumptions about the assembly because they might get patched, without actually promising that all assembly blocks can be patched in useful ways.

That's certainly already what happens in some cases?
Given

#![no_std]
use core::arch::asm;
pub fn foo<A>() {
    unsafe { asm!("nop"); }
}

fn main() {
    let _ = foo::<u16>();
    let _ = foo::<i32>();
    let _ = foo::<f64>();
}

The compiler does not promise there will be exactly three nops in the program.

@asquared31415
Copy link
Contributor

I'm not sure there's a way to locate instructions in a non-naked function in ways that are guaranteed to work by Rust. There can be arbitrary code around the asm, so an exported fn symbol doesn't suffice. You can't export a symbol from the asm! because it's not guaranteed that it's only emitted once, so there would be conflicting symbols. The best thing I can think of is inserting a well known constant pattern and searching for that, but I'm not sure if it's permitted to read the bytes of a function (maybe only via assembly and that's arch specific?)

However this bug also occurs with #[naked] functions where it would be possible to modify the asm using the exported function symbol, so there's at least a bug there, even if the compiler would be permitted to merge non-naked functions.

@asquared31415
Copy link
Contributor

@rustbot label -needs-triage +T-compiler +A-codegen

@rustbot rustbot added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 18, 2023
@workingjubilee
Copy link
Contributor

I'm not sure there's a way to locate instructions in a non-naked function in ways that are guaranteed to work by Rust. There can be arbitrary code around the asm, so an exported fn symbol doesn't suffice. You can't export a symbol from the asm! because it's not guaranteed that it's only emitted once, so there would be conflicting symbols. The best thing I can think of is inserting a well known constant pattern and searching for that, but I'm not sure if it's permitted to read the bytes of a function (maybe only via assembly and that's arch specific?)

...Technically, a SIGILL is always guaranteed to find the instructions, no?

@saethlin saethlin added the T-opsem Relevant to the opsem team label Dec 25, 2023
@saethlin
Copy link
Member

cc @rust-lang/opsem is this a soundness hole?

@BattyBoopers
Copy link

BattyBoopers commented Dec 25, 2023

I just came across a very similar issue here.

However, while it could be argued that two actually identical functions like in the original example might get deduplicated, my issue here is that apparently code is getting deduplicated just based on the text-contents of the asm block, even though repeating the block in two different functions has different meaning.
This basically defines a static variable in assembler and returns a pointer to it, so any repetition of the exact same inline-assembler code is semantically different.

use std::arch::asm;
use std::hint::black_box;

fn foo_() -> *mut u64 {
    let p: *mut u64;
    unsafe {
        asm! {
            "lea {p},[rip + 1f]",
            ".pushsection .bss",
            ".align 8",
            "1:",
            ".quad 0",
            ".popsection",
            p = out(reg) p,
        }
    };
    p
}

fn foo() -> *mut u64 {
    let f: fn() -> _ = black_box(foo_); // prevent inlining
    f()
}

fn bar_() -> *mut u64 {
    let p: *mut u64;
    unsafe {
        asm! {
            "lea {p},[rip + 1f]",
            ".pushsection .bss",
            ".align 8",
            "1:",
            ".quad 0",
            ".popsection",
            p = out(reg) p,
        }
    };
    p
}

fn bar() -> *mut u64 {
    let f: fn() -> _ = black_box(bar_); // prevent inlining
    f()
}

fn main() {
    println!("{:x?} {:x?}", foo(), bar());
}

The two addresses printed should be different, but they are identical due to the incorrect code-deduplication.

I'm not enitirely sure if this should be considered a separate issue instead, since the case for this being incorrect is a lot stronger than the original issue.

Furthermore, I would also expect any inlining of one of the functions to return the same address, which it currently doesn't, likely for the same reason. The only way the asm instructions should currently be allowed to be repeated verbatim and thus generate a different static variable for the same function call is if it is used inside generic code which is monomorphisized in different code-generation units (The reason for that is the the same reason we currently can't have any generic static variables, which is annoying, but a different issue).

@RalfJung
Copy link
Member

@Amanieu what is the expected behavior here?

@Amanieu
Copy link
Member

Amanieu commented Dec 25, 2023

The current behavior is correct: the compiler is allowed to deduplicate identical inline assembly. Perhaps this could be clarified in the documentation.

Essentially, if you intend to do any kind of hot patching then you need some way to uniquely identify the assembly that you intend to patch. This is usually done by recording the address of the assembly code in a separate section like this:

0:
    my_patchable_instruction

.section patch_list, "a"
.balign 8
.quad 0b
.previous

This will work correctly no matter how many times the asm is duplicated or deduplicated since you'll just end up with more or fewer entries in the patch_list section. If you need different behavior for different blocks then you should use different section names, which will inhibit deduplication.

@RalfJung
Copy link
Member

Should this be mentioned or at least hinted at in the documentation? I can see how the current docs (quoted in the OP) could lead to misunderstandings here.

@Amanieu
Copy link
Member

Amanieu commented Dec 25, 2023

rust-lang/reference#1441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-inline-assembly Area: inline asm!(..) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants