[Clang] Emit a warning for ambiguous joined '-o' arguments

[jhuber6]

Summary

The offloading toolchain makes heavy use of options beginning with
--o. This is problematic when combined with the joined -o flag. In
the following situation, the user will not get the expected output and
will not notice as the expected output will still be written.

clang++ -x cuda foo.cu -offload-arch=sm_80 -o foo

This patch introduces a warning that checks for joined -o arguments
that would also be a valid driver argument if an additional - were
added. I believe this situation is uncommon enough to warrant a warning,
and can be trivially fixed by the end user by using the more common
separate form instead.

[tra]

LGTM in principle.

clang/lib/Driver/Driver.cpp

337

This looks for all 1-symbol mismatches. and may trigger on differences other than --option vs -option. E.g. if user passes -oip-link (which looks like something we may see in real life), we'll try to find the nearest for --oip-link and will get a distance-1 match for the existing option --hip-link.

We may need to verify that the nearest match that we've found is indeed due to a missing leading dash.

clang/test/Driver/unknown-arg.c

68

I'd also add a test verifying that -o ffload-device-only does not trigger the warning.

[jhuber6]

Adjusting check to ensure that this only applies for an edit length of zero once the - is added. Also adding a negative check line for other inputs.

[jhuber6]

clang/lib/Driver/Driver.cpp

337

You're right, I changed it to use the raw string from the input. Since it's joined it will be the full string, so if we append a - to it and the edit length is zero then we know for a fact that it's due to a missing dash.

[tra]

Test nit. LGTM otherwise.

clang/test/Driver/unknown-arg.c

73

I think this check will never trigger, because, if the warning would happen to be issued, it would happen before the warning for -output above and we'd miss it.

Doing negative test in a separate RUN would probably be the easiest way to deal with it.

[jhuber6]

Changing test to just check that we emit to warnings in a separate run line.

[tra]

clang/test/Driver/unknown-arg.c

74

Has this patch been updated with incomplete changes? This RUN line seems to miss 2>&1 | FileCheck and the check lines.

[jhuber6]

clang/test/Driver/unknown-arg.c

74

I'm relying on the fact that -Werror will cause clang to return a non-zero error code and make the test fail if any warnings show up.

[jhuber6]

Apparently -### prevents clang from returning a non-zero error code. Fixing.

[tra]

clang/test/Driver/unknown-arg.c

74

Got it. I've missed that.

[jhuber6]

clang/test/Driver/unknown-arg.c

74

You were somewhat right, I had -### which apparently makes clang return 0 even on an error. So thanks for making me double check.

[MaskRay]

clang/test/Driver/unknown-arg.c

74

Use -### --implicit-check-not=warning: instead of -emit-llvm which runs more than the driver.

[jhuber6]

Using the implicit check suggestion.

[thakis]

This breaks tests on windows: http://45.33.8.238/win/67644/step_7.txt

Please take a look and revert for now if it takes a while to fix.

clang/include/clang/Basic/DiagnosticDriverKinds.td

 
 def warn_drv_ycyu_different_arg_clang_cl : Warning<
   "support for '/Yc' and '/Yu' with different filenames not implemented yet; flags ignored">,
   InGroup<ClangClPch>;
 def warn_drv_yc_multiple_inputs_clang_cl : Warning<
   "support for '/Yc' with more than one source file not implemented yet; flag ignored">,
   InGroup<ClangClPch>;
 
+def warn_drv_potentially_misspelled_joined_argument : Warning<
+  "joined argument treated as '%0'; did you mean '%1'?">, InGroup<UnknownArgument>;
+
 def err_drv_invalid_value : Error<"invalid value '%1' in '%0'">;
 def err_drv_invalid_int_value : Error<"invalid integral value '%1' in '%0'">;
 def err_drv_invalid_value_with_suggestion : Error<
     "invalid value '%1' in '%0', expected one of: %2">;
 def err_drv_alignment_not_power_of_two : Error<"alignment is not a power of 2 in '%0'">;
 def err_drv_invalid_remap_file : Error<
     "invalid option '%0' not of the form <from-file>;<to-file>">;
 def err_drv_invalid_gcc_install_dir : Error<"'%0' does not contain a GCC installation">;

clang/lib/Driver/Driver.cpp

if (getOpts().findNearest(ArgString, Nearest, IncludedFlagsBitmask,
                    ? 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;
   }
 
+  for (const Arg *A : Args.filtered(options::OPT_o)) {
+    if (ArgStrings[A->getIndex()] == A->getSpelling())
+      continue;
+
+    // Warn on joined arguments that are similar to a long argument.
+    std::string ArgString = ArgStrings[A->getIndex()];
+    std::string Nearest;
+    if (getOpts().findNearest("-" + ArgString, Nearest, IncludedFlagsBitmask,
+                              ExcludedFlagsBitmask) == 0)
+      Diags.Report(diag::warn_drv_potentially_misspelled_joined_argument)
+          << A->getAsString(Args) << Nearest;
+  }
+
   return Args;
 }
 
 // Determine which compilation mode we are in. We look for options which
 // affect the phase, starting with the earliest phases, and record which
 // option we used to determine the final phase.
 phases::ID Driver::getFinalPhase(const DerivedArgList &DAL,
                                  Arg **FinalPhaseArg) const {

clang/test/Driver/unknown-arg.c

 // SILENT-NOT: warning:
 // CC1AS-DID-YOU-MEAN: error: unknown argument '-hell'; did you mean '-help'?
 // CC1AS-DID-YOU-MEAN: error: unknown argument '--version'; did you mean '-version'?
 // UNKNOWN-INTEGRATED: error: unknown integrated tool '-cc1asphalt'. Valid tools include '-cc1' and '-cc1as'.
 
 // RUN: %clang -S %s -o %t.s  -Wunknown-to-clang-option 2>&1 | FileCheck --check-prefix=IGNORED %s
 
 // IGNORED: warning: unknown warning option '-Wunknown-to-clang-option'
+
+// RUN: %clang -### -offload-arch=sm_70 --offload-arch=sm_70 -offload-device-only -o ffload-device-only \
+// RUN:   -output -o foo %s 2>&1 | FileCheck --check-prefix=O-WARN %s
+//      O-WARN: warning: joined argument treated as '-o ffload-arch=sm_70'; did you mean '--offload-arch=sm_70'?
+// O-WARN-NEXT: warning: joined argument treated as '-o ffload-device-only'; did you mean '--offload-device-only'?
+// O-WARN-NEXT: warning: joined argument treated as '-o utput'; did you mean '--output'?
+//  O-WARN-NOT: warning: joined argument treated as '-o ffload-device-only'; did you mean '--offload-device-only'?