cc1: Report an error for multiple actions unless separated by -main-file-name #91140

MaskRay · 2024-05-05T20:06:38Z

When multiple actions are specified, the last one is used and others are
overridden. This might lead to confusion if the user is used to driver's
-S -emit-llvm behavior.

%clang_cc1 -S -emit-llvm a.c     # -S is overridden
%clang_cc1 -emit-llvm -S a.c     # -emit-llvm is overridden
%clang_cc1 -fsyntax-only -S a.c  # -fsyntax-only is overridden

However, we want to continue supporting overriding the driver action with -Xclang:

clang -c -Xclang -ast-dump a.c (%clang -cc1 -emit-obj ... -main-file-name a.c ... -ast-dump)
clang -c -xc++ -Xclang -emit-module stl.modulemap

As an exception, we allow -ast-dump* options to be composed together
(e.g. -ast-dump -ast-dump-lookups in AST/ast-dump-lookups.cpp).

Created using spr 1.3.5-bogner

llvmbot · 2024-05-05T20:07:11Z

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

Changes

When multiple actions are specified, the last one is used and others are
overridden. This might lead to confusion if the user is used to driver's
-S -emit-llvm behavior.

%clang_cc1 -S -emit-llvm a.c     # -S is overridden
%clang_cc1 -emit-llvm -S a.c     # -emit-llvm is overridden
%clang_cc1 -fsyntax-only -S a.c  # -fsyntax-only is overridden

However, we want to continue supporting overriding the driver action with -Xclang:

clang -c -Xclang -ast-dump a.c (%clang -cc1 -emit-obj ... -main-file-name a.c ... -ast-dump)
clang -c -xc++ -Xclang -emit-module stl.modulemap

As an exception, we allow -ast-dump* options to be composed together
(e.g. -ast-dump -ast-dump-lookups in AST/ast-dump-lookups.cpp).

Full diff: https://github.com/llvm/llvm-project/pull/91140.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticFrontendKinds.td (+2)
(modified) clang/lib/Frontend/CompilerInvocation.cpp (+24)
(added) clang/test/Frontend/multiple-actions.c (+7)

diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index fcffadacc8e631..48c174cfa9e8a5 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -134,6 +134,8 @@ def err_fe_no_pch_in_dir : Error<
     "no suitable precompiled header file found in directory '%0'">;
 def err_fe_action_not_available : Error<
     "action %0 not compiled in">;
+def err_fe_invalid_multiple_actions : Error<
+    "action %0 is specified, another action is not allowed: %1">;
 def err_fe_invalid_alignment : Error<
     "invalid value '%1' in '%0'; alignment must be a power of 2">;
 def err_fe_invalid_exception_model
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 8312abc3603953..4a4c02b03ce938 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2841,6 +2841,30 @@ static bool ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
     }
 
     Opts.ProgramAction = *ProgramAction;
