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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added a comment.

Thanks for the update.  I'll take a closer look tomorrow and try testing locally.  You can also use the new buildbot script to test in QEMU (see inline comment below).

@eugenis @vitalybuka If you could take a look as well that would be helpful.  I'm not too familiar with the stack retagging instrumentation.



================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:190
+                                    cl::desc("untag mem operate"), cl::Hidden,
+                                    cl::init(false));
+
----------------
morehouse wrote:
> xiangzhangllvm wrote:
> > morehouse wrote:
> > > 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.
> > Yes, of course, we can. But here ClUntagPointer is disabled in default (false), no affect our current tests.
> > I and back this option, because I find It is very helpful for me in developing HWASAN without hardware or simulator supported. I can run most simple test on my local machine.
> It may be inconvenient for now, but as part of setting up a buildbot I'm also automating the process of setting up the custom QEMU and kernel required to test LAM.  If all goes well, it should land later this week or next week.  Then you can use that instead of the `ClUntagPointer` option.
> 
> For now, could you keep the `ClUntagPointer` patch locally for your testing purposes, and remove it from the patch we actually submit?
Thanks.  FYI, the buildbot is now live:  https://lab.llvm.org/buildbot/#/builders/169

You can test locally using the buildbot script [here](https://github.com/llvm/llvm-zorg/blob/main/zorg/buildbot/builders/sanitizers/buildbot_qemu.sh).  You can comment out the Scudo stuff to avoid testing that.

You'll need a QEMU image located at `/b/qemu_image`.  You can build one using [this script](https://github.com/google/sanitizers/blob/master/hwaddress-sanitizer/create_qemu_image.sh).


================
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));
----------------
morehouse wrote:
> 
This comment wasn't addressed.


================
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"
----------------
morehouse wrote:
> Instead of making a new test, can we reuse the alloca tests for aarch64 and update the expected instrumentation?
I see this test is gone now, but I don't see any alloca tests copied into the X86 directory.  Maybe you forgot to add them in the patch?


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

https://reviews.llvm.org/D102901