[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Ana Pazos via Phabricator via cfe-commits]

apazos added a comment.

Can you confirm when the GNU toolchain will have this flag supported in their 
assembler? Compatibility between LLVM and GNU toolchains is important.

Stephen - I think we can abandon this review. Users will need to be made aware 
of this additional assembler flag when building .s files with BTI enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

In D93428#2461343 , @apazos wrote:

> Thanks Daniel for the explanation.

No problem at all.

In D93428#2461343 , @apazos wrote:

> Was the support added for GNU assembler as well? Is it the same flag name?

Not yet, I requested the same flag from our GNU team when the flag is added to 
Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Ana Pazos via Phabricator via cfe-commits]

apazos added a comment.

Thanks Daniel for the explanation. Was the support added for GNU assembler as 
well? Is it the same flag name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

> Is there a reason why assembly files have a different flag (i.e. 
> -mmark-bti-property) to create the .note.gnu.property with the BTI entry?

In assembly the compiler can't guarantee the landing pads are in place, 
therefore it doesn't add it automatically.
The original concept was this the developers should add the landing pads 
wherever needed and by adding the note they mark the file is compatible with 
BTI.
After adding BTI to many assembly code it was clear the note is error prone and 
cumbersome to handle and I thing it provides zero protection against regression 
issues , so the `-mmark-bti-property` is introduced. 
The developers still should add the landing pads but optionally they could mark 
the files in the build system instead of the assembly files. 
The worry is if the assembly file would be marked automatically the produces 
binary probably won't run correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Ana Pazos via Phabricator via cfe-commits]

apazos added a comment.

Thanks for clarifying - so the property is being set for C/C++ files but not 
for assembly files. I think it should be set automatically for both when one 
uses clang driver to compile/assemble.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Stephen Long via Phabricator via cfe-commits]

steplong added a comment.

In D93428#2460032 , @danielkiss wrote:

> The `.note.gnu.property` is already generated when C/C++ files are compiled 
> with `-mbranch-protection=bti`.  
> `-mmark-bti-property` is only for assembly file where the 
> `.note.gnu.property` should be added manually otherwise.
>
> Do you have any reproducer where C/C++ behaves unexpectedly?

I think Ana misunderstood the reason for this patch. We haven't seen any 
compilation of C/C++ behaving unexpectedly. -mbranch-protection=bti generates 
the .nute.gnu.property for C/C++ files from our experiments, but doesn't work 
for assembly files. Is there a reason why assembly files have a different flag 
(i.e. -mmark-bti-property) to create the .note.gnu.property with the BTI entry?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

The `.note.gnu.property` is already generated when C/C++ files are compiled 
with `-mbranch-protection=bti`.  
`-mmark-bti-property` is only for assembly file where the `.note.gnu.property` 
should be added manually otherwise.

Do you have any reproducer where C/C++ behaves unexpectedly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Ana Pazos via Phabricator via cfe-commits]

apazos added a comment.

So the intention here is to generate the property when BTI branch protection is 
enabled, to guarantee the generate .o indeed has the property set, without 
requiring to pass the flag -mmark-bti-property explicitly. This is convenient 
for users.

Or is there a clear reason why one would enable BIT branch protection and not 
have the property set? What would be that use case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93428

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

[PATCH] D93428: [AArch64] Add bti note property when compiling asm files with -mbranch-protection=bti

[Stephen Long via Phabricator via cfe-commits]

steplong created this revision.
Herald added subscribers: danielkiss, kristof.beyls.
steplong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Generate the .note.gnu.property section with GNU_PROPERTY_AARCH64_FEATURE_1_BTI
when compiling assembly files with -mbranch-protection=bti.

See https://reviews.llvm.org/D81930 for generating the section when compiling
C/C++ files with -mbranch-protection=bti.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93428

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/test/Driver/arm64-markbti.S


Index: clang/test/Driver/arm64-markbti.S
===
--- clang/test/Driver/arm64-markbti.S
+++ clang/test/Driver/arm64-markbti.S
@@ -1,10 +1,12 @@
 // REQUIRES: aarch64-registered-target
 
-// When -mmark-bti-property is passed the generated file object gets BTI 
marking.
+// When -mmark-bti-property or -mbranch-protection=bti is passed the generated 
file object gets BTI marking.
 // RUN: %clang -target arm64-linux-none -mmark-bti-property -c -o - %s | 
