[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers abandoned this revision.
nickdesaulniers added a comment.

I don't plan to land this; I wrote it in case it comes up in code review.  
llvm's codegen will `report_fatal_error` for garbage sysregs.  If it comes up 
again in the future, this phab review will still exist for reference, but I 
suggest we only add what's needed in that case, not all possible sysregs since 
most make sense in this context I suspect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

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

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 340931.
nickdesaulniers added a comment.

- rebase on D101387 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

Files:
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/AArch64.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c

Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -46,6 +46,10 @@
 // RUN:   -mstack-protector-guard-reg=sp_el0 \
 // RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
 // RUN: FileCheck -check-prefix=CHECK-AARCH64 %s
+// RUN: %clang -### -target aarch64-linux-gnu -mstack-protector-guard=sysreg \
+// RUN:   -mstack-protector-guard-reg=sp_el2 \
+// RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-AARCH64-SP_EL2 %s
 // RUN: %clang -### -target aarch64-linux-gnu \
 // RUN:   -mstack-protector-guard=tls %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-VALUE-AARCH64 %s
@@ -55,5 +59,6 @@
 // RUN: FileCheck -check-prefix=INVALID-REG-AARCH64 %s
 
 // CHECK-AARCH64: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el0"
-// INVALID-VALUE-AARCH64: error: invalid value 'tls' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:sysreg global'
+// CHECK-AARCH64-SP_EL2: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el2"
+// INVALID-VALUE-AARCH64: error: invalid value 'tls' in 'mstack-protector-guard=', valid arguments to '-mstack-protector-guard=' are:sysreg global
 // INVALID-REG-AARCH64: error: invalid value 'foo' in 'mstack-protector-guard-reg='
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3147,7 +3147,7 @@
   << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
   return;
 }
-if (EffectiveTriple.isAArch64() && Value != "sp_el0") {
+if (EffectiveTriple.isAArch64() && !aarch64::isValidSysReg(Value)) {
   D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
   return;
 }
Index: clang/lib/Driver/ToolChains/Arch/AArch64.h
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.h
+++ clang/lib/Driver/ToolChains/Arch/AArch64.h
@@ -27,6 +27,8 @@
 std::string getAArch64TargetCPU(const llvm::opt::ArgList ,
 const llvm::Triple , llvm::opt::Arg *);
 
+bool isValidSysReg(StringRef RegName);
+
 } // end namespace aarch64
 } // end namespace target
 } // end namespace driver
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -10,9 +10,10 @@
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Option/ArgList.h"
-#include "llvm/Support/TargetParser.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/TargetParser.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -493,3 +494,932 @@
   if (Args.hasArg(options::OPT_mno_neg_immediates))
 Features.push_back("+no-neg-immediates");
 }
