[PATCH] D23799: [ESan][MIPS] Adds support for MIPS64

[Qin Zhao via llvm-commits]

zhaoqin added a comment.

A few questions.
Nothing major, but better to have another round.


================
Comment at: lib/esan/esan_shadow.h:193
@@ -126,5 +192,3 @@
     Scale = ShadowScale;
-    if (Scale <= 2)
-      Offset = OffsetArray[Scale];
-    else
-      Offset = OffsetArray[0] << Scale;
+    switch (VmaSize) {
+      case 40: {
----------------
We are using virtual address in the user space to determine the shadow memory translation.
Are we sure it would work for all the archs that have the same VmaSize?
I guess it would work for now. so just curious how likely it would work for any possible future arch/os.

================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:620
@@ -619,1 +619,3 @@
   struct __sanitizer_kernel_sigaction_t {
+#if SANITIZER_MIPS
+    unsigned int sa_flags;
----------------
Why multiple #if separate from each other?
Can we move them together?


================
Comment at: test/esan/TestCases/mmap-shadow-conflict.c:30
@@ -19,5 +29,3 @@
   // (There can be a re-exec for stack limit here.)
-  // CHECK:      Shadow scale=2 offset=0x440000000000
-  // CHECK-NEXT: Shadow #0: [110000000000-114000000000) (256GB)
-  // CHECK-NEXT: Shadow #1: [124000000000-12c000000000) (512GB)
-  // CHECK-NEXT: Shadow #2: [148000000000-150000000000) (512GB)
+  // x86_64:      Shadow scale=2 offset=0x440000000000
+  // x86_64-NEXT: Shadow #0: [110000000000-114000000000) (256GB)
----------------
hmm feel better if we can add CHECK, something like CHECK-x86_64 and CHECK-NEXT-x86_64.
forget about it if it is too hard to do so.

================
Comment at: test/esan/TestCases/verbose-simple.c:15
@@ +14,3 @@
+  // mips64-NEXT: Shadow #2: [14c0000000-1500000000) (1GB)
+  // CHECK: in esan::finalizeLibrary
+  // CHECK: ==verbose-simple{{.*}}EfficiencySanitizer: total struct field access count = 0
----------------
Are we sure CHECK still works if we use --check-prefix option?
It probably is true, just be cautious.

================
Comment at: test/lit.common.cfg:91
@@ -90,1 +90,3 @@
 
+# Define %arch to check for architecture-dependent output.
+config.substitutions.append( ('%arch', (config.host_arch)))
----------------
can we do similar thing as CHECK-%os by adding CHECK-%arch, or there will be a conflict.


Repository:
  rL LLVM

https://reviews.llvm.org/D23799