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.
Details
Details
- Reviewers
echristo jyknight rsmith DavidSpickett
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/lib/Driver/ToolChains/Arch/AArch64.cpp | ||
|---|---|---|
| 1424 | We should avoid static constructors/destructors if possible. Just use a plain array and llvm::find(ValidRegs, RegName) | |
Comment Actions
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
Comment Actions
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.
Comment Actions
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.
We should avoid static constructors/destructors if possible. Just use a plain array and llvm::find(ValidRegs, RegName)