[PATCH] D14725: [compiler-rt] [dfsan] Unify aarch64 mapping

[Adhemerval Zanella via llvm-commits]

zatrazz added a comment.

Thanks for the review, comments below.


================
Comment at: lib/dfsan/dfsan.cc:371
@@ +370,3 @@
+static void InitializePlatformEarly() {
+#ifdef DFSAN_RUNTIME_VMA
+# ifdef __aarch64__
----------------
pcc wrote:
> You don't need this `#ifdef` (or the macro), as it is redundant with `__aarch64__`.
The idea of adding these two ifdefs, although currently redundant, is to make explicit DFSAN_RUNTIME_VMA is not tied to aarch64. I can remove it if you prefer.

================
Comment at: lib/dfsan/dfsan.cc:378
@@ +377,3 @@
+  } else {
+    Printf("FATAL: ThreadSanitizer: unsupported VMA range\n");
+    Printf("FATAL: Found %d - Supported 39 and 42\n", __dfsan::vmaSize);
----------------
pcc wrote:
> ThreadSanitizer -> DataFlowSanitizer
I will change that.

================
Comment at: lib/dfsan/dfsan_platform.h:92
@@ +91,3 @@
+uptr UnionTableAddr() {
+  return MappingArchImpl<MAPPING_UNION_TABLE_ADDR>();
+}
----------------
pcc wrote:
> I would prefer if these functions were written more directly.
> 
> e.g.
> 
> ```uptr UnionTableAddr() {
> #if defined(__x86_64__)
>   return 0x200000000000;
> #elif ...
> #elif defined(__aarch64__)
>   if (vmaSize == 39)
>     return ...;
>   else
>     return ...;
> #endif
> }```
I based this modifications on my pending thread sanitizer patch [1] and the idea it avoid the constant ifdef for multiple function which IHMO only make the code hard to read and duplicate logic. Which this approach you have only two required platform specific blocks: one for the mapping definition and other for multiple VMA handling. I sensed, based on thread sanitizer review, that this approach would be desirable one for compiler-rt.

[1] http://reviews.llvm.org/D14199


http://reviews.llvm.org/D14725