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.

Sign up for GitHub

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

Jump to bottom

Only assert the Parser size on specific arches #135855

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Only assert the Parser size on specific arches #135855

wants to merge 1 commit into from
+3 −2

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jan 22, 2025

The size of this struct depends on the alignment of u128, for example
powerpc64le and s390x have align-8 and end up with only 280 bytes. Our
64-bit tier-1 arches are the same though, so let's just assert on those.

r? nnethercote

@cuviper
Only assert the Parser size on specific arches
aef640a 
The size of this struct depends on the alignment of `u128`, for example
powerpc64le and s390x have align-8 and end up with only 280 bytes. Our
64-bit tier-1 arches are the same though, so let's just assert on those.
@rustbot
Copy link
Collaborator

rustbot commented Jan 22, 2025

Could not assign reviewer from: nnethercote.
User(s) nnethercote are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Jan 22, 2025

r? @oli-obk

rustbot has assigned @oli-obk.
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 Jan 22, 2025
@cuviper
Copy link
Member Author

cuviper commented Jan 22, 2025

I'm nominating this for backport because I ran into this problem on Fedora for 1.85-beta:
#133793 (comment)

@rustbot label +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 22, 2025
@wesleywiser
Copy link
Member

Nick is on vacation so the assignment didn't go through but I think we can go-ahead and merge this now.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 22, 2025

📌 Commit aef640a has been approved by wesleywiser

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 Jan 22, 2025
scottmcm
scottmcm reviewed Jan 22, 2025
View reviewed changes
compiler/rustc_parse/src/parser/mod.rs
// nonterminals. Make sure it doesn't unintentionally get bigger.
#[cfg(all(target_pointer_width = "64", not(target_arch = "s390x")))]
// nonterminals. Make sure it doesn't unintentionally get bigger. We only check a few arches
// though, because `TokenTypeSet(u128)` alignment varies on others, changing the total size.
Copy link
Member

Choose a reason for hiding this comment

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

Not this PR, and I don't know if it's worth bothering, but could try changing the other type to a https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/packed/struct.Pu128.html so it'd be smaller on x64 too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Either that, or put #[repr(packed(8))] directly on TokenTypeSet too. I have no idea if there are enough of these being passed around to really matter though.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jan 22, 2025
@jieyouxu
Rollup merge of rust-lang#135855 - cuviper:parser-size, r=wesleywiser
8acb4de 
Only assert the `Parser` size on specific arches

The size of this struct depends on the alignment of `u128`, for example
powerpc64le and s390x have align-8 and end up with only 280 bytes. Our
64-bit tier-1 arches are the same though, so let's just assert on those.

r? nnethercote
@jieyouxu jieyouxu mentioned this pull request Jan 22, 2025
Open
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2025
@bors
Auto merge of rust-lang#135862 - jieyouxu:rollup-s3ynwhx, r=jieyouxu
e5eea03 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#135557 (Point at invalid utf-8 span on user's source code)
 - rust-lang#135596 (Properly note when query stack is being cut off)
 - rust-lang#135638 (Make it possible to build GCC on CI)
 - rust-lang#135648 (support wasm inline assembly in `naked_asm!`)
 - rust-lang#135826 (Misc. `rustc_resolve` cleanups)
 - rust-lang#135827 (CI: free disk with in-tree script instead of GitHub Action)
 - rust-lang#135850 (Update the `wasm-component-ld` tool)
 - rust-lang#135855 (Only assert the `Parser` size on specific arches)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm left review comments

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@oli-obk oli-obk

Labels
beta-nominated Nominated for backporting to the compiler in the beta channel. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cuviper @rustbot @wesleywiser @bors @scottmcm @oli-obk