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

[Evgeniy Stepanov via llvm-commits]

eugenis added a comment.

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


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

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


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



http://reviews.llvm.org/D15607