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

[Evgeniy Stepanov]

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:39
@@ +38,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+THREADLOCAL u64 __msan_param_tls[100];
+
----------------
Alexander Potapenko wrote:
> Is 100 here and below the same constant? If so, make it a const int or #define please.
Just a random number, big enough for any function's arguments.
In fact, we can check it during compilation (and for varargs, in runtime). I'll turn it into a named constant for now.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:120
@@ +119,3 @@
+  f->poison_in_malloc = true;
+  f->exit_code = 67;
+  f->num_callers = 20;
----------------
Alexander Potapenko wrote:
> Why 67?
Made it 77, for 'M'. Just want it to be unique and noticable.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:274
@@ +273,3 @@
+    PrintCurrentStackTrace(pc, bp);
+    Die();
+  }
----------------
Alexander Potapenko wrote:
> Is it really a fatal error if an expected UMR doesn't occur?
It's OK for the tests (as well as the whole keep-going mode). We might want to change this later,

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_allocator.cc:42
@@ +41,3 @@
+
+void Init() {
+  if (inited) return;
----------------
Alexander Potapenko wrote:
> I wonder if it makes sense to make Init() an inline function.
And definitely - static.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:34
@@ +33,3 @@
+  if (!msan_inited) { \
+    __msan_init(); \
+  } \
----------------
Alexander Potapenko wrote:
> Is it enough to just call __msan_init without initializing the allocator and setting |inited|?
> It's somewhat confusing that some functions start with a call to Init() and others use ENSURE_MSAN_INITED()
I think so. ENSURE_MSAN_INITED ensures that shadow is mapped, flags are parsed, etc. Allocator initializes both itself and the stuff listed above at the first call to an allocation function.


================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:276
@@ +275,3 @@
+INTERCEPTOR(int, sprintf, char *str, const char *format, ...) {  // NOLINT
+  ENSURE_MSAN_INITED();
+  va_list ap;
----------------
Alexander Potapenko wrote:
> Note it's not really necessary to init MSan here (though it's better to keep it for consistency)
We do it in all interceptors, for consistency.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:490
@@ +489,3 @@
+  if (!res)
+    __msan_unpoison(pipefd, sizeof(int[2]));
+  return res;
----------------
Alexander Potapenko wrote:
> sizeof(pipefd)
Better not ;)

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:586
@@ +585,3 @@
+
+INTERCEPTOR(int, fstatfs64, int fd, void* buf) {
+  ENSURE_MSAN_INITED();
----------------
Alexander Potapenko wrote:
> How about declaring a common macro for such dummy interceptors?
Example?


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