[PATCH] D19921: [esan] EfficiencySanitizer shadow memory

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

Can you hide some more complexity of the Mapping?
This almost looks like it will have to be fully rewritten for any port (with all the different App spaces, etc, which will very likely not map to other OS).
isAppMem, isShadowMem, and other functions should be more general than they are here.


================
Comment at: lib/esan/esan_shadow.h:79
@@ +78,3 @@
+//
+// We also want to ensure that a wild access by the application into the shadow
+// regions will not corrupt our own shadow memory.  shadow(shadow) ends up
----------------
The rationale was that the type *should* already explicitly state that it was shadow related. Added to that, the struct members, since it's a struct used to describe a shadow mapping, shouldn't have "shadow" in their name, since that struct is "all about shadow memory". A bit like llvm's `struct ShadowMapping`, in AddressSanitizer.cpp. The ones where I asked to add shadow (other than the class name) were simply a way to avoid clashes in names.
`AsanMappingProfile` is a recent addition and it ends up being the profile for `asan_mapping.h`, so it makes sense to me to "keep" the file name, transformed to a proper variable name.

But this is only bike-shedding on names, so if you feel strongly, I won't push back on this any more. Except for the `initMapping` method in the `Mapping` class. No sense repeating `Mapping` all the time.

================
Comment at: lib/esan/esan_shadow.h:87
@@ +86,3 @@
+// [0x000013ff'ff601000, 0x00001400'00000000]
+// [0x000013ff'ff600000, 0x000013ff'ff601000]
+
----------------
Has this been measured?
We'll get huge 64-bit mov instructions vs a few loads with high locality.
I would prefer readibility over "potential performance" unless this has been shown to have a good performance impact.


http://reviews.llvm.org/D19921