[clang][driver] Emit an error for `/clang:-x`

[Fznamznon]

Summary

/clang:-x emits an error instead of a warning. And if the error is suppressed,
/clang:-x takes no effect.
Considering that /clang: is a recent addition in 2018-11 and there are MSVC
style alternatives, therefore /clang:-x doesn't seem useful and we just reject
it since properly supporting it would add lots of complexity.

Fixes #59307

[MaskRay]

See https://github.com/llvm/llvm-project/issues/59307#issuecomment-1407529807 I think we should just report an error for /clang:-xc

clang/lib/Driver/Driver.cpp

2571

-    assert(!Args.hasArg(options::OPT_x) && "-x and /TC or /TP is not allowed");
+    if (auto *A = Args.getLastArg(options::OPT_x))
+      Diag(diag::err_drv_unsupported_opt_with_suggestion)
+          << A->getAsString(Args) << "/TC' or '/TP";

clang/test/Driver/x-args.c

8

Delete //

[Fznamznon]

Emit an error if /clang:-x passed.

[Fznamznon]

clang/lib/Driver/Driver.cpp

2571

Thank you, the suggestion seems reasonable to me. Although, the concrete place doesn't.
The suggested line to change is under if (we have /TC or /TP argument), so if I put error emission there it won't be emitted if no /TC or /TP was passed and the original problem reported in https://github.com/llvm/llvm-project/issues/59307 won't be resolved. People will still be confused by the warning saying -x c passed after last input when in fact they passed /clang:-x before the input.

Also, the whole function doesn't return even if diagnostic is emitted, so I put error emission in a way to not emit a warning saying that -x is passed after last input together with it, because both errors emitted like this:

error: '-x c' after last input file has no effect
error: unsupported option '-x c'; did you mean '/TC' or '/TP'?

look confusing.

clang/test/Driver/x-args.c

8

Done, thanks.

[aaron.ballman]

This seems reasonable to me. I was a bit worried about the behavior being rather strange, but given that these are "escape hatch" command line options to skip the clang-cl driver, and there are existing clang-cl options that cover this functionality, it makes more sense to me now. LGTM, but please add a release note about the fix (to clang/docs/ReleaseNotes.rst). I'll leave the final sign-off to @MaskRay.

[Fznamznon]

Add a release note.

[MaskRay]

LGTM.

clang/docs/ReleaseNotes.rst

60 ↗

(On Diff #493326)

Double backtick

LLVM_ENABLE_SPHINX=on and run ninja docs-clang-html to check whether the docs build is good.

[MaskRay]

In CL mode values of /clang: arguments end up at the end of arguments list so passing /clang:-x doesn't make sense and produces confusing warning.

The description needs updating. clang:-x emits an error instead of a warning. And if the error is suppressed, /clang:-x takes no effect.
Considering that /clang: is a recent addition in 2018-11 and there are MSVC style alternatives, therefore /clang:-x doesn't seem useful and we just reject it since properly supporting it would add lots of complexity.

[MaskRay]

clang/lib/Driver/Driver.cpp

2571

You need to remove assert(!Args.hasArg(options::OPT_x) && "-x and /TC or /TP is not allowed"); as otherwise /clang:-x /TC leads to a crash in an assert build of clang (LLVM_ENABLE_ASSERTIONS=on).

[Fznamznon]

Apply suggestions

[Fznamznon]

clang/lib/Driver/Driver.cpp

2571

Ok, removed the assert. Verified there is no crash in the test.

[Fznamznon]

clang/docs/ReleaseNotes.rst

60 ↗

(On Diff #493326)

Ok fixed and verified.

[Fznamznon]

@MaskRay , thanks for the suggestions. Applied. Please let me know that it is ok now.

[MaskRay]

clang/test/Driver/x-args.c

12

You can remove this CL-NOT negative pattern. Instead, use --check-prefix=CL --implicit-check-not=error: to check that there is no other error.

[Fznamznon]

clang/test/Driver/x-args.c

12

I probably don't fully understand the suggestion, but there is a error: that is being checked in this test. The one that this revision adds. It is checked on the next line.

[Fznamznon]

clang/test/Driver/x-args.c

12

Is that ok if I submit the patch "as is"?

[MaskRay]

clang/test/Driver/x-args.c

12

CL-NOT: checks one specific error message does not appear before error: unsupported option .... You need another pattern to check that it does not appear after error: unsupported option .... If you use --implicit-check-not, you additionally check that there is no other error diagnostic whatever their message is.

[Fznamznon]

Apply suggestion and rebase

[Fznamznon]

Fix incorrect conflict resolution

[Fznamznon]

clang/test/Driver/x-args.c

12

Okay, this sounds reasonable, thank you. Applied.

[thakis]

This breaks tests on Mac: http://45.33.8.238/macm1/54097/step_7.txt

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

[Fznamznon]

In D142757#4100167, @thakis wrote:

This breaks tests on Mac: http://45.33.8.238/macm1/54097/step_7.txt

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

Thanks for reporting. It seems I should have added -- before the input. I will push the fix asap.

[Fznamznon]

Should be fixed by https://github.com/llvm/llvm-project/commit/108fa70cd213e63a591adb87ce444fb74afc6786 .

[thakis]

Thanks!

clang/lib/Driver/Driver.cpp

for (Arg *A :
         ShowNote = true;
       }
       Previous = A;
     }
     if (ShowNote)
       Diag(clang::diag::note_drv_t_option_is_global);
 
     // No driver mode exposes -x and /TC or /TP; we don't support mixing them.
     assert(!Args.hasArg(options::OPT_x) && "-x and /TC or /TP is not allowed");
   }
 
   // Warn -x after last input file has no effect
