[PATCH] D132531: [AArch64] Reserve more physical registers

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

See https://reviews.llvm.org/D56305 and https://reviews.llvm.org/D48580 for 
previous related discussions.
I think it would be helpful to understand the use case for being able to 
reserve x8, x16, x17 and x19 better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132531

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

[PATCH] D132531: [AArch64] Reserve more physical registers

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

My understanding is that X8, X16, X17 and X19 cannot be reserved because the 
code generator in places will make use of them.
For example, using X19 as a base register in some cases. X16 and X17 are 
defined by the ABI to potentially be clobbered on function calls & when a 
veneer needs to be inserted by a linker, it does get clobbered. IIRC, some of 
the security mitigations implemented in LLVM also clobber these 2 registers on 
function calls.
I'm not fully sure why X8 cannot be reserved.

In short, I don't think it's a good idea to enable users to reserve these 
registers as the compiler/toolchain/ABI will not respect their request to 
reserve these registers.

It seems your motivation for being able to reserve more registers is to be able 
to more easily write regression test for register allocation.
Are there other ways to achieve that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132531

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

[PATCH] D126137: [X86] Add support for `-mharden-sls=all`

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html, documentation for 
"mharden-sls": For AArch64, the options available on the command line are 
"retbr", "blr", "none" and "all".
I don't think the options necessarily have to be the same for x86.
But assuming I understand this patch correctly, it seems to me that with this 
patch -mharden-sls=all would mean fundamentally slightly different things for 
x86 vs arm and aarch64, which could be confusing to users.
IIUC this patch correctly, this patch implements the equivalent of 
aarch64/arm's -mharden-sls=retbr (i.e. add a straight-line-speculation 
mitigation for returns and indirect jumps, but not for indirect function calls).
Therefore, I wonder if it wouldn't be better to name this -mharden-sls=retbr 
for more consistency across architectures?
Or is the indirect function call case not relevant for x86 (sorry - I'm not up 
to speed on the details on the x86 side)?

Or does `MBB.back().getDesc().isIndirectBranch()` also return True for indirect 
calls, in which case my whole remark here can probably be ignored?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126137

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

[PATCH] D124836: [AArch64] Add support for -fzero-call-used-regs

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:752
+#undef CASE
+  }
+}

Just a drive-by comment: I'm wondering if SVE registers should also be listed 
here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124836

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

[PATCH] D113942: [NFC][clang] Inclusive language: replace master with main in convert_arm_neon.py

[Kristof Beyls via Phabricator via cfe-commits]

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

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113942

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

[PATCH] D112941: [clang] Add support for the new pointer authentication builtins.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2865-2872
+let Group = f_Group in {
+  let Flags = [CC1Option] in {
+def fptrauth_intrinsics : Flag<["-"], "fptrauth-intrinsics">,
+  HelpText<"Enable pointer-authentication intrinsics">;
+  }
+  def fno_ptrauth_intrinsics : Flag<["-"], "fno-ptrauth-intrinsics">;
+}

