[Driver] Do not generate error about unsupported target specific options when there is no compiler jobs

[mgabka]

Summary

The upstream commit: https://reviews.llvm.org/D151590
added a new flag to mark target specific compiler options.

The side effect of it was that in cases when -### or -v is used without any
input file, clang started emitting an error.
It happened like that becasue there is no compilation actions created
which could consume/verify these target specific options.

This patch changes that error to a warning about unused option, in situations
when there is no actions and still generates error when there are actions.

Fix for https://github.com/llvm/llvm-project/issues/64958

[awarzynski]

Thanks for working on this! I see 2 cases here:

1. In the case of -###, it makes sense that no errors are emitted as no actions are run (nor, AFAIK, created).

2. In the case of -v, clang -v file.c should behave like clang file.c (why wouldn't it?). So, this should give an error: %clang --target=x86_64-unknown-linux-gnu --verbose -march=.

IIUC, this patch will only affect the 1st case and that particular change makes sense to me. Not sure about 2, but if that's not affected by this patch then I'd just refocus on -### by updating the summary and the test.

Does this make sense?

clang/test/Driver/suppress-err-on-target-specific-options.c

4 ↗

(On Diff #555706)

You can use this new syntax. This way some will RUN lines will be executed even if only one of the require targets is available. Documentation: https://llvm.org/docs/TestingGuide.html#substitutions

9 ↗

(On Diff #555706)

IMO this should be an error:

clang: error: no input files

That's not the case ATM (ToT). That's not something "introduced" here, so I would just not test it here.

[mgabka]

In D159361#4638016, @awarzynski wrote:

Thanks for working on this! I see 2 cases here:

1. In the case of -###, it makes sense that no errors are emitted as no actions are run (nor, AFAIK, created).

2. In the case of -v, clang -v file.c should behave like clang file.c (why wouldn't it?). So, this should give an error: %clang --target=x86_64-unknown-linux-gnu --verbose -march=.

IIUC, this patch will only affect the 1st case and that particular change makes sense to me. Not sure about 2, but if that's not affected by this patch then I'd just refocus on -### by updating the summary and the test.

Does this make sense?

I wanted to show that my patch does not break existing behaviour for X86, as there is no tests for it at all.
The existing behaviour is is that for X86 -mcpu always generates error, my change is changing the behaviour so the error is generated when the compilation job is created, i.e when the file name is passed, this is why I think the tests I added are valuable (as I did't find any, unfortunately the https://reviews.llvm.org/D151590 didn't introduce any tests.

[mgabka]

clang/test/Driver/suppress-err-on-target-specific-options.c

9 ↗

(On Diff #555706)

so I am not sure, gcc behaves same way, so I doubt that both are wrong.

[awarzynski]

OK, so previously I made a wrong assumption. I assumed that both of these should return identical error (missing input in both cases):

$ clang
clang: error: no input files
$ clang --verbose
# Some info vebose info here, but no error

But looking at how SuppressMissingInputWarning is set explains the behavior above. So yes, in both cases (-v and -###) no action is created and hence this patch does apply to both -v and -###. Unlike I claimed before, apologies.

This change makes a lot of sense to me now. However, I'd appreciate a few more comments explaining that this affects the behavior _when no job/action is created_, e.g. for clang -### and clang -v <no-input-file>. That's already noted in the commit message, but a comment in the source and the test file would be appreciated.

clang/test/Driver/suppress-err-on-target-specific-options.c

9 ↗

(On Diff #555706)

Aha, see my latest reply, I was wrong.

[MaskRay]

clang/test/Driver/suppress-err-on-target-specific-options.c

1 ↗

(On Diff #556033)

There are too many RUN lines. We can combine some.

-mcpu= -march= can be tested in one command.

I think you can drop --verbose (-v) tests. The -v behavior is tested a lot in other files.

Perhaps just name this file: no-input-file.c. We don't have dedicated tests yet

Clang :: Driver/android-ndk-standalone.cpp
Clang :: Driver/baremetal.cpp
Clang :: Driver/cl-options.c
Clang :: Driver/config-file.c
Clang :: Driver/config-file3.c
Clang :: Driver/cuda-detect-path.cu
Clang :: Driver/cuda-detect.cu
Clang :: Driver/cuda-macosx.cu
Clang :: Driver/default-toolchain.c
Clang :: Driver/hip-version.hip
Clang :: Driver/lanai-toolchain.c
Clang :: Driver/print-supported-cpus.c
Clang :: Driver/solaris-sparc-gcc-search.test
Clang :: Driver/stack-size-section.c

[MaskRay]

clang/test/Driver/suppress-err-on-target-specific-options.c

12 ↗

(On Diff #556033)

%if x86-registered-target is not needed. You can build llvm without X86 in LLVM_TARGETS_TO_BUILD: to verify.

[awarzynski]

clang/test/Driver/suppress-err-on-target-specific-options.c

1 ↗

(On Diff #556033)

Perhaps just name this file: no-input-file.c.

That or "no-action.c" as this is testing the behavior when:

• no action is created.

From what I can see, only -v and -### fall into this category (potentially more), so I would test _both_. This way it would be a dedicated test for "what happens when no actions are created". We could add more flags that fall into this category over time.

[mgabka]

I renamed the file as requested and added extra comment in the source code, and removed the RUN lines where the file was present, AFAIK there is no tests for such cases at the moment but this is not what may patch is changing, so I hope these tests are not needed for this patch to land.

[awarzynski]

LGTM, thanks a ton for the fix! Please wait for @MaskRay to approve too.

clang/lib/Driver/Driver.cpp

4957–4959

[nit] "target specific options are not consumed/validated" -> it's worth adding that that's because no action is created.

[MaskRay]

It is fixing https://github.com/llvm/llvm-project/issues/64958

Nit: just say

"Fix https://github.com/llvm/llvm-project/issues/64958"

This means that we lose the ability to detect target-specific options for other targets, but I think it's better than erroring on supported options.

clang/test/Driver/no-action.c

8

Optional/super nit: it's not a pattern everybody uses, but I personally use /// for comments that are neither RUN nor CHECK. The /// comment stands out in some editors and helps certain automated analysis (e.g. auto detecting typo in possibly CHECK prefixes). This is super nit, so no need to do it.

clang/lib/Driver/Driver.cpp

if (!A->isClaimed()) {
         if (DuplicateClaimed)
           continue;
       }
 
       // In clang-cl, don't mention unknown arguments here since they have
       // already been warned about.
       if (!IsCLMode() || !A->getOption().matches(options::OPT_UNKNOWN)) {
         if (A->getOption().hasFlag(options::TargetSpecific) &&
-            !A->isIgnoredTargetSpecific() && !HasAssembleJob) {
+            !A->isIgnoredTargetSpecific() && !HasAssembleJob &&
+            // When for example -### or -v is used
+            // without a file, target specific options are not
+            // consumed/validated.
+            // Instead emitting an error emit a warning instead.
+            !C.getActions().empty()) {
           Diag(diag::err_drv_unsupported_opt_for_target)
               << A->getSpelling() << getTargetTriple();
         } else {
           Diag(clang::diag::warn_drv_unused_argument)
               << A->getAsString(C.getArgs());
         }
       }
     }

clang/test/Driver/no-action.c

+// RUN: %clang --target=aarch64-none-gnu --verbose -mcpu= -march= 2>&1 | FileCheck %s --check-prefix=WARNING
+// RUN: %clang --target=aarch64-none-gnu -### -mcpu= -march= 2>&1 | FileCheck %s --check-prefix=WARNING
+
+// RUN: %clang --target=x86_64-unknown-linux-gnu --verbose -mcpu= -march= 2>&1 | FileCheck %s --check-prefix=WARNING
+// RUN: %clang --target=x86_64-unknown-linux-gnu -### -mcpu= -march= 2>&1 | FileCheck %s --check-prefix=WARNING
+
+/// In situation when there is no compilation/linking clang should not emit error
+/// about target specific options, but just warn that are not used.
+WARNING: warning: argument unused during compilation
+WARNING: warning: argument unused during compilation