[PATCH] D100546: [ARM][AArch64] SLSHardening: make non-comdat thunks possible

Tue May 18 07:48:23 PDT 2021

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

Thanks @danielkiss !
I only have a few nit picky remarks.
Overall looks good!

================
Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:275
       Features.push_back("+harden-sls-blr");
+    if ((EnableRetBr || EnableBlr) && DisableComdat) {
+      Features.push_back("+harden-sls-nocomdat");
----------------
I'm wondering if the '(EnableRetBr || EnableBlr)' condition is needed here?
Just dropping it would make this code a little bit simpler.
I'm assuming here of course that the semantics of the +harden-sls-comdat target feature is to not produce comdat sections in case any of the other sls-hardening target features would produce thunks.
I agree this is a bit of a nit pick though.

================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:837
       Features.push_back("+harden-sls-blr");
+    if ((EnableRetBr || EnableBlr) && DisableComdat) {
+      Features.push_back("+harden-sls-nocomdat");
----------------
Same comment/thought on the "(EnableRetBr || EnableBlr)" condition as on the AArch64 side.

================
Comment at: llvm/lib/Target/AArch64/AArch64.td:528
+  "HardenSlsNoComdat", "true",
+  "Generate code related to SLS mitigation into the normal text segment">;

----------------
I think it would be more accurate if the word "thunk" would be present in the documentation.
What about the following:
"Generate thunk code for SLS mitigation in the normal text section"?

================
Comment at: llvm/lib/Target/ARM/ARM.td:578
+  "HardenSlsNoComdat", "true",
+  "Generate code related to SLS mitigation into the normal text segment">;

----------------
Same comment as on the AArch64 side on using the word "thunk" somewhere in the help text.

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

https://reviews.llvm.org/D100546