+
+    // Catch common mistakes when multiple actions are specified for cc1 (e.g.
+    // -S -emit-llvm means -emit-llvm while -emit-llvm -S means -S). However, to
+    // support driver `-c -Xclang ACTION` (-cc1 -emit-llvm file -main-file-name
+    // X ACTION), we suppress the error when the two actions are separated by
+    // -main-file-name.
+    //
+    // As an exception, accept composable -ast-dump*.
+    if (!A->getSpelling().starts_with("-ast-dump")) {
+      const Arg *SavedAction = nullptr;
+      for (const Arg *AA :
+           Args.filtered(OPT_Action_Group, OPT_main_file_name)) {
+        if (AA->getOption().matches(OPT_main_file_name)) {
+          SavedAction = nullptr;
+        } else if (!SavedAction) {
+          SavedAction = AA;
+        } else {
+          if (!A->getOption().matches(OPT_ast_dump_EQ))
+            Diags.Report(diag::err_fe_invalid_multiple_actions)
+                << A->getSpelling() << SavedAction->getSpelling();
+          break;
+        }
+      }
+    }
   }
 
   if (const Arg* A = Args.getLastArg(OPT_plugin)) {
diff --git a/clang/test/Frontend/multiple-actions.c b/clang/test/Frontend/multiple-actions.c
new file mode 100644
index 00000000000000..5dd9ac3c8754f7
--- /dev/null
+++ b/clang/test/Frontend/multiple-actions.c
@@ -0,0 +1,7 @@
+// RUN: not %clang_cc1 -S -emit-llvm -main-file-name %s 2>&1 | FileCheck %s --check-prefix=ERR1 --implicit-check-not=error:
+// ERR1: error: action -emit-llvm is specified, another action is not allowed: -S
+
+// RUN: not %clang_cc1 -main-file-name %s -emit-llvm-only -emit-llvm -S 2>&1 | FileCheck %s --check-prefix=ERR2 --implicit-check-not=error:
+// ERR2: error: action -S is specified, another action is not allowed: -emit-llvm-only
+
+// RUN: %clang_cc1 -S -main-file-name %s -emit-llvm -o /dev/null

ChuanqiXu9

LGTM.

jansvoboda11 · 2024-05-06T05:56:27Z

clang/lib/Frontend/CompilerInvocation.cpp

+    // -main-file-name.
+    //
+    // As an exception, accept composable -ast-dump*.
+    if (!A->getSpelling().starts_with("-ast-dump")) {


Any reason why use string-based prefix instead of checking for the OPT_ enums?

-ast-dump -ast-dump-lookups and -ast-dump-lookups -ast-dump are composable. llvm/utils/update_cc_test_checks.py appends -ast-dump=json when the cc1 command line may contain another action option.

Since there are 3+ -ast-dump* options that need an exception, I feel that starts_with("-ast-dump")) is simpler than listing all OPT_ast_dump*..

jansvoboda11 · 2024-05-06T06:05:02Z

This sounds good in principle, but some tests are failing in CI, so this might need more refinement.

erichkeane

CI seems to have noticed what I feared :) This is going to hit a good amount of our internal tests, there is no way our copy-paste cargo-cult didn't come up with this before.

erichkeane · 2024-05-06T14:08:56Z

clang/include/clang/Basic/DiagnosticFrontendKinds.td

@@ -134,6 +134,8 @@ def err_fe_no_pch_in_dir : Error<
    "no suitable precompiled header file found in directory '%0'">;
 def err_fe_action_not_available : Error<
    "action %0 not compiled in">;
+def err_fe_invalid_multiple_actions : Error<
+    "action %0 is specified, another action is not allowed: %1">;


Suggested change

"action %0 is specified, another action is not allowed: %1">;

"'%1' action ignored; '%0' action specified previously">;

Maybe something like this? I dont think the original fits very well? Also the single ticks around the actions is, IMO, better for readability.

Thanks for the suggestion. I adopted it by swapping %1 and %0 ...

Created using spr 1.3.5-bogner

MaskRay · 2024-05-06T18:02:21Z

This sounds good in principle, but some tests are failing in CI, so this might need more refinement.

There were %clang_analyze_cc1 -analyze (redundant -analyze) errors. Fixed by e4d2427

MaskRay · 2024-05-06T18:03:29Z

CI seems to have noticed what I feared :) This is going to hit a good amount of our internal tests, there is no way our copy-paste cargo-cult didn't come up with this before.

I've fixed all upstream tests. This one-liner might be useful to auto fix some downstream tests...

xargs sed -Ei 's/(-fsyntax-only|-emit-obj|-emit-llvm|-emit-llvm-only|-S|-E) (([^|%]|%s|%t)*)(-analyze|-ast-dump|-emit-llvm|-fsyntax-only|-emit-pch|-E|-S)/\2\4/' < /tmp/0

erichkeane

This looks good to me, please give this a day or so for any conversations to die down.

bogner · 2024-05-23T19:36:36Z

