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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/dfsan/dfsan.cc:371
@@ +370,3 @@
+static void InitializePlatformEarly() {
+#ifdef DFSAN_RUNTIME_VMA
+# ifdef __aarch64__
----------------
zatrazz wrote:
> 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.
I'd prefer to remove it. We can always add it back if it becomes necessary.

================
Comment at: lib/dfsan/dfsan_platform.h:92
@@ +91,3 @@
+uptr UnionTableAddr() {
+  return MappingArchImpl<MAPPING_UNION_TABLE_ADDR>();
+}
----------------
zatrazz wrote:
> 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
Okay, let's be consistent with TSan here.


http://reviews.llvm.org/D14725