[PATCH] D129693: [ARM] Don't emit Arm speculation hardening thunks under Thumb and vice-versa

[David Spickett via Phabricator via llvm-commits]

DavidSpickett edited reviewers, added: DavidSpickett; removed: ARMDavidSpickett.
DavidSpickett added a comment.

I can't claim to know what these thunks are doing in the first place, so just some general comments.



================
Comment at: llvm/include/llvm/CodeGen/IndirectThunks.h:28
 protected:
-  bool InsertedThunks;
+  unsigned InsertedThunks;
   void doInitialization(Module &M) {}
----------------
Can we document what this contains? Now that it's more than did or did not insert something.


================
Comment at: llvm/include/llvm/CodeGen/IndirectThunks.h:92
+    // can use InsertedThunks to detect whether relevant thunks have already
+    // been insertd.
     // FIXME: Conditionalize on indirect calls so we don't emit a thunk when
----------------
`inserted`


================
Comment at: llvm/lib/Target/ARM/ARMSLSHardening.cpp:169
+    if ((InsertedThunks & 0x1 && !MF.getSubtarget<ARMSubtarget>().isThumb()) ||
+        (InsertedThunks & 0x2 && MF.getSubtarget<ARMSubtarget>().isThumb()))
+      return false;
----------------
Magic number detector says why are we looking for numbers instead of named values?

Off the top of my head - because there's no good place to store such an enum/define that can be used in all the places it's needed.


================
Comment at: llvm/test/CodeGen/ARM/speculation-hardening-sls-boththunks.ll:3
+
+; Given both Arm and THumb functions in the same compilation unit, we should
+; get both arm and thumb thunks.
----------------
`Thumb`


================
Comment at: llvm/test/CodeGen/ARM/speculation-hardening-sls-boththunks.ll:4
+; Given both Arm and THumb functions in the same compilation unit, we should
+; get both arm and thumb thunks.
+
----------------
Is there reason to check that with only one of arm and thumb you only get thunks for that one?


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

https://reviews.llvm.org/D129693