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

Tracking Issue for cfg-target-abi #80970

Closed
2 of 4 tasks
KodrAus opened this issue Jan 13, 2021 · 22 comments · Fixed by #119590
Closed
2 of 4 tasks

Tracking Issue for cfg-target-abi #80970

KodrAus opened this issue Jan 13, 2021 · 22 comments · Fixed by #119590
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. O-ios Operating system: iOS S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@KodrAus
Copy link
Contributor

KodrAus commented Jan 13, 2021

This is a tracking issue for the RFC "Add target_abi configuration" (rust-lang/rfcs#2992).
The feature gate for the issue is #![feature(cfg_target_abi)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@KodrAus KodrAus added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jan 13, 2021
@nvzqz
Copy link
Contributor

nvzqz commented Feb 7, 2021

  • I came across x86_64_fortanix_unknown_sgx and I'm wondering if sgx should be changed from being env to abi? I assume not, but I figured I'd ask.

  • I'm also curious about aarch64_unknown_none_softfloat: should softfloat be listed as ABI? I figure it's already covered by target feature -fp-armv8.

  • I noticed that darwinpcs is passed as -target-abi to clang for arm64 iOS targets. So now I'm wondering if the macabi targets should actually be target_abi of darwinpcs and target_env of macabi.

@joshtriplett
Copy link
Member

rustc --print cfg should print target_abi. Right now, rustc --target arm-unknown-linux-gnueabi --print cfg and rustc --target arm-unknown-linux-gnueabihf --print cfg have the same output.

@joshtriplett
Copy link
Member

@nvzqz For x86_64-fortanix-sgx, I think "sgx" should remain the "os" and "fortanix" should be moved from "vendor" to either "abi" or "env".

@joshtriplett
Copy link
Member

Implementation in progress at #86922

@oli-obk
Copy link
Contributor

oli-obk commented Jul 8, 2021

@nvzqz
Copy link
Contributor

nvzqz commented Mar 18, 2022

@oli-obk custom targets like that would specify "abi": "eabihf" to match the underlying LLVM target's ABI. Otherwise #[cfg(target_abi = "eabihf")] should not match the custom target.

With #86922 merged, I think this is ready to stabilize.

@joshtriplett joshtriplett added the S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR label Jul 20, 2022
@joshtriplett
Copy link
Member

For anyone wanting to act on the ready-to-stabilize label: also note the FIXME comments in the target files, added in #86922, and make the corresponding adjustments when stabilizing. That PR will also need the relnotes label.

@jrose-signal
Copy link

Ended up here precisely because I wanted to distinguish iOS device and simulator targets in cfg. I'm not sure what the high-level difference is between target_env and target_abi, but by putting the iOS variants in target_abi, I can't get at them on stable.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jan 4, 2024

Stabilization report

I propose to stabilize #[cfg(target_abi = "...")], This implements RFC-2992 (cfg-target-abi). The implementation was completed in #86922 and this tracking issue was subsequently marked as ready for stabilization by @joshtriplett.

Summary

This stabilizes the cfg option called target_abi:

#[cfg(target_abi = "macabi")]

And target_abi is also shown when using --print=cfg (output snipped for length):

> rustc --print=cfg --target aarch64-apple-ios-sim

target_abi="sim"
target_arch="aarch64"
target_env=""
target_os="ios"
target_vendor="apple"

Without target_abi, cfgs are limited to target_arch, target_vendor, target_os, and target_env. However, some targets are only differentiated by their abi and thus it's necessary to resort to parsing the full target string in a build script when there's a need to disambiguate. For example, the following targets are the same if only using stable target_* cfgs:

  • aarch64-apple-ios and aarch64-apple-ios-sim (arch: "aarch64", vendor: "apple", os: "ios", env: "")
  • x86_64-pc-windows-gnullvm and x86_64-pc-windows-gnu (arch: "`x86_64", vendor: "pc", os: "windows", env: "gnu")

Notes

The target_abi defaults to "" (the empty string) and most targets don't set it. This is similar to target_env where if it's not needed for disambiguation then it's often not set.

In the future target_abi could be an array of zero or more properties that affect the ABI (e.g. softfloat may be combined with other ABI properties), However, this feature can be added later without breaking compatibility.

I guess an alternative to stabilizing this would be to extend target_env to be an array of values that includes abi and other things.

Tests

tests/ui/cfg/cfg-target-abi.rs
tests/ui/check-cfg/well-known-values.rs

Documentation

rust-lang/reference#1446

Stabilization PR

#119590. This also addresses the FIXMEs left by #86922, where existing (tier 3) values need to change once cfg_target_abi is stable.

@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

Nominating on behalf of @ChrisDenton on the basis of the stabilization report above.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 4, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Feb 6, 2024

Pardon, @traviscross @rust-lang/lang but this issue came up in discussion and we were wondering if this requires lang FCP to move forward? The RFC was FCPed but the stabilization report needs to have another one for final consensus check on shipping this implementation, right?

I'm just asking because I realized it's not actually clear to me if the lang-nom means T-lang should do a sync meeting or if this just needs an async FCP or what.

@workingjubilee
Copy link
Member

workingjubilee commented Feb 6, 2024

The rustc dev guide does document FCP itself being a requirement of stabilization though interestingly the tracking issue template doesn't have FCP as a separate checkbox. I guess because for a while stabilization PRs were basically considered synonymous with FCP? At some point we moved towards putting stabilization reports more often on the issue itself, although there's no formal rule requiring that, so they happen in both places. In fact, as far as the dev guide imagines it, the FCP is strictly sequenced first instead of the last few things happening concurrently (as they do in actual fact).

Perhaps the template should have a separate checkbox for FCP, so we don't lose track of that? A lot of tracking issues aren't updated with their final task boxes checked and their links updated, anyways, but that would at least make it easier.

@traviscross
Copy link
Contributor

@workingjubilee: Thanks for the question.

As you mention, this indeed would require some action by the lang team for this to stabilize. That most likely would be an FCP, but in certain cases of clear calls or where we've recently approved something related, it could also be simply a meeting consensus of the team that is then communicated here.

The nomination means that it's "nominated for us to discuss, typically synchronously." The nomination by itself does not affect whether or not, e.g., an FCP would be required. Since it is nominated, we're trying to get to this in a meeting so that we can discuss it and help it move forward.

@workingjubilee
Copy link
Member

Ah I see! Thank you for clarifying.

@joshtriplett
Copy link
Member

Based on the stabilization report, this does indeed look ready for stabilization.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Feb 11, 2024

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

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 11, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Feb 14, 2024
@rfcbot
Copy link

rfcbot commented Feb 14, 2024

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 14, 2024
@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the lang triage meeting today, and the consensus was that we want to do this. It's now in FCP. Thanks to @KodrAus and to @ChrisDenton for pushing forward on this.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 14, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 17, 2024
…ios, r=workingjubilee

Fix `cfg(target_abi = "sim")` on `i386-apple-ios`

Since rust-lang#80970 is stabilizing, I went and had a look, and found that the result was wrong on `i386-apple-ios`.

r? rust-lang/macos
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 17, 2024
…ios, r=workingjubilee

Fix `cfg(target_abi = "sim")` on `i386-apple-ios`

Since rust-lang#80970 is stabilizing, I went and had a look, and found that the result was wrong on `i386-apple-ios`.

r? rust-lang/macos
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 17, 2024
…ios, r=workingjubilee

Fix `cfg(target_abi = "sim")` on `i386-apple-ios`

Since rust-lang#80970 is stabilizing, I went and had a look, and found that the result was wrong on `i386-apple-ios`.

r? rust-lang/macos
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 17, 2024
Rollup merge of rust-lang#121210 - madsmtm:fix-target-abi-i386-apple-ios, r=workingjubilee

Fix `cfg(target_abi = "sim")` on `i386-apple-ios`

Since rust-lang#80970 is stabilizing, I went and had a look, and found that the result was wrong on `i386-apple-ios`.

r? rust-lang/macos
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 18, 2024
…rkingjubilee

Fix `cfg(target_abi = "sim")` on `i386-apple-ios`

Since rust-lang/rust#80970 is stabilizing, I went and had a look, and found that the result was wrong on `i386-apple-ios`.

r? rust-lang/macos
@madsmtm
Copy link
Contributor

madsmtm commented Feb 24, 2024

The target_abi defaults to "" (the empty string) and most targets don't set it.

target_os uses "none" instead of the empty string, maybe that's actually the saner value?

The empty string feels to me more like "no-one thought about what to put here so rustc just defaulted to something, but you can't actually rely on #[cfg(target_abi = "")] working".

Perhaps, if we want target_abi to potentially be multiple values, maybe we shouldn't even emit a cfg for when there is no target ABI? That way, people will be forced to write e.g. #[cfg(not(target_abi = "sim"))] if they want to write code for non-simulator targets, instead of #[cfg(target_abi = "")], which is probably more portable, e.g. in case we get something that sets both target_abi = "macabi" and target_abi = "sim" (hypothetical, but you get the idea)?

@joshtriplett
Copy link
Member

joshtriplett commented Feb 24, 2024

@madsmtm I think "" is consistent with how the ABI appears in target triples. target_os uses "none" because of target names like x86_64-unknown-none. target_abi uses "" rather than "none" because we don't, for instance, write x86_64-unknown-linux-muslnone the way we write armel-unknown-linux-musleabihf.

I think it's valid to say "there isn't a special ABI", rather than (for instance) having to enumerate ABIs.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 24, 2024
@rfcbot
Copy link

rfcbot commented Feb 24, 2024

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.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Feb 24, 2024
@bors bors closed this as completed in e13f454 Feb 25, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 25, 2024
Rollup merge of rust-lang#119590 - ChrisDenton:cfg-target-abi, r=Nilstrieb

Stabilize `cfg_target_abi`

This stabilizes the `cfg` option called `target_abi`:

```rust
#[cfg(target_abi = "eabihf")]
```

Tracking issue: rust-lang#80970

fixes rust-lang#78791
resolves rust-lang#80970
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. O-ios Operating system: iOS S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.