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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D32197#730755, @filcab wrote:

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


Thanks!

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

I had forgotten about that. I can't add a test yet because Clang doesn't add TBAA on atomic instructions. I need to fix that too.



================
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)
----------------
filcab wrote:
> 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?
I suspect it will just work. OTOH, I'll turn it off in Clang for now (pending confirmation that it functions properly).


================
Comment at: lib/tbaasan/tbaasan.h:23
+
+#include "tbaasan_platform.h"
+
----------------
filcab wrote:
> 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)
Currently, no.  The included header assumes those types have been brought in. FWIW,  I also generally don't like having using directives in headers, but I agree that it seems reasonable in this case (and essentially every sanitizer has `using __sanitizer::uptr` in a header).


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


================
Comment at: lib/tbaasan/tbaasan_interceptors.cc:198
+  // Instruct libc malloc to consume less memory.
+#if SANITIZER_LINUX
+  mallopt(1, 0);  // M_MXFAST
----------------
filcab wrote:
> We're using 8x the memory for shadow memory. Do these make a noticeable difference?
No, but I figured that it can't hurt (I copied these from TSan). Hopefully, one day (in the not-so-distant future), we'll use less memory (if we have the runtime manage type registration, for example, I think we could easily bring this down to 2-3 bytes per byte). I can obviously take this out if you'd prefer.


https://reviews.llvm.org/D32197