[PATCH] D148596: [KMSAN] Enable on SystemZ

[Alexander Potapenko via Phabricator via cfe-commits]

glider added inline comments.


================
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)
----------------
iii wrote:
> 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.
Ahh, pardon my ignorance, I've had no idea s390x is doing that fancy stuff with a hidden parameter.
Perhaps you could elaborate on it a bit more in the patch description? (I already educated myself reading the s390x ABI spec and playing with Godbolt, but still).

It might be still worth mentioning in this comment that for s390x we emulate the ABI manually.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:746
+                                                 ArgsTy... Args) {
+  if (TargetTriple.getArch() == Triple::systemz) {
+    return M.getOrInsertFunction(Name, Type::getVoidTy(*C),
----------------
iii wrote:
> 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.
Ok, sounds good.


================
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);
----------------
iii wrote:
> 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.
Ok, got it. Given that this is actually the desired way to pass the struct parameters in SystemZ, it should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148596