[PATCH] D102901: [HWASAN] Update pointer tag for X86_64

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:190
+                                    cl::desc("untag mem operate"), cl::Hidden,
+                                    cl::init(false));
+
----------------
Can we avoid creating `ClUntagPointer` for now?  I am able to test locally with QEMU, and I'm also setting up a buildbot to ensure new patches don't break the LAM functionality.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:192
+
+// TODO: Need to co-related with clang option.
+static cl::opt<bool> ClUsePageAlias("hwasan-use-page-alias",
----------------
Can we wire `ClUsePageAlias` up to `fsanitize-hwaddress-experimental-aliasing` in this patch?  Then we can make `ClUsePageAlias` default to false and can remove this TODO.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:193-194
+// TODO: Need to co-related with clang option.
+static cl::opt<bool> ClUsePageAlias("hwasan-use-page-alias",
+                                    cl::desc("hwasan use page alias"),
+                                    cl::Hidden, cl::init(true));
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:272
+    return 0xFFLL;
+  }
+
----------------
Since `getTargetTagShift` and `getTargetTagMask` are used only during `initializeModule`, and since they are both tiny functions, let's remove them and inline the logic in `initializeModule`.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:274
+
+  unsigned kPointerTagShift;
+  uint64_t TagMaskByte;
----------------
Since this is no longer constexpr, let's remove the `k` prefix.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:516
+  UsePageAliases = ClUsePageAlias && (TargetTriple.getArch() == Triple::x86_64);
   InstrumentWithCalls = UsePageAliases ? true : ClInstrumentWithCalls;
   InstrumentStack = UsePageAliases ? false : ClInstrumentStack;
----------------
For now we want to keep `InstrumentWithCalls == true` for x86 in general, even in LAM mode (no aliases).  Please update this line.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:563
         ClGlobals.getNumOccurrences() ? ClGlobals : NewRuntime;
     if (InstrumentGlobals && !UsePageAliases)
       instrumentGlobals();
----------------
We also want to avoid instrumenting globals for now, so please update this line to apply to x86 in general.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:715
-  if (TargetTriple.isAArch64() || TargetTriple.getArch() == Triple::x86_64)
-    return;
-
----------------
Please keep this check here for now.  I want to avoid reintroducing untagging to x86.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:983
+// it frome other targets now.
+Value *HWAddressSanitizer::retagTargetTag(IRBuilder<> &IRB, Value *OldTag) {
+  if (TargetTriple.getArch() == Triple::x86_64) {
----------------
The function name is confusing.  Can we name it something like `applyTagMask`?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1018
+
+  StackTag = retagTargetTag(IRB, StackTag);
+
----------------
This mask of the base tag seems unnecessary since we also mask the final tag.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1045
 
-// Add a tag to an address.
-Value *HWAddressSanitizer::tagPointer(IRBuilder<> &IRB, Type *Ty,
-                                      Value *PtrLong, Value *Tag) {
-  assert(!UsePageAliases);
+Value *HWAddressSanitizer::getTargetTagPtr(IRBuilder<> &IRB, Value *PtrLong,
+                                           Value *Tag) {
----------------
It seems like overkill to move the logic into this function.  Let's leave it as part of `tagPointer`.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1055
+      Mask = ConstantInt::get(IntptrTy, ((1ULL << kPointerTagShift) - 1) |
+                                            (1ULL << (64 - 1)));
+    else
----------------
Let's avoid implementing the kernel case for LAM, since we don't have a good way to test yet.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1534
     // Skip tag 0 in order to avoid collisions with untagged memory.
+    Tag &= TagMaskByte;
     if (Tag == 0)
----------------
Let's avoid doing this for now.  I'd like to focus just on stack support in this patch.  Future patches can address global support.


================
Comment at: llvm/test/Instrumentation/HWAddressSanitizer/X86/stack.ll:5
+
+source_filename = "stack.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
----------------
Instead of making a new test, can we reuse the alloca tests for aarch64 and update the expected instrumentation?


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

https://reviews.llvm.org/D102901