[Driver,ARM,AArch64] Ignore -mbranch-protection= diagnostics for assembler input

[MaskRay]

Summary

Some options are only claimed in AddARMTargetArgs/AddAArch64TargetArgs/etc (called by
Clang::RenderTargetOptions).
For assembler input, Add*TargetArgs is not called. If an option is
unclaimed, it either leads to a -Wunused-command-line-argument warning
or an error (if TargetSpecific is set)

// clang '-###' --target=aarch64 -mbranch-protection=bti -c a.s
clang: error: unsupported option '-mbranch-protection=' for target 'aarch64'

It seems that ignoring the diagnostics is most useful as it matches GCC
and users can do

clang --target=aarch64 -mbranch-protection=bti -S a.c
clang --target=aarch64 -mbranch-protection=bti -c a.s

without changing the options.

Planned for main and release/17.x

[MaskRay]

Ping:) This is probably a good candidate for release/17.x backporting.

[stuij]

LGTM. Thanks!

[simon_tatham]

The change LGTM, and "agree with gcc" seems like a reasonable justification in this case.

But I'm curious more generally about what options should / shouldn't be covered by -Wunused-command-line-argument. Doesn't the same reasoning apply to most options that C compilation uses and assembly doesn't? If you have a command of the form clang -someoption -c foo.c, it's surely always convenient for a user to be able to change the .c into a .s, or to put a variable list of files on the end of the command line which might or might not include any .c files. Why is this option in particular different from others? Is there a documented policy anywhere?

[MaskRay]

In D158688#4624267, @simon_tatham wrote:

The change LGTM, and "agree with gcc" seems like a reasonable justification in this case.

Thank you both!

But I'm curious more generally about what options should / shouldn't be covered by -Wunused-command-line-argument. Doesn't the same reasoning apply to most options that C compilation uses and assembly doesn't? If you have a command of the form clang -someoption -c foo.c, it's surely always convenient for a user to be able to change the .c into a .s, or to put a variable list of files on the end of the command line which might or might not include any .c files.

-Wunused-command-line-argument does fire for most options when the only input kind is assembly without preprocessing.
It seems that the diagnostics are for assembler input, not assembler-with-cpp...

Why is this option in particular different from others? Is there a documented policy anywhere?

I am not aware of a documented policy anywhere, but I have some notes on https://maskray.me/blog/2023-08-25-clang-wunused-command-line-argument#assembler-input .

Let me ask on Discourse: https://discourse.llvm.org/t/wunused-command-line-argument-for-assembly-files/73111

[MaskRay]

In D158688#4625839, @MaskRay wrote:

In D158688#4624267, @simon_tatham wrote:

The change LGTM, and "agree with gcc" seems like a reasonable justification in this case.

Thank you both!

But I'm curious more generally about what options should / shouldn't be covered by -Wunused-command-line-argument. Doesn't the same reasoning apply to most options that C compilation uses and assembly doesn't? If you have a command of the form clang -someoption -c foo.c, it's surely always convenient for a user to be able to change the .c into a .s, or to put a variable list of files on the end of the command line which might or might not include any .c files.

-Wunused-command-line-argument does fire for most options when the only input kind is assembly without preprocessing.
It seems that the diagnostics are for assembler input, not assembler-with-cpp...

Why is this option in particular different from others? Is there a documented policy anywhere?

I am not aware of a documented policy anywhere, but I have some notes on https://maskray.me/blog/2023-08-25-clang-wunused-command-line-argument#assembler-input .

Let me ask on Discourse: https://discourse.llvm.org/t/wunused-command-line-argument-for-assembly-files/73111

I think we should do D159173 .. The warnings are still useful, but we can do it in Driver.cpp to avoid the ForAS hacks.

[MaskRay]

Obsoleted by D159173.

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

void aarch64::getAArch64TargetFeatures(const Driver &D,
                                        const ArgList &Args,
                                        std::vector<StringRef> &Features,
                                        bool ForAS) {
   Arg *A;
   bool success = true;
   // Enable NEON by default.
   Features.push_back("+neon");
   llvm::StringRef WaMArch;
-  if (ForAS)
+  if (ForAS) {
+    // Some target-specific options are only handled in AddAArch64TargetArgs,
+    // not called for assembler input. Claim them.
+    Args.claimAllArgs(options::OPT_mbranch_protection_EQ,
+                                options::OPT_msign_return_address_EQ);
+
     for (const auto *A :
          Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler))
       for (StringRef Value : A->getValues())
         if (Value.startswith("-march="))
           WaMArch = Value.substr(7);
+  }
   // Call getAArch64ArchFeaturesFromMarch only if "-Wa,-march=" or
   // "-Xassembler -march" is detected. Otherwise it may return false
   // and causes Clang to error out.
   if (!WaMArch.empty())
     success = getAArch64ArchFeaturesFromMarch(D, WaMArch, Args, Features);
   else if ((A = Args.getLastArg(options::OPT_march_EQ)))
     success = getAArch64ArchFeaturesFromMarch(D, A->getValue(), Args, Features);
   else if ((A = Args.getLastArg(options::OPT_mcpu_EQ)))

clang/lib/Driver/ToolChains/Arch/ARM.cpp

