[PATCH] D32197: [TBAASan] A TBAA Sanitizer (Runtime Library)

Filipe Cabecinhas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 19 10:53:01 PDT 2017


filcab added a comment.

I just did a very quick first pass. Looks good in general, but I have some nits.

Also: Can you add a test for `ATOMIC UPDATE` access type?



================
Comment at: cmake/config-ix.cmake:500
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND TBAASAN_SUPPORTED_ARCH AND
+    OS_NAME MATCHES "Linux")
+  set(COMPILER_RT_HAS_TBAASAN TRUE)
----------------
clang enables it on FreeBSD too. I guess maybe enabling it on FreeBSD in the clang patch was a bug and you'd rather finish work on Linux first?
Or do you want to use the bots/keep testing on FreeBSD as you commit the parts, and expect FreeBSD to "just work" too?


================
Comment at: lib/tbaasan/tbaasan.h:23
+
+#include "tbaasan_platform.h"
+
----------------
Can you move this include above?
When reading, one can't tell if the `using` directives above are required before including the header or not. We can probably just stick them there, if they're really needed (I'd rather they weren't (in general), but given how fundamental the `uptr` and `u16` types are, I don't see having those `using` directives in a header as a problem)


================
Comment at: lib/tbaasan/tbaasan.h:30
+
+struct tbaasan_member_type_descriptor {
+  struct tbaasan_type_descriptor *Base;
----------------
Can these go inside `namespace __tbaasan`?


================
Comment at: lib/tbaasan/tbaasan_interceptors.cc:198
+  // Instruct libc malloc to consume less memory.
+#if SANITIZER_LINUX
+  mallopt(1, 0);  // M_MXFAST
----------------
We're using 8x the memory for shadow memory. Do these make a noticeable difference?


https://reviews.llvm.org/D32197





More information about the llvm-commits mailing list