[PATCH] D158301: Add back overriding-t-options for -m<os>-version-min diagnostic

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 23 12:11:57 PDT 2023 

aaron.ballman added inline comments.


================
Comment at: clang/test/Driver/darwin-version.c:217
 // 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]
 
----------------
MaskRay wrote:
> aaron.ballman wrote:
> > hans wrote:
> > > MaskRay wrote:
> > > > dblaikie wrote:
> > > > > MaskRay wrote:
> > > > > > dexonsmith wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > dexonsmith wrote:
> > > > > > > > > MaskRay wrote:
> > > > > > > > > > hans wrote:
> > > > > > > > > > > MaskRay wrote:
> > > > > > > > > > > > hans wrote:
> > > > > > > > > > > > > dexonsmith wrote:
> > > > > > > > > > > > > > MaskRay wrote:
> > > > > > > > > > > > > > > dexonsmith wrote:
> > > > > > > > > > > > > > > > Why would we want to use the old name here? An alias seems strictly better to me. 
> > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 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).
> > > > > > > > > > > > > > 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.
> > > > > > > > > > > > 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.
> > > > > > > > > > > > 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.
> > > > > > > > > > `-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).
> > > > > > > > > > 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.
> > > > > > > > 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.
> > > > > > > > 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.
> > > > > > 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.
> > > > > > 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.
> > > > (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.
> > > > 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?
> > > 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.
> >> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158301/new/

https://reviews.llvm.org/D158301

More information about the cfe-commits mailing list