[davidtwco]

View all comments

cc rust-lang/compiler-team#980 (#t-compiler/major changes > group target modifier options under `-T` compiler-team#980)

This patch re-implements target modifiers so that they are grouped under -T instead of being regular flags.

The current implementation is quite complex, using lots of macro_rules!s to filter out the target modifier options from the rest of the codegen options, and then storing the original string values of the target modifiers in cross-crate metadata before re-parsing them again in the downstream crate. Some of this complexity is desirable - we don't want to store every option value in the cross-crate metadata because that would hurt compile times. This patch removes the current approach entirely and starts again, leveraging simplifications made possible by the -T design.

There's a decent amount of code here but it hopefully isn't too tricky to review.

• The first commit removes the existing target modifier infrastructure
• The second commit is a small renaming
• The next set of commits (from sess: option metadata until sess: -Tllvm-target-feature) implements group target modifier options under -T compiler-team#980
  • A new TargetOptions struct is introduced that is parsed from -T and target modifier options are moved to it
  • TargetOptions is serialised in its entirety into cross-crate metadata, which is a lot simpler and isn't storing any more information than necessary
  • Local crate-only tracking of whether an option was actually set by the user is introduced, which is necessary to reproduce the previous diagnostics
  • Options can be set as TRACKED_UNSTABLE to require that -Zunstable-options be provided if they are used - all current target modifiers are unstable so this is used for all of them
  • Sanitizers are in both -T and -Z, with only address and leak being in -Z and everything else being in -T w/ -Zunstable-options
  • -Ttarget-cpu is required for targets with TargetOptions::requires_consistent_cpu, otherwise -Ctarget-cpu remains usable
  • -T itself is insta-stable, but all the options under are unstable, so it's effectively unstable
• The final set of commits (after sess: -Tllvm-target-feature) implements @Darksonn's suggestion from #t-compiler > Using -T for target modifiers and forwards compatibility @ 💬
  • tl;dr all -C options can also be used as -T options
    • We can remove these commits if we decide that we don't actually want to go this route, but I figured I'd try implement it
  • TargetOptions is removed again and target modifier options are now moved into CodegenOptions (they're still unstable thanks to the aforementioned TRACKED_UNSTABLE)
  • CodegenOptions is parsed from both -C and -T
    • -T takes precedence
  • Options can optionally be marked as TargetModifierFilter::Only (only -T, no -C) or TargetModifierFilter::Never (only -C, no -T)
  • -T option values are stored into a FxHashMap, these are stored into cross-crate metadata so we know which options were set as target modifiers in dependencies and we only store those that were set with -T
    • This hashmap is keyed by an CodegenOptionsKey enum that has a variant for each option
    • This hashmap's values are a TargetModifierValue type that contains variants for each distinct value type of the options we support as being target modifiers
      • Any option value types that TargetModifierValue doesn't support should be marked as never-target-modifiers, I haven't done that yet and rustc will ICE if you use them with -T
      • TargetModifierValue was introduced as an attempt to try and keep cross-crate metadata size down - avoiding Box<dyn Any> or storing strings for every option
  • -T is insta-stable for any of the currently stable -C options (but this is prevented for each flag currently with TargetModifierFilter::Never)

Questions:

• Which version of the patch do we want? Options can't be used with -C and -T most of the time, or flags can interchangably be used with -C and -T
• (for the interchangeable -C/-T version) All flags that aren't currently target modifiers are marked as not being usable with -T to minimise the change from this patch
• (for the interchangeable -C/-T version) -Csanitizers only accepts leaf and address in this patch, rather than relying on the -C/-T interchangability, is that desirable?
• (for the interchangeable -C/-T version) All of the target modifier flags are limited to -T, is that desirable?
• -Zsanitizers-cfi-normalize-integers was previously only a target modifier for -Zsanitizers=kcfi, not -Zsanitizers=cfi, but it is now a target modifier for both, what do we want to do about this?
• Sanitizers require -Zunstable-options in addition to the per-target stability check that the flag has always had

[rustbot]

r? @adwinwhite

rustbot has assigned @adwinwhite.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 17 candidates

[adwinwhite]

@rustbot reroll

[mu001999]

@rustbot reroll

[rust-bors]

☔ The latest upstream changes (presumably #161347) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[RalfJung]

What is the behavior if one crate has -Cflag=a and another has -Tflag=b? I assume this will error? What if one crate has -Cflag=a -Tflag=b and another has -Tflag=b? Do we just error when -C and -T are both used for the same flag?

-Csanitizers only accepts leaf and address in this patch, rather than relying on the -C/-T interchangability, is that desirable?

I think it makes sense to treat it like target-cpu -- sometimes you're not allowed to use -C, you have to use -T.

All of the target modifier flags are limited to -T, is that desirable?

In the interest of having this PR change as few things at the same time as possible, that makes sense to me. We can always relax it later.

-Zsanitizers-cfi-normalize-integers was previously only a target modifier for -Zsanitizers=kcfi, not -Zsanitizers=cfi, but it is now a target modifier for both, what do we want to do about this?

I would expect this to behave like -Ctarget-cpu: if you use -Zsanitizers-cfi-normalize-integers it's not a target modifier, if you use -Tsanitizers-cfi-normalize-integers it is a target modifier; for kcfi and cfi we require you to use the -T variant.

[petrochenkov]

I did a high level review today.

• First, is is possible to break this into parts that can be landed as separate PRs? This PR is large and I'll have to review it as a whole, not (only) per-commit, because the commits have many back and forth changes. If it's possible with some minimal effort, then it would be better to do that, I'll be able to process and merge the individual parts quickly.
• In particular, the part about -C/-T support has design questions, is it possible to defer the parts with design questions to a separate later PR?
• It's clear that some -T options will need to keep the -C form for backward compatibility. Is it possible to do it in a minimal way, in which we just continue doing the same thing as before this PR, without making changes raising design questions?
• This is a major breakage for nightly users of -Z options. When -Z options are stabilized as -C there's typically some migration period in which both versions are supported. Perhaps it makes sense for the -T options as well?

In the interest of having this PR change as few things at the same time as possible, that makes sense to me. We can always relax it later.

I'd also suggest to merge the strictest version first - options can't be used with -C and -T most of the time, etc.

[RalfJung]

There is one design question that has major impact on the code, I think -- is there one pool of options that -C and -T both feed into (with flags controlling which option is exposed via which "entry point"), or are there two separate pools of options (with duplication if an option appears in both)? I'm honestly still undecided about that. We do have relatively few options that we want to support in both (I think?), but that list may grow and it seems annoying to start out by having to duplicate some stuff.

[petrochenkov]

There is one design question that has major impact on the code, I think -- is there one pool of options that -C and -T both feed into (with flags controlling which option is exposed via which "entry point"), or are there two separate pools of options (with duplication if an option appears in both)? I'm honestly still undecided about that. We do have relatively few options that we want to support in both (I think?), but that list may grow and it seems annoying to start out by having to duplicate some stuff.

To me -T looks more like a subset of -C options with some additional requirements, not a separate thing (i.e. basically what we have now on nightly), so the "one pool" alternative looks more suitable.

[lqd]

I've recently been helping to make progress on this PR, rebasing it (here is the branch, as of last week), discussing with Vadim about their questions and review comments and how they can be addressed, what additional work we need to do in the PR vs future PRs, and so on.

David has implemented the approach described by Alice on Zulip (to be able to use most -C options as -T), and I've also expanded this forward compatibility to the current unstable target modifiers (so -Z in addition to -C/-T).

However, after doing the work, David and I discussed some of our conclusions about this plan, and the tradeoffs we saw made us feel like the idea of using -T for target modifiers may not be worth it in practice. From the experience of implementing it, seeing the complexity, the impact on tests and users, etc versus the perceived usefulness of it, it makes us think we probably wouldn't want to actually land this PR (or rust-lang/compiler-team#980 in general), not even the first implementation before implementing Alice's suggestion, that we can see in the first 16 commits or so.

I'll post updates in the other PRs marked as blocked on that work as well. And let's close this PR as well.

[RalfJung]

However, after doing the work, David and I discussed some of our conclusions about this plan, and the tradeoffs we saw made us feel like the idea of using -T for target modifiers may not be worth it in practice. From the experience of implementing it, seeing the complexity, the impact on tests and users, etc versus the perceived usefulness of it, it makes us think we probably wouldn't want to actually land this PR (or rust-lang/compiler-team#980 in general), not even the first implementation before implementing Alice's suggestion, that we can see in the first 16 commits or so.

Oh. What is the alternative though? The current target modifier system is kind of messy on the implementation side and a bit ad hoc too. I quite liked the easy "-T means it's consistent everywhere". Also we don't really have an alternative plan for flags that are sometimes target modifiers, and sometimes not, do we? (like -Ctarget-cpu)

[Darksonn]

The main alternative I can see is to only have -C for flags, and to just have the compiler know whether it's a target modified or not (optionally depending on --target or similar) and apply the target modifier logic to flags that are target modifiers.

[RalfJung]

Yeah that is pretty much the status quo. I guess we can just keep going in that direction. Hopefully the implementation can be cleaned up a bit, it is currently very macro-heavy and does not work well for values that need to be recorded and in sync across the crate graph without being literally the value of a -C flag.

And let's close this PR as well.

Let's do that then.

[lqd]

The current target modifier system is kind of messy on the implementation side and a bit ad hoc too.

Yeah, I think everyone who has looked into this agrees 100%.

What is the alternative though?

I'm not sure we need an alternative. To me, we keep doing what we're already doing, with the currently suboptimal system and try to slowly improve it to remove the cruft.