[PATCH] D129693: [ARM] Don't emit Arm speculation hardening thunks under Thumb and vice-versa
David Spickett via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 6 08:47:14 PST 2022
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
More information about the llvm-commits
mailing list