+
+/// Checks if a string is a valid sysreg.
+// While it would be good to avoid duplication of
+// llvm::AArch64SysReg::lookupSysRegByName, we don't want to couple CFE to
+// Target/. This list is taken from the generated
+// lib/Target/AArch64/AArch64GenSystemOperands.inc.
+bool aarch64::isValidSysReg(StringRef RegName) {
+  static const char *ValidRegs[] = {
+  "ACCDATA_EL1",
+  "ACTLR_EL1",
+  "ACTLR_EL2",
+  "ACTLR_EL3",
+  "AFSR0_EL1",
+  "AFSR0_EL12",
+  "AFSR0_EL2",
+  "AFSR0_EL3",
+  "AFSR1_EL1",
+  "AFSR1_EL12",
+  "AFSR1_EL2",
+  "AFSR1_EL3",
+  "AIDR_EL1",
+  "AMAIR_EL1",
+  "AMAIR_EL12",
+  "AMAIR_EL2",
+  "AMAIR_EL3",
+  "AMCFGR_EL0",
+  "AMCGCR_EL0",
+  "AMCNTENCLR0_EL0",
+  "AMCNTENCLR1_EL0",
+  "AMCNTENSET0_EL0",
+  "AMCNTENSET1_EL0",
+  "AMCR_EL0",
+  "AMEVCNTR00_EL0",
+  "AMEVCNTR01_EL0",
+  "AMEVCNTR02_EL0",
+  "AMEVCNTR03_EL0",
+  "AMEVCNTR10_EL0",
+  "AMEVCNTR110_EL0",
+  "AMEVCNTR111_EL0",
+  "AMEVCNTR112_EL0",
+  "AMEVCNTR113_EL0",
+  "AMEVCNTR114_EL0",
+  "AMEVCNTR115_EL0",
+  "AMEVCNTR11_EL0",
+  "AMEVCNTR12_EL0",
+  "AMEVCNTR13_EL0",
+ 

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D101327#2718989 , @DavidSpickett 
wrote:

> So if kernel builds are using a small subset of registers, just check for 
> those.

The kernel uses just one for aarch64. It looks like GCC just feeds through 
whatever is specified at the command line to GAS without any validation; 
letting GAS fail or not.  For reference, here are the ISAs+regs used currently 
by the Linux kernel.

- riscv: tp
- powerpc: r13, r2
- x86: fs
- arm64: sp_el0

So it's definitely overkill to validate all possible sysregs.  Just thought I'd 
post a patch in case we ever wanted to revisit this, but whether this patch 
lands or not doesn't matter to me; https://reviews.llvm.org/D100919 is what we 
need, hence the separation.

> Otherwise we've got another duplicated list that we (Arm) will probably 
> forget to update anyway when new registers are added.

Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

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

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 340915.
nickdesaulniers added a comment.

- prefer vanilla array + llvm::find


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

Files:
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/AArch64.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c

Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -46,6 +46,10 @@
 // RUN:   -mstack-protector-guard-reg=sp_el0 \
 // RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
 // RUN: FileCheck -check-prefix=CHECK-AARCH64 %s
+// RUN: %clang -### -target aarch64-linux-gnu -mstack-protector-guard=sysreg \
+// RUN:   -mstack-protector-guard-reg=sp_el2 \
+// RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-AARCH64-SP_EL2 %s
 // RUN: %clang -### -target aarch64-linux-gnu \
 // RUN:   -mstack-protector-guard=tls %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-VALUE-AARCH64 %s
@@ -55,5 +59,6 @@
 // RUN: FileCheck -check-prefix=INVALID-REG-AARCH64 %s
 
 // CHECK-AARCH64: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el0"
+// CHECK-AARCH64-SP_EL2: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el2"
 // INVALID-VALUE-AARCH64: error: invalid value 'tls' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:sysreg global'
 // INVALID-REG-AARCH64: error: invalid value 'foo' in 'mstack-protector-guard-reg='
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3147,7 +3147,7 @@
   << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
   return;
 }
-if (EffectiveTriple.isAArch64() && Value != "sp_el0") {
+if (EffectiveTriple.isAArch64() && !aarch64::isValidSysReg(Value)) {
   D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
   return;
 }
Index: clang/lib/Driver/ToolChains/Arch/AArch64.h
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.h
+++ clang/lib/Driver/ToolChains/Arch/AArch64.h
@@ -27,6 +27,8 @@
 std::string getAArch64TargetCPU(const llvm::opt::ArgList ,
 const llvm::Triple , llvm::opt::Arg *);
 
+bool isValidSysReg(StringRef RegName);
+
 } // end namespace aarch64
 } // end namespace target
 } // end namespace driver
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -10,9 +10,10 @@
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Option/ArgList.h"
-#include "llvm/Support/TargetParser.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/TargetParser.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -493,3 +494,932 @@
   if (Args.hasArg(options::OPT_mno_neg_immediates))
 Features.push_back("+no-neg-immediates");
 }
