Add back overriding-t-options for -m<os>-version-min diagnostic

[MaskRay]

Summary

This restores the -m<os>-version-min diagnostic behavior before D158137.
The diagnostic is more likely to be used above the threshold so that we
need to care about -Werror compatibility. Here are the primary behavior
difference:

• -Wno-overriding-t-options does not trigger -Wunknown-warning-option
• Both -Wno-overriding-t-options and -Wno-overriding-t-options can disable the -m<os>-version-min diagnostic (darwin-version.c)

overriding-t-options is not for the -ffp-model= diagnostic.
There are some uses cases (e.g. https://github.com/madgraph5/madgraph4gpu/issues/516),
but I do not want -Wno-overriding-t-options to affect -ffp-model= and
future diagnostics.

[aaron.ballman]

This seems like a reasonable approach to me. CC @dblaikie and @dexonsmith to make sure this addresses their concerns.

[dexonsmith]

clang/test/Driver/darwin-version.c

217

Why would we want to use the old name here? An alias seems strictly better to me.

[MaskRay]

clang/test/Driver/darwin-version.c

217

Making overriding-t-option an alias for overriding-option would make -Wno-overriding-t-option applies to future overriding option diagnostics, which is exactly what I want to avoid.

[dexonsmith]

clang/test/Driver/darwin-version.c

217

I understand that you don't want -t- to apply to work on future overriding option diagnostics, but I think the platform divergence you're adding here is worse.

Having a few Darwin-specific options use -Woverriding-t-option (and everything else use -Woverriding-option) as the canonical spelling is hard to reason about for maintainers, and for users.

And might not users on other platforms have -Woverriding-t-option hardcoded in build settings? (So @dblaikie's argument that we shouldn't arbitrarily make things hard for users would apply to all options?)

IMO, if we're not comfortable removing -Woverriding-t-option entirely, then it should live on as an alias (easy to reason about), not as canonical-in-special-cases (hard to reason about).

[hans]

clang/test/Driver/darwin-version.c

217

IMO, if we're not comfortable removing -Woverriding-t-option entirely, then it should live on as an alias (easy to reason about), not as canonical-in-special-cases (hard to reason about).

+1 if we can't drop the old spelling, an alias seems like the best option.

[MaskRay]

clang/test/Driver/darwin-version.c

217

Making overriding-t-option an alias for overriding-option, as I mentioned, will make -Wno-overriding-t-option affect new overriding-options uses. This is exactly what I want to avoid.

I know there are some -Wno-overriding-t-option uses. Honestly, they are far fewer than other diagnostics we are introducing or changing in Clang. I can understand the argument "make -Werror users easier for this specific diagnostic" (but -Werror will complain about other new diagnostics), but do we really want to in the Darwin case? I think no. They can remove the version from the target triple like https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50　or make the version part consistent with -m.*os-version-min.

This change may force these users to re-think how they should fix their build. I apology to these users, but I don't feel that adding an alias is really necessary.

[hans]

clang/test/Driver/darwin-version.c

217

Making overriding-t-option an alias for overriding-option, as I mentioned, will make -Wno-overriding-t-option affect new overriding-options uses. This is exactly what I want to avoid.

Is keeping them separate actually important, though? -Wno-overriding-option has the same issue in that case, that using the flag will also affect any new overriding-options uses, and I don't think that's a problem.

[MaskRay]

clang/test/Driver/darwin-version.c

217

-Wno-overriding-option is properly named, so affecting new overriding-options uses looks fine to me.
-Wno-overriding-t-option is awkward, and making it affect new uses makes me nervous.

The gist of my previous comment is whether the uses cases really justify a tiny bit of tech bit in clang and I think the answer is no.

This change is not about changing a semantic warning that has mixed opinions, e.g. -Wbitwise-op-parentheses (many consider it not justified).

[dexonsmith]

clang/test/Driver/darwin-version.c

217

The gist of my previous comment is whether the uses cases really justify a tiny bit of tech bit in clang and I think the answer is no.

I think we agree that we should add the minimal technical debt to clang.

This patch is harder-to-reason about, and thus bigger IMO, technical debt than adding an alias would be.

[MaskRay]

clang/test/Driver/darwin-version.c

217

Honestly when people asked whether we need back compatibility for -Werror uses. I disagree with the idea after considering the number of uses and legitimate uses. I've well summarized them up-thread.

Making overriding-option a super set of overriding-t-option is IMHO the only solution to make -Wno-overriding-t-option not affect other uses, which is what I strive to achieve.

If -Woverriding-t-option looks strange for the Darwin diagnostic and we really want to work around such -Werror users (I disagree as I mentioned), we could rename it to something like -Woverriding-darwin-option or something else, and add -Woverriding-t-option as an alias. Then the diagnostic becomes:

overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' [-Woverriding-darwin-option]

This would still achieve my goal of not making overriding-t-option affect overriding-option.

My most honest thinking is that we don't need any of the overriding-t-option tech debt. The users need to migrate. It's some work and I apologize to these users, but I don't think these uses are anything close to reasonable that justifies any debt on the clang side.

[dexonsmith]

clang/test/Driver/darwin-version.c

217

Making overriding-option a super set of overriding-t-option is IMHO the only solution to make -Wno-overriding-t-option not affect other uses, which is what I strive to achieve.

It's not clear why this specific piece matters. It seems moot to me. Any current users of overriding-t-option will blindly switch to the new spelling and, in effect, their old uses of -Woverriding-t-option [sic] will affect new instances of overriding-option.

Stepping back, here's what I think the effects of the three choices are.

With ToT:

• Current users of overriding-t-option will need to migrate to overriding-option. Whatever their reasons for having overriding-t-option, existing uses will blindly migrate to overriding-option, and thus blindly affect all future overriding-option diagnostics.
• Diagnostics will report as -Woverriding-option. Anyone seeing a new diagnostic will use the new spelling.
• Maintainers don't have to think about overriding-t-option anymore, except for supporting user migration.

With an alias:

• Current users of overriding-t-option will not need to migrate to overriding-option. Just like ToT, their existing uses will blindly affect all overriding-option diagnostics.
• Diagnostics will report as -Woverriding-option. Anyone seeing a new diagnostic will use the new spelling.
• Maintainers don't have to think about overriding-t-option anymore; it'll be clear that it's just an old spelling.

With this patch:

• Some current users of overriding-t-option will need to migrate to overriding-option; others will not.
• Some diagnostics will report as -Woverriding-option, others as -Woverriding-t-option, so new users hitting the latter will continue to add the old spelling to build settings.
• The difference between which are canonically "t" (or not) is an accident of history and will be hard to reason about.
• Maintainers who overriding-t-option will be tempted to clean it up, and need to dig up this thread to understand why it's like this, or land a change and hit the same problems.

Do you agree with these effects? If not, what part have I got wrong? Or have I missed another important effect?

If you agree that I have the effects correct, then I'm still confused as to how this patch would be easier to maintain or better for users than an alias.

Note that my personal stake in this is low. My only current involvement in LLVM is volunteering my time as a reviewer. If those with users (e.g., @dblaikie or @aaron.ballman, who added post-commit review to https://reviews.llvm.org/D158137) agree with you that this patch is the right way forward, I'm happy to let it go through.

[MaskRay]

clang/test/Driver/darwin-version.c

217

Thanks for taking time to write the summary. I agree with the analysis and sorry that this discussion has taken your valuable time.

If you agree that I have the effects correct, then I'm still confused as to how this patch would be easier to maintain or better for users than an alias.

This patch wasn't created with a good motivation. It was for discussion when people raised compatibility concern (valid) that I don't agree with, considering the scope of affected users and how reasonable the -Wno-overriding-t-option use is.

I do not want -Wno-overriding-t-option (even it is hidden) to affect good uses while an alias. I think I am happy with an alias that will be removed, say one year.

[dblaikie]

clang/test/Driver/darwin-version.c

217

Note that my personal stake in this is low. My only current involvement in LLVM is volunteering my time as a reviewer. If those with users (e.g., @dblaikie or @aaron.ballman, who added post-commit review to https://reviews.llvm.org/D158137) agree with you that this patch is the right way forward, I'm happy to let it go through.

(really appreciate your work, @dexonsmith, btw - both having historic context, and any help out with review load, etc, is really valuable)

Yeah, I'd rather see this as an alias. I don't feel like it's worth removing later, though. I don't think it's substantial technical debt to keep an old alias around. It doesn't add significant friction to the project that I can think of.

[MaskRay]

clang/test/Driver/darwin-version.c

217

(I was expecting more reasoning than "it's substantial technical debt, so we take it".)

I have performed a survey on existing Wno-overriding-t-option uses. Only some Darwin use cases like https://github.com/oldzhu/4dotnet/blob/master/package/dotnetcore/dotnetruntime/modified/configurecompiler.cmake.v6.0.2#L404 requires some thoughts. It may be on the boundary of the scale that I'd consider a workaround. Hence one of my previous comments said:

If -Woverriding-t-option looks strange for the Darwin diagnostic and we really want to work around such -Werror users (I disagree as I mentioned), we could rename it to something like -Woverriding-darwin-option or something else, and add -Woverriding-t-option as an alias. Then the diagnostic becomes:

If we rename warn_drv_overriding_t_option below and clarify the comment, I think the concern of new uses adopting warn_drv_overriding_t_option (to-be-renamed) will be very low.

// Don't use warn_drv_overriding_t_option for new diagnostics.
def warn_drv_overriding_t_option : Warning<
  "overriding '%0' option with '%1'">,
  InGroup<OverridingTOption>;
def warn_drv_overriding_option : Warning<
  "overriding '%0' option with '%1'">,
  InGroup<OverridingOption>;

Essentially, we split the overriding-option group and make the Darwin use its own group.

With this patch:

• Some current users of overriding-t-option will need to migrate to overriding-option; others will not.

Yes.

• Some diagnostics will report as -Woverriding-option, others as -Woverriding-t-option, so new users hitting the latter will continue to add the old spelling to build settings.

Yes.

The difference between which are canonically "t" (or not) is an accident of history and will be hard to reason about.

If we add -Wno-overriding-darwin-option as the canonical spelling, this concern can be addressed.

• Maintainers who overriding-t-option will be tempted to clean it up, and need to dig up this thread to understand why it's like this, or land a change and hit the same problems.

Yes.

[hans]

clang/test/Driver/darwin-version.c

217

I do not want -Wno-overriding-t-option (even it is hidden) to affect good uses while an alias.

This seems to be the core of the disagreement. Could you expand on why this is important?

[aaron.ballman]

clang/test/Driver/darwin-version.c

217

Note that my personal stake in this is low. My only current involvement in LLVM is volunteering my time as a reviewer. If those with users (e.g., @dblaikie or @aaron.ballman, who added post-commit review to https://reviews.llvm.org/D158137) agree with you that this patch is the right way forward, I'm happy to let it go through.

Assuming the analysis from @dexonsmith is accurate (thank you, that was a really handy summary!), I think going with an alias is a good tradeoff between maintenance burden and user experience, but I'd also like to understand this point better:

I do not want -Wno-overriding-t-option (even it is hidden) to affect good uses while an alias.

If we eventually get rid of the alias, then I can see why this would be a problem (particularly for -Werror users), but... I don't imagine a need to get rid of the alias. At most, I think we'd want to remove any mention of the alias from documentation, etc and so over time search engines will "forget" about it and so users won't organically run into the -t- alias.

[MaskRay]

clang/test/Driver/darwin-version.c

217

I do not want -Wno-overriding-t-option (even it is hidden) to affect good uses while an alias.

This seems to be the core of the disagreement. Could you expand on why this is important?

I think going with an alias is a good tradeoff between maintenance burden and user experience, but I'd also like to understand this point better:

This thought comes from my view of previous discussions regarding improved diagnostics break -Werror users. Frankly, I believe there are numerous other diagnostics that are hundred-time more disruptive than what we are currently observing as a relatively minor disturbance. Thus, I am having difficulty comprehending the rationale behind wanting to maintain this workaround indefinitely.

[aaron.ballman]

clang/test/Driver/darwin-version.c

217

For me, I think -Werror is a red herring -- users who want warnings to be errors are explicitly asking to make compiler upgrades more disruptive because compiler upgrades *always* come with new/different diagnostic behaviors. So I don't consider -Werror breaking the user to be an issue; we're doing what they asked us to do. We shouldn't make that more painful than it needs to be, but that's all.

What compels me are users who aren't using -Werror and do the compiler upgrade. Some class of users aren't going to see that this option has been renamed, it will simply stop working for them and they possibly won't even notice it. Adding an alias means these folks don't run into as many problems as they otherwise would, though supporting these users does effectively mean we support the option indefinitely. But as I understand it, the cost to the community is minimal (approx one line of code to add the alias and a test case proving the alias works), so I'm not certain what maintenance burden you have in mind.

[MaskRay]

clang/test/Driver/darwin-version.c

217

I am fine with a temporary alias, but I am not comfortable for a permanent alias. I have analyzed numerous -Wno-overriding-t-option uses and conclude that they deserve a warning (a very small number of -ffp-model= related users deserve it as well, though I agree our -ffp-model= warning model is a bit stronger our practice for other options).

With ToT, using -Wno-overriding-t-option gives this typo correction

% fclang -Wno-overriding-t-option -c a.c
warning: unknown warning option '-Wno-overriding-t-option'; did you mean '-Wno-overriding-option'? [-Wunknown-warning-option]
1 warning generated.

So users noticing the warning will know what to do, if they decide to silence it.

By reading the cdb5240287897ff7649d40e3752ecf23e50b57f5 change, I can see an attempt to make the warning preciser, but I do not see an attempt to support -Werror users having the conflicting target triple and -m<os>-version-min=.

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def warn_drv_unsupported_float_abi_by_lib : Warning<
   "float ABI '%0' is not supported by current library">,
   InGroup<UnsupportedABI>;
 def warn_drv_no_floating_point_registers: Warning<
   "'%0': selected processor lacks floating point registers">,
   InGroup<UnsupportedABI>;
 def warn_ignoring_ftabstop_value : Warning<
   "ignoring invalid -ftabstop value '%0', using default value %1">;
+// Don't use warn_drv_overriding_t_option for new diagnostics.
+def warn_drv_overriding_t_option : Warning<
+  "overriding '%0' option with '%1'">,
+  InGroup<OverridingTOption>;
 def warn_drv_overriding_option : Warning<
   "overriding '%0' option with '%1'">,
-  InGroup<DiagGroup<"overriding-option">>;
+  InGroup<OverridingOption>;
 def warn_drv_treating_input_as_cxx : Warning<
   "treating '%0' input as '%1' when in C++ mode, this behavior is deprecated">,
   InGroup<Deprecated>;
 def warn_drv_pch_not_first_include : Warning<
   "precompiled header '%0' was ignored because '%1' is not first '-include'">;
 def warn_missing_sysroot : Warning<"no such sysroot directory: '%0'">,
   InGroup<DiagGroup<"missing-sysroot">>;
 def warn_incompatible_sysroot : Warning<"using sysroot for '%0' but targeting '%1'">,

clang/include/clang/Basic/DiagnosticGroups.td

 def ObjCReadonlyPropertyHasSetter : DiagGroup<"objc-readonly-with-setter-property">;
 def ObjCInvalidIBOutletProperty : DiagGroup<"invalid-iboutlet">;
 def ObjCRootClass : DiagGroup<"objc-root-class">;
 def ObjCPointerIntrospectPerformSelector : DiagGroup<"deprecated-objc-pointer-introspection-performSelector">;
 def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [ObjCPointerIntrospectPerformSelector]>;
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">;
 def ObjCBoxing : DiagGroup<"objc-boxing">;
+def OverridingTOption : DiagGroup<"overriding-t-option">;
+def OverridingOption : DiagGroup<"overriding-option", [OverridingTOption]>;
 def CompletionHandler : DiagGroup<"completion-handler">;
 def CalledOnceParameter : DiagGroup<"called-once-parameter", [CompletionHandler]>;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 def UnderalignedExceptionObject : DiagGroup<"underaligned-exception-object">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def OrderedCompareFunctionPointers : DiagGroup<"ordered-compare-function-pointers">;
 def PackedNonPod : DiagGroup<"packed-non-pod">;

clang/lib/Driver/ToolChains/Darwin.cpp

if (OSVersionArgTarget) {
             !OSTarget->hasOSVersion()) {
           OSTarget->setOSVersion(OSVersionArgTarget->getOSVersion());
         } else {
           // Warn about -m<os>-version-min that doesn't match the OS version
           // that's specified in the target.
           std::string OSVersionArg =
               OSVersionArgTarget->getAsString(Args, Opts);
           std::string TargetArg = OSTarget->getAsString(Args, Opts);
-          getDriver().Diag(clang::diag::warn_drv_overriding_option)
+          getDriver().Diag(clang::diag::warn_drv_overriding_t_option)
               << OSVersionArg << TargetArg;
         }
       }
     }
   } else if ((OSTarget = getDeploymentTargetFromMTargetOSArg(Args, getDriver(),
                                                              SDKInfo))) {
     // The OS target can be specified using the -mtargetos= argument.
     // Disallow mixing -mtargetos= and -m<os>version-min=.

clang/test/Driver/darwin-version.c

 // RUN: %clang -target unknown-apple-darwin12  -arch armv7  -c %s -### 2>&1 | \
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TDARWIN-FALL5 %s
 // CHECK-VERSION-TDARWIN-FALL5: "thumbv7-apple-ios5.0.0"
 
 // Warn about -m<os>-version-min when it's used with target:
 
 // RUN: %clang -target x86_64-apple-macos10.11.2 -mmacos-version-min=10.6 -c %s -### 2>&1 | \
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 
 // RUN: %clang -target x86_64-apple-macos -miphoneos-version-min=9.1 -c %s -### 2>&1 | \
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV2 %s
-// CHECK-VERSION-TNO-OSV2: overriding '-miphoneos-version-min=9.1' option with '-target x86_64-apple-macos'
+// CHECK-VERSION-TNO-OSV2: overriding '-miphoneos-version-min=9.1' option with '-target x86_64-apple-macos' [-Woverriding-t-option]
 
 // RUN: %clang -target x86_64-apple-ios -miphonesimulator-version-min=10.0 -c %s -### 2>&1 | \
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV3 %s
 // CHECK-VERSION-TNO-OSV3: "x86_64-apple-ios10.0.0-simulator"
 // CHECK-VERSION-TNO-OSV3-NOT: overriding '-mios-simulator-version-min
 // CHECK-VERSION-TNO-OSV3-NOT: argument unused during compilation
 
 // RUN: %clang -target arm64-apple-ios10.1.0 -miphoneos-version-min=10.1.0.1 -c %s -### 2>&1 | \