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

Dynamic library generates functions with no meaningful assembly. #125299

Closed
pirocks opened this issue May 20, 2024 · 6 comments
Closed

Dynamic library generates functions with no meaningful assembly. #125299

pirocks opened this issue May 20, 2024 · 6 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues.

Comments

@pirocks
Copy link

pirocks commented May 20, 2024

I tried this code(compiling in release mode):

use std::mem::transmute;

#[no_mangle]
pub unsafe extern "system" fn foo_bar_baz(ptr: i64) -> i8 {
    let ptr: *mut i8 = transmute(ptr);
    ptr.read()
}

in with the following Cargo.toml:

[package]
name = "repro_dynamic"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[lib]
crate-type = ["cdylib"]

[dependencies]

I expected to see this happen:
I expected a dynamic executable with a function labelled foo_bar_baz in it.

Instead, this happened:

I get a label for the function foo_bar_baz, but it is an undefined instruction only. On older rust versions I get the expected assembly.

I have a much larger project which is affected by this, and it seems to be broadly the same, affects many no_mangle unsafe extern functions(but not all such functions, not really clear as to what the deciding factor is but probably has something to do with function body).

$ objdump -D target/release/librepro_dynamic.so | grep -C 25 foo_bar_baz
00000000000126c0 <frame_dummy>:
   126c0:       f3 0f 1e fa             endbr64 
   126c4:       e9 77 ff ff ff          jmp    12640 <register_tm_clones>
   126c9:       cc                      int3   
   126ca:       cc                      int3   
   126cb:       cc                      int3   
   126cc:       cc                      int3   
   126cd:       cc                      int3   
   126ce:       cc                      int3   
   126cf:       cc                      int3   

00000000000126d0 <foo_bar_baz>:
   126d0:       0f 0b                   ud2    
   126d2:       cc                      int3   
   126d3:       cc                      int3   
   126d4:       cc                      int3   
   126d5:       cc                      int3   
   126d6:       cc                      int3   
   126d7:       cc                      int3   
   126d8:       cc                      int3   
   126d9:       cc                      int3   
   126da:       cc                      int3   
   126db:       cc                      int3   
   126dc:       cc                      int3   
   126dd:       cc                      int3   
   126de:       cc                      int3   
   126df:       cc                      int3   

00000000000126e0 <__rust_alloc>:
   126e0:       e9 bb 9b 01 00          jmp    2c2a0 <__rdl_alloc>
   126e5:       cc                      int3   
   126e6:       cc                      int3   
   126e7:       cc                      int3   
   126e8:       cc                      int3   
   126e9:       cc                      int3   

Meta

Repros in latest stable and nightly. cargo-rustc-bisect says:

Regression in 317d14a56cb8c748bf0e2f2afff89c2249ab4423

rustc --version --verbose:

francis@francis-desktop::~/RustroverProjects/repro$ rustc --version --verbose
rustc 1.78.0 (9b00956e5 2024-04-29)
binary: rustc
commit-hash: 9b00956e56009bab2aa15d7bff10916599e3d6d6
commit-date: 2024-04-29
host: x86_64-unknown-linux-gnu
release: 1.78.0
LLVM version: 18.1.2

Edit:
Clarified that I was compiling in release mode.

@pirocks pirocks added the C-bug Category: This is a bug. label May 20, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 20, 2024
@workingjubilee workingjubilee added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label May 20, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 20, 2024
@workingjubilee
Copy link
Contributor

@pirocks You should avoid using transmute. The docs for mem::transmute specify transmutes from integers to pointers are dicey.

The nightly docs for mem::transmute are even louder, and are clearer that what you have done is UB:

Transmuting integers to pointers is a largely unspecified operation. It is likely not equivalent to an as cast. Doing non-zero-sized memory accesses with a pointer constructed this way is currently considered undefined behavior.

Specifically using transmute in your case is something you should consider unsettlingly close to this:

use std::{ptr, mem};

#[no_mangle]
pub unsafe extern "system" fn foo_bar_baz(addr: i64) -> i8 {
    let ptr: *const i8 = ptr::null().wrapping_offset(addr);
    ptr.read()
}

The following, however, uses an as cast. An as cast is safe, and thus while it can't make turning integers into pointers a good idea... it isn't... it can make this succeed. The cost is program deoptimization:

#[no_mangle]
pub unsafe extern "system" fn foo_bar_baz(addr: i64) -> i8 {
    let ptr: *const i8 = addr as ptr;
    ptr.read()
}

If you can, it would be a good idea to redeclare the relevant interfaces to accept pointers instead... *mut ffi::c_void is right there if you don't want to specify which one.

@fmease fmease added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 20, 2024
@bjorn3
Copy link
Member

bjorn3 commented May 20, 2024

Specifically using transmute in your case is something you should consider unsettlingly close to this:

We actually lower it to pretty much exactly that since #121282.

@pirocks
Copy link
Author

pirocks commented May 20, 2024

I apologize for my kinda dumb mistake there. I was under the impression I should at least get some kind of assembly from that function since I thought transmuting from pointer to integer to pointer again was defined, but docs seem to not agree on that.

@pirocks pirocks closed this as completed May 20, 2024
@pirocks
Copy link
Author

pirocks commented May 20, 2024

Well as a slight follow up question:

Would a clippy and/or compiler warning PR along the lines of "transmute from integer to pointer is ub" be welcome?

@bjorn3
Copy link
Member

bjorn3 commented May 20, 2024

Clippy already lints against it, but it lints it for a different reason than UB. It doesn't explain that dereferencing the resulting pointer is UB. And the way it is worded suggests that the replacement of using a cast has identical behavior, which is not the case at all.

warning: transmute from an integer to a pointer
 --> src/lib.rs:5:24
  |
5 |     let ptr: *mut i8 = transmute(ptr);
  |                        ^^^^^^^^^^^^^^ help: try: `ptr as *mut i8`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_transmute
  = note: `#[warn(clippy::useless_transmute)]` on by default

@workingjubilee
Copy link
Contributor

@pirocks Yes, PRing a lint against doing transmute::<_, *mut NonZst>(int).read() would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues.
Projects
None yet
Development

No branches or pull requests

5 participants