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

[Sagar Thakur via llvm-commits]

slthakur added inline comments.

================
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: {
----------------
zhaoqin wrote:
> 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.
Yes, it would work on any arch with same VmaSize provided that proper application memory ranges for that arch are defined.

================
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;
----------------
zhaoqin wrote:
> Why multiple #if separate from each other?
> Can we move them together?
> 
Yes we can 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)
----------------
zhaoqin wrote:
> 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.
We can do a similar thing for CHECK-%acrh as done for CHECK-%os but we will not be able to check the order of output lines with CHECK-NEXT-%arch. Thats why I switched to --check-prefix=%arch only. 

================
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
----------------
zhaoqin wrote:
> Are we sure CHECK still works if we use --check-prefix option?
> It probably is true, just be cautious.
No it does not work. The CHECK lines are not checked by FileCheck. I will add --check-prefix=CHECK to the FileCheck command to make it work. Thanks for catching this.

================
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)))
----------------
zhaoqin wrote:
> can we do similar thing as CHECK-%os by adding CHECK-%arch, or there will be a conflict.
We can do a similar thing for CHECK-%acrh as done for CHECK-%os but we will not be able to check the order of output lines with CHECK-NEXT-%arch. Thats why I switched to --check-prefix=%arch only. 


Repository:
  rL LLVM

https://reviews.llvm.org/D23799