[PATCH] D43025: [tsan] Add support for linux/powerpc64 in buildgo.sh

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: lib/tsan/go/buildgo.sh:48
+			$SRCS
+                        ../rtl/tsan_platform_linux.cc
+                        ../../sanitizer_common/sanitizer_posix.cc
----------------
List of sources is completely identical to amd64 version, please don't duplicate it. Just keep it outside of the arch if.


================
Comment at: lib/tsan/rtl/tsan_platform.h:445
+/* Go on linux/powerpc64 (47-bit VMA)
+0000 1000 0000 - 00c0 0000 0000: main Go binary + Go heap
+00c0 0000 0000 - 0200 0000 0000: -
----------------
kLoAppMemEnd is 0x010000000000ull, please adjust this line.


================
Comment at: lib/tsan/rtl/tsan_platform.h:448
+2000 0000 0000 - 3000 0000 0000: shadow
+3000 0000 0000 - 3000 0000 0000: -
+3000 0000 0000 - 3400 0000 0000: metainfo (memory blocks and sync objects)
----------------
remove this line


================
Comment at: lib/tsan/rtl/tsan_platform.h:464
+  static const uptr kShadowEnd     = 0x300000000000ull;
+  static const uptr kHeapMemBeg    = 0x7d0000000000ull;
+  static const uptr kHeapMemEnd    = 0x7e0000000000ull;
----------------
I don't see where/how this is used.


================
Comment at: lib/tsan/rtl/tsan_platform.h:468
+  static const uptr kLoAppMemEnd   = 0x010000000000ull;
+  static const uptr kHiAppMemBeg   = 0x7e8000000000ull;
+  static const uptr kHiAppMemEnd   = 0x800000000000ull; // 47 bits
----------------
What lives in kHiAppMemBeg-kHiAppMemEnd range?


================
Comment at: lib/tsan/rtl/tsan_platform.h:472
+  static const uptr kAppMemXor     = 0x080000000000ull;
+  static const uptr kVdsoBeg       = 0x7800000000000000ull;
+};
----------------
I don't see where/how this is used.


================
Comment at: lib/tsan/rtl/tsan_platform.h:879
+# if defined(__powerpc64__)
+  return (u32*)(((((x) & ~(Mapping::kAppMemMsk | (kMetaShadowCell - 1)))) /
+      kMetaShadowCell * kMetaShadowSize) | Mapping::kMetaShadowBeg);
----------------
This is full copy of the C++ mapping, right?
If so, I think it will be better to just say `#if !SANITIZER_GO || defined(__powerpc64__)` rather than duplicate it.
This also applies to some other functions that do the same.


================
Comment at: lib/tsan/rtl/tsan_platform.h:951
+#   else
   return (s & ~Mapping::kShadowBeg) / kShadowCnt;
+#   endif
----------------
Why?


================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:239
+  vmaSize =
+    (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1);
+  if (vmaSize != 46 && vmaSize != 47) {
----------------
No need to duplicate this call.
Let's do:

```
vmaSize = (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1);
...
#   elif defined(__powerpc64__)
#     if !SANITIZER_GO
if (vmaSize != 44 && vmaSize != 46 && vmaSize != 47) { ... }
#     else
if (vmaSize != 44 && vmaSize != 46) { ... }
#     endif
```


================
Comment at: lib/tsan/rtl/tsan_rtl.cc:309
         CHECK(IsAppMem(p));
+	VPrintf(3, "  p: %p s: %p \n", p, s);
         CHECK(IsShadowMem(s));
----------------
These VPrintf's look redundant (and mis-aligned). We already print p, s and MemToMeta(p) few lines above.
ShadowToMem(s) will be printed if CHECK fails (it prints argument values).


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D43025