[llvm-commits] [PATCH] [msan] MemorySanitizer runtime
Alexander Potapenko
glider at google.com
Mon Dec 10 06:33:02 PST 2012
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:133
@@ +132,3 @@
+ __msan_stack_bounds.stack_top = __msan_stack_bounds.stack_bottom = 1;
+ GetThreadStackTopAndBottom(false,
+ &__msan_stack_bounds.stack_top,
----------------
Please add an inline comment describing the first parameter (something like "/*parameter_name*/ false")
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:39
@@ +38,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+THREADLOCAL u64 __msan_param_tls[100];
+
----------------
Is 100 here and below the same constant? If so, make it a const int or #define please.
================
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;
----------------
Why 67?
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:238
@@ +237,3 @@
+ Printf("msan_track_origins\n");
+ if (!InitShadow(/*true*/ false, true, true, __msan_track_origins)) {
+ // FIXME: eugenis, do we need *false* above?
----------------
"/*true*/ false"
Please fix the comment
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:274
@@ +273,3 @@
+ PrintCurrentStackTrace(pc, bp);
+ Die();
+ }
----------------
Is it really a fatal error if an expected UMR doesn't occur?
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_allocator.cc:39
@@ +38,3 @@
+
+
+static int inited = 0;
----------------
Nit: spare newline
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_allocator.cc:42
@@ +41,3 @@
+
+void Init() {
+ if (inited) return;
----------------
I wonder if it makes sense to make Init() an inline function.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_flags.h:1
@@ +1,2 @@
+namespace __msan {
+
----------------
Please add a license header and an #ifdef sentry.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:19
@@ +18,3 @@
+#include "sanitizer_common/sanitizer_libc.h"
+#include <interception/interception.h>
+
----------------
Are the angle brackets intended here?
Also, mind the alphabetical order of the includes in this section.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:34
@@ +33,3 @@
+ if (!msan_inited) { \
+ __msan_init(); \
+ } \
----------------
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()
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:130
@@ +129,3 @@
+
+INTERCEPTOR(void, free, void *ptr) {
+ ENSURE_MSAN_INITED();
----------------
Please be consistent with spaces in pointer types. E.g. here you have "void *ptr" and in the function below: "const char* s"
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:153
@@ +152,3 @@
+ ENSURE_MSAN_INITED();
+ size_t n = REAL(strlen)(src);
+ char* res = REAL(strcpy)(dest, src); // NOLINT
----------------
Please check for the shadow of the trailing \0 byte (or at least add a FIXME)
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:180
@@ +179,3 @@
+ char* res = REAL(gcvt)(number, ndigit, buf);
+ if (!__msan_has_dynamic_component()) {
+ size_t n = REAL(strlen)(buf);
----------------
Please add a comment describing why you need the check for dynamic component here.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:227
@@ +226,3 @@
+
+INTERCEPTOR(unsigned long, strtoul, const char *nptr, char **endptr, //NOLINT
+ int base) {
----------------
Nit: space between // and "NOLINT" (here and below)
================
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;
----------------
Note it's not really necessary to init MSan here (though it's better to keep it for consistency)
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:314
@@ +313,3 @@
+ size_t res = REAL(wcstombs)(dest, src, size);
+ if (res != (size_t)-1) __msan_unpoison(dest, res + 1);
+ return res;
----------------
Nit: two spaces.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:399
@@ +398,3 @@
+UNSUPPORTED(wcsdup)
+// UNSUPPORTED(wcsftime) // FIXME
+// UNSUPPORTED(wcsstr)
----------------
Why are these commented out?
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:403
@@ +402,3 @@
+// UNSUPPORTED(wctob)
+
+
----------------
Nit: spare newline.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:490
@@ +489,3 @@
+ if (!res)
+ __msan_unpoison(pipefd, sizeof(int[2]));
+ return res;
----------------
sizeof(pipefd)
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:586
@@ +585,3 @@
+
+INTERCEPTOR(int, fstatfs64, int fd, void* buf) {
+ ENSURE_MSAN_INITED();
----------------
How about declaring a common macro for such dummy interceptors?
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:664
@@ +663,3 @@
+ }
+
+ return MsanReallocate(&stack, 0, nmemb * size, sizeof(u64), true);
----------------
Is this newline necessary?
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:679
@@ +678,3 @@
+INTERCEPTOR(void *, mmap, void *addr, size_t length, int prot, int flags,
+ int fd, off_t offset) {
+ ENSURE_MSAN_INITED();
----------------
Please fix the indentation here and below.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:698
@@ +697,3 @@
+void *fast_memset(void *ptr, int c, size_t n) {
+#if 1
+ // hack until we have a really fast internal_memset
----------------
Dunno if it's a good idea to commit "#if 1". If you ever want to switch it to zero, consider defining a constant (especially if you want to switch both fast_memset and fast_memcpy at the same time)
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:735
@@ +734,3 @@
+// fast_memset, etc.
+
+void __msan_unpoison(void *a, uptr size) {
----------------
Nit: spare newline.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_linux.cc:14
@@ +13,3 @@
+//===----------------------------------------------------------------------===//
+
+#include "msan.h"
----------------
I guess there should be a #ifdef guard.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_new_delete.cc:49
@@ +48,3 @@
+void operator delete(void *ptr, std::nothrow_t const&) { OPERATOR_DELETE_BODY; }
+void operator delete[](void *ptr, std::nothrow_t const&)
+{ OPERATOR_DELETE_BODY; }
----------------
Please leave the open curly brace on this line.
================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_platform_limits_posix.cc:29
@@ +28,3 @@
+
+
+namespace __msan {
----------------
Nit: spare newline.
http://llvm-reviews.chandlerc.com/D191
More information about the llvm-commits
mailing list