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

Alexey Samsonov samsonov at google.com
Fri Dec 7 15:20:31 PST 2012 

  Mostly general and style comments. I haven't learned the algorithm well enough yet...


================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_platform_limits.h:29
@@ +28,3 @@
+
+  void* __msan_get_msghdr_iov_iov_base(void* msg, int idx);
+  uptr __msan_get_msghdr_iov_iov_len(void* msg, int idx);
----------------
Why do you need __msan prefix? Looks like there are internal msan functions.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_platform_limits.cc:1
@@ +1,2 @@
+//===-- msan_platform_limits.cc -------------------------------------------===//
+//
----------------
This file is Linux- (or Posix-) specific. Probably, we should rename it and wrap in #ifdef __linux__ macro?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:32
@@ +31,3 @@
+
+uptr ReadFromFile(const char *path, char *buff, uptr size);
+bool ProtectRange(uptr beg, uptr end);
----------------
There is ReadFileToBuffer() function in sanitizer_common.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:34
@@ +33,3 @@
+bool ProtectRange(uptr beg, uptr end);
+void CatProcSelfMaps();
+bool InitShadow(bool prot1, bool prot2, bool map_shadow, bool init_origins);
----------------
I wonder if you can use DumpProcessMap() in sanitizer_common

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:33
@@ +32,3 @@
+uptr ReadFromFile(const char *path, char *buff, uptr size);
+bool ProtectRange(uptr beg, uptr end);
+void CatProcSelfMaps();
----------------
This function is not used.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:45
@@ +44,3 @@
+
+bool StackIsUnlimited();
+void SetSaneStackLimit();
----------------
Function is not used

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:46
@@ +45,3 @@
+bool StackIsUnlimited();
+void SetSaneStackLimit();
+void MsanDie();
----------------
ditto

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:52
@@ +51,3 @@
+// Flags.
+struct Flags {
+  bool poison_heap_with_zeroes;  // default: false
----------------
Consider moving this into separate header and providing more meaningful comments, as well as default values. This is required for TSan. For ASan we use different strategy for overriding flags in the user program, but it would probably help anyway.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.h:59
@@ +58,3 @@
+  bool report_umrs;
+  bool verbosity;
+};
----------------
Why bool?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:26
@@ +25,3 @@
+
+#include <interception/interception.h>
+
----------------
Um, why <> instead of "" here?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:42
@@ +41,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+THREADLOCAL u32       __msan_param_origin_tls[100];
+
----------------
why so many spaces?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:63
@@ +62,3 @@
+
+StaticSpinMutex report_mu;
+
----------------
Should StaticSpinMutex be static? :)

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:48
@@ +47,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+THREADLOCAL u32       __msan_retval_origin_tls;
+
----------------
ditto

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:57
@@ +56,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+THREADLOCAL u32       __msan_origin_tls;
+
----------------
ditto

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:85
@@ +84,3 @@
+};
+int msan_inited = 0;
+bool msan_init_is_running;
----------------
Is it really safe to explicitly initialize this? Can __msan_init() run before this module is initialized?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:138
@@ +137,3 @@
+void PrintWarningWithOrigin(uptr pc, uptr bp, u32 origin) {
+  if (!__msan::flags.report_umrs) return;
+  if (msan_expect_umr) {
----------------
why not
Flags* flags() { return &__msan::flags; }
and "flags()->whatever" here and below?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interface.h:41
@@ +40,3 @@
+// (i.e. -mllvm -msan-keep-going)
+SANITIZER_INTERFACE_ATTRIBUTE __attribute__((noreturn))
+void __msan_warning_noreturn();
----------------
I think we have NORETURN macro in sanitizer_internal_defs.h

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:231
@@ +230,3 @@
+                             &__msan_stack_bounds.stack_bottom);
+  // Printf("MemorySanitizer init done\n");
+  msan_init_is_running = 0;
----------------
print this in verbose mode?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:212
@@ +211,3 @@
+  if (__msan_track_origins)
+    Printf("msan_track_origins\n");
+  if (!InitShadow(/*true*/ false, true, true, __msan_track_origins)) {
----------------
This is user-facing message which is always printed, can we make it more understable (or hide under verbosity)?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:201
@@ +200,3 @@
+  if (StackSizeIsUnlimited()) {
+    // Printf("Unlimited stack, doing reexec\n");
+    SetStackSizeLimitInBytes(32 * 1024 * 1024);
----------------
hide under verbosity?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:175
@@ +174,3 @@
+// Interface.
+
+void __msan_warning() {
----------------
WDYT of
using namespace __msan;
and removing __msan:: below (for consistency w/ other tools)?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:239
@@ +238,3 @@
+}
+void __msan_set_expect_umr(int expect_umr) {
+  if (expect_umr) {
----------------
empty line here

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:337
@@ +336,3 @@
+  uptr beg = x & ~3UL;  // align down.
+  uptr end = (x + size + 3) & ~3UL;  // align up.
+  u64 origin64 = ((u64)origin << 32) | origin;
----------------
These magic numbers seem pretty scary to me.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_allocator.cc:63
@@ +62,3 @@
+    CHECK_EQ((stack_id >> 31), 0);  // Higher bit is occupied by stack origins.
+    // Printf("ALLOC: stack.size = %zd id=%d\n", stack->size, stack_id);
+    __msan_set_origin(res, size, stack_id);
----------------
hide under verbosity?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:208
@@ +207,3 @@
+            int base) {
+  long res = REAL(strtol)(nptr, endptr, base);  // NOLINT
+  if (!__msan_has_dynamic_component()) {
----------------
Do we need ENSURE_MSAN_INITED() here (and below)?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:374
@@ +373,3 @@
+
+UNSUPPORTED(wcscoll_l);
+UNSUPPORTED(wcsnrtombs);
----------------
Does it actually work (you don't provide args to INTERCEPTOR)?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:715
@@ +714,3 @@
+
+// These interface functions reside here so that they can use
+// fast_memset, etc.
----------------
Then it's probably better to move them to another file together with fast_memset?
should fast_memset be marked inline?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:789
@@ +788,3 @@
+
+#undef IS_IN_SHADOW
+
----------------
Why not define this macro in header and make it universally available?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interface.h:19
@@ +18,3 @@
+
+using __sanitizer::uptr;
+using __sanitizer::sptr;
----------------
I think you don't need this.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interface.h:12
@@ +11,3 @@
+//
+// Public interface header.
+//===----------------------------------------------------------------------===//
----------------
Consider moving it to //projects/compiler-rt/include/sanitizer


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

More information about the llvm-commits mailing list