Page MenuHomePhabricator

[MSan] Add instrumentation for SystemZ
ClosedPublic

Authored by iii on Mar 23 2020, 9:37 AM.

Details

Summary

This patch establishes memory layout and adds instrumentation. It does
not add runtime support and does not enable MSan, which will be done
separately.

Memory layout is based on PPC64, with the exception that XorMask
is not used - low and high memory addresses are chosen in a way that
applying AndMask to low and high memory produces non-overlapping
results.

VarArgHelper is based on AMD64. It might be tempting to share some
code between the two implementations, but we need to keep in mind that
all the ABI similarities are coincidental, and therefore any such
sharing might backfire.

copyRegSaveArea() indiscriminately copies the entire register save area
shadow, however, fragments thereof not filled by the corresponding
visitCallSite() invocation contain irrelevant data. Whether or not this
can lead to practical problems is unclear, hence a simple TODO comment.
Note that the behavior of the related copyOverflowArea() is correct: it
copies only the vararg-related fragment of the overflow area shadow.

VarArgHelper test is based on the AArch64 one.

s390x ABI requires that arguments are zero-extended to 64 bits. This is
particularly important for msan_maybe_warning_*() and
msan_maybe_store_origin_*() shadow and origin arguments, since non
zeroed upper parts thereof confuse these functions. Therefore, add ZExt
attribute to the corresponding parameters.

Add ZExt attribute checks to msan-basic.ll. Since with
-msan-instrumentation-with-call-threshold=0 instrumentation looks quite
different, introduce the new CHECK-CALLS check prefix.

Diff Detail

Event Timeline

iii created this revision.Mar 23 2020, 9:37 AM
eugenis added inline comments.Mar 23 2020, 11:40 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
815

Add a test for zext attribute somewhere, ex. in msan_basic.ll

llvm/test/Instrumentation/MemorySanitizer/SystemZ/vararg.ll
97

CHECK-LABEL: @many_args

vitalybuka resigned from this revision.Mar 23 2020, 1:31 PM

LGTM

iii updated this revision to Diff 252385.Mar 24 2020, 11:48 AM

Added a test for zext attribute to msan_basic.ll, added missing CHECK-LABEL: @many_args.

iii edited the summary of this revision. (Show Details)Mar 24 2020, 11:49 AM
iii updated this revision to Diff 252418.Mar 24 2020, 1:35 PM
iii edited the summary of this revision. (Show Details)

Fixed a typo in vararg.ll test.

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

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
4639

typo?

4664

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();
4666

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).

4671

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).

4698

Might be best to add an ArgAllocSize <= ArgSize assertion here.

4718

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.)

iii marked 8 inline comments as done.Apr 1 2020, 5:12 PM
iii added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
4664

No, we get the original (non-coerced) type here. The offset calculation is still correct in presence of type coercion, because the slot size is always 8, but the code currently lacks instrumentation that would fill the shadow for the extensions bits. I'll add it.

4666

I couldn't find a way to query floating point ABI, so I'll use the indirect way to detect this and assume that userspace is always hardfloat, and kernel is always softfloat.

iii updated this revision to Diff 254374.Apr 1 2020, 5:19 PM
iii marked 2 inline comments as done.

Implemented Ulrich's suggestions.

iii updated this revision to Diff 254629.Apr 2 2020, 3:02 PM

Resolved shadow zext/sext TODO using CreateShadowCast.
Resolved softfloat TODO using getFnAttribute("use-soft-float").
Added shadow propagation checks to tests.

iii marked 2 inline comments as done.Apr 2 2020, 3:04 PM

The Z ABI implementation now looks correct to me, except for one corner case: an LLVM-level argument type of f128 is passed via implicit reference. Now, in most cases this is already handled at the clang level, i.e. when you have a "long double" at C source level, you'll already see a pointer type in the LLVM IR instead. However, there are still a few cases where there is a f128 at the LLVM IR level, e.g. as arguments to some compiler builtin / libgcc routines. Those are only transformed to implicit reference in the LLVM back-end, so I believe for completeness this case should also be handled here.

iii updated this revision to Diff 256115.Apr 8 2020, 2:37 PM

Added fp128 handling and a test for it.

iii updated this revision to Diff 256220.Apr 9 2020, 2:35 AM

Added i128 handling and a test for it.

Just a minor cosmetic suggestion inline. Otherwise the Z ABI parts now all LGTM.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
4669

The size check now seems redundant with the i128 check. OTOH maybe it would be preferable to move the fp128 and i128 checks to here and return a new ArgKind::Indirect -- that way all the type classification checks would be in one place.

iii updated this revision to Diff 256240.Apr 9 2020, 4:08 AM

Introduced ArgKind::Indirect.

iii marked an inline comment as done.Apr 9 2020, 4:08 AM
iii added a comment.Apr 9 2020, 4:59 AM

@eugenis now that VarArgSystemZHelper part seems to be in order, could you please have another look?

eugenis accepted this revision.Apr 9 2020, 11:14 AM

LGTM

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
397

wrong indent here and below

This revision is now accepted and ready to land.Apr 9 2020, 11:14 AM
iii marked an inline comment as done.Apr 9 2020, 12:14 PM
iii added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
397

I tried changing this, but the result doesn't pass arcanist linter. Apparently it thinks that the existing indentation is wrong.

eugenis marked an inline comment as done.Apr 9 2020, 12:29 PM

LGTM

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
397

Ha, you (and arcanist) are correct, this is actually the right indentation.
I don't want to reformat the entire file at once because it would mess with the history.

This revision was automatically updated to reflect the committed changes.