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

RFC: Add target_abi configuration #2992

Merged
merged 3 commits into from
Jan 13, 2021
Merged

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Sep 27, 2020

This proposes a new cfg: target_abi, which specifies certain aspects of the target's Application Binary Interface (ABI). This also adds a CARGO_CFG_TARGET_ABI environment variable for parity with other CARGO_CFG_TARGET_* variables.

Rendered

@jonas-schievink jonas-schievink added A-cfg Conditional compilation related proposals & ideas A-target Target related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC. labels Sep 27, 2020
# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

`target_abi` is a key-value option set once with the target's ABI. The value is
Copy link

Choose a reason for hiding this comment

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

Does rustc already have the concept of target ABI internally? If not it seems really important to spell out how exactly this is defined. The concept of ABIs is really poorly understood by developers in my experience. Obviously there's a 1:1 mapping of target : ABI, but the inverse mapping is a little fuzzy to me and I'm not sure how useful it is in practice.

For example, I'm not really aware of an existing convention for talking about the "x86-64 System V ABI" despite it being the ABI used in both x86-64 Linux and macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically what I'm interested in is the component that comes after target_env in the triple string. What it indicates varies greatly between different targets. On some targets, it indicates the availability of hardware floats (e.g. eabihf for embedded abi, hardware float). On others, it indicates the running environment (e.g. x86_64-apple-ios-macabi for iOS apps that target macOS).

Copy link

Choose a reason for hiding this comment

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

To answer my own question, yes it does:
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/spec/abi/enum.Abi.html

However, this enum seems a lot more like it's describing "calling convention" than "ABI".

@luser
Copy link

luser commented Oct 1, 2020

So I definitely understand the motivation here: to distinguish between "iOS Simulator" and "iOS on macOS (Catalyst)" at build time, since they are otherwise both targeting the same CPU and OS.

Looking at the two built-in specs:
https://github.com/rust-lang/rust/blob/9cba260df0f1c67ea3690035cd5611a7465a1560/compiler/rustc_target/src/spec/x86_64_apple_ios.rs
https://github.com/rust-lang/rust/blob/9cba260df0f1c67ea3690035cd5611a7465a1560/compiler/rustc_target/src/spec/x86_64_apple_ios_macabi.rs

Effectively the only differences are:

  1. the llvm_target, which does include the -macabi suffix for the catalyst target
  2. The link_env_remove field, which is a quirky internal implementation detail used to determine specific environment variables to remove from compilation that affect LLVM and can get mixed up when cross-compiling.

However I note that target_env is unset for both specs, and the reference for that option describes it as:

Key-value option set with further disambiguating information about the target platform with information about the ABI or libc used.

I think you could make a pretty compelling argument for setting target_env = "macabi" for the Catalyst target, which would then let you distinguish the two by writing things like #[cfg(all(target_os = "ios", target_env = "macabi"))], which is very close to your provided example but avoids defining a new cfg attribute.

@cuviper
Copy link
Member

cuviper commented Oct 3, 2020

I think powerpc64 might want this for ELFv1 vs. ELFv2 -- rust-lang/rust#60617

@joshtriplett
Copy link
Member

We discussed this in the @rust-lang/lang meeting today. We're entirely in favor of doing this, and we just had one question:

Does this propose to change the value of target_env on any platform? Are there any platforms where the target_env contains the environment and the ABI concatenated together? target_env would need to keep its current value on those platforms, for compatibility.

If there's no compatibility break here, then 👍 for this RFC.

@rfcbot merge
@rfcbot concern compatibility

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 28, 2020

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Oct 28, 2020
@cuviper
Copy link
Member

cuviper commented Oct 28, 2020

Does this propose to change the value of target_env on any platform? Are there any platforms where the target_env contains the environment and the ABI concatenated together? target_env would need to keep its current value on those platforms, for compatibility.

If you're talking about targets like ARM, where the RFC says:

Embedded ABIs such as gnueabihf will define target_env as "gnu" and target_abi as "eabihf".

ARM Linux already has a simple target_env="gnu" (or "musl"), and there's no cfg reflecting such "eabihf" parts yet. However, it looks like a few arm*bsd targets do have the ABI in target_env. Those are Tier 3, and IMO should be fixed.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@joshtriplett
Copy link
Member

@cuviper Sounds reasonable. Could you please file an issue that lists the targets you found? We should also search the crater code corpus for instances of those target names to evaluate impact. And we should add some documentation in the codebase in places people will see when adding a new target, to make it clear that target_env should not include any ABI-related suffixes like eabi or eabihf.

@rfcbot resolved compatibility

@cuviper
Copy link
Member

cuviper commented Nov 5, 2020

@joshtriplett See rust-lang/rust#78791.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Dec 9, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 9, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Dec 19, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 19, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@KodrAus KodrAus merged commit d9d459f into rust-lang:master Jan 13, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Jan 13, 2021

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/rust#80970

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg Conditional compilation related proposals & ideas A-target Target related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants