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

library: Compute Rust exception class from its string repr #130381

Conversation

workingjubilee
Copy link
Member

Noticed this awkwardness while scanning through the code. I think we can do better than that.

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2024

r? @thomcc

rustbot has assigned @thomcc.
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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 15, 2024
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

@Noratrieb
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 15, 2024

📌 Commit fef7373 has been approved by Noratrieb

It is now in the queue for this repository.

@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 Sep 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#129195 (Stabilize `&mut` (and `*mut`) as well as `&Cell` (and `*const Cell`) in const)
 - rust-lang#130118 (move Option::unwrap_unchecked into const_option feature gate)
 - rust-lang#130295 (Fix target-cpu fpu features on Armv8-R.)
 - rust-lang#130371 (Correctly account for niche-optimized tags in rustc_transmute)
 - rust-lang#130381 (library: Compute Rust exception class from its string repr)

r? `@ghost`
`@rustbot` modify labels: rollup
// M O Z \0 R U S T -- vendor, language
0x4d4f5a_00_52555354
}
const RUST_EXCEPTION_CLASS: uw::_Unwind_Exception_Class = u64::from_be_bytes(*b"MOZ\0RUST");
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing, but should we really reverse the stored order between little-endian and big-endian systems? We currently show MOZ\0RUST on big-endian systems and TSUR\0ZOM on little-endian systems when interpreted as string/doing a hexdump.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that when I poked at it. 👀 It is funny to me. All the other runtimes make the same mistake, so I initially decided not to change it. I think I wanted to actually look at the data through a hex dump before doing so.

Choose a reason for hiding this comment

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

Maybe a relevant datapoint: windows-drivers-rs' wdk-alloc specifically chose u32::from_ne_bytes(*b"rust") for their allocator tag: https://github.com/microsoft/windows-drivers-rs/blob/c3b7c4aa06e43e928f27eb704f76932688802921/crates/wdk-alloc/src/lib.rs#L50

Copy link
Member

@RalfJung RalfJung Sep 23, 2024

Choose a reason for hiding this comment

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

Maybe it's not a mistake, maybe the type of this should be thought of as [u8; 8] rather than u64 -- with bytes always being interpreted left-to-right no matter the endianess.

Copy link
Member

Choose a reason for hiding this comment

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

The current impl is a mistake if it should be interpreted as [u8; 8] as currently little endian systems put the string in reverse order, while big endian systems put it in the correct order.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, that would have to use from_ne_bytes... then transmuting back to a u8 array would always give the same results.

Copy link
Member

Choose a reason for hiding this comment

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

i assume it was just someone filling out the u64 literal in left to right order, as the previous comment described.

Copy link
Member Author

@workingjubilee workingjubilee Sep 26, 2024

Choose a reason for hiding this comment

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

cool, I checked the hexdump emitted and yeah, it is indeed backwards on x86-64. silly Rust.

@bors bors merged commit 729aa49 into rust-lang:master Sep 15, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2024
Rollup merge of rust-lang#130381 - workingjubilee:sometimes-code-really-is-self-descriptive, r=Noratrieb

library: Compute Rust exception class from its string repr

Noticed this awkwardness while scanning through the code. I think we can do better than that.
@rustbot rustbot added this to the 1.83.0 milestone Sep 15, 2024
@workingjubilee workingjubilee deleted the sometimes-code-really-is-self-descriptive branch September 15, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants