[PATCH] D15607: [sanitizer] [msan] Fix origin store of array types

[Adhemerval Zanella via llvm-commits]

zatrazz added a comment.

In http://reviews.llvm.org/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.


================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:728
@@ +727,3 @@
+                ConvertedShadow, i);
+            Value *CleanShadowElem = IRB.CreateExtractValue(CleanShadow, i);
+            Value *Cmp = IRB.CreateICmpNE(
----------------
eugenis wrote:
> getCleanShadow(ConvertedShadowElem)
I will change that.

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:737
@@ +736,3 @@
+            IRBuilder<> IRBNew(CheckTerm);
+            paintOrigin(IRBNew, updateOrigin(Origin, IRBNew),
+                        getOriginPtr(Addr, IRBNew, Alignment), StoreSize,
----------------
eugenis wrote:
> Do you need to update Addr here?
> Move updateOrigin call out of the loop.
> 
I will change that.

================
Comment at: test/Instrumentation/MemorySanitizer/origin-array.ll:41
@@ +40,3 @@
+  %1 = bitcast %struct.mypair* %z to [2 x i64]*
+  store [2 x i64] %call, [2 x i64]* %1, align 8
+  %x = getelementptr inbounds %struct.mypair, %struct.mypair* %z, i32 0, i32 0
----------------
eugenis wrote:
> 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.
> 
I will simplify the testcase. I created this based on the original bug trigger (test/msan/insertvalue_origin.cc).


http://reviews.llvm.org/D15607