[Clang] Make Clang driver suggest '-Xclang' for CC1 options passed to the driver

[jhuber6]

Summary

This patch adds an additional check for if an options passed to the
Clang driver could've been intended for the clang compiler. This is
primarily done for the times when a user attempts to pass an option like
-ast-dump to the driver instead.

[aaron.ballman]

This is a neat idea, but I think we should think about it carefully. In general, -Xclang is not something we want to actively recommend users use, so do we really want to make it easier for users to use?

One potential problem that could be caused by this change is when we have a driver option that the user typos but happens to match a -cc1 argument; I could see inattentive users going "cool, I'll add -Xclang then" instead of fixing the driver arg; but I've not looked through our existing options to see if that situation is particularly *likely* or not.

[jhuber6]

In D134550#3812369, @aaron.ballman wrote:

This is a neat idea, but I think we should think about it carefully. In general, -Xclang is not something we want to actively recommend users use, so do we really want to make it easier for users to use?

Thanks for the feedback, I mostly figured I'd throw out this patch since it took a few minutes to make and I've forgotten a few times which arguments need the -Xclang during Clang development. If you think that users shouldn't be exposed to that then it's probably best to hide it from them.

One potential problem that could be caused by this change is when we have a driver option that the user typos but happens to match a -cc1 argument; I could see inattentive users going "cool, I'll add -Xclang then" instead of fixing the driver arg; but I've not looked through our existing options to see if that situation is particularly *likely* or not.

The logic outlined here should only fire if there is no close match. We only check if the input is a cc1 option right before outputting the normal unknown argument warning without a suggestion.

[MaskRay]

I think this is fine. Driver options take precedence and cc1 options cannot be suggested.
If the user writes a cc1-only option without -Xclang, they likely have the particular need and will likely add -Xclang anyway.

This will need to wait until Aaron is happy.

[aaron.ballman]

I'm convinced, thanks for the discussion! LGTM!

[jyknight]

I really don't think we should have this behavior. The cc1 options are supposed to be an internal implementation detail. It's already a problem that the option name doesn't shout "hey I'm an internal interface with no stability guarantees! Don't use me!". Having clang automatically recommend these internal options will just make the situation that much worse.

There are a handful of cc1 options which are sort of "internal but kinda public" like "-verify" -- things which expected for clang developers, at least, to use. But there's a great many more which are the interface between the clang Driver and the clang frontend. Sometimes that interface option shares the name of a Driver option -- but in many cases it does not. And for those which do not, we certainly do not want to recommend them to anyone.

E.g. consider -- if a user types clang -triple x86_64-linux-gnu, do we really want to suggest that they meant -Xclang -triple? No way! They should use the actual driver option -target! Similarly for many other cc1-only options, e.g. -mrelocation-model, -mframe-pointer=, etc...none of those should be used or recommended.

[aaron.ballman]

In D134550#3813259, @jyknight wrote:

I really don't think we should have this behavior. The cc1 options are supposed to be an internal implementation detail. It's already a problem that the option name doesn't shout "hey I'm an internal interface with no stability guarantees! Don't use me!". Having clang automatically recommend these internal options will just make the situation that much worse.

There are a handful of cc1 options which are sort of "internal but kinda public" like "-verify" -- things which expected for clang developers, at least, to use. But there's a great many more which are the interface between the clang Driver and the clang frontend. Sometimes that interface option shares the name of a Driver option -- but in many cases it does not. And for those which do not, we certainly do not want to recommend them to anyone.

E.g. consider -- if a user types clang -triple x86_64-linux-gnu, do we really want to suggest that they meant -Xclang -triple? No way! They should use the actual driver option -target! Similarly for many other cc1-only options, e.g. -mrelocation-model, -mframe-pointer=, etc...none of those should be used or recommended.

Thank you for speaking up; this echoes a lot of my concerns. But what convinced me was that we sometimes put experimental options into cc1 with the intent that users can try them out, such as -fexperimental-max-bitint-width= and it would be nice for those options to be suggested when the user forgets to add -Xclang. Perhaps a different approach is to allow select cc1 options to opt into this suggestion so that we can control which options we are willing to recommend to users? Alternatively, perhaps those experimental options should be exposed from the driver instead of being a cc1-only flag?

[jhuber6]

In D134550#3813259, @jyknight wrote:

I really don't think we should have this behavior. The cc1 options are supposed to be an internal implementation detail. It's already a problem that the option name doesn't shout "hey I'm an internal interface with no stability guarantees! Don't use me!". Having clang automatically recommend these internal options will just make the situation that much worse.

