[PATCH] D17883: [ASAN] Add support for mips/mips64 android

[Duane Sand via llvm-commits]

duanesand added inline comments.

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:160
@@ -160,1 +159,3 @@
+#if !SANITIZER_FREEBSD && (!SANITIZER_ANDROID || SANITIZER_MIPS) && \
+  !SANITIZER_GO
 static uptr g_tls_size;
----------------
Sorry, my mistake;  this should revert back to the original condition, without SANITIZER_MIPS

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:162
@@ -160,3 +161,3 @@
 static uptr g_tls_size;
 #endif
 
----------------
This #endif should be moved down around all things using this variable; either after TlsPreTcbSize, or better yet, after InitTlsSize.

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:173
@@ -171,3 +172,3 @@
 // head structure. It lies before the static tls blocks.
 static uptr TlsPreTcbSize() {
 # if defined(__mips__)
----------------
This function is only used when the nonempty version of InitTlsSize() is used, ie when

#if !SANITIZER_FREEBSD && !SANITIZER_ANDROID && !SANITIZER_GO

This TlsPreTcbSize() function must similarly be skipped when that condition is false, otherwise there is an unresolved ref to  g_tls_size  during mips Android builds.



================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:189
@@ -187,3 +188,3 @@
 void InitTlsSize() {
 #if !SANITIZER_FREEBSD && !SANITIZER_ANDROID && !SANITIZER_GO
 // all current supported platforms have 16 bytes stack alignment
----------------
This condition (which must match line 159 in the original) is prone to breaking again in the future.  It would be best to not duplicate it, or else give it a positive name like  SANITIZER_USES_TLS.  

Rather than defining it as a bunch of negatives (against what world of possible os's?), I suggest redefining it more exactly as
    SANITIZER_LINUX && !SANITIZER_ANDROID
if that is indeed the current situation

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:207
@@ -205,3 +206,3 @@
 }
 
 #if (defined(__x86_64__) || defined(__i386__) || defined(__mips__) \
----------------
This defines a do-nothing body for InitTlsSize for some os's including Android.  That allows its callers' source to be free of os dependencies, which is good.  But the current way of skipping its body requires two matching instances of a non-obvious #if.  These could be combined into a single #if by repeating the entire header and body of the do-nothing function in an #else part, ie

}
#else
void InitTlsSize() { }
#endif  // !SANITIZER_FREEBSD && !SANITIZER_ANDROID && !SANITIZER_GO

================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:554
@@ -545,1 +553,3 @@
+    };
+    __sanitizer_sigset_t sa_mask;
 #elif SANITIZER_ANDROID && (SANITIZER_WORDSIZE == 32)
----------------
final  };  is missing


Repository:
  rL LLVM

http://reviews.llvm.org/D17883