-  {
+  if (!IsCLMode()) {
     Arg *LastXArg = Args.getLastArgNoClaim(options::OPT_x);
     Arg *LastInputArg = Args.getLastArgNoClaim(options::OPT_INPUT);
-    if (LastXArg && LastInputArg && LastInputArg->getIndex() < LastXArg->getIndex())
+    if (LastXArg && LastInputArg &&
+        LastInputArg->getIndex() < LastXArg->getIndex())
       Diag(clang::diag::warn_drv_unused_x) << LastXArg->getValue();
+  } else {
+    // In CL mode suggest /TC or /TP since -x doesn't make sense if passed via
+    // /clang:.
+    if (auto *A = Args.getLastArg(options::OPT_x))
+      Diag(diag::err_drv_unsupported_opt_with_suggestion)
+          << A->getAsString(Args) << "/TC' or '/TP";
   }
 
   for (Arg *A : Args) {
     if (A->getOption().getKind() == Option::InputClass) {
       const char *Value = A->getValue();
       types::ID Ty = types::TY_INVALID;
 
       // Infer the input type if necessary.

clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl_by_mod_name.json

 [
 {
   "directory": "DIR",
-  "command": "clang-cl /E /IInputs /D INCLUDE_HEADER2 /clang:-MD /clang:-MF /clang:DIR/modules_cdb2_clangcl.d /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps /clang:-x /clang:c++ --",
+  "command": "clang-cl /E /IInputs /D INCLUDE_HEADER2 /clang:-MD /clang:-MF /clang:DIR/modules_cdb2_clangcl.d /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps /TP --",
   "file": ""
 },
 {
   "directory": "DIR",
-  "command": "clang-cl /E /IInputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps /clang:-x /clang:c++ --",
+  "command": "clang-cl /E /IInputs /clang:-fmodules /clang:-fcxx-modules /clang:-fmodules-cache-path=DIR/module-cache_clangcl /clang:-fimplicit-modules /clang:-fimplicit-module-maps /TP --",
   "file": ""
 },
 ]

clang/test/Driver/x-args.c

 // RUN: %clang -fsyntax-only -Werror -xc %s
 // RUN: %clang -fsyntax-only -Werror %s -xc %s
 
 // RUN: %clang -fsyntax-only %s -xc++ -fsyntax-only 2>&1 | FileCheck %s
 // RUN: %clang -fsyntax-only -xc %s -xc++ -fsyntax-only 2>&1 | FileCheck %s
 // RUN: %clang -fsyntax-only %s -xc %s -xc++ -fsyntax-only 2>&1 | FileCheck %s
 // CHECK: '-x c++' after last input file has no effect
+
+// RUN: not %clang_cl /WX /clang:-xc /clang:-E /clang:-dM %s 2>&1 | FileCheck -check-prefix=CL %s
+// RUN: not %clang_cl /WX /clang:-E /clang:-dM %s /clang:-xc 2>&1 | FileCheck -check-prefix=CL %s
+// CL-NOT: '-x c' after last input file has no effect
+// CL: error: unsupported option '-x c'; did you mean '/TC' or '/TP'?