[PATCH] [msan] MSanDR: a simple DynamoRio-based tool that handles uninstrumented libraries and dynamically generated code

[Evgeniy Stepanov]

  I've added a README with full build instructions (including DR and DrM). It should be enough to get people started, provided they are on x86_64 linux.

  MSanDR plays the role of a runtime support library for MSan. Unlike the rest of compiler-rt, it depends on some heavyweight external libraries. At the moment it also has a weird way of "binding" to the executable (through a shell script), but that will change soon when we start linking it into the executable.

  Compiler-rt seems like the right place for such library.


================
Comment at: lib/msandr/msandr.cc:12
@@ +11,3 @@
+//
+// DynamoRio client for MemorySanitizer.
+//===----------------------------------------------------------------------===//
----------------
Kostya Serebryany wrote:
> This deserves a detailed comment (and/or a link) describing why we needs this.
done

================
Comment at: lib/msandr/msandr.cc:28
@@ +27,3 @@
+
+using std::string;
+
----------------
Reid Kleckner wrote:
> Evgeniy Stepanov wrote:
> > Kostya Serebryany wrote:
> > > Can we avoid using STL?
> > > It may give us lots of trouble with DR's native exec. 
> > The real question here is why does it work at all? :)
> > Probably because we never have both instrumented libc++/libstdc++ and run under MSanDR at the same time. With native_exec _and_ a statically linked client and DR we are probably fine, too, because all of them will live in high addresses. With native_exec and dynamic linking we might be in trouble.
> For regular (no -native_exec) dynamic DR, this Just Works because we spent all that effort on our private loader.  :)
> 
> For dynamic DR with -native_exec (not ready yet anyway), I don't think we'll be able to use the private loader and the segment swapping it does.  Any STL or libc call can then clobber errno, which is a big problem.
> 
> With STATIC_LIBRARY, I have no idea why it works.  I think you'll end up using the app's STL copy which will allocate on the app heap, probably using the msan allocator, which is bad for isolation.
> 
> So, since it works with dynamic DR right now, and apparently it works somewhat with static DR, we can probably commit as-is, but we should exorcise STL stuff eventually.
I agree that getting rid of STL is the right thing to do, and is not that hard, but I'd prefer to do this work on a versioned file, and land in a separate commit.

================
Comment at: lib/msandr/msandr.cc:43
@@ +42,3 @@
+
+#if defined(VERBOSE_VERBOSE) && !defined(VERBOSE)
+#define VERBOSE
----------------
Kostya Serebryany wrote:
> Can this be VERBOSITY_LEVEL, which is one of 0, 1, 2 so that you use if () instead of #ifdef  ?
done

================
Comment at: lib/msandr/msandr.cc:288
@@ +287,3 @@
+  // TODO: unpoison more bytes?
+  PRE(i, mov_st(drcontext, OPND_CREATE_MEM64(DR_REG_XAX, msan_param_tls_offset),
+                OPND_CREATE_INT32(0)));
----------------
Reid Kleckner wrote:
> I guess msan is not 32-bit ready?  I still like for loop + OPND_CREATE_MEMPTR + i * sizeof(void*) better.
Yes, we don't care about 32 bits for now.
Replaced with a loop.

================
Comment at: lib/msandr/msandr.cc:362
@@ +361,3 @@
+
+// TODO(rnk): Make sure we instrument after __asan_init.
+dr_emit_flags_t
----------------
Kostya Serebryany wrote:
> __asan ? 
__msan

================
Comment at: lib/msandr/msandr.cc:416
@@ +415,3 @@
+    if (opcode == OP_call_ind || opcode == OP_call_far_ind ||
+        opcode == OP_jmp_ind || opcode == OP_jmp_far_ind) {
+      InstrumentIndirectBranch(drcontext, bb, i);
----------------
Reid Kleckner wrote:
> Hrm, tail calls through the PLT?  This should probably have a comment about why we include these.
Added a comment.
At a first sight, we need all of those. There may be some optimization opportunities, but they are probably low priority since we expect a vast majority of bb executions to be covered by compiler instrumentation.


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