[sanitizer] [msan] Fix origin store of array types

[zatrazz]

Summary

This patch fixes the memory sanitizer origin store instrumentation for
array types. This can be triggered by cases where frontend lowers
function return to array type instead of aggregation.

For instance, the C code:

---

struct mypair {
int64_t x;
int y;
};

mypair my_make_pair(int64_t x, int y) {
mypair p;
p.x = x;
p.y = y;
return p;
}

int foo (int p)
{

mypair z = my_make_pair(p, 0);
return z.y + z.x;

}

It will be lowered with target set to aarch64-linux and -O0 to:

---

[...]
define i32 @_Z3fooi(i32 %p) #0 {
[...]
%call = call [2 x i64] @_Z12my_make_pairxi(i64 %conv, i32 0)
%1 = bitcast %struct.mypair* %z to [2 x i64]*
store [2 x i64] %call, [2 x i64]* %1, align 8

[...]

The origin store will emit a 'icmp' to test each store value again the
TLS origin array. However since 'icmp' does not support ArrayType the
memory instrumentation phase will bail out with an error.

This patch change it by extracting and applying the 'icmp' operation on
each array element.

It fixes the 'test/msan/insertvalue_origin.cc' for aarch64 (the -O0 case).

[eugenis]

The origin store will emit a 'icmp' to test each store value again the

TLS origin array.
Are you sure? It is supposed to test against a "zeroinitializer" constant.

Origin is 4 byte aligned, so this code would not work correctly for [i16 x 2], for example.
There is a function in compiler-rt that does this sort of thing correctly for larger arrays (in memcpy, for example), but calling it for all array stores would be too slow.
This patch does not do anything for StructType (line 695), which is a very similar case.

To do this 100% correctly, one would bitcast the shadow to a long integer, call paintOrigin for the 4-aligned part of the 4-aligned size in the middle of the store location, and conditionally (on the corresponding parts of the shadow being non-zero) store origin for the left and right partial quads.

I'd suggest not going there until we have evidence that it matters.

The simplest fix would be to bitcast the shadow to a long integer before the icmp. While you are at it, wrap the StructType case into this branch as well.

lib/Transforms/Instrumentation/MemorySanitizer.cpp

728

getCleanShadow(ConvertedShadowElem)

737

Do you need to update Addr here?
Move updateOrigin call out of the loop.

test/Instrumentation/MemorySanitizer/origin-array.ll

42

Does this test really need all this code?
Just write one function that accepts [2 x i64] %v, [2 x i64]* %p and stores one to the other.

[zatrazz]

In D15607#313344, @eugenis wrote:

The origin store will emit a 'icmp' to test each store value again the

TLS origin array.
Are you sure? It is supposed to test against a "zeroinitializer" constant.

My mistake here, you are right.

Origin is 4 byte aligned, so this code would not work correctly for [i16 x 2], for example.
There is a function in compiler-rt that does this sort of thing correctly for larger arrays (in memcpy, for example), but calling it for all array stores would be too slow.
This patch does not do anything for StructType (line 695), which is a very similar case.

To do this 100% correctly, one would bitcast the shadow to a long integer, call paintOrigin for the 4-aligned part of the 4-aligned size in the middle of the store location, and conditionally (on the corresponding parts of the shadow being non-zero) store origin for the left and right partial quads.

I'd suggest not going there until we have evidence that it matters.

Right, In fact I noted at least for aarch64 only very specific vector types are generated from functions that return aggregated type (test/msan/insertvalue_origin.cc is an example and change the struct size also changes clang IR by not using a vector type as well).

The simplest fix would be to bitcast the shadow to a long integer before the icmp. While you are at it, wrap the StructType case into this branch as well.

Well the problem is I do not think it is possible to cast the shadow to a long integer with the shadow being a array type. The problem is bitcast explicit forbid cast from aggregate types:

lib/IR/Instructions.cpp:

3019 bool
3020 CastInst::castIsValid(Instruction::CastOps op, Value *S, Type *DstTy) {
3021 
3022   // Check for type sanity on the arguments
3023   Type *SrcTy = S->getType();
3024 
3025   if (!SrcTy->isFirstClassType() || !DstTy->isFirstClassType() ||
3026       SrcTy->isAggregateType() || DstTy->isAggregateType())
3027     return false;

I tried to simple cast the converted shadow to a long integer and it bails with an invalid cast due 'SrcTy->isAggregateType()' being true (and it will be true as well for struct types).

I will update the patch with the comments you suggested.

lib/Transforms/Instrumentation/MemorySanitizer.cpp

728

I will change that.

737

I will change that.

test/Instrumentation/MemorySanitizer/origin-array.ll

42

I will simplify the testcase. I created this based on the original bug trigger (test/msan/insertvalue_origin.cc).

[zatrazz]

Changes from previous versions:

• Use getCleanShadow on the extracted elements;
• Moved updateOrigin call out of the loop;
• Simplified the testcase.

[eugenis]

lib/Transforms/Instrumentation/MemorySanitizer.cpp

733

This code seems to build nested if() blocks, effectively testing that all shadow elements are non-null. This is wrong, it should test that any of those is non-null. And even that is a rough approximation.

Would it be enough to change the isa<StructType> test above to isAggregateType() ?

[zatrazz]

Changes from previous version:

• Drop the extract array logic in favor or handling as the struct type.

[eugenis]

LGTM

lib/Transforms/Instrumentation/MemorySanitizer.cpp

     const DataLayout &DL = F.getParent()->getDataLayout();
     unsigned OriginAlignment = std::max(kMinOriginAlignment, Alignment);
     unsigned StoreSize = DL.getTypeStoreSize(Shadow->getType());
-    if (isa<StructType>(Shadow->getType())) {
+    if (Shadow->getType()->isAggregateType()) {
       paintOrigin(IRB, updateOrigin(Origin, IRB),
                   getOriginPtr(Addr, IRB, Alignment), StoreSize,
                   OriginAlignment);

test/Instrumentation/MemorySanitizer/origin-array.ll

+; RUN: opt < %s -msan -msan-check-access-address=0 -msan-track-origins=2 -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
+; Check origin handling of array types.
+
+define void @foo([2 x i64] %v, [2 x i64]* %p) sanitize_memory {
+entry:
+  store [2 x i64] %v, [2 x i64]* %p, align 8
+  ret void
+}
+
+; CHECK-LABEL: @foo
+; CHECK: [[PARAM:%[01-9a-z]+]] = load {{.*}} @__msan_param_tls
+; CHECK: [[ORIGIN:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls
+
+; CHECK: [[TMP1:%[01-9a-z]+]] = ptrtoint
+; CHECK: [[TMP2:%[01-9a-z]+]] = xor i64 [[TMP1]]
+; CHECK: [[TMP3:%[01-9a-z]+]] = inttoptr i64 [[TMP2]] to [2 x i64]*
+; CHECK: store [2 x i64] [[PARAM]], [2 x i64]* [[TMP3]]
+
+; CHECK: {{.*}} call i32 @__msan_chain_origin(i32 {{.*}}[[ORIGIN]])