Conversation
Target modifier flags are those that must be set across all compilation units as they indicate an ABI incompatibility. Knowledge of enabled target modifiers is necessary for naked functions or asm in general to appropriately generate the right assembly (for BTI on AArch64, for example). Therefore, all target modifier flags now set a corresponding `target_modifier_$x` cfg. Flags that do not accept values similarly have cfgs without values, whereas flags that do have values have cfgs with values. Flags that take comma-separated values will set multiple cfgs. Not all target modifiers need to have cfgs - for example, sanitizers are a target modifier but have their own cfg already so do not have an automatically generated target modifier cfg. It would be relatively straightforward to change these to `target_modifier($x)` or just `$x` if that were preferred.
Each of the target modifier cfgs for unstable target modifier flags are gated on `feature(cfg_unstable_target_modifier)` and will be stabilised automatically alongside their flag counterparts. All target modifiers are currently unstable so this doesn't stabilise anything. It might be better to separate these so that cfgs can be stabilised separately, but given the motivation for target modifier cfgs, perhaps not. feature gate test
As with all other builtin cfgs, target modifier cfgs cannot be set by users.
`check-cfg` needs to be aware of the name of cfgs to avoid the `unexpected_cfgs` lint from triggering.
As one of the motivating examples of making target modifiers have cfgs, `-Zbranch-protection` should be made a target modifier.
|
Some changes occurred in cfg and check-cfg configuration cc @Urgau Some changes occurred in compiler/rustc_attr_parsing |
| `target_modifier_branch_protection` | ||
| `target_modifier_fixed_x18` | ||
| `target_modifier_indirect_branch_cs_prefix` | ||
| `target_modifier_reg_struct_return` | ||
| `target_modifier_regparm` | ||
| `target_modifier_retpoline` | ||
| `target_modifier_retpoline_external_thunk` |
There was a problem hiding this comment.
Here I would like to remind everyone of this section from the target modifiers RFC:
Teaching
We should be careful to not introduce too many concepts that end-users have to learn.
It is intentional that the guide-level section of this RFC does not use the word “target modifier”. The “target modifier” name is not intended to be used outside of the compiler internals and very technical documentation. Compiler errors should not say “error: trying to mix target modifiers” or something like that; rather the error should just say that mixing
-Cfoomay cause ABI issues.For similar reasons, the flag for silencing the error is called
-Cunsafe-allow-abi-mismatchwith the word “ABI” to avoid having to teach the user about mismatched flags or target modifiers.
Some of the proposed namings above would involve changing the above decision. That's okay with me, but if so, we should be explicit that we're changing it.
There was a problem hiding this comment.
Echoing what I said on Zulip: I feel like the cfg needs to match the CLI option. That is, if the flag is -Cbranch-protection=bti, the cfg should be cfg(branch_protection = "bti"). Or, if we want to treat target modifiers as more akin to target features (from Alice's snip I don't think this is the case), then -Ctarget-modifier=branch-protection=bti and cfg(target_modifier = "branch-protection=bti").
I'm not sure if the target_modifier prefix adds too much here, outside of perhaps reminding the user that it's an ABI flag?
There was a problem hiding this comment.
I'm aware of both concerns - I picked the name in the patch relatively arbitrarily figuring that it'll be bikeshed a bit in t-lang discussions and the name in the prototype isn't all that important. I'm happy with either surfacing "target modifier" in the flags and cfgs, or not doing so.
There was a problem hiding this comment.
Not really this PR's problem, but it would almost be nice if this was -T branch-protection=bti, along with a consistent rule of "you can mismatch -C flags across crates, but -T flags need to be the same".
Conceptually, at least, I like the thought of having a way to talk about these things that's used-facing other than that "well there's a bunch of flags that blow up if you're inconsistent about them in the crate graph".
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| let gate = | ||
| Z_OPTIONS.iter().filter_map(|opt| opt.target_modifier_cfg()).find(|cfg| *cfg == name).map( | ||
| |cfg| { | ||
| ( | ||
| cfg, | ||
| sym::cfg_unstable_target_modifier, | ||
| Features::cfg_unstable_target_modifier as for<'a> fn(&'a Features) -> _, | ||
| ) | ||
| }, | ||
| ); | ||
| if let (Some(feats), Some(gated_cfg)) = (features, gate) { | ||
| gate_cfg(&gated_cfg, span, sess, feats); | ||
| } |
There was a problem hiding this comment.
| let gate = | |
| Z_OPTIONS.iter().filter_map(|opt| opt.target_modifier_cfg()).find(|cfg| *cfg == name).map( | |
| |cfg| { | |
| ( | |
| cfg, | |
| sym::cfg_unstable_target_modifier, | |
| Features::cfg_unstable_target_modifier as for<'a> fn(&'a Features) -> _, | |
| ) | |
| }, | |
| ); | |
| if let (Some(feats), Some(gated_cfg)) = (features, gate) { | |
| gate_cfg(&gated_cfg, span, sess, feats); | |
| } | |
| let gate = Z_OPTIONS.iter().filter_map(|opt| opt.target_modifier_cfg()).find(|cfg| *cfg == name); | |
| if let (Some(feats), Some(cfg)) = (features, gate) { | |
| let gated_cfg = (cfg, sym::cfg_unstable_target_modifier, Features::cfg_unstable_target_modifier as _); | |
| gate_cfg(&gated_cfg, span, sess, feats); | |
| } |
Think this may read a bit better than constructing the tuple in the map
|
We discussed this in the lang team triage call:
|
This is a rough draft that can be nominated for t-lang, plenty of the details will change.
The issue this aims to solve...
In #151486, Chromium uses
-Zbranch-protection=bti, which is an AArch64 extension for control-flow integrity (e.g. to protect against ROP attacks). The idea is that every indirect branch target has to start with a BTI landing pad instruction that tells the CPU whether it is a valid target for calls or jumps, otherwise the CPU faults.Since #149633 and #144938, we enable a
outline-atomicsfeature on AArch64 targets. This lowers atomic operations to out-of-line helper functions, which can use the Large System Extension (LSE) if present, and otherwise falls back to an implementation that doesn't use LSE. Rust's fallback implementation is provided by compiler-builtins.When
-Zbranch-protection=btiis used, compiler-builtins'outline-atomicsroutines need to have the BTI hint instruction so that the program can jump to it without the CPU faulting. These are normally added automatically by-Zbranch-protection=btibut outline-atomics' implementations use naked functions, so correctly don't automatically have the BTI hint instructions.We'd like to add the BTI hint instruction to these, but there is no way of knowing that BTI is enabled when writing the naked function. LLVM does this conditionally when they generate their implementations (here), because they have a preprocessor define they can check. BTI isn't a target feature, so there's no
cfg(target_feature = "bti")that could be used to cfg this in compiler-builtins.(Technically BTI uses hint-space instructions, that older CPUs without support for BTI would ignore, so it could be added to these functions unconditionally. However, this still has a code-size cost and the other overheads of a no-op, like instruction cache pressure.)
-Zbranch-protectionisn't a target modifier but should be, we just haven't made it one yet because it's waiting on build-std to be stabilised anyway. It would be useful to have a cfg for BTI to be able be able to write naked functions that are aware of it, e.g.cfg(branch_protection = "bti"). During the discussion on Zulip, it was suggested that perhaps this same logic applies to all target modifiers.What this proposes..
Target modifier flags are those that must be set across all compilation units as they indicate an ABI incompatibility (or in general that the flag is only effective if the entire crate graph also has it set, e.g. for pointer authentication, the security isn't as useful if there are functions compiled without it). Knowledge of enabled target modifiers is necessary for naked functions or asm in general to appropriately generate the right assembly (for BTI on AArch64, for example).
This patch proposes automatically adding a cfg for target modifier flags (e.g.
-Zbranch-protection=bti->cfg(target_modifier_branch_protection="bti"). Flags that do not accept values similarly have cfgs without values (e.g.-Zfixed-x18->cfg(target_modifier_fixed_x18)), whereas flags that do have values have cfgs with values (e.g.-Zregparm=0->cfg(target_modifier_regparm="0")). Flags that take comma-separated values will set multiple cfgs (e.g.-Zbranch-protection=bti,pac-ret->cfg(target_modifier_branch_protection="bti"),cfg(target_modifier_branch_protection="pac-ret")).Not all target modifiers need to have cfgs - for example, sanitizers are a target modifier but have their own cfg already so this patch does not have an automatically-generated target modifier cfg.
Each of the target modifier cfgs for unstable target modifier flags are gated on
feature(cfg_unstable_target_modifier)and will be stabilised automatically alongside their flag counterparts. All target modifiers are currently unstable so this doesn't stabilise anything.Like all other builtin cfgs, users cannot set target modifier cfgs via
--cfg.This patch makes
-Zbranch-protectiona target modifier in the same patch, just to test this works with a target modifier that accepts comma-separated values, but that can be split out into a separate PR.With this patch, the original issue could be resolved by adding..
..in the
naked_asm!of the compiler-builtins implementations.Unresolved questions for t-lang...
Do we want this?
-Zbranch-protectionto solve the original issue?What should these be called?
target_modifier_$flag,target_modifier($flag),$flag, something different for eachDoes it make sense to group target modifier flags together to match the cfg?
-Ztarget-modifier branch-protection="bti"?Should each target modifier cfg have a separate feature to permit its use?
feature(target_modifier_branch_protection_cfg)rather thanfeature(cfg_unstable_target_modifier)Should target modifier cfgs be stabilised alongside their flags?
Compiler flags have different stability guarantees than language features, as sometimes the output of a flag can't be guaranteed to never change (e.g. it might depend on LLVM's behaviour), or the values accepted by a flag may change over time (e.g.
--targetas targets can be added and removed), or a flag may be deprecated (-Csoft-float). Does the fact that target modifier flags have cfgs that are subject to the language's stability guarantees change anything?cc #t-libs > bti in compiler-builtins' aarch64 outline-atomics @ 💬, #151486
r? @ghost (the exact implementation isn't super important yet until we've decided if we want this or not)