Lower transmutes from int to pointer type as gep on null by saethlin · Pull Request #121282 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation56 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

saethlin

I thought of this while looking at #121242. See that PR's description for why this lowering is preferable.

The UI test that's being changed here crashes without changing the transmutes into casts. Based on that, this PR should not be merged without a crater build-and-test run.

@rustbot

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Feb 19, 2024

@saethlin

@rust-timer

This comment has been minimized.

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 19, 2024

@bors

…e, r=

Lower transmutes from int to pointer type as gep on null

I thought of this while looking at rust-lang#121242

The UI test that's being changed here crashes without changing the transmutes into casts. Based on that, this PR should not be merged without a crater build-and-test run.

@bors

☀️ Try build successful - checks-actions
Build commit: d073071 (d073071d77ce0f93b4fd8cc567a1e2b9e1b22126)

@rust-timer

This comment has been minimized.

@rust-timer

Finished benchmarking commit (d073071): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌ (primary) - - 0
Regressions ❌ (secondary) 3.5% [3.5%, 3.5%] 1
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) - - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌ (primary) - - 0
Regressions ❌ (secondary) 2.1% [2.1%, 2.2%] 2
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) - - 0
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌ (primary) 0.1% [0.1%, 0.1%] 1
Regressions ❌ (secondary) - - 0
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) - - 0
All ❌✅ (primary) 0.1% [0.1%, 0.1%] 1

Bootstrap: 640.336s -> 641.418s (0.17%)
Artifact size: 308.82 MiB -> 308.83 MiB (0.00%)

@saethlin

@craterbot

@saethlin

@craterbot

🗑️ Experiment pr-121282 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@saethlin

@craterbot run mode=build-and-test start=master#61223975d46f794466efa832bc7562b9707ecc46+rustflags=-Copt-level=3 end=try#d073071d77ce0f93b4fd8cc567a1e2b9e1b22126+rustflags=-Copt-level=3

@craterbot

scottmcm

@@ -49,7 +49,7 @@ pub fn ptr_to_int(p: *mut u16) -> usize {
}
// CHECK: define{{.*}}ptr @int_to_ptr([[USIZE]] %i)
// CHECK: %_0 = inttoptr [[USIZE]] %i to ptr
// CHECK: %_0 = getelementptr i8, ptr null, i64 %i

Choose a reason for hiding this comment

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

I think to make this pass on non-64-bit you need

// CHECK: %_0 = getelementptr i8, ptr null, i64 %i
// CHECK: %_0 = getelementptr i8, ptr null, [[USIZE]] %i

@craterbot

🚧 Experiment pr-121282 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@bors

@rust-timer