I don't really understand the rationale for this, and it's kind of annoying. Most of the compiler's flags behave in the "last one wins" fashion (such as -O2 and -O0) and it's always been convenient to add the flag you want at the end. Why treat action flags any differently? Also, even if this is worthwhile for some reason I haven't considered, why is it an error rather than a warning?

MaskRay · 2024-05-28T04:06:05Z

I don't really understand the rationale for this, and it's kind of annoying. Most of the compiler's flags behave in the "last one wins" fashion (such as -O2 and -O0) and it's always been convenient to add the flag you want at the end. Why treat action flags any differently? Also, even if this is worthwhile for some reason I haven't considered, why is it an error rather than a warning?

@bogner Some action options are shared between driver and cc1 but the behaviors could be quite different. See my example in the description.

%clang_cc1 -S -emit-llvm a.c     # -S is overridden
%clang_cc1 -emit-llvm -S a.c     # -emit-llvm is overridden
%clang_cc1 -fsyntax-only -S a.c  # -fsyntax-only is overridden

The strictness helps ensure that %clang_cc1 tests do not have misleading, overridden action options.

bogner · 2024-05-28T17:21:51Z

I don't really understand the rationale for this, and it's kind of annoying. Most of the compiler's flags behave in the "last one wins" fashion (such as -O2 and -O0) and it's always been convenient to add the flag you want at the end. Why treat action flags any differently? Also, even if this is worthwhile for some reason I haven't considered, why is it an error rather than a warning?

@bogner Some action options are shared between driver and cc1 but the behaviors could be quite different. See my example in the description.
%clang_cc1 -S -emit-llvm a.c     # -S is overridden
%clang_cc1 -emit-llvm -S a.c     # -emit-llvm is overridden
%clang_cc1 -fsyntax-only -S a.c  # -fsyntax-only is overridden
The strictness helps ensure that %clang_cc1 tests do not have misleading, overridden action options.

So the oddity here really is the driver, which translates both -S -emit-llvm and -emit-llvm -S to -emit-llvm-bc in the -cc1 invocation. Before this change the cc1 invocation treated -S and the various -emit-XYZ flags consistently with other options like -O, -g, and most -f group flags, letting later ones override earlier.

I suppose that it's somewhat reasonable to warn users that -cc1 behaves differently than the driver for certain combinations of -S and -emit-llvm, and it's probably too impactful of a change to change the driver. Even so, I really think this should just be a warning and not a hard error.

MaskRay · 2024-05-29T20:07:06Z

I don't really understand the rationale for this, and it's kind of annoying. Most of the compiler's flags behave in the "last one wins" fashion (such as -O2 and -O0) and it's always been convenient to add the flag you want at the end. Why treat action flags any differently? Also, even if this is worthwhile for some reason I haven't considered, why is it an error rather than a warning?

@bogner Some action options are shared between driver and cc1 but the behaviors could be quite different. See my example in the description.
%clang_cc1 -S -emit-llvm a.c     # -S is overridden
%clang_cc1 -emit-llvm -S a.c     # -emit-llvm is overridden
%clang_cc1 -fsyntax-only -S a.c  # -fsyntax-only is overridden
The strictness helps ensure that %clang_cc1 tests do not have misleading, overridden action options.
So the oddity here really is the driver, which translates both -S -emit-llvm and -emit-llvm -S to -emit-llvm-bc in the -cc1 invocation. Before this change the cc1 invocation treated -S and the various -emit-XYZ flags consistently with other options like -O, -g, and most -f group flags, letting later ones override earlier.

I suppose that it's somewhat reasonable to warn users that -cc1 behaves differently than the driver for certain combinations of -S and -emit-llvm, and it's probably too impactful of a change to change the driver. Even so, I really think this should just be a warning and not a hard error.

The driver behavior isn't that odd. "Last option wins" applies to -ffoo -fno-foo but only in a few cases -ffoo -fbar.
cc1 reuses the option names but gives them different semantics (last action group wins).

The -emit-llvm option might be weird: it modifies -c and -S in the driver while it is an action in cc1.

