[PATCH] D148596: [KMSAN] Enable on SystemZ

[Ilya Leoshkevich via Phabricator via cfe-commits]

iii added a comment.

The kernel patch is currently WIP; I'm trying to get rid of false positives.
Just some examples of the findings so far:

- s390x always uses separate address spaces for kernel and userspace, so we need to skip the `addr < TASK_SIZE` check in `is_bad_asm_addr()`.
- There is a 4k asm store, so the limit in `__msan_instrument_asm_store()` needs to be bumped.

There were a couple build issues with the common code too:

- `_PAGE_NX` is called `_PAGE_NOEXEC` on s390x
- `asm/pgtable_64_types.h` is called `asm/pgtable.h` on s390x



================
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)
----------------
glider wrote:
> You are changing KMSAN API, so this comment needs to be updated.
The API stays exactly the same here; I haven't touched kernel function signatures.

What changes here is that I've implemented the s390x ABI for the KMSAN API.


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


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:746
+                                                 ArgsTy... Args) {
+  if (TargetTriple.getArch() == Triple::systemz) {
+    return M.getOrInsertFunction(Name, Type::getVoidTy(*C),
----------------
glider wrote:
> 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.
We could do that, but that's really the detail of the s390x ABI. Other platforms might have different conventions for large return values. That's why I didn't want to introduce generic concepts for this and highlight that it's s390x specific, where possible.


================
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);
----------------
glider wrote:
> 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.
Yes, but I was wondering if there was any benefit? I thought that maybe that would save a frame for leaf functions, but since we are calling KMSAN API, we already need one (and technically they are not leaf functions anymore).

Passing two separate allocas would actually change the API, as opposed to implementing the new ABI, so I wanted to avoid that for now, even if it were more efficient.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148596