[PATCH] D76624: [MSan] Add instrumentation for SystemZ

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

Had a look at the vararg handling.  Reviewed only the ABI-relevant parts.



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:4637
+  static const unsigned SystemZFpEndOffset = 160;
+  static const unsigned SysteZRegSaveAreaSize = 160;
+  static const unsigned SystemZOverflowOffset = 160;
----------------
typo?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:4662
+    // of what it can be. In particular, single element structs and large
+    // types have already been taken care of.
+    if (T->isFloatingPointTy())
----------------
Is argument type coercion also taken care of?  This is about the implicit promotion to 64 bits.

For example, a 32-bit integer is passed as zero- or sign-extended 64-bit value, so all 8 bytes of the slot in either the GPR save area or overflow area are filled by the caller and may be validly accessed by the callee.

In clang's SystemZABIInfo::EmitVAArg this is done by this call:
    if (AI.getCoerceToType())
      ArgTy = AI.getCoerceToType();



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:4664
+    if (T->isFloatingPointTy())
+      return AK_FloatingPoint;
+    if (T->isIntegerTy() && T->getPrimitiveSizeInBits() <= 64)
----------------
We recently added -msoft-float support in particular to support kernel builds.

I guess this also needs to be supported here ... (in soft-float mode, floating-point arguments are passed in GPRs).


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:4669
+      return AK_GeneralPurpose;
+    return AK_Memory;
+  }
----------------
What about vector arguments?   The variable arguments of vector type will always be in memory, but the fixed portion of the argument list can still include vector type arguments that would be passed in vector registers (and not via memory, so your memory overflow offset would be off).


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:4696
+          if (!IsFixed) {
+            uint64_t ArgAllocSize = DL.getTypeAllocSize(A->getType());
+            ShadowBase = getShadowPtrForVAArgument(
----------------
Might be best to add an ArgAllocSize <= ArgSize assertion here.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:4716
+            ShadowBase = getShadowPtrForVAArgument(
+                A->getType(), IRB, FpOffset + (ArgSize - ArgAllocSize));
+            if (MS.TrackOrigins)
----------------
Actually, 32-bit floats are passed in the **high** part of the register, so there should be no offset here.  (This applies only for the in-memory FPR copies.  For floating-point arguments passed in the overflow area, the offset is there.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76624