for (const Arg *A :
         } else if (Value.startswith("-mhwdiv=")) {
           WaHDiv = std::make_pair(A, Value.substr(8));
         } else if (Value.startswith("-march=")) {
           WaArch = std::make_pair(A, Value.substr(7));
         }
       }
     }
 
+    // Some target-specific options are only handled in AddARMTargetArgs, not
+    // called for assembler input. Claim them.
+    Args.claimAllArgs(options::OPT_mbranch_protection_EQ);
+
     // The integrated assembler doesn't implement e_flags setting behavior for
     // -meabi=gnu (gcc -mabi={apcs-gnu,atpcs} passes -meabi=gnu to gas). For
     // compatibility we accept but warn.
     if (Arg *A = Args.getLastArgNoClaim(options::OPT_mabi_EQ))
       A->ignoreTargetSpecific();
   }
 
   if (getReadTPMode(D, Args, Triple, ForAS) == ReadTPMode::TPIDRURW)

clang/test/Driver/aarch64-security-options.c

 // RUN: FileCheck %s --check-prefix=CONFLICT --check-prefix=WARN
 
 // RUN: not %clang --target=aarch64 -c %s -### -msign-return-address=foo     2>&1 | \
 // RUN: FileCheck %s --check-prefix=BAD-RA-PROTECTION --check-prefix=WARN
 
 // RUN: not %clang --target=aarch64 -c %s -### -mbranch-protection=bar     2>&1 | \
 // RUN: FileCheck %s --check-prefix=BAD-BP-PROTECTION --check-prefix=WARN
 
-// RUN: %clang --target=aarch64 -### -o /dev/null -mbranch-protection=standard /dev/null 2>&1 | \
-// RUN: FileCheck --allow-empty %s --check-prefix=LINKER-DRIVER
+/// Check that the linker driver doesn't warn about -mbranch-protection=standard
+/// as an unused option.
+// RUN: %clang --target=aarch64 -Werror -### -o /dev/null -mbranch-protection=standard /dev/null
+
+/// Don't warn for assembler input.
+// RUN: %clang -### --target=aarch64 -Werror -c -x assembler %s -mbranch-protection=standard -msign-return-address=all
 
 // WARN-NOT: warning: ignoring '-mbranch-protection=' option because the 'aarch64' architecture does not support it [-Wbranch-protection]
 
 // RA-OFF: "-msign-return-address=none"
 // RA-NON-LEAF: "-msign-return-address=non-leaf"
 // RA-ALL: "-msign-return-address=all"
 
 // KEY-A: "-msign-return-address-key=a_key"
 // KEY-NOT: "-msign-return-address-key"
 
 // BTE-OFF-NOT: "-mbranch-target-enforce"
 // BTE-ON: "-mbranch-target-enforce"
 
 // CONFLICT: "-msign-return-address=none"
 
 // BAD-RA-PROTECTION: unsupported argument 'foo' to option '-msign-return-address='
 // BAD-BP-PROTECTION: unsupported argument 'bar' to option '-mbranch-protection='
 
 // BAD-B-KEY-COMBINATION: unsupported argument 'b-key' to option '-mbranch-protection='
 // BAD-LEAF-COMBINATION: unsupported argument 'leaf' to option '-mbranch-protection='
-
-// Check that the linker driver doesn't warn about -mbranch-protection=standard
-// as an unused option.
-// LINKER-DRIVER-NOT: warning: argument unused during compilation: '-mbranch-protection=standard'

clang/test/Driver/arm-security-options.c

 // RUN: FileCheck %s --check-prefix=INCOMPATIBLE-ARCH
 
 /// RUN: %clang -target arm-arm-none-eabi -march=armv7-a -mthumb -c %s -### -mbranch-protection=bti 2>&1 | \
 /// RUN: FileCheck %s --check-prefix=INCOMPATIBLE-ARCH
 
 // RUN: %clang -target arm-arm-none-eabi -march=armv7-r -c %s -### -mbranch-protection=bti 2>&1 | \
 // RUN: FileCheck %s --check-prefix=INCOMPATIBLE-ARCH
 
+/// Don't warn for assembler input.
+// RUN: %clang -### -Werror --target=arm-arm-none-eabi -march=armv7-m -x assembler -c %s -mbranch-protection=pac-ret
+
 // RA-OFF: "-msign-return-address=none"
 // RA-NON-LEAF: "-msign-return-address=non-leaf"
 // RA-ALL: "-msign-return-address=all"
 
 // KEY-A: "-msign-return-address-key=a_key"
 // KEY-B: "-msign-return-address-key=b_key"
 
 // BTE-OFF-NOT: "-mbranch-target-enforce"
 // BTE-ON: "-mbranch-target-enforce"
 
 // BAD-BP-PROTECTION: unsupported argument 'bar' to option '-mbranch-protection='
 
 // BAD-B-KEY-COMBINATION: unsupported argument 'b-key' to option '-mbranch-protection='
 // BAD-LEAF-COMBINATION: unsupported argument 'leaf' to option '-mbranch-protection='
 
 // INCOMPATIBLE-ARCH: '-mbranch-protection=' option is incompatible with the '{{.*}}' architecture