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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:190
+                                    cl::desc("untag mem operate"), cl::Hidden,
+                                    cl::init(false));
+
----------------
morehouse wrote:
> 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).
Thanks for your work! let me do a try.


================
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:
> morehouse wrote:
> > 
> This comment wasn't addressed.
Oh, yes, I should use "hwasan-experimental-use-page-aliases", thank you very much!


================
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:
> 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?
Did you mean the llvm/test/Instrumentation/HWAddressSanitizer/alloca.ll ? 
It already at the parent directory of this test (llvm/test/Instrumentation/HWAddressSanitizer/X86)
So, the check-all can test it.


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

https://reviews.llvm.org/D102901