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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay abandoned this revision.
MaskRay added a comment.

Obsoleted by D159173 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158688/new/

https://reviews.llvm.org/D158688

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158688/new/

https://reviews.llvm.org/D158688

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158688/new/

https://reviews.llvm.org/D158688

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham accepted this revision.
simon_tatham added a comment.
This revision is now accepted and ready to land.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158688/new/

https://reviews.llvm.org/D158688

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ties Stuij via Phabricator via cfe-commits]

stuij added a comment.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158688/new/

https://reviews.llvm.org/D158688

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158688/new/

https://reviews.llvm.org/D158688

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: chill, peter.smith, simon_tatham, stuij.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158688

Files:
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Driver/aarch64-security-options.c
  clang/test/Driver/arm-security-options.c


Index: clang/test/Driver/arm-security-options.c
===
--- clang/test/Driver/arm-security-options.c
+++ clang/test/Driver/arm-security-options.c
@@ -75,6 +75,9 @@
 // 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"
Index: clang/test/Driver/aarch64-security-options.c
===
--- clang/test/Driver/aarch64-security-options.c
+++ clang/test/Driver/aarch64-security-options.c
@@ -27,8 +27,12 @@
 // 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]
 
@@ -49,7 +53,3 @@
 
 // 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'
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -539,6 +539,10 @@
   }
 }
 
+// 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.
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -259,12 +259,18 @@
   // 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"