llvm-readobj -n - | FileCheck -check-prefix=CHECK  -check-prefix=CHECK_GEN %s
 // RUN: %clang -target arm64-linux-none -DNOTE_PRESENT -c %s -o - | 
llvm-readobj -n - | FileCheck -check-prefix=CHECK  -check-prefix=CHECK_PRESET %s
 // RUN: %clang -target arm64-linux-none -mmark-bti-property -DNOTE_PRESENT -c 
%s -o - | llvm-readobj -n - | FileCheck -check-prefix=CHECK  
-check-prefix=CHECK_PRESET %s
 // RUN: %clang -target arm64-linux-none -mmark-bti-property -DNOTE_PRESENT -c 
%s -o - 2>&1 |  FileCheck -check-prefix=CHECK_WARNING %s
+// RUN: %clang -target arm64-linux-none -mbranch-protection=bti -c -o - %s | 
llvm-readobj -n - | FileCheck -check-prefix=CHECK  -check-prefix=CHECK_GEN %s
+// RUN: %clang -target arm64-linux-none -mbranch-protection=bti -DNOTE_PRESENT 
-c %s -o - 2>&1 |  FileCheck -check-prefix=CHECK_WARNING %s
 //
 // CHECK_WARNING: The .note.gnu.property is not emitted because it is already 
present.
 // CHECK: Name: .note.gnu.property
Index: clang/lib/Driver/ToolChains/Clang.h
===
--- clang/lib/Driver/ToolChains/Clang.h
+++ clang/lib/Driver/ToolChains/Clang.h
@@ -128,6 +128,8 @@
 llvm::opt::ArgStringList ) const;
   void AddRISCVTargetArgs(const llvm::opt::ArgList ,
   llvm::opt::ArgStringList ) const;
+  void AddAArch64TargetArgs(const llvm::opt::ArgList ,
+llvm::opt::ArgStringList ) const;
   bool hasGoodDiagnostics() const override { return true; }
   bool hasIntegratedAssembler() const override { return false; }
   bool hasIntegratedCPP() const override { return false; }
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7015,6 +7015,29 @@
   CmdArgs.push_back(ABIName.data());
 }
 
+void ClangAs::AddAArch64TargetArgs(const ArgList ,
+   ArgStringList ) const {
+  bool BranchTargetEnforce = false;
+  const auto  = getToolChain().getDriver();
+
+  if (Arg *A = Args.getLastArg(options::OPT_mbranch_protection_EQ)) {
+StringRef Err;
+llvm::AArch64::ParsedBranchProtection PBP;
+
+if (!llvm::AArch64::parseBranchProtection(A->getValue(), PBP, Err)) {
+  D.Diag(diag::err_invalid_branch_protection)
+  << Err << A->getAsString(Args);
+} else {
+  BranchTargetEnforce = PBP.BranchTargetEnforcement;
+}
+  }
+
+  if (Args.hasArg(options::OPT_mmark_bti_property) || BranchTargetEnforce) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-aarch64-mark-bti-property");
+  }
+}
+
 void ClangAs::ConstructJob(Compilation , const JobAction ,
const InputInfo , const InputInfoList 
,
const ArgList ,
@@ -7193,10 +7216,7 @@
   case llvm::Triple::aarch64:
   case llvm::Triple::aarch64_32:
   case llvm::Triple::aarch64_be:
-if (Args.hasArg(options::OPT_mmark_bti_property)) {
-  CmdArgs.push_back("-mllvm");
-  CmdArgs.push_back("-aarch64-mark-bti-property");
-}
+AddAArch64TargetArgs(Args, CmdArgs);
 break;
 
   case llvm::Triple::riscv32:


Index: clang/test/Driver/arm64-markbti.S
===
--- clang/test/Driver/arm64-markbti.S
+++ clang/test/Driver/arm64-markbti.S
@@ -1,10 +1,12 @@
 // REQUIRES: aarch64-registered-target
 
-// When -mmark-bti-property is passed the generated file object gets BTI marking.
+// When -mmark-bti-property or -mbranch-protection=bti is passed the generated file object gets BTI marking.
 // RUN: %clang -target arm64-linux-none -mmark-bti-property -c -o - %s | llvm-readobj -n - | FileCheck -check-prefix=CHECK  -check-prefix=CHECK_GEN %s
 //