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

[Derek Bruening via llvm-commits]

bruening added inline comments.

================
Comment at: lib/esan/esan.cpp:144
@@ -143,1 +143,3 @@
 
+uptr vmaSize;
+
----------------
Style

================
Comment at: lib/esan/esan.cpp:150
@@ +149,3 @@
+  vmaSize =
+    (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1); 
+
----------------
This is making assumptions about where the initial stack is allocated.  Please comment those assumptions (and why they would hold on all supported platforms X ulimit or other stack parameters).

================
Comment at: lib/esan/esan.h:37
@@ -36,2 +36,3 @@
 extern bool EsanDuringInit;
+extern uptr vmaSize;
 
----------------
Please follow LLVM style in these new files: variables should start with upper-case

================
Comment at: lib/esan/esan_shadow.h:118
@@ +117,3 @@
+//
+// The resulting shadow memory regions for a 0 scaling are:
+//
----------------
Providing the formula would help clarify

================
Comment at: lib/esan/esan_shadow.h:127
@@ +126,3 @@
+  {0xaa00000000u, 0xab00000000u, false},
+  {0xff00000000u, 0xffffffffffu, true},
+};
----------------
The final 2 aren't adjacent, so I don't understand why they would be merged?

================
Comment at: lib/esan/esan_shadow.h:136
@@ -135,3 @@
-// We'll want to use templatized functions over the ShadowMapping once
-// we support more platforms.
-#error Platform not supported
----------------
It may be worth keeping this comment, as the shadow translation is getting more expensive with extra variable loads that we could turn back into constants.

================
Comment at: lib/esan/esan_shadow.h:148
@@ +147,3 @@
+    static uptr OffsetArray[3];
+    switch(vmaSize)
+    {
----------------
Style: h (

================
Comment at: lib/esan/esan_shadow.h:149
@@ +148,3 @@
+    switch(vmaSize)
+    {
+      case 40:
----------------
Style: { on prior line

================
Comment at: lib/esan/esan_shadow.h:151
@@ +150,3 @@
+      case 40:
+      {
+        OffsetArray[0] = 0x0000001300000000u;
----------------
Style: { on prior line

================
Comment at: lib/esan/esan_shadow.h:166
@@ -126,1 +165,3 @@
+      break;
+    }
     Scale = ShadowScale;
----------------
Seems best to have a default raising an error

================
Comment at: lib/sanitizer_common/sanitizer_linux_mips64.S:18
@@ +17,3 @@
+
+        li $v0,5211
+        syscall
----------------
Suggest: a comment identifying this magic # 5211 as SYS_rt_sigreturn

================
Comment at: lib/sanitizer_common/sanitizer_linux_mips64.S:20
@@ +19,3 @@
+        syscall
+        nop
+
----------------
Why a nop?


Repository:
  rL LLVM

https://reviews.llvm.org/D23799