+
+/// Checks if a string is a valid sysreg.
+// While it would be good to avoid duplication of
+// llvm::AArch64SysReg::lookupSysRegByName, we don't want to couple CFE to
+// Target/. This list is taken from the generated
+// lib/Target/AArch64/AArch64GenSystemOperands.inc.
+bool aarch64::isValidSysReg(StringRef RegName) {
+  static const char *ValidRegs[] = {
+  "ACCDATA_EL1",
+  "ACTLR_EL1",
+  "ACTLR_EL2",
+  "ACTLR_EL3",
+  "AFSR0_EL1",
+  "AFSR0_EL12",
+  "AFSR0_EL2",
+  "AFSR0_EL3",
+  "AFSR1_EL1",
+  "AFSR1_EL12",
+  "AFSR1_EL2",
+  "AFSR1_EL3",
+  "AIDR_EL1",
+  "AMAIR_EL1",
+  "AMAIR_EL12",
+  "AMAIR_EL2",
+  "AMAIR_EL3",
+  "AMCFGR_EL0",
+  "AMCGCR_EL0",
+  "AMCNTENCLR0_EL0",
+  "AMCNTENCLR1_EL0",
+  "AMCNTENSET0_EL0",
+  "AMCNTENSET1_EL0",
+  "AMCR_EL0",
+  "AMEVCNTR00_EL0",
+  "AMEVCNTR01_EL0",
+  "AMEVCNTR02_EL0",
+  "AMEVCNTR03_EL0",
+  "AMEVCNTR10_EL0",
+  "AMEVCNTR110_EL0",
+  "AMEVCNTR111_EL0",
+  "AMEVCNTR112_EL0",
+  "AMEVCNTR113_EL0",
+  "AMEVCNTR114_EL0",
+  "AMEVCNTR115_EL0",
+  "AMEVCNTR11_EL0",
+  "AMEVCNTR12_EL0",
+  "AMEVCNTR13_EL0",
+  "AMEVCNTR14_EL0",
+  "AMEVCNTR15_EL0",
+  "AMEVCNTR16_EL0",
+  "AMEVCNTR17_EL0",
+  "AMEVCNTR18_EL0",
+  "AMEVCNTR19_EL0",
+  

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

A lot of these system registers are going to be unsuitable for the stack 
canary. They're either read only, they have fixed bits, or they'll get 
overwritten by various system events.

I'm not suggesting we audit this list for those things, what seems reasonable 
to me is just to support the ones that the Linux kernel uses. (what GCC allows 
I'm not sure)

https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/AArch64-Options.html

  -mstack-protector-guard=guard
  -mstack-protector-guard-reg=reg
  -mstack-protector-guard-offset=offset
  Generate stack protection code using canary at guard. Supported locations are 
‘global’ for a global canary or ‘sysreg’ for a canary in an appropriate system 
register.
  
  With the latter choice the options -mstack-protector-guard-reg=reg and 
-mstack-protector-guard-offset=offset furthermore specify which system register 
to use as base register for reading the canary, and from what offset from that 
base register. There is no default register or offset as this is entirely for 
use within the Linux kernel.

So if kernel builds are using a small subset of registers, just check for 
those. Otherwise we've got another duplicated list that we (Arm) will probably 
forget to update anyway when new registers are added.

Either way it's one more stick on the "why doesn't clang just use the backend 
info" fire, but it's smaller at least if we limit the registers. E.g.

  error: invalid value 'foo' in 'mstack-protector-guard-reg=','for AArch64' 
valid values are sp_el1 sp_el2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

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

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:1425
+  };
+  return std::find(ValidRegs.begin(), ValidRegs.end(), RegName.upper()) !=
+ ValidRegs.end();

We should avoid static constructors/destructors if possible. Just use a plain 
array and `llvm::find(ValidRegs, RegName)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

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

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 340662.
nickdesaulniers added a comment.

- remove comment about string case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101327

Files:
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/AArch64.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c

Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -46,6 +46,10 @@
 // RUN:   -mstack-protector-guard-reg=sp_el0 \
 // RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
 // RUN: FileCheck -check-prefix=CHECK-AARCH64 %s
+// RUN: %clang -### -target aarch64-linux-gnu -mstack-protector-guard=sysreg \
+// RUN:   -mstack-protector-guard-reg=sp_el2 \
+// RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-AARCH64-SP_EL2 %s
 // RUN: %clang -### -target aarch64-linux-gnu \
 // RUN:   -mstack-protector-guard=tls %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-VALUE-AARCH64 %s
@@ -55,5 +59,6 @@
 // RUN: FileCheck -check-prefix=INVALID-REG-AARCH64 %s
 
 // CHECK-AARCH64: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el0"
+// CHECK-AARCH64-SP_EL2: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el2"
 // INVALID-VALUE-AARCH64: error: invalid value 'tls' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:sysreg global'
 // INVALID-REG-AARCH64: error: invalid value 'foo' in 'mstack-protector-guard-reg=','for AArch64'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3140,7 +3140,7 @@
   << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
   return;
 }
-if (EffectiveTriple.isAArch64() && Value != "sp_el0") {
+if (EffectiveTriple.isAArch64() && !aarch64::isValidSysReg(Value)) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
   << A->getOption().getName() << Value << "for AArch64";
   return;
Index: clang/lib/Driver/ToolChains/Arch/AArch64.h
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.h
+++ clang/lib/Driver/ToolChains/Arch/AArch64.h
@@ -27,6 +27,8 @@
 std::string getAArch64TargetCPU(const llvm::opt::ArgList ,
 const llvm::Triple , llvm::opt::Arg *);
 
+bool isValidSysReg(StringRef RegName);
+
 } // end namespace aarch64
 } // end namespace target
 } // end namespace driver
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -14,6 +14,8 @@
 #include "llvm/Support/TargetParser.h"
 #include "llvm/Support/Host.h"
 
+#include 
+
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
@@ -493,3 +495,933 @@
   if (Args.hasArg(options::OPT_mno_neg_immediates))
 Features.push_back("+no-neg-immediates");
 }
+
+/// Checks if a string is a valid sysreg.
+// While it would be good to avoid duplication of
+// llvm::AArch64SysReg::lookupSysRegByName, we don't want to couple CFE to
+// Target/. This list is taken from the generated
+// lib/Target/AArch64/AArch64GenSystemOperands.inc.
+bool aarch64::isValidSysReg(StringRef RegName) {
+  static llvm::SmallVector ValidRegs = {
+  "ACCDATA_EL1",
+  "ACTLR_EL1",
+  "ACTLR_EL2",
+  "ACTLR_EL3",
+  "AFSR0_EL1",
+  "AFSR0_EL12",
+  "AFSR0_EL2",
+  "AFSR0_EL3",
+  "AFSR1_EL1",
+  "AFSR1_EL12",
+  "AFSR1_EL2",
+  "AFSR1_EL3",
+  "AIDR_EL1",
+  "AMAIR_EL1",
+  "AMAIR_EL12",
+  "AMAIR_EL2",
+  "AMAIR_EL3",
+  "AMCFGR_EL0",
+  "AMCGCR_EL0",
+  "AMCNTENCLR0_EL0",
+  "AMCNTENCLR1_EL0",
+  "AMCNTENSET0_EL0",
+  "AMCNTENSET1_EL0",
+  "AMCR_EL0",
+  "AMEVCNTR00_EL0",
+  "AMEVCNTR01_EL0",
+  "AMEVCNTR02_EL0",
+  "AMEVCNTR03_EL0",
+  "AMEVCNTR10_EL0",
+  "AMEVCNTR110_EL0",
+  "AMEVCNTR111_EL0",
+  "AMEVCNTR112_EL0",
+  "AMEVCNTR113_EL0",
+  "AMEVCNTR114_EL0",
+  "AMEVCNTR115_EL0",
+  "AMEVCNTR11_EL0",
+  "AMEVCNTR12_EL0",
+  "AMEVCNTR13_EL0",
+  "AMEVCNTR14_EL0",
+  "AMEVCNTR15_EL0",
+  "AMEVCNTR16_EL0",
+  "AMEVCNTR17_EL0",
+  "AMEVCNTR18_EL0",
+  "AMEVCNTR19_EL0",
+  "AMEVCNTVOFF00_EL2",
+  "AMEVCNTVOFF010_EL2",
+  "AMEVCNTVOFF011_EL2",
+  "AMEVCNTVOFF012_EL2",
+  "AMEVCNTVOFF013_EL2",
+

[PATCH] D101327: [Clang][Driver] validate sysregs for -mstack-protector-guard-reg=

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
nickdesaulniers added reviewers: echristo, jyknight, rsmith.
Herald added a subscriber: kristof.beyls.
nickdesaulniers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In order to support arbitrary AArch64 sysregs for
-mstack-protector-guard-reg=, copy the generated list from
lib/Target/AArch64/AArch64GenSystemOperands.inc's lookupSysRegByName().
This avoids requiring the AArch64 backend to be linked to validate such
registers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101327

Files:
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/AArch64.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c

Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -46,6 +46,10 @@
 // RUN:   -mstack-protector-guard-reg=sp_el0 \
 // RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
 // RUN: FileCheck -check-prefix=CHECK-AARCH64 %s
+// RUN: %clang -### -target aarch64-linux-gnu -mstack-protector-guard=sysreg \
+// RUN:   -mstack-protector-guard-reg=sp_el2 \
+// RUN:   -mstack-protector-guard-offset=0 %s 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-AARCH64-SP_EL2 %s
 // RUN: %clang -### -target aarch64-linux-gnu \
 // RUN:   -mstack-protector-guard=tls %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-VALUE-AARCH64 %s
@@ -55,5 +59,6 @@
 // RUN: FileCheck -check-prefix=INVALID-REG-AARCH64 %s
 
 // CHECK-AARCH64: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el0"
+// CHECK-AARCH64-SP_EL2: "-cc1" {{.*}}"-mstack-protector-guard=sysreg" "-mstack-protector-guard-offset=0" "-mstack-protector-guard-reg=sp_el2"
 // INVALID-VALUE-AARCH64: error: invalid value 'tls' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:sysreg global'
 // INVALID-REG-AARCH64: error: invalid value 'foo' in 'mstack-protector-guard-reg=','for AArch64'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3140,7 +3140,7 @@
   << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
   return;
 }
-if (EffectiveTriple.isAArch64() && Value != "sp_el0") {
+if (EffectiveTriple.isAArch64() && !aarch64::isValidSysReg(Value)) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
   << A->getOption().getName() << Value << "for AArch64";
   return;
Index: clang/lib/Driver/ToolChains/Arch/AArch64.h
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.h
+++ clang/lib/Driver/ToolChains/Arch/AArch64.h
@@ -27,6 +27,8 @@
 std::string getAArch64TargetCPU(const llvm::opt::ArgList ,
 const llvm::Triple , llvm::opt::Arg *);
 
+bool isValidSysReg(StringRef RegName);
+
 } // end namespace aarch64
 } // end namespace target
 } // end namespace driver
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -14,6 +14,8 @@
 #include "llvm/Support/TargetParser.h"
 #include "llvm/Support/Host.h"
 
+#include 
+
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
@@ -493,3 +495,933 @@
   if (Args.hasArg(options::OPT_mno_neg_immediates))
 Features.push_back("+no-neg-immediates");
 }
+
+/// Checks if a lowercase string is a valid sysreg.
+// While it would be good to avoid duplication of
+// llvm::AArch64SysReg::lookupSysRegByName, we don't want to couple CFE to
+// Target/. This list is taken from the generated
+// lib/Target/AArch64/AArch64GenSystemOperands.inc.
+bool aarch64::isValidSysReg(StringRef RegName) {
+  static llvm::SmallVector ValidRegs = {
+  "ACCDATA_EL1",
+  "ACTLR_EL1",
+  "ACTLR_EL2",
+  "ACTLR_EL3",
+  "AFSR0_EL1",
+  "AFSR0_EL12",
+  "AFSR0_EL2",
+  "AFSR0_EL3",
+  "AFSR1_EL1",
+  "AFSR1_EL12",
+  "AFSR1_EL2",
+  "AFSR1_EL3",
+  "AIDR_EL1",
+  "AMAIR_EL1",
+  "AMAIR_EL12",
+  "AMAIR_EL2",
+  "AMAIR_EL3",
+  "AMCFGR_EL0",
+  "AMCGCR_EL0",
+  "AMCNTENCLR0_EL0",
+  "AMCNTENCLR1_EL0",
+  "AMCNTENSET0_EL0",
+  "AMCNTENSET1_EL0",
+  "AMCR_EL0",
+  "AMEVCNTR00_EL0",
+  "AMEVCNTR01_EL0",
+  "AMEVCNTR02_EL0",
+  "AMEVCNTR03_EL0",
+  "AMEVCNTR10_EL0",
+  "AMEVCNTR110_EL0",
+  "AMEVCNTR111_EL0",
+  "AMEVCNTR112_EL0",
+  "AMEVCNTR113_EL0",
+  "AMEVCNTR114_EL0",
+  "AMEVCNTR115_EL0",
+  "AMEVCNTR11_EL0",