[PATCH] [lsan] [mips] adding support of lsan for mips/mipsel arch
Sergey Matveev
earthdok at google.com
Tue Oct 7 09:26:04 PDT 2014
Actually it just dawned me that there is no MIPS64 support in this patch, so there's nothing to review. I did not realize this until I was halfway through reviewing it, so I'll post those comments anyway.
================
Comment at: lib/lsan/lsan_allocator.cc:32
@@ +31,3 @@
+#if defined (__mips__) && (SANITIZER_WORDSIZE == 32)
+ uptr requested_size : 22;
+#elif(SANITIZER_WORDSIZE == 64)
----------------
This is not right. This field must be wide enough to hold kMaxAllowedMallocSize (3*2^30).
================
Comment at: lib/lsan/lsan_common.cc:151
@@ +150,3 @@
+ // we can assume it as lower bound on heap.
+ const uptr kMinAddress = (uptr) &end;
+#endif
----------------
Why can't this be a constant like on x86_64? Maybe a slightly smaller one if 16k is too much. I don't think you can get any measurable performance improvement from fine-tuning this.
================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:496
@@ +495,3 @@
+#ifdef __mips__
+// For MIPS wait4 is not implmentation in SYSCALL
+ return wait4(pid, status, options, 0);
----------------
I don't get it. How does it work then?
================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:848
@@ +847,3 @@
+ int *parent_tidptr, void *newtls, int *child_tidptr) {
+ return clone(fn, child_stack, flags, arg, parent_tidptr,
+ newtls, child_tidptr);
----------------
Please actually implement clone(). There is a comment above the x86_64 implementation explaining why it is needed.
http://reviews.llvm.org/D5616
More information about the llvm-commits
mailing list