[llvm-commits] [PATCH] [msan] MemorySanitizer runtime

[Evgeniy Stepanov]

================
Comment at: llvm/projects/compiler-rt/include/sanitizer/msan_interface.h:44
@@ +43,3 @@
+
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_unpoison(void *a, uptr size);
----------------
Alexander Potapenko wrote:
> It's good to have comments for all the interface functions (a FIXME is fine for now).
> Also, are __msan_memset and __msan_memcpy interface functions?
Fixme it is.
Yes, they are interface functions - they are emitted by the compiler.

================
Comment at: llvm/projects/compiler-rt/include/sanitizer/msan_interface.h:22
@@ +21,3 @@
+using __sanitizer::u32;
+
+#ifdef __cplusplus
----------------
Alexander Potapenko wrote:
> How about adding a compile-time check for sizeof(uptr) (or __LP64__) to ensure we're building for 64 bits?
> Not sure if it should be in this particular file (probably yes, since it's included everywhere).
I don't think it's needed. Build system makes sure we only build it for x86_64.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:70
@@ +69,3 @@
+
+static bool IsRunningUnderDr() {
+  bool result = false;
----------------
Alexander Potapenko wrote:
> Why isn't this in the __msan namespace?
moved

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:188
@@ +187,3 @@
+  }
+  if (__msan::flags()->exit_code >= 0) {
+    Printf("Exiting\n");
----------------
Alexander Potapenko wrote:
> Are you going to have negative exit codes? I don't think it's a good idea.
> If no, it's better to sanitize the exit code when you parse the flags and call MsanDie here.
OK, I disabled the "keep-going" effect of the exit_code flag. Now the fatality of msan warning is controlled by the compile-time flag only. exit_code is verified to be in [0, 128).

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:423
@@ +422,3 @@
+  char *res = REAL(fcvt)(x, a, b, c);
+  if (!__msan_has_dynamic_component()) {
+    __msan_unpoison(b, sizeof(*b));
----------------
Alexander Potapenko wrote:
> It's inevident why you unpoison memory only in the absence of a dynamic component here and below.
> Please add a comment.
There is already such comment in gcvt interceptor above.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:613
@@ +612,3 @@
+  if (res > 0) {
+    __msan_unpoison(events, __msan::struct_epoll_event_sz * res);
+  }
----------------
Alexander Potapenko wrote:
> Please remove the space after "*"
That's a different kind of "*"

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:659
@@ +658,3 @@
+  if (!msan_inited) {
+    // Hack: dlsym calls calloc before REAL(calloc) is retrieved from dlsym.
+    const size_t kCallocPoolSize = 1024;
----------------
Alexander Potapenko wrote:
> You can use LowLevelAlloc instead (I don't insist since we have the same in ASan now)
Let's keep it in sync with ASan.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:794
@@ +793,3 @@
+  void *res = fast_memset(s, c, n);
+  if (MEM_TO_SHADOW((uptr)s) != (uptr)s)
+    __msan_unpoison(s, n);
----------------
Alexander Potapenko wrote:
> Please add an assertion or (if it's possible that MEM_TO_SHADOW((uptr)s) == (uptr)s) a comment.
In fact, __msan_inpoison takes care of that. Removed the check.


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