There are a handful of cc1 options which are sort of "internal but kinda public" like "-verify" -- things which expected for clang developers, at least, to use. But there's a great many more which are the interface between the clang Driver and the clang frontend. Sometimes that interface option shares the name of a Driver option -- but in many cases it does not. And for those which do not, we certainly do not want to recommend them to anyone.

E.g. consider -- if a user types clang -triple x86_64-linux-gnu, do we really want to suggest that they meant -Xclang -triple? No way! They should use the actual driver option -target! Similarly for many other cc1-only options, e.g. -mrelocation-model, -mframe-pointer=, etc...none of those should be used or recommended.

Thanks for bringing up your concerns. I think the question is mostly what we want to be exposed to the users. As you mentioned there are several options that have implementations in the driver and shouldn't be called directly by the user. We definitely do not want to encourage users to use CC1 options that have driver equivalents. We could assume that the user would be aware that anything requiring -Xclang is a manual override, but many users may just see that as a way to get the behaviour they want without knowing about the expected option. If you think this is not desirable go ahead and revert the patch, I won't complain.

In D134550#3813269, @aaron.ballman wrote:

Thank you for speaking up; this echoes a lot of my concerns. But what convinced me was that we sometimes put experimental options into cc1 with the intent that users can try them out, such as -fexperimental-max-bitint-width= and it would be nice for those options to be suggested when the user forgets to add -Xclang. Perhaps a different approach is to allow select cc1 options to opt into this suggestion so that we can control which options we are willing to recommend to users? Alternatively, perhaps those experimental options should be exposed from the driver instead of being a cc1-only flag?

I generally think the check added in this review is useful, but this is speaking as a developer. Adding a special flag that enables this behavior might be helpful, but it would require some more justification for the extra work I'd say.

[jyknight]

In D134550#3813269, @aaron.ballman wrote:

Alternatively, perhaps those experimental options should be exposed from the driver instead of being a cc1-only flag?

IMO: yes. If we want end-users to use a particular flag, we should expose it as a Driver flag. If we want to reserve the right to change or delete it, putting "experimental" in the name conveys that -- significantly more than telling users to spell the flag -Xclang -foo does.

There's a real underlying problem here that the name -Xclang seems like "clang-specific flag" not "unstable internal API don't depend on me" -- to everyone who is not a Clang Driver developer...

[ychen]

Drive by thoughts: keyed this on assertion/debug build?

[aaron.ballman]

In D134550#3813285, @jyknight wrote:

In D134550#3813269, @aaron.ballman wrote:

Alternatively, perhaps those experimental options should be exposed from the driver instead of being a cc1-only flag?

IMO: yes. If we want end-users to use a particular flag, we should expose it as a Driver flag. If we want to reserve the right to change or delete it, putting "experimental" in the name conveys that -- significantly more than telling users to spell the flag -Xclang -foo does.

Agreed, I think we should expose those flags via Driver with a sufficiently clear name rather than leave them in cc1.

There's a real underlying problem here that the name -Xclang seems like "clang-specific flag" not "unstable internal API don't depend on me" -- to everyone who is not a Clang Driver developer...

Yeah, that's an excellent point! Perhaps we want to consider renaming that to something like -X clang-internal-option or something along those lines?

In D134550#3813523, @ychen wrote:

Drive by thoughts: keyed this on assertion/debug build?

That's an interesting idea because it means the functionality is unlikely to get out to users in practice but folks working on Clang get some extra help.

In terms of this patch -- is there sentiment to revert (even temporarily while we discuss)?

[jhuber6]

In D134550#3816239, @aaron.ballman wrote:

In terms of this patch -- is there sentiment to revert (even temporarily while we discuss)?

I think alternatively we could just change the message to mention that the provided flag matches a CC1 option without the suggestion. Then developers and users would know that they probably got something wrong but won't be encouraged to use the CC1 option directly.

[aaron.ballman]

In D134550#3816255, @jhuber6 wrote:

In D134550#3816239, @aaron.ballman wrote:

In terms of this patch -- is there sentiment to revert (even temporarily while we discuss)?

I think alternatively we could just change the message to mention that the provided flag matches a CC1 option without the suggestion. Then developers and users would know that they probably got something wrong but won't be encouraged to use the CC1 option directly.

Does the existing "unknown argument 'whatever'" diagnostic make it sufficiently clear that they probably got something wrong, though?

clang/lib/Driver/Driver.cpp

