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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

I just have a few more very minor nit-picky comments. Feel free to disagree with them.
I'm happy for this to go in.
I haven't looked in detail into the comments @DavidSpickett mad , so please also explicitly get his approval before committing.

Thank you!



================
Comment at: llvm/include/llvm/CodeGen/IndirectThunks.h:24
 
-template <typename Derived> class ThunkInserter {
+template <typename Derived, typename InsertedThunksTy = bool>
+class ThunkInserter {
----------------
I see later in the code that InsertedThunksTy must support operator |= and that the semantics of the |= operation must be somewhat like doing a union/max kind-of operation.
Not sure if it would be worthwhile to document that at this location, or if there is a way in modern C++ to somehow enforce that.
Probably best action is not to change anything in the patch - I thought I'd just share my thought in case there is anything about it that can be improved in this patch.


================
Comment at: llvm/include/llvm/CodeGen/IndirectThunks.h:29-30
 protected:
-  bool InsertedThunks;
+  // A variable used to track whether (and possible which) thunks have been
+  // inserted.
+  InsertedThunksTy InsertedThunks;
----------------
Extreme nit-pick: I'd add "so far" to the end of the sentence.


================
Comment at: llvm/lib/Target/ARM/ARMSLSHardening.cpp:164
 
+enum ArmInsertedThunks {
+  ArmThunk = 1,
----------------
Maybe InsertedThunkKind would be a little bit more descriptive than ArmInsertedThunks for what this enum is modelling?


================
Comment at: llvm/lib/Target/ARM/ARMSLSHardening.cpp:174
 namespace {
-struct SLSBLRThunkInserter : ThunkInserter<SLSBLRThunkInserter> {
+struct SLSBLRThunkInserter : ThunkInserter<SLSBLRThunkInserter, ArmInsertedThunks> {
   const char *getThunkPrefix() { return SLSBLRNamePrefix; }
----------------
Could it be that this has not been clang-formatted?


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

https://reviews.llvm.org/D129693