Before this patch and the associated cleanups, a lot of tests had more than one actions and the authors were unaware of the issue, e.g. -ast-dump -emit-pch (-ast-dump is ignored), -emit-obj -emit-llvm (-emit-obj is ignored). With this error behavior, these %clang_cc1 usage mistakes will be caught. A warning would not provide protection...

Follow-up to a previous simplification 2473b1a. The xor difference between a SHT_NOTE and a read-only SHT_PROGBITS (previously >=NOT_SPECIAL) should be smaller than RF_EXEC. Otherwise, for the following section layout, `findOrphanPos` would place .text before note. ``` // simplified from linkerscript/custom-section-type.s non orphans: progbits 0x8060c00 NOT_SPECIAL note 0x8040003 orphan: .text 0x8061000 NOT_SPECIAL ```

[𝘀𝗽𝗿] initial version

c867733

Created using spr 1.3.5-bogner

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 5, 2024

MaskRay requested review from AaronBallman, erichkeane, jansvoboda11 and ChuanqiXu9 May 5, 2024 20:07

ChuanqiXu9 approved these changes May 6, 2024

View reviewed changes

jansvoboda11 reviewed May 6, 2024

View reviewed changes

erichkeane reviewed May 6, 2024

View reviewed changes

rebase on -analyze fixes: e4d2427

3099ab6

Created using spr 1.3.5-bogner

erichkeane approved these changes May 6, 2024

View reviewed changes

jansvoboda11 approved these changes May 7, 2024

View reviewed changes

MaskRay merged commit e74a7a9 into main May 7, 2024
4 checks passed

MaskRay deleted the users/MaskRay/spr/cc1-report-an-error-for-multiple-actions-unless-separated-by-main-file-name branch May 7, 2024 16:15

MaskRay mentioned this pull request May 9, 2024

[AArch64][PAC][clang][ELF] Support PAuth ABI core info #85235

Merged

momchil-velikov mentioned this pull request May 9, 2024

[AArch64] Add intrinsics for multi-vector to ZA array vector accumulators #88266

Merged

MaskRay mentioned this pull request May 14, 2024

[CIR] Build out AST consumer patterns to reach the entry point into CIRGen #91007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cc1: Report an error for multiple actions unless separated by -main-file-name #91140

cc1: Report an error for multiple actions unless separated by -main-file-name #91140

MaskRay commented May 5, 2024

llvmbot commented May 5, 2024

ChuanqiXu9 left a comment

jansvoboda11 May 6, 2024

MaskRay May 6, 2024

jansvoboda11 commented May 6, 2024

erichkeane left a comment

erichkeane May 6, 2024

MaskRay May 6, 2024

MaskRay commented May 6, 2024

MaskRay commented May 6, 2024 •

edited

Loading

erichkeane left a comment

bogner commented May 23, 2024

MaskRay commented May 28, 2024

bogner commented May 28, 2024

MaskRay commented May 29, 2024 •

edited

Loading

	"action %0 is specified, another action is not allowed: %1">;
	"'%1' action ignored; '%0' action specified previously">;

cc1: Report an error for multiple actions unless separated by -main-file-name #91140

cc1: Report an error for multiple actions unless separated by -main-file-name #91140

Conversation

MaskRay commented May 5, 2024

llvmbot commented May 5, 2024

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

jansvoboda11 May 6, 2024

Choose a reason for hiding this comment

MaskRay May 6, 2024

Choose a reason for hiding this comment

jansvoboda11 commented May 6, 2024

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane May 6, 2024

Choose a reason for hiding this comment

MaskRay May 6, 2024

Choose a reason for hiding this comment

MaskRay commented May 6, 2024

MaskRay commented May 6, 2024 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

bogner commented May 23, 2024

MaskRay commented May 28, 2024

bogner commented May 28, 2024

MaskRay commented May 29, 2024 • edited Loading

MaskRay commented May 6, 2024 •

edited

Loading

MaskRay commented May 29, 2024 •

edited

Loading