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

USIZE_MARKER: make sure the function is monomorphized only once #123780

Closed
wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Member

This entire USIZE_MARKER thing is a hack and not guaranteed to actually work according to any op.sem proposal I am aware of. Honestly I'd prefer if we could rip it out; trying to provide any kind of guarantee for function pointer identity is basically impossible in Rust. Keeping this hack working is causing major headaches for MIR optimizations.

But meanwhile let's at least make it slightly less likely that it will break.

@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 Apr 11, 2024
static USIZE_MARKER: fn(&usize, &mut Formatter<'_>) -> Result = {
// Make sure this function is only monomorphized once, and not considered cross-crate
// inlineable.
#[inline(never)]
Copy link
Member

Choose a reason for hiding this comment

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

#[inline(never)] is a suggestion to the optimizer, not a hard requirement. As such this change still doesn't guarantee that the function is only monomorphized once.

https://doc.rust-lang.org/reference/attributes/codegen.html#the-inline-attribute

  • #[inline(never)] suggests that an inline expansion should never be performed.

Note: #[inline] in every form is a hint, with no requirements on the language to place a copy of the attributed function in the caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't think there is a way to actually do what USIZE_MARKER wants to do. But I don't think t-libs will let me remove this thing entirely, so using #[inline(never)] seems like an improvement...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
Get rid of `USIZE_MARKER` in formatting infrastructure

An alternative to rust-lang#123780.

The `USIZE_MARKER` function used to differentiate between placeholder and count arguments is never called anyway, so we can just replace the function-pointer-comparison hack with an `enum` and an `unreachable_unchecked`, hopefully without causing a regression.

CC `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2024
…lacrum

Get rid of `USIZE_MARKER` in formatting infrastructure

An alternative to rust-lang#123780.

The `USIZE_MARKER` function used to differentiate between placeholder and count arguments is never called anyway, so we can just replace the function-pointer-comparison hack with an `enum` and an `unreachable_unchecked`, hopefully without causing a regression.

CC `@RalfJung`
@bors
Copy link
Contributor

bors commented Apr 14, 2024

☔ The latest upstream changes (presumably #123819) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member Author

Superseded by #123819.

@RalfJung RalfJung closed this Apr 14, 2024
@RalfJung RalfJung deleted the usize-marker branch April 14, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants