Skip to content

Let CStrings be either 1 or 2 byte aligned. #139785

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

fneddy
Copy link
Contributor

@fneddy fneddy commented Apr 14, 2025

We see a regression on the tests/assembly/cstring-merging.rs test on s390x.

Some architectures (like s390x) require strings to be 2 byte aligned. Therefor the section name will be marked with a .2 postfix on this architectures.

Allowing a section name with a .1 or .2 postfix will make the test pass on either platform.

Some architectures (like s390x) require strings to be 2 byte aligned.
Therefor the section name will be marked with a .2  postfix on this
architectures.

Allowing a section name with a .1 or .2 postfix will make the test pass
on either platform.
@rustbot
Copy link
Collaborator

rustbot commented Apr 14, 2025

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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 14, 2025
@Mark-Simulacrum
Copy link
Member

strings to be 2 byte aligned

Can you say more about this? Is this just in static strings embedded in the binary? In general we don't/can't guarantee that an arbitrary str is 2-byte aligned...

@fneddy
Copy link
Contributor Author

fneddy commented Apr 14, 2025

I am not entirely sure but here are my insights:

The s390x minimal alignment is set to 2 bytes here
Then this propagates through here1 here2.
Further this(LLVM Target Machine Data Layout) may play in here.

If this is not enough i'll ask @uweigand to elaborate further.

@fneddy
Copy link
Contributor Author

fneddy commented Apr 14, 2025

hmm so i guess i need to rephrase:

s390x requires global strings to be 2 byte aligned.

@uweigand
Copy link
Contributor

Basically, the s390x ABI requires that every global symbol is at least 2 byte aligned. The reason is that our "load address (PC-relative)" instruction requires a minimum 2 byte alignment of its target address.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 14, 2025

📌 Commit 1ac3d6b has been approved by Mark-Simulacrum

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 Apr 14, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 15, 2025
…ignment, r=Mark-Simulacrum

Let CStrings be either 1 or 2 byte aligned.

We see a regression on the `tests/assembly/cstring-merging.rs` test on s390x.

Some architectures (like s390x) require strings to be 2 byte aligned. Therefor the section name will be marked with a .2  postfix on this architectures.

Allowing a section name with a .1 or .2 postfix will make the test pass on either platform.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2025
Rollup of 11 pull requests

Successful merges:

 - rust-lang#139669 (Overhaul `AssocItem`)
 - rust-lang#139671 (Proc macro span API redesign: Replace proc_macro::SourceFile by Span::{file, local_file})
 - rust-lang#139750 (std/thread: Use default stack size from menuconfig for NuttX)
 - rust-lang#139785 (Let CStrings be either 1 or 2 byte aligned.)
 - rust-lang#139797 (Allow (but don't require) `#[unsafe(naked)]` so that `compiler-builtins` can upgrade to it)
 - rust-lang#139798 (normalize: prefer `ParamEnv` over `AliasBound` candidates)
 - rust-lang#139799 (Specify `--print info=file` syntax in `--help`)
 - rust-lang#139811 (Use `newtype_index!`-generated types more idiomatically)
 - rust-lang#139813 (Miri subtree update)
 - rust-lang#139822 (Fix: Map EOPNOTSUPP to ErrorKind::Unsupported on Unix)
 - rust-lang#139836 (Basic tests of MPMC receiver cloning)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2025
Rollup of 17 pull requests

Successful merges:

 - rust-lang#138374 (Enable contracts for const functions)
 - rust-lang#138380 (ci: add runners for vanilla LLVM 20)
 - rust-lang#138393 (Allow const patterns of matches to contain pattern types)
 - rust-lang#139517 (std: sys: process: uefi: Use NULL stdin by default)
 - rust-lang#139554 (std: add Output::exit_ok)
 - rust-lang#139660 (compiletest: Add an experimental new executor to replace libtest)
 - rust-lang#139669 (Overhaul `AssocItem`)
 - rust-lang#139671 (Proc macro span API redesign: Replace proc_macro::SourceFile by Span::{file, local_file})
 - rust-lang#139750 (std/thread: Use default stack size from menuconfig for NuttX)
 - rust-lang#139772 (Remove `hir::Map`)
 - rust-lang#139785 (Let CStrings be either 1 or 2 byte aligned.)
 - rust-lang#139789 (do not unnecessarily leak auto traits in item bounds)
 - rust-lang#139791 (drop global where-bounds before merging candidates)
 - rust-lang#139798 (normalize: prefer `ParamEnv` over `AliasBound` candidates)
 - rust-lang#139822 (Fix: Map EOPNOTSUPP to ErrorKind::Unsupported on Unix)
 - rust-lang#139833 (Fix some HIR pretty-printing problems)
 - rust-lang#139836 (Basic tests of MPMC receiver cloning)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5a9455f into rust-lang:master Apr 15, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 15, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2025
Rollup merge of rust-lang#139785 - fneddy:fix_test_cstring_merging_alignment, r=Mark-Simulacrum

Let CStrings be either 1 or 2 byte aligned.

We see a regression on the `tests/assembly/cstring-merging.rs` test on s390x.

Some architectures (like s390x) require strings to be 2 byte aligned. Therefor the section name will be marked with a .2  postfix on this architectures.

Allowing a section name with a .1 or .2 postfix will make the test pass on either platform.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 19, 2025
Rollup of 17 pull requests

Successful merges:

 - rust-lang#138374 (Enable contracts for const functions)
 - rust-lang#138380 (ci: add runners for vanilla LLVM 20)
 - rust-lang#138393 (Allow const patterns of matches to contain pattern types)
 - rust-lang#139517 (std: sys: process: uefi: Use NULL stdin by default)
 - rust-lang#139554 (std: add Output::exit_ok)
 - rust-lang#139660 (compiletest: Add an experimental new executor to replace libtest)
 - rust-lang#139669 (Overhaul `AssocItem`)
 - rust-lang#139671 (Proc macro span API redesign: Replace proc_macro::SourceFile by Span::{file, local_file})
 - rust-lang#139750 (std/thread: Use default stack size from menuconfig for NuttX)
 - rust-lang#139772 (Remove `hir::Map`)
 - rust-lang#139785 (Let CStrings be either 1 or 2 byte aligned.)
 - rust-lang#139789 (do not unnecessarily leak auto traits in item bounds)
 - rust-lang#139791 (drop global where-bounds before merging candidates)
 - rust-lang#139798 (normalize: prefer `ParamEnv` over `AliasBound` candidates)
 - rust-lang#139822 (Fix: Map EOPNOTSUPP to ErrorKind::Unsupported on Unix)
 - rust-lang#139833 (Fix some HIR pretty-printing problems)
 - rust-lang#139836 (Basic tests of MPMC receiver cloning)

r? `@ghost`
`@rustbot` modify labels: rollup
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

5 participants