Linker scripts might not handle COMDAT sections. SLSHardeing adds
new section for each __llvm_slsblr_thunk_xN. This new option allows
the generation of the thunks into the normal text section to handle these
exceptional cases.
,comdat or ,noncomdat can be added to harden-sls to control the codegen.
-mharden-sls=[all|retbr|blr],nocomdat.
Details
Diff Detail
Event Timeline
llvm/include/llvm/CodeGen/IndirectThunks.h | ||
---|---|---|
56 | NIT: tabs to spaces |
Apart from maybe needing to run clang-format on the patch, the code changes look good to me.
Before this could be committed, this needs tests.
llvm/include/llvm/CodeGen/IndirectThunks.h | ||
---|---|---|
43–44 | I'm not entirely sure, but is this how clang-format formats these lines? |
Adding a fronted flag and tests.
comdat is still the default, feels fine but I'm not sure.
llvm/include/llvm/CodeGen/IndirectThunks.h | ||
---|---|---|
43–44 | yes, patch went thru git clang-format |
clang/lib/Driver/ToolChains/Arch/ARM.cpp | ||
---|---|---|
837–840 ↗ | (On Diff #344079) | I think it would be better if comdat/nocomdat were also communicated through Features. I haven't really thought about how to best specify that feature. Maybe just have an extra 2 features: +harden-sls-comdat and +harden-sls-nocomdat? |
llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll | ||
218–220 ↗ | (On Diff #344079) | Would it be possible to also have check lines for HARDEN-COMDAT-OFF, i.e. look for lines that HARDEN-COMDAT-OFF should produce? |
llvm/test/CodeGen/ARM/speculation-hardening-sls.ll | ||
249–251 ↗ | (On Diff #344079) | Would it be possible to also have check lines for HARDEN-COMDAT-OFF, i.e. look for lines that HARDEN-COMDAT-OFF should produce? |
clang/lib/Driver/ToolChains/Arch/ARM.cpp | ||
---|---|---|
837–840 ↗ | (On Diff #344079) | I almost added a feature for the harden-sls-noncomdat but that is not really a HW feature so felt the backed flag is better. |
Thanks @danielkiss !
I only have a few nit picky remarks.
Overall looks good!
clang/lib/Driver/ToolChains/Arch/AArch64.cpp | ||
---|---|---|
275 ↗ | (On Diff #346151) | I'm wondering if the '(EnableRetBr || EnableBlr)' condition is needed here? |
clang/lib/Driver/ToolChains/Arch/ARM.cpp | ||
837 ↗ | (On Diff #346151) | Same comment/thought on the "(EnableRetBr || EnableBlr)" condition as on the AArch64 side. |
llvm/lib/Target/AArch64/AArch64.td | ||
528 ↗ | (On Diff #346151) | I think it would be more accurate if the word "thunk" would be present in the documentation. |
llvm/lib/Target/ARM/ARM.td | ||
578 ↗ | (On Diff #346151) | Same comment as on the AArch64 side on using the word "thunk" somewhere in the help text. |
clang/lib/Driver/ToolChains/Arch/AArch64.cpp | ||
---|---|---|
275 ↗ | (On Diff #346151) | the check is a bit overkill, the backend won't do anything anyway without the EnableRetBr || EnableBlr |
Is the default argument really required?