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

Fix armv7-sony-vita-newlibeabihf LLVM target triple #138426

Merged
merged 1 commit into from
Apr 1, 2025
Merged

Fix armv7-sony-vita-newlibeabihf LLVM target triple #138426

merged 1 commit into from
Apr 1, 2025

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Mar 12, 2025 •
edited
Loading

It was previously normalized by LLVM to thumbv7a-vita-unknown-eabihf (can be seen with clang -target thumbv7a-vita-eabihf -v), which seems wrong, as Vita is the OS name.

Motivation: To make it easier to verify that cc-rs' conversion from rustc to Clang/LLVM triples is correct.

CC target maintainers @nikarh, @pheki and @zetanumbers.
r? jieyouxu

@madsmtm
Fix armv7-sony-vita-newlibeabihf LLVM target triple
65bd61d 
It was previously normalized by LLVM to `thumbv7a-vita-unknown-eabihf`,
which is probably wrong, as Vita is the OS.
@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 Mar 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

@madsmtm
Copy link
Contributor Author

madsmtm commented Mar 12, 2025

Another thing to consider: Is thumbv7a the correct arch to pass to LLVM, or should it actually be armv7 like the target name?

@jieyouxu
Copy link
Member

This seems to be basically if the target maintainers agree with this change, so please r=me if they do.
@bors delegate+ rollup

@bors
Copy link
Collaborator

bors commented Mar 12, 2025

✌️ @madsmtm, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@jieyouxu
Copy link
Member

@rustbot blocked (waiting to hear back from target maintainers, not much for me or PR author to do)

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2025
@pheki
Copy link
Contributor

pheki commented Mar 13, 2025

Interestingly some docs from Clang indicate that the vendor is optional but sys not so much: "The vendor needs to be specified only if there’s a relevant change, for instance between PC and Apple. Most of the time it can be omitted (and Unknown) will be assumed, which sets the defaults for the specified architecture. The system name is generally the OS (linux, darwin), but could be special like the bare-metal “none”."

That said, I'm personally fine with this change with one caveat: sony does not seem to be a valid target for LLVM[0] [1], it seems like they use SCEI for the PS4 and PS5, so that would probably make more sense. I checked for LLVM code checking for SCEI specifically and they don't seem to have any special cases that would affect us, so this probably wouldn't break anything.

@zetanumbers
Copy link
Contributor

I have originally used bare-metal llvm target arm7a-unknown-none or something like that, cause there's no vita support in clang. thumb7a was intentional, assuming such llvm target is sound.

@madsmtm madsmtm mentioned this pull request Mar 13, 2025
Open
@madsmtm
Copy link
Contributor Author

madsmtm commented Mar 13, 2025 •
edited
Loading

Interestingly some docs from Clang indicate that the vendor is optional but sys not so much: "The vendor needs to be specified only if there’s a relevant change, for instance between PC and Apple. Most of the time it can be omitted (and Unknown) will be assumed, which sets the defaults for the specified architecture. The system name is generally the OS (linux, darwin), but could be special like the bare-metal “none”."

Yeah, arguably the invalid normalization could be a LLVM bug?

I picked sony because that's what's passed by the mipsel-sony-psp/mipsel-sony-psx targets, and because LLVM is fine with invalid/unknown target vendors (and this does allow a forked LLVM to handle it differently if it wants to).

I'm fine with arm7a-unknown-none-eabihf or something else instead if that's what you prefer (though I do think it'd be nice if it was consistent with the other sony targets).

@pheki
Copy link
Contributor

pheki commented Mar 13, 2025 •
edited
Loading

thumbv7a is probably better as it is a thumb processor this target has thumb mode enabled. I'm personally in favor of either thumbv7a-sony-vita-eabihf or thumbv7a-scei-vita-eabihf in case we ever need to fork / contribute to llvm.

Edit: fixed target names

@jieyouxu
Copy link
Member

The new llvm target passed here is thumbv7a-sony-vita-eabihf, to double-check, @pheki do you have a preference of thumbv7a-scei-vita-eabihf instead of thumbv7a-sony-vita-eabihf? Otherwise, I'm inclined to r+ this.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 29, 2025
@jieyouxu
Copy link
Member

Please r=me with/without the llvm target name changed, subject to what @pheki prefers.
@bors delegate+

@bors
Copy link
Collaborator

bors commented Mar 29, 2025

✌️ @madsmtm, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2025
@pheki
Copy link
Contributor

pheki commented Mar 31, 2025

to double-check, @pheki do you have a preference of thumbv7a-scei-vita-eabihf instead of thumbv7a-sony-vita-eabihf

I do not, I'm fine either way, but I think that the argument for consistency with other sony targets is compelling, so I would just go with the PR as is, using thumbv7a-sony-vita-eabihf

@pheki pheki mentioned this pull request Mar 31, 2025
Open
@madsmtm
Copy link
Contributor Author

madsmtm commented Mar 31, 2025

Cool then!

@bors r=jieyouxu

@bors
Copy link
Collaborator

bors commented Mar 31, 2025

📌 Commit 65bd61d has been approved by jieyouxu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 31, 2025
@matthiaskrgr
Rollup merge of rust-lang#138426 - madsmtm:vita-llvm-target, r=jieyouxu
3646287 
Fix `armv7-sony-vita-newlibeabihf` LLVM target triple

It was previously normalized by LLVM to `thumbv7a-vita-unknown-eabihf` (can be seen with `clang -target thumbv7a-vita-eabihf -v`), which seems wrong, as Vita is the OS name.

Motivation: To make it easier to verify that [`cc-rs`' conversion from `rustc` to Clang/LLVM triples](rust-lang/cc-rs#1431) is correct.

CC target maintainers `@nikarh,` `@pheki` and `@ZetaNumbers.`
r? jieyouxu
This was referenced Mar 31, 2025
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 1, 2025
@bors
Auto merge of rust-lang#139194 - matthiaskrgr:rollup-2mhep38, r=matth…
97ea17b 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#138426 (Fix `armv7-sony-vita-newlibeabihf` LLVM target triple)
 - rust-lang#138840 (rustc_resolve: Test the order that preludes are resolved)
 - rust-lang#139039 (Reduce kw::Empty usage, part 4)
 - rust-lang#139151 (tidy: properly check for orphaned unstable_book pages)
 - rust-lang#139176 (Remove fragile equal-pointers-unequal/*/print3.rs tests.)
 - rust-lang#139179 (Remove me from vacation)
 - rust-lang#139181 (Fix invalid link in docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3257415 into rust-lang:master Apr 1, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 1, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 1, 2025
@rust-timer
Unrolled build for rust-lang#138426
25f9c7e 
Rollup merge of rust-lang#138426 - madsmtm:vita-llvm-target, r=jieyouxu

Fix `armv7-sony-vita-newlibeabihf` LLVM target triple

It was previously normalized by LLVM to `thumbv7a-vita-unknown-eabihf` (can be seen with `clang -target thumbv7a-vita-eabihf -v`), which seems wrong, as Vita is the OS name.

Motivation: To make it easier to verify that [`cc-rs`' conversion from `rustc` to Clang/LLVM triples](rust-lang/cc-rs#1431) is correct.

CC target maintainers ``@nikarh,`` ``@pheki`` and ``@ZetaNumbers.``
r? jieyouxu
@madsmtm madsmtm deleted the vita-llvm-target branch April 1, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jieyouxu jieyouxu

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
Milestone
1.88.0
Development

Successfully merging this pull request may close these issues.
6 participants
@madsmtm @rustbot @jieyouxu @bors @pheki @zetanumbers