bruno wrote:
> ab wrote:
> > rjmccall wrote:
> > > kristof.beyls wrote:
> > > > My impression is that generally for `__builtin_XXX` intrinsics, there 
> > > > are no compiler flags to make them available or remove their 
> > > > availability.
> > > > Is there a good reason why a command line option is needed for the 
> > > > `__builtin_ptrauth` intrinsics, but not (IIUC) for most or any other 
> > > > existing `__builtin_XXX` intrinsic?
> > > > If there is no good reason, it seems better to me to not have a command 
> > > > line option so there is better consistency across all `__builtin_XXX` 
> > > > intrinsics?
> > > > 
> > > > (after having read more of the patch): my impression has changed now 
> > > > that the f(no-)ptrauth-intrinsics flag rather selects whether the 
> > > > ptrauth intrinsics get lowered to PAuth hardware instructions, or to 
> > > > "regular" instructions emulating the behavior of authenticated 
> > > > pointers. If that is correct (and assuming it's a useful option to 
> > > > have), I would guess a different name for the command line option could 
> > > > be less misleading. As is, it suggests this selects whether ptrauth_ 
> > > > intrinsics are available or not. If instead, as I'm guessing above, 
> > > > this selects whether ptrauth_ intrinsics get lowered to PAuth 
> > > > instructions or not, maybe something like '-femulate-ptrauth' would 
> > > > describe the effect of the command line switch a bit better?
> > > The ptrauth features were implemented gradually, beginning with the 
> > > intrinsics.  Originally we needed a way to enable the intrinsics feature 
> > > without relying on target information.  We do still need a way to enable 
> > > them without necessarily enabling automatic type/qualifier-based pointer 
> > > authentication.  I don't know if we need to be able to *disable* them 
> > > when the target supports them; I agree that that would be a little 
> > > strange.
> > > 
> > > If not, we could just enable the intrinsics whenever either the target 
> > > says they're okay or software emulation (a separate, experimental 
> > > feature) is enabled.  The AArch64 target has a `+pauth` target feature.  
> > > However, I don't know if `-arch arm64e` actually adds that feature on 
> > > Apple targets.  Also, the `HasPAuth` field in the clang `TargetInfo` does 
> > > not appear to be properly initialized to `false` when `+pauth` *isn't* 
> > > present; fortunately, that field never used.
> > > 
> > > I'm not sure if it would actually be okay to remove the 
> > > `-fptrauth-intrinsics` driver option if we just enabled the intrinsics 
> > > based on the target feature.  That does feel cleaner, but unfortunately, 
> > > we at Apple probably have explicit uses of the option that we'd have to 
> > > clean up before we could remove the option.  We could treat that as an 
> > > Apple problem and keep it out of the open source tree, though, and maybe 
> > > remove the option altogether someday.
> > > 
> > > Ahmed, thoughts?
> > Hmm, I agree it would be strange to need to disable the intrinsics, but we 
> > do also gate the various higher-level qualifiers (and intrinsics) on 
> > `ptrauth_intrinsics`.  So, in `ptrauth.h` (and in various users) the 
> > feature now really means "we're in a 'ptrauth-aware' environment".  And it 
> > does make more sense to keep that separate from "we're running on a CPU 
> > that theoretically could support ptrauth".  It comes down to what 
> > "ptrauth-aware" really means, and that's probably also an Apple problem, 
> > and all current users of `ptrauth_intrinsics` should use something like 
> > `__arm64e__` instead.
> > 
> > That still means there's no equivalent for other targets and/or software 
> > emulation, but that seems okay: `ptrauth.h` already needs changes to be 
> > usable from anywhere other than arm64e (cf. the discussion about keys), and 
> > we can cross that bridge when we get there.
> > 
> > (One could argue that all the language-feature-specific qualifiers and 
> > intrinsics should be gated on the appropriate ptrauth_whatever feature, but 
> > the qualifiers are often used in precisely the glue/runtime code that 
> > doesn't build in the appropriate mode, so doesn't have the feature enabled.)
> > 
> > 
> > So, concretely, we could:
> > - continue gating these plain intrinsics on `ptrauth_intrinsics` in 
> > ptrauth.h (IIRC there's an ACLE feature macro but it's specific to return 
> > address signing and BTI defaults; I'll check)
> > - enable the feature when `+pauth`
> > - replace all other uses of `ptrauth_intrinsics` with 

[PATCH] D112941: [clang] Add support for the new pointer authentication builtins.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/lib/Headers/ptrauth.h:19-37
+  /* A process-independent key which can be used to sign code pointers.
+ Signing and authenticating with this key is a no-op in processes
+ which disable ABI pointer authentication. */
+  ptrauth_key_process_independent_code = ptrauth_key_asia,
+
+  /* A process-specific key which can be used to sign code pointers.
+ Signing and authenticating with this key is enforced even in processes

rjmccall wrote:
> kristof.beyls wrote:
> > I think, but am not sure, that the decision of which keys are process 
> > independent and which ones are process-dependent is a software platform 
> > choice?
> > If so, maybe ptrauth_key_process_{in,}dependent_* should only get defined 
> > conditionally?
> > I'm not sure if any decisions have been taken already for e.g. linux, 
> > Android, other platforms.
> > If not, maybe this block of code should be surrounded by an ifdef that is 
> > enabled only when targeting Darwin?
> Yes, in retrospect it was a bad idea to define these particular generic 
> names.  I believe Apple platforms no longer have "process-independent" keys.  
> It should really just be (1) the concrete keys, (2) recommended default keys 
> for code and data pointers, and then (3) the specific keys used in specific 
> schemas.  Beyond that, if people want a specific different key for some 
> purpose, they should ask for it.
> 
> Unfortunately, there's already a fair amount of code using these names.  We 
> could deprecate the old names and point people towards the new names, though.
Thanks for those background insights!
I was thinking that maybe the keys that should be deprecated could be enabled 
only when targeting Apple platforms? I'm assuming here that most existing code 
using these only target Apple platforms; so making them available only when 
targeting Apple platforms could help with not letting the use of them spread 
further without impacting existing code much?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112941

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

[PATCH] D112941: [clang] Add support for the new pointer authentication builtins.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2865-2872
+let Group = f_Group in {
+  let Flags = [CC1Option] in {
+def fptrauth_intrinsics : Flag<["-"], "fptrauth-intrinsics">,
+  HelpText<"Enable pointer-authentication intrinsics">;
+  }
+  def fno_ptrauth_intrinsics : Flag<["-"], "fno-ptrauth-intrinsics">;
+}

My impression is that generally for `__builtin_XXX` intrinsics, there are no 
compiler flags to make them available or remove their availability.
Is there a good reason why a command line option is needed for the 
`__builtin_ptrauth` intrinsics, but not (IIUC) for most or any other existing 
`__builtin_XXX` intrinsic?
If there is no good reason, it seems better to me to not have a command line 
option so there is better consistency across all `__builtin_XXX` intrinsics?

(after having read more of the patch): my impression has changed now that the 
f(no-)ptrauth-intrinsics flag rather selects whether the ptrauth intrinsics get 
lowered to PAuth hardware instructions, or to "regular" instructions emulating 
the behavior of authenticated pointers. If that is correct (and assuming it's a 
useful option to have), I would guess a different name for the command line 
option could be less misleading. As is, it suggests this selects whether 
ptrauth_ intrinsics are available or not. If instead, as I'm guessing above, 
this selects whether ptrauth_ intrinsics get lowered to PAuth instructions or 
not, maybe something like '-femulate-ptrauth' would describe the effect of the 
command line switch a bit better?



Comment at: clang/lib/Headers/ptrauth.h:19-37
+  /* A process-independent key which can be used to sign code pointers.
+ Signing and authenticating with this key is a no-op in processes
+ which disable ABI pointer authentication. */
+  ptrauth_key_process_independent_code = ptrauth_key_asia,
+
+  /* A process-specific key which can be used to sign code pointers.
+ Signing and authenticating with this key is enforced even in processes

I think, but am not sure, that the decision of which keys are process 
independent and which ones are process-dependent is a software platform choice?
If so, maybe ptrauth_key_process_{in,}dependent_* should only get defined 
conditionally?
I'm not sure if any decisions have been taken already for e.g. linux, Android, 
other platforms.
If not, maybe this block of code should be surrounded by an ifdef that is 
enabled only when targeting Darwin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112941

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

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Kristof Beyls via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3b93dc6880f7: Add basic aarch64-none-elf bare metal driver. 
(authored by kristof.beyls).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D34

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/test/Driver/baremetal.cpp
  clang/test/Driver/gcc_forward.c


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,16 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths:
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" 
"[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include/c++/v1"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[RESOURCE]]/include"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" 
"[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include"
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -125,6 +125,20 @@
   return true;
 }
 
+/// Is the triple aarch64-none-elf?
+static bool isAArch64BareMetal(const llvm::Triple ) {
+  if (Triple.getArch() != llvm::Triple::aarch64)
+return false;
+
+  if (Triple.getVendor() != llvm::Triple::UnknownVendor)
+return false;
+
+  if (Triple.getOS() != llvm::Triple::UnknownOS)
+return false;
+
+  return Triple.getEnvironmentName() == "elf";
+}
+
 static bool isRISCVBareMetal(const llvm::Triple ) {
   if (Triple.getArch() != llvm::Triple::riscv32 &&
   Triple.getArch() != llvm::Triple::riscv64)
@@ -151,7 +165,8 @@
 }
 
 bool BareMetal::handlesTarget(const llvm::Triple ) {
-  return isARMBareMetal(Triple) || isRISCVBareMetal(Triple);
+  return isARMBareMetal(Triple) || isAArch64BareMetal(Triple) ||
+ isRISCVBareMetal(Triple);
 }
 
 Tool *BareMetal::buildLinker() const {


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,16 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths:
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include/c++/v1"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[RESOURCE]]/include"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include"
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

@MaskRay - gentle ping: I wonder if you have any further remarks after I 
updated the patch based on your earlier feedback?


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

https://reviews.llvm.org/D34

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

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls updated this revision to Diff 377818.
kristof.beyls added a comment.

run clang-format on the patch.


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

https://reviews.llvm.org/D34

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/test/Driver/baremetal.cpp
  clang/test/Driver/gcc_forward.c


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,16 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths:
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" 
"[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include/c++/v1"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[RESOURCE]]/include"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" 
"[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include"
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -125,6 +125,20 @@
   return true;
 }
 
+/// Is the triple aarch64-none-elf?
+static bool isAArch64BareMetal(const llvm::Triple ) {
+  if (Triple.getArch() != llvm::Triple::aarch64)
+return false;
+
+  if (Triple.getVendor() != llvm::Triple::UnknownVendor)
+return false;
+
+  if (Triple.getOS() != llvm::Triple::UnknownOS)
+return false;
+
+  return Triple.getEnvironmentName() == "elf";
+}
+
 static bool isRISCVBareMetal(const llvm::Triple ) {
   if (Triple.getArch() != llvm::Triple::riscv32 &&
   Triple.getArch() != llvm::Triple::riscv64)
@@ -151,7 +165,8 @@
 }
 
 bool BareMetal::handlesTarget(const llvm::Triple ) {
-  return isARMBareMetal(Triple) || isRISCVBareMetal(Triple);
+  return isARMBareMetal(Triple) || isAArch64BareMetal(Triple) ||
+ isRISCVBareMetal(Triple);
 }
 
 Tool *BareMetal::buildLinker() const {


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,16 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths:
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include/c++/v1"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[RESOURCE]]/include"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include"
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -125,6 +125,20 @@
   

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:133
+
+  if (Triple.getVendor() != llvm::Triple::UnknownVendor)
+return false;

MaskRay wrote:
> Is vendor check necessary?
I'm guessing that without the vendor check, triples aarch64-//anything//-elf 
would also be considered bare metal triples?
Or would it be aarch64-none-//anything//-elf?
I'm afraid I don't know the answer to the question on whether the vendor check 
should be removed or not.

However, I think it's best for this patch to keep this as is to keep 
consistency with the arm and riscv bare metal targets in this file, which also 
have the same check for the triple vendor to be UnknownVendor.

If it would be best to drop this check, it probably would be best to do it as a 
separate patch, and potentially make the change for the arm and riscv triples 
too?



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

https://reviews.llvm.org/D34

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

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls updated this revision to Diff 377816.
kristof.beyls added a comment.

Updated test based on feedback from @MaskRay


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

https://reviews.llvm.org/D34

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/test/Driver/baremetal.cpp
  clang/test/Driver/gcc_forward.c


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,17 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths:
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" 
"[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include/c++/v1"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[RESOURCE]]/include"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" 
"[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include"
+
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -125,6 +125,21 @@
   return true;
 }
 
+/// Is the triple aarch64-none-elf?
+static bool isAArch64BareMetal(const llvm::Triple ) {
+  if (Triple.getArch() != llvm::Triple::aarch64)
+return false;
+
+  if (Triple.getVendor() != llvm::Triple::UnknownVendor)
+return false;
+
+  if (Triple.getOS() != llvm::Triple::UnknownOS)
+return false;
+
+  return Triple.getEnvironmentName() == "elf";
+}
+
+
 static bool isRISCVBareMetal(const llvm::Triple ) {
   if (Triple.getArch() != llvm::Triple::riscv32 &&
   Triple.getArch() != llvm::Triple::riscv64)
@@ -151,7 +166,8 @@
 }
 
 bool BareMetal::handlesTarget(const llvm::Triple ) {
-  return isARMBareMetal(Triple) || isRISCVBareMetal(Triple);
+  return isARMBareMetal(Triple) || isAArch64BareMetal(Triple) ||
+isRISCVBareMetal(Triple);
 }
 
 Tool *BareMetal::buildLinker() const {


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,17 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths:
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include/c++/v1"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[RESOURCE]]/include"
+// CHECK-AARCH64-NO-HOST-INC-SAME: "-internal-isystem" "[[INSTALLEDDIR]]/../lib/clang-runtimes/aarch64-none-elf/include"
+
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ 

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls created this revision.
kristof.beyls added reviewers: psmith, miyuki, srhines.
Herald added subscribers: s.egerton, simoncook.
kristof.beyls requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D34

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/test/Driver/baremetal.cpp
  clang/test/Driver/gcc_forward.c


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,21 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths.
+// I.e. verify it only includes paths relative to the clang binary, when no
+// sysroot is specified.
+// We are constrained a little bit by FileCheck's features here, so just
+// check that the first -internal-isystem points to an include path in the
+// clang install, not somewhere else. Ideally, we'd verify this for all
+// -internal-isystem paths, but we don't know how many to expect, so that is
+// hard to test for exactly here.
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-internal-isystem" "[[INSTALLEDDIR]]
+
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -125,6 +125,21 @@
   return true;
 }
 
+/// Is the triple aarch64-none-elf?
+static bool isAArch64BareMetal(const llvm::Triple ) {
+  if (Triple.getArch() != llvm::Triple::aarch64)
+return false;
+
+  if (Triple.getVendor() != llvm::Triple::UnknownVendor)
+return false;
+
+  if (Triple.getOS() != llvm::Triple::UnknownOS)
+return false;
+
+  return Triple.getEnvironmentName() == "elf";
+}
+
+
 static bool isRISCVBareMetal(const llvm::Triple ) {
   if (Triple.getArch() != llvm::Triple::riscv32 &&
   Triple.getArch() != llvm::Triple::riscv64)
@@ -151,7 +166,8 @@
 }
 
 bool BareMetal::handlesTarget(const llvm::Triple ) {
-  return isARMBareMetal(Triple) || isRISCVBareMetal(Triple);
+  return isARMBareMetal(Triple) || isAArch64BareMetal(Triple) ||
+isRISCVBareMetal(Triple);
 }
 
 Tool *BareMetal::buildLinker() const {


Index: clang/test/Driver/gcc_forward.c
===
--- clang/test/Driver/gcc_forward.c
+++ clang/test/Driver/gcc_forward.c
@@ -1,4 +1,4 @@
-// RUN: %clang -### %s -target aarch64-none-elf \
+// RUN: %clang -### %s -target x86-none-elf \
 // RUN:   --coverage -e _start -fuse-ld=lld --ld-path=ld -nostartfiles \
 // RUN:   -nostdlib -r -rdynamic -specs=nosys.specs -static -static-pie \
 // RUN:   2>&1 | FileCheck --check-prefix=FORWARD %s
Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -102,6 +102,21 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
 // CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:  -target aarch64-none-elf \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64-NO-HOST-INC %s
+// Verify that the bare metal driver does not include any host system paths.
+// I.e. verify it only includes paths relative to the clang binary, when no
+// sysroot is specified.
+// We are constrained a little bit by FileCheck's features here, so just
+// check that the first -internal-isystem points to an include path in the
+// clang install, not somewhere else. Ideally, we'd verify this for all
+// -internal-isystem paths, but we don't know how many to expect, so that is
+// hard to test for exactly here.
+// CHECK-AARCH64-NO-HOST-INC: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-AARCH64-NO-HOST-INC: "-internal-isystem" "[[INSTALLEDDIR]]
+
+
 // RUN: %clang -no-canonical-prefixes %s -### 

[PATCH] D103080: [CMake] Ignore arm_*.h for non-ARM build

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D103080#2790139 , @sunlin wrote:

> Hi @kristof.beyls The  original `lib/clang/12.0.1/include` is about total 
> ~10M, and the `arm*.h` take about ~5M. Ignore these unused header files will 
> save the developers who work on the low storage device.

Thanks for sharing that rationale!
I have two immediate thoughts on this patch:

- arm_neon.h and other header files need to be kept also for the AArch64 
backend. As is, I think the patch will remove them resulting in a broken 
toolchain if the AArch64 backend is requested but the Arm backend is not.
- We should implement either (a) remove all target-specific header files if a 
target is not built or (b) keep all of them. Selecting ad hoc (e.g. applying 
this design principle only for the Arm backend) which ones to apply this policy 
to doesn't seem like a good design to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103080

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

[PATCH] D103080: [CMake] Ignore arm_*.h for non-ARM build

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

I'm wondering what the rationale for this change is.
If there is a good rationale for this; wouldn't it need to be applied to all 
target-specific header files, not only the Arm-specific header files?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103080

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

[PATCH] D100919: [AArch64] Support customizing stack protector guard

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/test/Driver/stack-protector-guard.c:26
 
-// RUN: not %clang -target aarch64-linux-gnu -mstack-protector-guard-offset=10 
%s 2>&1 | \
+// RUN: not %clang -target arm-linux-gnuebi -mstack-protector-guard-offset=10 
%s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-ARCH3 %s

I guess you meant to type "-target arm-linux-gnueabi"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100919

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

[PATCH] D93347: [Test] Fix undef var in attr-speculative-load-hardening.c

[Kristof Beyls via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93347

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

[PATCH] D98277: [release][docs] List all cores Arm has added support for in LLVM 12.

[Kristof Beyls via Phabricator via cfe-commits]

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

Still LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98277

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

[PATCH] D98277: [release][docs] List all cores Arm has added support for in LLVM 12.

[Kristof Beyls via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98277

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

[PATCH] D93221: [ARM] Add clang command line support for -mharden-sls=

[Kristof Beyls via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9c895aea118a: [ARM] Add clang command line support for 
-mharden-sls= (authored by kristof.beyls).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93221

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  clang/test/Driver/aarch64-sls-hardening-options.c
  clang/test/Driver/sls-hardening-options.c

Index: clang/test/Driver/sls-hardening-options.c
===
--- /dev/null
+++ clang/test/Driver/sls-hardening-options.c
@@ -0,0 +1,97 @@
+// Check the -mharden-sls= option, which has a required argument to select
+// scope.
+// RUN: %clang -target aarch64--none-eabi -c %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr,blr,retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,r 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr,blr,r 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=none,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all,-blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=all,-blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RETBR-OFF-NOT: "harden-sls-retbr"
+// RETBR-ON:  "+harden-sls-retbr"
+
+// BLR-OFF-NOT: "harden-sls-blr"
+// BLR-ON:  "+harden-sls-blr"
+
+// BAD-SLS-SPEC: invalid sls hardening option '{{[^']+}}' in '-mharden-sls=
+
+// RUN: %clang -target armv6a--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=SLS-NOT-SUPPORTED
+
+// RUN: %clang -target armv6a--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=SLS-NOT-SUPPORTED
+

[PATCH] D93221: [ARM] Add clang command line support for -mharden-sls=

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D93221#2459903 , @ostannard wrote:

> Why is this restricted to v7-A or later? The DSB and ISB instructions have 
> existed since v6T2 and v6M.

This mitigation is never needed for M-class cores nor for v6T2.
By restricting it to v7-A it's simpler to explain in the diagnostic for which 
targets this is supported.
I thought that overall this was a better trade-off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93221

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

[PATCH] D93221: [ARM] Add clang command line support for -mharden-sls=

[Kristof Beyls via Phabricator via cfe-commits]

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

The command line syntax is identical to the -mharden-sls= command line
syntax for AArch64 targets.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93221

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  clang/test/Driver/aarch64-sls-hardening-options.c
  clang/test/Driver/sls-hardening-options.c

Index: clang/test/Driver/sls-hardening-options.c
===
--- /dev/null
+++ clang/test/Driver/sls-hardening-options.c
@@ -0,0 +1,97 @@
+// Check the -mharden-sls= option, which has a required argument to select
+// scope.
+// RUN: %clang -target aarch64--none-eabi -c %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr,blr,retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,r 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=retbr,blr,r 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=none,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all,-blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+// RUN: %clang -target armv7a--none-eabi -c %s -### -mharden-sls=all,-blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RETBR-OFF-NOT: "harden-sls-retbr"
+// RETBR-ON:  "+harden-sls-retbr"
+
+// BLR-OFF-NOT: "harden-sls-blr"
+// BLR-ON:  "+harden-sls-blr"
+
+// BAD-SLS-SPEC: invalid sls hardening option '{{[^']+}}' in '-mharden-sls=
+
+// RUN: %clang -target armv6a--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=SLS-NOT-SUPPORTED
+
+// RUN: %clang -target armv6a--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s 

[PATCH] D92245: -fstack-clash-protection: Return an actual error when used on unsupported OS

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:276-277
   "-fembed-bitcode is not supported on versions of iOS prior to 6.0">;
+def err_drv_stack_clash_protection_unsupported_on_toolchain : Error<
+  "-fstack-clash-protection is not supported on Windows or Mac OS X">;
 

There are more OSes than Linux, Windows or OSX.
Maybe it's somewhat better to say "-fstack-clash-protection is not supported on 
%0", with the targeted OS being fed in.
If that is not easily possible, maybe just say "-fstack-clash-protection is not 
supported on the targeted OS"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92245

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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Kristof Beyls via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.
Herald added a project: LLVM.

This looks fine to me; I just have a number of nit picks.
The only part where I don't understand the code logic is around the comment 
starting with "If this is the final byte of a multi-byte sequence".




Comment at: llvm/include/llvm/Support/FormattedStream.h:23-26
 /// formatted_raw_ostream - A raw_ostream that wraps another one and keeps 
track
 /// of line and column position, allowing padding out to specific column
 /// boundaries and querying the number of lines written to the stream.
 ///

I think it would be useful to add a description to this comment that:
(a) This class assumes that a UTF-8 encoding is used on the Stream; and
(b) "This doesn't attempt to handle everything unicode can do (combining 
characters, right-to-left markers, ...), but hopefully covers most things 
likely to be common in messages and source code we might want to print."



Comment at: llvm/lib/Support/FormattedStream.cpp:30
 
-  // Keep track of the current column and line by scanning the string for
-  // special characters
-  for (const char *End = Ptr + Size; Ptr != End; ++Ptr) {
-++Column;
-switch (*Ptr) {
+  auto ProcessCodePoint = [, ](StringRef CP) {
+int Width = sys::locale::columnWidth(CP);


Given that ProcessCodePoint assumes that the Unicode code point represented in 
the UTF-8 encoding, maybe it be slightly better to name the lambda as 
ProcessUTF8CodePoint rather than ProcessCodePoint?




Comment at: llvm/lib/Support/FormattedStream.cpp:31
+  auto ProcessCodePoint = [, ](StringRef CP) {
+int Width = sys::locale::columnWidth(CP);
+// columnWidth returns -1 for non-printing characters.

I'm wondering if using sys::unicode::columnWidthUTF8 instead of 
sys::locale::columnWidth would be more future-proof and more clearly describe 
the intent that this function only processes UTF-8 and not strings encoded in 
other encodings?



Comment at: llvm/lib/Support/FormattedStream.cpp:33
+// columnWidth returns -1 for non-printing characters.
+if (Width != -1)
+  Column += Width;

The documentation for sys::unicode::columnWidthUTF8 documents it returns 
ErrorNonPrintableCharacter (-1) if the text contains non-printable characters.
Maybe it's more self-documenting to compare against ErrorNonPrintableCharacter 
rather than -1 in the above if condition?



Comment at: llvm/lib/Support/FormattedStream.cpp:36-37
+
+// If this is the final byte of a multi-byte sequence, it can't be any of
+// the special whitespace characters below.
+if (CP.size() > 1)

Reading through the code linearly from the top to the bottom,  I'm a bit 
surprised by this comment.
I would expect CP to contain exactly the bytes that when interpreted as a UTF-8 
encoded Unicode character, form exactly one Unicode character.
Therefore, I'm not sure how to interpret "If this is the final byte of a 
multi-byte sequence.".
I'm expecting "this" to refer to "CP" in this context. But that cannot be 
"just" the final byte of a multi-byte sequence, unless my assumption that CP 
contains exactly the bytes forming a single UTF-8 encoded Unicode character is 
wrong.
Could CP contain a partial UTF-8 encoded character? If so, maybe it'd be better 
to change the name ProcessCodePoint to something that suggests that could be 
possible?



Comment at: llvm/unittests/Support/formatted_raw_ostream_test.cpp:134
+
+  // This character encodes as three bytes, so will cause the buffer to be
+  // flushed after the first byte (4 byte buffer, 3 bytes already written). We

I guess "This" refers to \u2468? If so, it'd be easier to read this comment if 
it was written like: "// \u2468 encodes as three bytes, ..."



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291



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

[PATCH] D81404: [AArch64] Add clang command line support for -mharden-sls=

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls marked 2 inline comments as done.
kristof.beyls added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:229
+  Scope.split(Opts, ",");
+  for (int I = 0, E = Opts.size(); I != E; ++I) {
+StringRef Opt = Opts[I].trim();

ostannard wrote:
> Could this be a range-based for loop?
Yep, thanks for the suggestion. I've changed that before committing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81404



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

[PATCH] D81404: [AArch64] Add clang command line support for -mharden-sls=

[Kristof Beyls via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc113b59ef525: [AArch64] Add clang command line support for 
-mharden-sls= (authored by kristof.beyls).

Changed prior to commit:
  https://reviews.llvm.org/D81404?vs=269247=271925#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81404

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/test/Driver/aarch64-sls-hardening-options.c

Index: clang/test/Driver/aarch64-sls-hardening-options.c
===
--- /dev/null
+++ clang/test/Driver/aarch64-sls-hardening-options.c
@@ -0,0 +1,45 @@
+// Check the -mharden-sls= option, which has a required argument to select
+// scope.
+// RUN: %clang -target aarch64--none-eabi -c %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,r 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all,-blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RETBR-OFF-NOT: "harden-sls-retbr"
+// RETBR-ON:  "+harden-sls-retbr"
+
+// BLR-OFF-NOT: "harden-sls-blr"
+// BLR-ON:  "+harden-sls-blr"
+
+// BAD-SLS-SPEC: invalid sls hardening option '{{[^']+}}' in '-mharden-sls=
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -218,6 +218,39 @@
   D.Diag(diag::err_drv_invalid_mtp) << A->getAsString(Args);
   }
 
+  // Enable/disable straight line speculation hardening.
+  if (Arg *A = Args.getLastArg(options::OPT_mharden_sls_EQ)) {
+StringRef Scope = A->getValue();
+bool EnableRetBr = false;
+bool EnableBlr = false;
+if (Scope != "none" && Scope != "all") {
+  SmallVector Opts;
+  Scope.split(Opts, ",");
+  for (auto Opt : Opts) {
+Opt = Opt.trim();
+if (Opt == "retbr") {
+  EnableRetBr = true;
+  continue;
+}
+if (Opt == "blr") {
+  EnableBlr = true;
+  continue;
+}
+D.Diag(diag::err_invalid_sls_hardening)
+<< Scope << A->getAsString(Args);
+break;
+  }
+} else if (Scope == "all") {
+  EnableRetBr = true;
+  EnableBlr = true;
+}
+
+if (EnableRetBr)
+  Features.push_back("+harden-sls-retbr");
+if (EnableBlr)
+  Features.push_back("+harden-sls-blr");
+  }
+
   // En/disable crc
   if (Arg *A = Args.getLastArg(options::OPT_mcrc, options::OPT_mnocrc)) {
 if (A->getOption().matches(options::OPT_mcrc))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2313,6 +2313,9 @@
 def mbranch_protection_EQ : Joined<["-"], "mbranch-protection=">,
   HelpText<"Enforce targets of indirect branches and function returns">;
 
+def mharden_sls_EQ : Joined<["-"], "mharden-sls=">,
+  HelpText<"Select straight-line speculation hardening scope">;
+
 def msimd128 : Flag<["-"], "msimd128">, Group;
 def munimplemented_simd128 : Flag<["-"], "munimplemented-simd128">, Group;
 def mno_unimplemented_simd128 : Flag<["-"], 

[PATCH] D81404: [AArch64] Add clang command line support for -mharden-sls=

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls created this revision.
kristof.beyls added a reviewer: ostannard.
Herald added subscribers: cfe-commits, danielkiss.
Herald added a project: clang.

The accepted options to -mharden-sls= are:

- all: enable all mitigations against Straight Line Speculation that are 
implemented.
- none: disable all mitigations against Straight Line Speculation.
- retbr: enable the mitigation against Straight Line Speculation for RET and BR 
instructions.
- blr: enable the mitigation against Straight Line Speculation for BLR 
instructions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81404

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/test/Driver/aarch64-sls-hardening-options.c

Index: clang/test/Driver/aarch64-sls-hardening-options.c
===
--- /dev/null
+++ clang/test/Driver/aarch64-sls-hardening-options.c
@@ -0,0 +1,45 @@
+// Check the -mharden-sls= option, which has a required argument to select
+// scope.
+// RUN: %clang -target aarch64--none-eabi -c %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-OFF --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=blr -mharden-sls=retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,retbr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RETBR-ON --check-prefix=BLR-ON
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=retbr,blr,r 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=none,blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mharden-sls=all,-blr 2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-SLS-SPEC
+
+// RETBR-OFF-NOT: "harden-sls-retbr"
+// RETBR-ON:  "+harden-sls-retbr"
+
+// BLR-OFF-NOT: "harden-sls-blr"
+// BLR-ON:  "+harden-sls-blr"
+
+// BAD-SLS-SPEC: invalid sls hardening option '{{[^']+}}' in '-mharden-sls=
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -218,6 +218,39 @@
   D.Diag(diag::err_drv_invalid_mtp) << A->getAsString(Args);
   }
 
+  // Enable/disable straight line speculation hardening.
+  if (Arg *A = Args.getLastArg(options::OPT_mharden_sls_EQ)) {
+StringRef Scope = A->getValue();
+bool EnableRetBr = false;
+bool EnableBlr = false;
+if (Scope != "none" && Scope != "all") {
+  SmallVector Opts;
+  Scope.split(Opts, ",");
+  for (int I = 0, E = Opts.size(); I != E; ++I) {
+StringRef Opt = Opts[I].trim();
+if (Opt == "retbr") {
+  EnableRetBr = true;
+  continue;
+}
+if (Opt == "blr") {
+  EnableBlr = true;
+  continue;
+}
+D.Diag(diag::err_invalid_sls_hardening)
+<< Scope << A->getAsString(Args);
+break;
+  }
+} else if (Scope == "all") {
+  EnableRetBr = true;
+  EnableBlr = true;
+}
+
+if (EnableRetBr)
+  Features.push_back("+harden-sls-retbr");
+if (EnableBlr)
+  Features.push_back("+harden-sls-blr");
+  }
+
   // En/disable crc
   if (Arg *A = Args.getLastArg(options::OPT_mcrc, options::OPT_mnocrc)) {
 if (A->getOption().matches(options::OPT_mcrc))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2302,6 +2302,9 @@
 def mbranch_protection_EQ : Joined<["-"], "mbranch-protection=">,
   HelpText<"Enforce targets of indirect branches and function returns">;
 
+def mharden_sls_EQ : Joined<["-"], "mharden-sls=">,
+  

[PATCH] D78481: [ARM] Release notes for the Custom Datapath Extension (CDE)

[Kristof Beyls via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78481



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

[PATCH] D76513: [ReleaseNotes,ARM] MVE intrinsics are all implemented!

[Kristof Beyls via Phabricator via cfe-commits]

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

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76513



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D74918#1923151 , @zoecarver wrote:

> There are a lot of different ways we could implement the feature. We may want 
> to only enable it when `-march=native`, or maybe only in the unstable ABI, 
> and maybe we want to support aligned pairs on some architectures. I think 
> that's an important discussion to have but I'm not sure _this_ patch is the 
> best place to have that discussion.
>
> Even if we don't use this patch in the implementation I think it would still 
> be a good utility to have. Here's what I suggest: I commit this, create 
> another patch to add a builtin that exposes this API, and then open a libc++ 
> patch with a _possible_ implementation. In that patch, we can discuss how we 
> should actually implement the feature and after we have a consensus I can do 
> the work to implement it. Any objections to that plan?


Discussing the implementation strategy for 
std::hardware_{constructive,destructive}_interference_size on a libc++ thread 
rather than here makes sense.
I'm afraid I don't have a good view on all the ways the API and associated 
intrinsic proposed here will or could be used in practice.
My only thought on it is that we cannot guarantee that different versions of 
LLVM will keep on reporting the same number, even for identical targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D74918#1898191 , @__simt__ wrote:

> In D74918#1897636 , @kristof.beyls 
> wrote:
>
> > If these values are part of the C++ target platform ABI, it seems to me the 
> > values for std::hardware_{constructive,destructive}_interference_size 
> > should be set by whoever has the authority to decide C++ platform ABI for 
> > specific platforms.
> >  Assuming my thought in the previous sentence is correct; discussions on 
> > which values to chose for 
> > std::hardware_{constructive,destructive}_interference_size should happen in 
> > whichever forums decide C++ platform ABI for the various platforms? (Maybe 
> > for some platforms that forum might be clang-related fora like 
> > reviews.llvm.org, but probably not for all platforms).
> >  With my (probably limited) understanding of the requirements, it seems 
> > like deriving std::hardware_{constructive,destructive}_interference_size 
> > from actual cache line size on a specific micro-architecture doesn't seem 
> > to be the right approach?
>
>
> They will be in the library ABI, meaning the libc++ ABI.
>
> It's valid for libstdc++ and libc++ to have different values there. If we 
> wish, we could try for an alignment (no pun intended) on these values, but 
> even then that's just between these two libraries.
>
> Which is good and encouraging, because I don't know what forum we would have 
> to go to.


I see.
So IIUC, this is library C++ ABI, to be defined by the C++ library. Since no 2 
C++ libraries can co-exist in a single application, there is no need for 
different C++ libraries to agree?

I think there is still an issue then with getting the values of 
std::hardware_{constructive,destructive}_interference_size in the library 
implementation derived from compiler builtins.
There are quite a few systems where clang supports targeting a different C++ 
library than libc++, e.g. libstdc++ on linux or  (IIUC) the MSVC C++ library on 
Windows.
If these C++ libraries implement 
std::hardware_{constructive,destructive}_interference_size based on a value 
returned by a compiler builtin, and the different compilers that are used with 
these libraries return different values for such a builtin, then the library 
C++ ABI here will be dependent on which compiler used?
Doesn't this indicate that either:

- std::hardware_{constructive,destructive}_interference_size should not be 
implemented using a compiler builtin, or
- all compilers must return the same value for the builtin; for all targets 
they support.

Overall, that makes me doubt that using a compiler builtin to implement 
std::hardware_{constructive,destructive}_interference_size is the right 
direction.
If the functionality in this patch does not need to support implementing 
std::hardware_{constructive,destructive}_interference_size, then the design 
constraints for this patch change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D74918#1896930 , @__simt__ wrote:

> (//I assume I'm not seeing a code review being used to veto a C++ Standard 
> feature, but actually the other points are the reason for the red flag.//)
>
> I can see a desire for hyper-precise definitions to achieve the best possible 
> performance, but we really need a healthy does of conservatism here.
>
> The C++ values can't change as a result of selecting between 
> microarchitecture variations that are supposed to link, it takes an ABI break 
> to change these.


If these values are part of the C++ target platform ABI, it seems to me the 
values for std::hardware_{constructive,destructive}_interference_size should be 
set by whoever has the authority to decide C++ platform ABI for specific 
platforms.
Assuming my thought in the previous sentence is correct; discussions on which 
values to chose for std::hardware_{constructive,destructive}_interference_size 
should happen in whichever forums decide C++ platform ABI for the various 
platforms? (Maybe for some platforms that forum might be clang-related fora 
like reviews.llvm.org, but probably not for all platforms).
With my (probably limited) understanding of the requirements, it seems like 
deriving std::hardware_{constructive,destructive}_interference_size from actual 
cache line size on a specific micro-architecture doesn't seem to be the right 
approach?

> We specified two numbers here so we could conservatively set them (e.g. 
> constructive: smallest; destructive: largest) if we want, but then they are 
> fixed.
> 
> I think there's just two plausible answers for x86_64:
> 
> 1. constructive=64, destructive=64 (the only plausible answer for X86 classic 
> edition)
> 2. constructive=64, destructive=128 (reserve some room for line-pair designs)
> 
>   Recapping: precision is nice but we need to fix this in the ABI so ultimate 
> precision isn't required nor desirable; we should choose one of these pairs 
> (or debate if another pair is viable that I'm not thinking of); then we 
> should ship C++17.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D72449: [PATCH] [llvm-ranlib] Take in consideration UTC offset for D-flag.test

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: llvm/test/tools/llvm-ranlib/D-flag.test:13
 # RUN: cp %t-no-index.a %t.a && llvm-ranlib -D %t.a
-# RUN: env TZ=UTC llvm-ar tv %t.a | FileCheck %s 
--check-prefix=DETERMINISTIC-VALUES
+# RUN: (env TZ=UTC date -d '@0' +%H:%M; env TZ=UTC llvm-ar tv %t.a) | 
FileCheck %s --check-prefix=DETERMINISTIC-VALUES
 

I wonder if 'date' is available on all platforms that LLVM builds on. For 
example, is it available on Windows (with GnuWin32 installed, as per the 
requirements listed at https://llvm.org/docs/GettingStartedVS.html)? 
http://gnuwin32.sourceforge.net/packages.html doesn't seem to list "date" 
explicitly?


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

https://reviews.llvm.org/D72449



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

[PATCH] D70779: AArch64: add support for newer Apple CPUs

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:143
 MtuneLowerCase = llvm::sys::getHostCPUName();
-  if (MtuneLowerCase == "cyclone") {
+  if (MtuneLowerCase == "cyclone" || MtuneLowerCase.find("apple") == 0) {
 Features.push_back("+zcm");

t.p.northover wrote:
> fhahn wrote:
> > It might be slightly more obvious to use MtuneLowerCAse.StartsWith("apple")
> I'd have preferred to, but unfortnately it's a `std::string` so doesn't have 
> that function.
At least not until C++20... 
https://en.cppreference.com/w/cpp/string/basic_string/starts_with


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

https://reviews.llvm.org/D70779



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

[PATCH] D70779: AArch64: add support for newer Apple CPUs

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

I agree the bare names could cause a lot of confusion, and that the naming 
scheme proposed in this patch resolves that potential confusion.


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

https://reviews.llvm.org/D70779



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

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: docs/ReleaseNotes.rst:84
+  In a future release of Clang, we intend to change the default to
+  ``-fno-lax-vector-conversions``.
+

rsmith wrote:
> efriedma wrote:
> > kristof.beyls wrote:
> > > efriedma wrote:
> > > > rsmith wrote:
> > > > > efriedma wrote:
> > > > > > And if you want to allow your code to build with both clang-9 and 
> > > > > > clang-10, you have to do version detection in your build scripts?
> > > > > I guess you'd detect whether the compiler supports 
> > > > > `-flax-vector-conversions=all`, and pass that if so, and otherwise 
> > > > > pass `-flax-vector-conversions`. Well, either that or you fix your 
> > > > > code to not rely on lax vector conversions between int and float 
> > > > > vectors. If your code builds with GCC, you did that already (they 
> > > > > never supported lax conversions between float and int vectors, as far 
> > > > > as I can tell).
> > > > > 
> > > > > Do you have a preferred alternative?
> > > > All the alternatives are terrible in their own way:
> > > > 
> > > > 1. This status quo, which breaks compatibility with previous versions 
> > > > of clang
> > > > 2. We could make -flax-vector-conversions mean the same thing as 
> > > > earlier versions of clang.  So the flag wouldn't have the same meaning 
> > > > as gcc's flag.
> > > > 3. Some mix of the previous options: we could wait until there are one 
> > > > or two released versions that support -flax-vector-conversions=all , 
> > > > then change the meaning of -flax-vector-conversions.  But I have no 
> > > > idea how we would decide on a timeline.
> > > > 
> > > > I ran into this issue recently updating our compiler.  That said, the 
> > > > code in question was only using the implicit conversion in a couple 
> > > > places by accident, so it was easy enough to just fix the source.
> > > Maybe adding an entry in the release notes about this change could help 
> > > with making option 1 slightly more palatable?
> > > My guess is that option 1 is the right one for the long term 
> > > (compatibility between gcc and clang so code is more portable between 
> > > both compilers).
> > It probably makes sense to call out the behavior change to 
> > -flax-vector-conversions in the release notes, yes.
> @kristof.beyls Are you looking for more changes to the release notes in 
> addition to what's already in this change? If so, what would you like to see?
@rsmith I'm afraid I reacted to the review comments above and completely missed 
you had already added an entry to the release notes! My apologies.
I think it might still take some time for someone getting a build error who 
then goes through the release notes to easily spot that it's the change to lax 
vector conversions that's making their build fail.
However, I can't think of a much better way to describe this in the release 
notes so that a developer would spot this more easily, unless we'd put a source 
code example of something that now fails by default that didn't before. Putting 
source code examples in the release notes for all changes might make the 
release notes too long/complex?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67678



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

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: docs/ReleaseNotes.rst:84
+  In a future release of Clang, we intend to change the default to
+  ``-fno-lax-vector-conversions``.
+

efriedma wrote:
> rsmith wrote:
> > efriedma wrote:
> > > And if you want to allow your code to build with both clang-9 and 
> > > clang-10, you have to do version detection in your build scripts?
> > I guess you'd detect whether the compiler supports 
> > `-flax-vector-conversions=all`, and pass that if so, and otherwise pass 
> > `-flax-vector-conversions`. Well, either that or you fix your code to not 
> > rely on lax vector conversions between int and float vectors. If your code 
> > builds with GCC, you did that already (they never supported lax conversions 
> > between float and int vectors, as far as I can tell).
> > 
> > Do you have a preferred alternative?
> All the alternatives are terrible in their own way:
> 
> 1. This status quo, which breaks compatibility with previous versions of clang
> 2. We could make -flax-vector-conversions mean the same thing as earlier 
> versions of clang.  So the flag wouldn't have the same meaning as gcc's flag.
> 3. Some mix of the previous options: we could wait until there are one or two 
> released versions that support -flax-vector-conversions=all , then change the 
> meaning of -flax-vector-conversions.  But I have no idea how we would decide 
> on a timeline.
> 
> I ran into this issue recently updating our compiler.  That said, the code in 
> question was only using the implicit conversion in a couple places by 
> accident, so it was easy enough to just fix the source.
Maybe adding an entry in the release notes about this change could help with 
making option 1 slightly more palatable?
My guess is that option 1 is the right one for the long term (compatibility 
between gcc and clang so code is more portable between both compilers).


Repository:
  rC Clang

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

https://reviews.llvm.org/D67678



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

[PATCH] D67160: [clang, ARM] Default to -fno-lax-vector-conversions in ARM v8.1-M.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

Would it still make sense to have this patch after D68683 
 lands? At first sight, it seems this patch 
might no longer make sense then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67160



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:1-6
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM-FAST-ISEL
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM-GLOBAL-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB-FAST-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB-GLOBAL-ISEL

It seems the -fast-isel/-global-isel command line options are missing in the 
RUN lines aiming to test fast and global isel do the right thing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

I've just added a few fly-by nits; I'm afraid I didn't do an in-depth review.




Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1156
   MachineInstr  = *MBBI;
+  LLVM_DEBUG(dbgs() << "ARMExpandPseudo::ExpandMI: " << MI << "\n");
   unsigned Opcode = MI.getOpcode();

I wonder whether this is a good debug printing line to commit?
IIUC, this will print every MI instruction that gets looked at by 
ArmExpandPseudo.
I would imagine that that could produce too much noise. It'd be more 
interesting if only the MIs that actually got transformed would be printed.
But maybe best to just not add this debug printing line in this patch?



Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1931-1932
+// Replace with the pseudo instruction with a call instruction
+MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(),
+  TII->get(ARM::tBL));
+  } else {

Did you clang-format the patch?



Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:1-2
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB
+

Given that the push-lr transform only gets implemented for DAGISel (IIUC), 
maybe it'd be useful to also have test run lines that check the correct thing 
happens when using fastisel and globalisel (presumably by falling back to 
DAGISel)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D59827: [slh] x86 impl of ARM instrinsic + builtin for SLH

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

This intrinsic got added to gcc a while ago and should become available in the 
upcoming gcc 9 release.
In gcc however, the prototype of the intrinsic is slightly different (see 
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html):
type __builtin_speculation_safe_value (type val, type failval)
It provides a second optional argument "failval". From the gcc documentation: 
"The function may use target-dependent speculation tracking state to cause 
failval to be returned when it is known that speculative execution has 
incorrectly predicted a conditional branch operation."
So, when implementing the intrinsic using a speculation barrier such as lfence, 
that failval argument doesn't have any effect. However, when lowering the 
intrinsic using speculation tracking similar to how that's used in SLH, this 
failval parameter is used to return a non-zero value on miss-speculation, in 
case the developer prefers that over the default zero value.

I think we should make the intrinsic compatible with the one introduced in gcc.




Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:1
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s 
--check-prefix=X64
+

I guess the -mtriple command line option may not be needed since the IR file 
contain "target triple" and "target datalayout" information?



Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:3-4
+
+; ModuleID = 'hello.cpp'
+source_filename = "hello.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

I guess this is not strictly necessary for this test, so should be removed?



Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:8-62
+; Function Attrs: noinline nounwind optnone uwtable
+define dso_local i32 @_Z5foo32i(i32 %a) #0 {
+entry:
+  %a.addr = alloca i32, align 4
+  %b = alloca i32, align 4
+  %b_safe = alloca i32, align 4
+  %c = alloca i32, align 4

Thanks for those updates, Zola. It makes it easier to compare this patch with 
the code I wrote earlier.
Doing that comparison, I see that I had a few changes too in target-independent 
SelectionDAG under lib/Codegen/SelectionDAG.
IIRC, you might find that you'll need that code if you also add tests here to 
test the correct thing happens when applying the intrinsic on other types than 
i32 or i64.
You probably also would want a test on a pointer data type here, I guess.



Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:64-71
+attributes #0 = { noinline nounwind optnone uwtable 
"correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" 
"less-precise-fpmad"="false" "min-legal-vector-width"="0" 
"no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" 
"no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" 
"no-signed-zeros-fp-math"="false" "no-trapping-math"="false" 
"stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
"target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" 
"use-soft-float"="false" }
+attributes #1 = { nounwind }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}

I guess this is not strictly necessary for this test, so should be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59827



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

[PATCH] D59827: [slh] x86 impl of ARM instrinsic for SLH

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

Thanks for picking this up, Zola!

I quickly looked through the patch - comparing it with what I had done under 
D49070  and D49073 
.
Apart from the point remarks inline, I had the following immediate thoughts:

1. Could you clang-format the patch?
2. Could you rebase the patch to top-of-trunk (it seems it is a bit behind ToT)?
3. For discussions, seeing the whole patch as it is might be helpful. OTOH, I 
think it also makes reviewing easier if the target-dependent and the 
target-independent parts would be split. I think that could also help others in 
implementing the intrinsics for their targets: they'd have guidance on what 
might be needed from that target-dependent implementation patches for X86 and 
AArch64.
4. Lowering to LFENCE seems a correct lowering to me, but someone more 
knowledgeable about x86 should confirm.
5. I think the LLVM-IR intrinsic should be target-independent, and not 
x86-specific. That would result in less duplication of code when implementing 
support for multiple architectures. I seem to remember that's how I implemented 
this in D49070 . I didn't look so far at the 
SelectionDAG parts of this patch, as I think the differences between my 
implementation in D49070  and this patch may 
go away after making the intrinsic target-independent.

If we'd take the discussion about adding support for intrinsic `T 
__builtin_speculation_safe_value(T v)` further here, I'd be happy to abandon 
the patch series at D49073 .
However, in that case, I think the explanation of the intrinsic there should be 
copied over here to provide a bit more context.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13
 
+#include 
"/usr/local/google/home/zbrid/repos/llvm-project/clang/lib/CodeGen/CodeGenTypeCache.h"
 #include "CGCXXABI.h"

This line doesn't seem to be needed?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3947
+llvm::Type *T = ConvertType(E->getType());
+assert((isa(T) || isa(T)) && 
"unsupported type");
+

line too long - run clang-format?



Comment at: clang/lib/Sema/SemaChecking.cpp:1496
+  case Builtin::BI__builtin_speculation_safe_value:
+   return SemaBuiltinSpeculationSafeValueOverloaded(TheCallResult);
   }

needs one more space of indentation?



Comment at: clang/lib/Sema/SemaChecking.cpp:5325
+  // Too many args
+  if (TheCall->getNumArgs() < 1)
+return Diag(TheCall->getEndLoc(), 
diag::err_typecheck_call_too_many_args_at_most)

Should this be "TheCall->getNumArgs() > 1" (larger than rather then less than)?



Comment at: clang/test/CodeGen/builtin-speculation-safe-value.c:1-2
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck 
-check-prefix=CHECK-SUPPORTED %s
+

When I wrote this test in D49073 this line read "REQUIRES: 
aarch64-registered-target". Looking at this now, I wonder why the requires 
might be needed, beyond the RUN line containing "-triple x86_64-linux-gnu". 
It'd be nice if this test didn't need a REQUIRES line But maybe there is a 
good reason it does need a requires line after all?



Comment at: clang/test/Preprocessor/init.c:9215
 // WEBASSEMBLY-NEXT:#define __GXX_ABI_VERSION 1002
+// WEBASSEMBLY-NEXT:#define __HAVE_SPECULATION_SAFE_VALUE 1
 // WEBASSEMBLY32-NEXT:#define __ILP32__ 1

It seems this is the only intended change in this file; all the other changes 
in this file were unintended for this patch?




Comment at: llvm/include/llvm/IR/Intrinsics.td:1171
  [IntrNoMem, Returned<0>]>;
+
 
//===--===//

accidental new line diff?



Comment at: llvm/include/llvm/IR/IntrinsicsX86.td:4819-4822
+
+//===- Intrinsics to mitigate against miss-speculation exploits 
---===//
+
+def int_speculationsafevalue : Intrinsic<[llvm_any_ty], [LLVMMatchType<0>], 
[]>;

I think this needs to be a target independent LLVM IR intrinsic, not x86 
specific. See D49070. This will also need documentation in LangRef.rst then, 
also see D49070 for a possible documentation I proposed for this intrinsic 
there.



Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:614-628
+  if (Opcode == X86::SpeculationSafeValue32) {
+BuildMI(MBB, NMBBI, DebugLoc(), TII->get(X86::LFENCE));
+++NumInstsInserted;
+++NumLFENCEsInserted;
+MRI->replaceRegWith(MI.getOperand(0).getReg(), 
MI.getOperand(1).getReg());
+MI.eraseFromParent();
+Modified = true;

The lowering of the intrinsic on a 

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: test/Driver/arm-mfpu.c:410
+
+// RUN: %clang -target armv7-linux-androideabi23 %s -mfpu=vfp3-d16 -### -c 
2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM-ANDROID-M-FP-D16 %s

danalbert wrote:
> >>! In D53121#1261602, @kristof.beyls wrote:
> > Seems fine to me too. I'd maybe just add an additional test case to verify 
> > that things still work as expected when users explicitly specify that they 
> > want to target a different FPU (e.g. "-mfpu=none").
> 
> Is this test (and it's counterpart in `CHECK-ARM-ANDROID-L-FP-NEON`) not 
> sufficient? It shows that `-mfpu` is honored regardless of the default. Is 
> there something special about `-mfpu=none` that this doesn't exercise?
you're right - this test does what I was asking for. Apologies, I should've 
looked more closely - I hadn't picked up this mfpu isn't the default one...


Repository:
  rC Clang

https://reviews.llvm.org/D53121



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

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a subscriber: peter.smith.
kristof.beyls added a comment.

In https://reviews.llvm.org/D53121#1261408, @srhines wrote:

> This LGTM, but we should wait to hear from Kristof before submitting.


Seems fine to me too. I'd maybe just add an additional test case to verify that 
things still work as expected when users explicitly specify that they want to 
target a different FPU (e.g. "-mfpu=none").


Repository:
  rC Clang

https://reviews.llvm.org/D53121



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls accepted this revision.
kristof.beyls added a comment.

I'm not an expert on many of the areas touched by this patch, but it looks fine 
from me from a high-level point-of-view, modulo a few nits I have on a few 
comments.




Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:151
+  // flags). This is a bit hacky but keeps existing usages working. We should
+  // consider deprecated this and instead warning if the user requests external
+  // retpoline thunks and *doesn't* request some form of retpolines.

s/deprecated/deprecating/
s/warning/warn/



Comment at: llvm/include/llvm/IR/Attributes.td:181-185
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and the default merge strategy of retaining the caller's
+/// attribute. This specifically matches the intent for this attribute which is
+/// that the context dominates, and inlined code will become hardened or lose
+/// its hardening based on the caller's attribute.

After updating the LangRef.rst text, I think this comment also needs to be 
updated. As is, it still documents the old inlining behaviour?
I'm also not sure in how far the comment makes most sense here. This is already 
documented in LangRef.rst, and AFAIK, the inlining compatibility mode is not 
something that is defined here?



Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:78-82
+static cl::opt EnableSpeculativeLoadHardening(
+"x86-speculative-load-hardening",
+cl::desc("Force enable speculative load hardening"), cl::init(false),
+cl::Hidden);
+

I'm not sure, but do you really still need an option to force enable SLH when 
you have function attributes now to enable it?
When you generate LLVM-IR using clang, you now have a clang option to enable 
that function attribute on all functions, so do you still have scenarios where 
you need an LLVM backend option to override the function attribute?



Comment at: llvm/lib/Target/X86/X86TargetMachine.cpp:474
 
-  if (EnableSpeculativeLoadHardening)
-addPass(createX86SpeculativeLoadHardeningPass());
+  // Will only run if force enabled or detects the relevant attribute.
+  addPass(createX86SpeculativeLoadHardeningPass());

I guess this is true for some other passes too, and they don't add such a 
comment here. Maybe best to remove this comment if my guess is right?


Repository:
  rL LLVM

https://reviews.llvm.org/D51157



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: llvm/docs/LangRef.rst:1659-1661
+that hardening. It should also be possible to *not* harden a hot and/or 
safe
+function and have code inlined there *not* be hardened (even if the generic
+form is hardened).

It feels wrong to me to have source code that is annotated to get hardened, but 
that actually will not get hardened (whether it is due to it being inlined 
somewhere or due to any other automatic behind-the-back-of-the-developer 
transformation). I fear this may lower trust in the protection this attribute 
provides.
I assume there is a use case where the developer wants to indicate "no 
hardening in this function nor in any functions inlined here". If that needs to 
be supported, my feel is that we may need to support that in another way.
I guess that there must be some cases where just duplicating the function to be 
inlined in the source code into a hardened and a non-hardened version could be 
too hard to do for some programs.
So, in short, I don't know what the best solution here is. I just want to raise 
my concern that I don't think it's a good idea that functions that are marked 
to be hardened end up not getting hardened under some circumstances.


Repository:
  rL LLVM

https://reviews.llvm.org/D51157



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

[PATCH] D49073: Introducing __builtin_speculation_safe_value

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls created this revision.
Herald added subscribers: cfe-commits, aheejin, dschuff.

This is part of implementing a technique to mitigate against Spectre v1,
similar in spirit to what has been proposed by Chandler for X86_64 at
http://lists.llvm.org/pipermail/llvm-dev/2018-March/122085.html.

This patch adds a new builtin function that provides a mechanism for
limiting the effects of miss-speculation by a CPU.
This patch provides the clang-side of the needed functionality; there is
also an llvm-side patch this patch is dependent on.

We've tried to design this in such a way that it can be used for any
target where this might be necessary. The patch provides a generic
implementation of the builtin, with most of the target-specific
support in the LLVM counter part to this clang patch.

The signature of the new, polymorphic, builtin is:

T __builtin_speculation_safe_value(T v)

T can be any integral type (signed or unsigned char, int, short, long,
etc) or any pointer type.

The builtin assures that value v will be made 0 on execution paths that
are being executed under control flow miss-speculation by the CPU, when
the miss-speculated path originated due to misprediction of a direct
conditional branch.

Whereas this still leaves open the possibility of execution on a
miss-speculated path starting at misprediction of other control flow
instructions, our believe is that the above guarantee is still useful in
mitigating vulnerability to Spectre v1-style attacks and implementable
for most, if not all, target instruction sets.

This also introduces the predefined pre-processor macro
__HAVE_SPECULATION_SAFE_LOAD, that allows users to check if their
version of the compiler supports this intrinsic.


Repository:
  rC Clang

https://reviews.llvm.org/D49073

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/CodeGen/CGBuiltin.cpp
  lib/Frontend/InitPreprocessor.cpp
  lib/Sema/SemaChecking.cpp
  test/CodeGen/builtin-speculation-safe-value.c
  test/Preprocessor/init.c
  test/Sema/builtin-speculation-safe-value.c
  test/Sema/builtin-speculation-safe-value.cpp

Index: test/Sema/builtin-speculation-safe-value.cpp
===
--- /dev/null
+++ test/Sema/builtin-speculation-safe-value.cpp
@@ -0,0 +1,58 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64 -x c++ -std=c++11 -DENABLE_ERRORS -verify %s
+// RUN: %clang_cc1 -triple aarch64 -x c++ -std=c++11 %s -emit-llvm -o -
+
+void test_type() {
+  char c;
+  c = __builtin_speculation_safe_value(c);
+
+  short s;
+  s = __builtin_speculation_safe_value(s);
+
+  int i;
+  i = __builtin_speculation_safe_value(i);
+
+  long l;
+  l = __builtin_speculation_safe_value(l);
+
+  long long ll;
+  ll = __builtin_speculation_safe_value(ll);
+
+  int *ip;
+  ip = __builtin_speculation_safe_value(ip);
+
+  int (*fp)(int, int);
+  fp = __builtin_speculation_safe_value(fp);
+
+  enum {e1, e2} e;
+  e = __builtin_speculation_safe_value(e);
+
+#ifdef enable_errors
+  float f;
+  f = __builtin_speculation_safe_value(f); // expected-error {{argument to speculation_safe_value builtin must be a pointer or integer ('float' invalid)}}
+
+  struct s { int a; } s;
+  s = __builtin_speculation_safe_value(s); // expected-error {{argument to speculation_safe_value builtin must be a pointer or integer ('struct s' invalid)}}
+
+  union u { int a; } u;
+  u = __builtin_speculation_safe_value(u); // expected-error {{argument to speculation_safe_value builtin must be a pointer or integer ('union u' invalid)}}
+
+  char __attribute__((vector_size(16))) v;
+  v = __builtin_speculation_safe_value(v); // expected-error {{argument to speculation_safe_value builtin must be a pointer or integer ('__attribute__((__vector_size__(16 * sizeof(char char' (vector of 16 'char' values) invalid)}}
+#endif
+}
+
+#ifdef ENABLE_ERRORS
+template
+T load(const T v) {
+  return __builtin_speculation_safe_value(v); // expected-error {{argument to speculation_safe_value builtin must be a pointer or integer ('float' invalid)}}
+}
+
+void test_templates() {
+  int i;
+  load(i);
+
+  float f;
+  load(f); // expected-note {{in instantiation of function template specialization 'load' requested here}}
+}
+#endif
Index: test/Sema/builtin-speculation-safe-value.c
===
--- /dev/null
+++ test/Sema/builtin-speculation-safe-value.c
@@ -0,0 +1,46 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64 -DENABLE_ERRORS -verify %s
+// RUN: %clang_cc1 -triple aarch64 %s -emit-llvm -o -
+
+void test_type() {
+  char c;
+  c = __builtin_speculation_safe_value(c);
+
+  short s;
+  s = __builtin_speculation_safe_value(s);
+
+  int i;
+  i = __builtin_speculation_safe_value(i);
+
+  long l;
+  l = __builtin_speculation_safe_value(l);
+
+  long long ll;
+  ll = __builtin_speculation_safe_value(ll);
+
+  int 

[PATCH] D41760: Introduce __builtin_load_no_speculate

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

The API design has been discussed over the past weeks in detail on the gcc 
mailing list. As a result of that, we propose to adapt the API, to enable 
efficient code generation also on architectures that need to generate a barrier 
instruction to achieve the desired semantics.

The main change in the proposed API is to drop the failval parameter and to 
tweak the semantics to the below.
There is a more detailed rationale for these changes at 
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01546.html

I haven't updated the code to implement the new specification yet, but thought 
I'd share the new specification as soon as possible, while I find the time to 
adapt the implementation:

The signature of the new, polymorphic, builtin is:

  T __builtin_speculation_safe_load(const volatile T *ptr,
const volatile void *lower,
const volatile void *upper,
const volatile void *cmpptr)

T can be any integral type (signed or unsigned char, int, short, long, etc) or 
any pointer type.

This builtin provides a means to limit the extent to which a processor can 
continue speculative execution with the result of loading a value stored at 
ptr. The boundary conditions, described by cmpptr, lower_bound and upper_bound, 
define the conditions under which execution after the load can continue safely:

- When the builtin is not being executed speculatively:
  - if lower_bound <= cmpptr < upper_bound, the value at address ptr is 
returned.
  - if cmpptr is not within these bounds, the behaviour is undefined.
- When the builtin is being executed speculatively, either:
  - Execution of instructions following the builtin that have a dependency on 
the result of the intrinsic will be blocked, until the builtin is no longer 
executing speculatively. At this point, the semantics under point 1 above apply.
  - Speculation may continue using the value at address ptr as the return value 
of the builtin, if lower_bound <= cmpptr < upper_bound, or an unspecified 
constant value if cmpptr is outside these bounds.

The final argument, cmpptr, may be omitted if it is the same as ptr.

The builtin is supported for all architectures, but on machines where 
target-specific support for inhibiting speculation is not implemented, or not 
necessary, the compiler will emit a warning.

The pre-processor macro __HAVE_SPECULATION_SAFE_LOAD is defined with the value 
1 when the compiler supports this builtin.


https://reviews.llvm.org/D41760



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

[PATCH] D41760: Introduce __builtin_load_no_speculate

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls created this revision.
kristof.beyls added a reviewer: olista01.
Herald added subscribers: aheejin, javed.absar, dschuff, jfb, aemerson.

Recently, Google Project Zero disclosed several classes of attack
against speculative execution. One of these, known as variant-1
(CVE-2017-5753), allows explicit bounds checks to be bypassed under
speculation, providing an arbitrary read gadget. Further details can
be found on the GPZ blog [1].

This patch adds a new builtin function that provides a mechanism for
limiting speculation by a CPU after a bounds-checked memory access.
This patch provides the clang-side of the needed functionality; there is
also an llvm-side patch this patch is dependent on.
We've tried to design this in such a way that it can be used for any
target where this might be necessary.  The patch provides a generic
implementation of the builtin, with most of the target-specific
support in the LLVM counter part to this clang patch.

The signature of the new, polymorphic, builtin is:

T __builtin_load_no_speculate(const volatile T *ptr,

  const volatile void *lower,
  const volatile void *upper,
  T failval,
  const volatile void *cmpptr)

T can be any integral type (signed or unsigned char, int, short, long,
etc) or any pointer type.

The builtin implements the following logical behaviour:

inline T __builtin_load_no_speculate(const volatile T *ptr,

 const volatile void *lower,
 const volatile void *upper, T failval,
 const volatile void *cmpptr) {
  T result;
  if (cmpptr >= lower && cmpptr < upper)
result = *ptr;
  else
result = failval;
  return result;

}

In addition, the builtin ensures that future speculation using *ptr may
only continue iff cmpptr lies within the bounds specified.

To make the builtin easier to use, the final two arguments can both be
omitted: failval will default to 0 in this case and if cmpptr is omitted
ptr will be used for expansions of the range check.  In addition, either
lower or upper (but not both) may be a literal NULL and the expansion
will then ignore that boundary condition when expanding.

This also introduces the predefined pre-processor macro
__HAVE_LOAD_NO_SPECULATE, that allows users to check if their version of
the compiler supports this intrinsic.

The builtin is defined for all architectures, even if they do not
provide a mechanism for inhibiting speculation.  If they do not have
such support the compiler will emit a warning and simply implement the
architectural behavior of the builtin.

This patch can be used with the header file that Arm recently
published here: https://github.com/ARM-software/speculation-barrier.

Kernel patches are also being developed, eg:
https://lkml.org/lkml/2018/1/3/754.  The intent is that eventually
code like this will be able to use support directly from the compiler
in a portable manner.

Similar patches are also being developed for GCC and have been posted to
their development list, see
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00205.html

[1] More information on the topic can be found here:
https://googleprojectzero.blogspot.co.uk/2018/01/reading-privileged-memory-with-side.html
Arm specific information can be found here:
https://www.arm.com/security-update


https://reviews.llvm.org/D41760

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/TargetInfo.h
  include/clang/Sema/Sema.h
  lib/Basic/Targets/AArch64.cpp
  lib/Basic/Targets/AArch64.h
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/ARM.h
  lib/CodeGen/CGBuiltin.cpp
  lib/Frontend/InitPreprocessor.cpp
  lib/Sema/SemaChecking.cpp
  test/CodeGen/builtin-load-no-speculate.c
  test/Preprocessor/init.c
  test/Sema/builtin-load-no-speculate-c.c
  test/Sema/builtin-load-no-speculate-cxx.cpp
  test/Sema/builtin-load-no-speculate-target-not-supported.c

Index: test/Sema/builtin-load-no-speculate-target-not-supported.c
===
--- /dev/null
+++ test/Sema/builtin-load-no-speculate-target-not-supported.c
@@ -0,0 +1,6 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple thumbv8m.baseline -fsyntax-only -verify %s
+
+void test_valid(int *ptr, int *lower, int *upper, int failval, int *cmpptr) {
+  __builtin_load_no_speculate(ptr, lower, upper, failval, cmpptr); // expected-warning {{this target does not support anti-speculation operations. Your program will still execute correctly, but speculation will not be inhibited}}
+}
Index: test/Sema/builtin-load-no-speculate-cxx.cpp
===
--- /dev/null
+++ test/Sema/builtin-load-no-speculate-cxx.cpp
@@ -0,0 +1,144 @@
+// REQUIRES: arm-registered-target
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64 -x c++ -std=c++11 -DENABLE_ERRORS -verify %s

[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

Still LGTM; please commit.


https://reviews.llvm.org/D34878



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

[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Kristof Beyls via Phabricator via cfe-commits]

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

Thanks Strahinja!
I thought that some indentations looked a bit strange, so I'd just still check 
that clang-format formats your changes the same way.
Otherwise LGTM!


https://reviews.llvm.org/D34878



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

[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:137
+  const Driver  = TC.getDriver();
+  arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid;
+  if (Arg *A =

With the new version of the code, there's no need to have a ThreadPointer 
variable declared here; it can be declared inside the if statement below 
instead.



Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:149-150
+else
+  D.Diag(diag::err_drv_invalid_mtp) << A->getAsString(Args);
+  }
+  return ReadTPMode::Soft;

a return ReadTPMode::Invalid is missing here.


https://reviews.llvm.org/D34878



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

[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:145-146
+  // choose soft mode.
+  if (ThreadPointer == ReadTPMode::Invalid)
+ThreadPointer = ReadTPMode::Soft;
+  return ThreadPointer;

spetrovic wrote:
> kristof.beyls wrote:
> >  and always give an error if an invalid mtp command line option was 
> > given, rather than default back to soft mode?
> If 'mtp' takes invalid value, error is always provided. This is the case when 
> there is no -mtp option in command line, you can see how the case of invalid 
> mtp argument is handled in the code above.
Right.
I just thought that the function would be ever so slightly simpler if it had 
the following structure roughly:

```
if (Arg *A = ...) {
  ThreadPointer = llvm::StringSwitch... ;
  if (!Invalid)
return ThreadPointer;
  if (empty)
D.Diag();
  else
D.Diag();
  return Invalid;
}
return ReadTPMode::Soft;
```

And probably is also slightly closer to the coding standard described in 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
But this is a really minor comment.




https://reviews.llvm.org/D34878



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

[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added inline comments.



Comment at: include/clang/Driver/Options.td:1664-1665
   HelpText<"Allow generation of data access to code sections (ARM only)">;
+def mtp_mode_EQ : Joined<["-"], "mtp=">, Group, 
Values<"soft, cp15">,
+  HelpText<"Read thread pointer from coprocessor register (ARM only)">;
 def mpure_code : Flag<["-"], "mpure-code">, Alias; // Alias for 
GCC compatibility

Looking at the gcc documentation for this option 
(https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html), gcc accepts 3 values: 
'soft', 'cp15' and 'auto', with the default setting being 'auto'.
This patch implements just 2 of those values: 'soft' and 'cp15'.
I think this is fine, as long as the default value is 'soft'.
The 'auto' value should automatically pick 'cp15' if that's going to work on 
what you're targeting. If I understood correctly, that depends both on the 
architecture version you're targeting and the operating system/kernel you're 
targeting. So, there could be a lot of details to go through to get 'auto' 
right in all cases. Which is why I think it's fine to leave an implementation 
of 'auto' for later.
Is the default value 'soft'?



Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:128
+  const Driver  = TC.getDriver();
+  arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid;
+  if (Arg *A =

Wouldn't it be better to default to ReadTPMode::Soft when not -mtp command line 
option is given? 



Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:145-146
+  // choose soft mode.
+  if (ThreadPointer == ReadTPMode::Invalid)
+ThreadPointer = ReadTPMode::Soft;
+  return ThreadPointer;

 and always give an error if an invalid mtp command line option was given, 
rather than default back to soft mode?



Comment at: lib/Driver/ToolChains/Clang.cpp:1348-1358
+  arm::ReadTPMode ThreadPointer = arm::getReadTPMode(getToolChain(), Args);
+  if (ThreadPointer == arm::ReadTPMode::Cp15) {
+CmdArgs.push_back("-mtp");
+CmdArgs.push_back("cp15");
+  } else {
+assert(ThreadPointer == arm::ReadTPMode::Soft &&
+   "Invalid mode for reading thread pointer");

My inexperience in this part of the code base is probably showing, but why is 
this needed at all?
IIUC, in D34408, you modelled TPMode in the backend using a target feature, and 
there isn't a custom -mtp option there?
Maybe this is left-over code from an earlier revision of D34408, that's no 
longer needed?



Comment at: test/Driver/clang-translation.c:78-82
+// RUN: %clang -target arm-linux -mtp=cp15 -### -S %s -arch armv7 2>&1 | \
+// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER %s
+// ARMv7_THREAD_POINTER: "-target-feature" "+read-tp-hard"
+// ARMv7_THREAD_POINTER: "-mtp" "cp15"
+// ARMv7_THREAD_POINTER-NOT: "mtp" "soft"

It probably would be good to also have a test that when no mtp option is given, 
the equivalent of when '-mtp soft' is specified would happen.
Furthermore, my inexperience in this part of the code base probably shows, but 
I'm puzzled as to why this test is looking for '-mtp' in the output. Wouldn't 
the '-target-feature +read-tp-hard' be enough to convey the information to the 
mid- and back-end?


Repository:
  rL LLVM

https://reviews.llvm.org/D34878



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