[PATCH] D148596: [KMSAN] Enable on SystemZ

[Alexander Potapenko via Phabricator via cfe-commits]

glider added a comment.

Do you have a working kernel patch for SystemZ?
I think we'd better name the functions taking the extra parameter differently - maybe at some point we'll even want them to coexist on certain architectures. Anyway, it might just look cleaner in the kernel code.



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:118
 ///    (X=1,2,4,8) are accessed through pointers obtained via the
 ///      __msan_metadata_ptr_for_load_X(ptr)
 ///      __msan_metadata_ptr_for_store_X(ptr)
----------------
You are changing KMSAN API, so this comment needs to be updated.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:138
 ///
 ///  KernelMemorySanitizer only supports X86_64 at the moment.
 ///
----------------
Please update this comment as well.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:746
+                                                 ArgsTy... Args) {
+  if (TargetTriple.getArch() == Triple::systemz) {
+    return M.getOrInsertFunction(Name, Type::getVoidTy(*C),
----------------
Can we add a bool to MemorySanitizerPass to indicate whether or not we want to return the metadata as a pointer?
We'll then set it based on Triple::systemz once and check everywhere, making the code more readable.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1492
                       {Zero, IRB.getInt32(6)}, "retval_origin");
+    if (MS.TargetTriple.getArch() == Triple::systemz)
+      MS.MsanMetadataAlloca = IRB.CreateAlloca(MS.MsanMetadata, (unsigned)0);
----------------
Have you considered using per-CPU data instead of this alloca (there might be pitfalls as well, so just curious).
Same question for passing a single struct vs. two separate allocas for shadow/origin.


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/SystemZ/basic-kernel.ll:6
+
+define void @Store1(ptr %p, i8 %x) sanitize_memory {
+entry:
----------------
This test should cover all the newly introduced API functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148596