[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