Finished benchmarking commit (0fa7fea): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌ (primary) 2.6% [2.6%, 2.6%] 1
Regressions ❌ (secondary) 2.8% [2.8%, 2.8%] 1
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) -7.8% [-7.8%, -7.8%] 1
All ❌✅ (primary) 2.6% [2.6%, 2.6%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.319s -> 672.448s (0.02%)
Artifact size: 310.04 MiB -> 310.05 MiB (0.00%)

@saethlin saethlin deleted the gep-null-means-no-provenance branch

March 12, 2024 13:40

bors added a commit to rust-lang/crater that referenced this pull request

Mar 18, 2024

@bors

Ignore some more crates

Adding all the spurious failures I identified in rust-lang/rust#121282 (comment)

I'm also not sure what blacklist.md is for, but it hasn't been updated in 6 years so I imagine it's not important.

bors added a commit to rust-lang/crater that referenced this pull request

Mar 20, 2024

@bors

Ignore some more crates

Adding all the spurious failures I identified in rust-lang/rust#121282 (comment)

I'm also not sure what blacklist.md is for, but it hasn't been updated in 6 years so I imagine it's not important.

bors added a commit to rust-lang/crater that referenced this pull request

Mar 20, 2024

@bors

Ignore some more crates

Adding all the spurious failures I identified in rust-lang/rust#121282 (comment)

I'm also not sure what blacklist.md is for, but it hasn't been updated in 6 years so I imagine it's not important.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 23, 2024

@matthiaskrgr

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 23, 2024

@matthiaskrgr

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 24, 2024

@matthiaskrgr

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Mar 24, 2024

@workingjubilee

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Mar 24, 2024

@workingjubilee

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 24, 2024

@rust-timer

Rollup merge of rust-lang#122379 - RalfJung:int2ptr-transmute, r=m-ou-se

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request

Mar 25, 2024

@matthiaskrgr @RenjiSann

transmute: caution against int2ptr transmutation

This came up in rust-lang#121282. Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

May 4, 2024

@he32

Pkgsrc changes:

Upstream chnages:

Version 1.78.0 (2024-05-02)

Language

Compiler

Target changes:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Liorst4 added a commit to Liorst4/liorforth that referenced this pull request

May 24, 2024

@Liorst4

…Cell`

Reduces the amount of unsafe in the code.

Looks like in newer rustc versions, this causes crashes. Could be related to rust-lang/rust#121282

@Liorst4

Is there a difference between casting an isize to a pointer using as and using std::mem::transmute?

@saethlin @scottmcm

When this change was proposed it crashed on one of my projects #121282 (comment)

@saethlin discussed this with me here: Liorst4/liorforth#1

The discussion didn't go very deep into the issue, and it left me kind of confused.
I assumed its a bug in the patch and @saethlin was going to fix it before pushing it into master.

Now when I run the test in release the test crashes.
To fix it, all I needed to do is change a std::mem::transmute cast into an as cast.
Liorst4/liorforth@fba984a
But I don't really understand why, shouldn't these be the same thing? Is there something I'm missing? Alignment issues?

Code that fails now

type Byte = u8;

type Cell = isize;

#[repr(packed(1))] struct CountedString { len: Byte, data: [Byte; 0], }

#[no_mangle] fn test_breaks() { let buffer: [u8; 6] = [5, b'h', b'e', b'l', b'l', b'o']; let counted_string_address: Cell = (&buffer).as_ptr() as Cell;

// Using `transmute` to cast
let counted_string: &CountedString = unsafe {
    std::mem::transmute::<Cell, *const CountedString>(counted_string_address)
        .as_ref()
        .unwrap()
};

let counted_string_as_slice: &[u8] = unsafe {
    std::slice::from_raw_parts(counted_string.data.as_ptr(), counted_string.len as usize)
};
let counted_string_as_rust_string = std::str::from_utf8(counted_string_as_slice).unwrap();
println!("{}", counted_string_as_rust_string);

}

#[no_mangle] fn test_works() { let buffer: [u8; 6] = [5, b'h', b'e', b'l', b'l', b'o']; let counted_string_address: Cell = (&buffer).as_ptr() as Cell;

// Using `as` to cast
let counted_string: &CountedString = unsafe {
    (counted_string_address as *const CountedString)
        .as_ref()
        .unwrap()
};

let counted_string_as_slice: &[u8] = unsafe {
    std::slice::from_raw_parts(counted_string.data.as_ptr(), counted_string.len as usize)
};
let counted_string_as_rust_string = std::str::from_utf8(counted_string_as_slice).unwrap();
println!("{}", counted_string_as_rust_string);

}

pub fn main() { test_works(); test_breaks(); }

https://gcc.godbolt.org/z/b8ba9Y7G5

@saethlin

Yes there is a difference. To quote the RFC https://rust-lang.github.io/rfcs/3559-rust-has-provenance.html

Furthermore, the section on “Integer types” is extended to say that integers do not have provenance, and therefore transmuting (via transmute or type punning) from a pointer to an integer is a lossy operation and might even be UB. (The exact semantics of that operation involve some subtle trade-offs and are not decided by this RFC.)

as is a different operation from transmute for other types as well. Conversions between integers and floating point, for example.

@Liorst4

Yes there is a difference. To quote the RFC https://rust-lang.github.io/rfcs/3559-rust-has-provenance.html

Furthermore, the section on “Integer types” is extended to say that integers do not have provenance, and therefore transmuting (via transmute or type punning) from a pointer to an integer is a lossy operation and might even be UB. (The exact semantics of that operation involve some subtle trade-offs and are not decided by this RFC.)

as is a different operation from transmute for other types as well. Conversions between integers and floating point, for example.

OK, thanks, this clears stuff up 👍

@scottmcm

@Liorst4 Reminder that you can also confirm this using Miri. This PR changed only codegen, which doesn't affect Miri, but your test_breaks example also fails in Miri: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=46c3f8c80ae90b5fb22dd59023628e88

error: Undefined Behavior: out-of-bounds pointer use: 0x25d44[noalloc] is a dangling pointer (it has no provenance)
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:358:57
    |
358 |         if self.is_null() { None } else { unsafe { Some(&*self) } }
    |                                                         ^^^^^^ out-of-bounds pointer use: 0x25d44[noalloc] is a dangling pointer (it has no provenance)
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::ptr::const_ptr::<impl *const CountedString>::as_ref::<'_>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:358:57: 358:63
note: inside `test_breaks`
   --> src/main.rs🔞9
    |
18  | /         std::mem::transmute::<Cell, *const CountedString>(counted_string_address)
19  | |             .as_ref()
    | |_____________________^
note: inside `main`
   --> src/main.rs:31:5
    |
31  |     test_breaks();
    |     ^^^^^^^^^^^^^

(Miri has diagnosed this for a long time; this PR just means that LLVM also knows about the UB.)

@ldm0 ldm0 mentioned this pull request

Jul 30, 2024

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Oct 13, 2024

@he32

This is based on the pkgsrc-wip rust180 package, retaining the main pkgsrc changes as best as I could.

Pkgsrc changes:

Upstream chnages:

Version 1.80.1 (2024-08-08)

Version 1.80.0 (2024-07-25)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.79.0 (2024-06-13)

Language

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Misc

Compatibility Notes

Version 1.78.0 (2024-05-02)

Language

Compiler

Target changes:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.77.0 (2024-03-21)

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

Cargo

Rustdoc

Misc

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Labels

merged-by-bors

This PR was explicitly merged by bors.

relnotes

Marks issues that should be documented in the release notes of the next release.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-opsem

Relevant to the opsem team