if (A->getOption().matches(options::OPT_mcpu_EQ) && A->containsValue("")) {
                            SourceLocation()) > DiagnosticsEngine::Warning;
     }
   }
 
   for (const Arg *A : Args.filtered(options::OPT_UNKNOWN)) {
     unsigned DiagID;
     auto ArgString = A->getAsString(Args);
     std::string Nearest;
-    if (getOpts().findNearest(
-          ArgString, Nearest, IncludedFlagsBitmask, ExcludedFlagsBitmask) > 1) {
+    if (getOpts().findNearest(ArgString, Nearest, IncludedFlagsBitmask,
+                              ExcludedFlagsBitmask) > 1) {
+      if (getOpts().findNearest(ArgString, Nearest, options::CC1Option) == 0 &&
+          !IsCLMode()) {
+        DiagID = diag::err_drv_unknown_argument_with_suggestion;
+        Diags.Report(DiagID) << ArgString << "-Xclang " + Nearest;
+      } else {
         DiagID = IsCLMode() ? diag::warn_drv_unknown_argument_clang_cl
                             : diag::err_drv_unknown_argument;
         Diags.Report(DiagID) << ArgString;
+      }
     } else {
       DiagID = IsCLMode()
                    ? diag::warn_drv_unknown_argument_clang_cl_with_suggestion
                    : diag::err_drv_unknown_argument_with_suggestion;
       Diags.Report(DiagID) << ArgString << Nearest;
     }
     ContainsError |= Diags.getDiagnosticLevel(DiagID, SourceLocation()) >
                      DiagnosticsEngine::Warning;

clang/test/Driver/debug-options.c

 
 // DIRECTORY-NOT: "-fno-dwarf-directory-asm"
 // NODIRECTORY: "-fno-dwarf-directory-asm"
 
 // RUN: %clang -### -target x86_64 -c -g -gsimple-template-names %s 2>&1 | FileCheck --check-prefixes=SIMPLE_TMPL_NAMES,FWD_TMPL_PARAMS %s
 // SIMPLE_TMPL_NAMES: -gsimple-template-names=simple
 // FWD_TMPL_PARAMS-DAG: -debug-forward-template-params
 // RUN: not %clang -### -target x86_64 -c -g -gsimple-template-names=mangled %s 2>&1 | FileCheck --check-prefix=MANGLED_TEMP_NAMES %s
-// MANGLED_TEMP_NAMES: error: unknown argument: '-gsimple-template-names=mangled'
+// MANGLED_TEMP_NAMES: error: unknown argument '-gsimple-template-names=mangled'; did you mean '-Xclang -gsimple-template-names=mangled'
 // RUN: %clang -### -target x86_64 -c -g %s 2>&1 | FileCheck --check-prefix=FULL_TEMP_NAMES --implicit-check-not=debug-forward-template-params %s
 // FULL_TEMP_NAMES-NOT: -gsimple-template-names

clang/test/Driver/unknown-arg.c

 // This needs to exit non-0, for configure scripts.
 // RUN: not %clang /GR-
 
 // CHECK: error: unknown argument: '-cake-is-lie'
 // CHECK: error: unknown argument: '-%0'
 // CHECK: error: unknown argument: '-%d'
 // CHECK: error: unknown argument: '-HHHH'
 // CHECK: error: unknown argument: '-munknown-to-clang-option'
-// CHECK: error: unknown argument: '-print-stats'
+// CHECK: error: unknown argument '-print-stats'; did you mean '-Xclang -print-stats'?
 // CHECK: error: unknown argument: '-funknown-to-clang-option'
 // CHECK: error: unknown argument: '-ifoo'
 // MULTILIB: warning: argument unused during compilation: '-imultilib dir'
 // DID-YOU-MEAN: error: unknown argument '-stdlibs=foo'; did you mean '-stdlib=foo'?
 // DID-YOU-MEAN: error: unknown argument '-hell'; did you mean '-help'?
 // DID-YOU-MEAN: error: unknown argument '-version'; did you mean '--version'?
 // CL: warning: unknown argument ignored in clang-cl: '-cake-is-lie'
 // CL: warning: unknown argument ignored in clang-cl: '-%0'

clang/test/Misc/driver-verify.c

 // RUN: not %clang -verify %s 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -verify %s
 // expected-no-diagnostics
 
 // Test that -verify is strictly rejected as unknown by the driver.
-// CHECK: unknown argument: '-verify'
+// CHECK: unknown argument '-verify'; did you mean '-Xclang -verify'?