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

Add binary_format to rustc target specs #135724

Closed
Noratrieb opened this issue Jan 19, 2025 · 10 comments · Fixed by #136637
Closed

Add binary_format to rustc target specs #135724

Noratrieb opened this issue Jan 19, 2025 · 10 comments · Fixed by #136637
Assignees
Labels
A-target-specs Area: Compile-target specifications A-targets Area: Concerning the implications of different compiler targets C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Noratrieb
Copy link
Member

Noratrieb commented Jan 19, 2025

There are some places in rustc that need to know the binary format of the target (to generate object files). It would be good to have this as a property on the target spec to avoid needing to have ad-hoc logic for it.

See #135695 (comment)

@Noratrieb Noratrieb added A-target-specs Area: Compile-target specifications A-targets Area: Concerning the implications of different compiler targets C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 19, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 19, 2025
@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 19, 2025
@yegeunyang

This comment has been minimized.

@yegeunyang

This comment has been minimized.

@Pyr0de
Copy link
Contributor

Pyr0de commented Feb 4, 2025

Hey,
I would like to give this issue a go. Could you please provide me with some more info about it?

Do I need to add a new field in Target that has the BinaryFormat?

@Noratrieb
Copy link
Member Author

Yes. You need to create a new field in the target with the binary format (create a new enum for this, don't use the one from the object crate). You will need to do a few other things as well but you may be able to figure that out from test failures and other code in the area.

To populate them for existing targets, you can find the existing logic where I linked it.

@Pyr0de
Copy link
Contributor

Pyr0de commented Feb 6, 2025

For the new enum, should it contain all the enum variants from BinaryFormat(object crate) or just the ones given in the logic mentioned in the OP?

To populate the existing targets, do I manually set the binary_format for each target in compiler/rustc_target/src/spec/targets or can I just use a function and call it in each file?

@rustbot claim

@Noratrieb
Copy link
Member Author

You should manually populate it, since the target is exactly to stop having this function. And only add the variants that we actually need.

@Pyr0de
Copy link
Contributor

Pyr0de commented Feb 6, 2025

Can the binary format be added to TargetOptions instead of Target since the logic is also based on TargetOptions?

@Noratrieb
Copy link
Member Author

Yes, I'd say put it there.

@Pyr0de
Copy link
Contributor

Pyr0de commented Feb 12, 2025

I have pushed the changes for the binary_format field in this PR. Do I need to make use of this field in place of is_like_*?

@Noratrieb
Copy link
Member Author

Yes

@bors bors closed this as completed in 2c6fa32 Feb 24, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 24, 2025
Rollup merge of rust-lang#136637 - Pyr0de:binary-format, r=Noratrieb

Add binary_format to rustc target specs

Added binary format field to `TargetOptions`

Fixes rust-lang#135724

r? `@Noratrieb`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-specs Area: Compile-target specifications A-targets Area: Concerning the implications of different compiler targets C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants