[llvm-commits] [PATCH] [msan] Heuristic handling of unknown intrinsics

[Timur Iskhodzhanov]

  Pre-word: I'm not familiar with the MSan code, nor am I familiar with LLVM passes =P


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1212
@@ +1211,3 @@
+  void handleUnknownIntrinsic(IntrinsicInst &I) {
+    // TODO: handle struct types in CreateShadowCast
+    if (hasStructArgumentOrRetVal(I)) {
----------------
Make it clearer that this is not implemented yet.
Maybe but the // TODO inside the {} ?

Use FIXME rather than TODO ?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1242
@@ +1241,3 @@
+        } else {
+          // Two pointer operands? Meh.
+          visitInstruction(I);
----------------
Do you know which intrinsics have this behavior?
Can we put an assertion here?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1239
@@ +1238,3 @@
+      if (Op->getType()->isPointerTy()) {
+        if (pointerOpIdx < 0) {
+          pointerOpIdx = i;
----------------
(Optional) invert the order of if/else and you'll get

```
  if (pointerOpIds >= 0) {
    // ???
    return;
  }
  <normal code>
```

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1247
@@ +1246,3 @@
+      } else if (!MemAccessType) {
+        MemAccessType = Op->getType();
+      }
----------------
This implies that the first non-pointer operand defines the access type.
Are there intrinsics where this gives us wrong results (i.e. intrinsics with more than one non-pointer operands)?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1280
@@ +1279,3 @@
+    for (int i = 0, n = I.getNumArgOperands(); i < n; ++i) {
+      // For nomem intrinsics, we mix in the shadow of the pointer argument,
+      // as well.
----------------
Suggestion/nit:
"We also mix in the shadow of the pointer argument for nomem instrinsics."
?
Will fit into one line I think.

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1294
@@ +1293,3 @@
+        } else {
+          assert(Shadow);
+          Value *S = convertToShadowTyNoVec(Shadow, IRB);
----------------
Nonsense assert or you can move it up (outside if's)

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1289
@@ +1288,3 @@
+        Shadow = IRB.CreateOr(Shadow, OpShadow, "_msprop");
+      if (ClTrackOrigins) {
+        Value* OpOrigin = getOrigin(Op);
----------------
Do you have code like this (OR'ing N values, their shadows and origins) reused in many places?
Maybe extract into a function?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1318
@@ +1317,3 @@
+        } else {
+          assert(Shadow);
+          Value *S = convertToShadowTyNoVec(Shadow, IRB);
----------------
ditto re: moving assert

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1327
@@ +1326,3 @@
+    assert(Shadow);
+    if (ClTrackOrigins) assert(Origin);
+
----------------
assert(!ClTrackOrigins || Origin) ?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1329
@@ +1328,3 @@
+
+    // Store shadow by the pointer argument.
+    if (pointerOpIdx >= 0 && writesMemory) {
----------------
Store shadow for the memory referenced by the pointer argument?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:979
@@ +978,3 @@
+    return IRB.CreateBitCast(V2, dstTy);
+    // TODO: handle struct types.
+  }
----------------
add assertion?


http://llvm-reviews.chandlerc.com/D184