[PATCH] D20485: [esan] Add working set base runtime library

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

Doing some minor post-commit review. Please don't add svn properties unless they're really needed.

Thank you,

Filipe


================
Comment at: compiler-rt/trunk/lib/esan/esan.cpp:169
@@ -159,3 +168,3 @@
   VPrintf(1, "in esan::%s\n", __FUNCTION__);
-  if (WhichTool != ESAN_CacheFrag) {
+  if (WhichTool <= ESAN_None || WhichTool >= ESAN_Max) {
     Printf("ERROR: unknown tool %d requested\n", WhichTool);
----------------
We should just validate `Tool` at the start of `initializeLibrary`, and set `WhichTool` after we validated it. That way we know that `WhichTool` is either uninitialized or a valid tool. We should also remove the assignment to `WhichTool` from `__esan_init` and only do it here, in `initializeLibrary` (currently we're setting it twice (at least)).

================
Comment at: compiler-rt/trunk/lib/esan/esan_interface_internal.h:32
@@ -31,1 +31,3 @@
+  ESAN_WorkingSet,
+  ESAN_Max,
 } ToolType;
----------------
I'd rather have something like `ESAN_MaxTool` or something. A more descriptive name than simply `Max`.

================
Comment at: compiler-rt/trunk/lib/esan/working_set.cpp:52
@@ +51,3 @@
+  // Write shadow bytes until we're word-aligned.
+  while (I < NumLines && (uptr)Shadow % 4 != 0) {
+    if ((*Shadow & ShadowAccessedVal) != ShadowAccessedVal)
----------------
Please, no magic constants. `alignof(u32)` would be better.

================
Comment at: compiler-rt/trunk/lib/esan/working_set.cpp:63
@@ +62,3 @@
+    ShadowAccessedVal << 16 | ShadowAccessedVal << 24;
+  while (I + 4 <= NumLines) {
+    if ((*(u32*)Shadow & WordValue) != WordValue)
----------------
`sizeof(u32)`

================
Comment at: compiler-rt/trunk/lib/esan/working_set.cpp:64
@@ +63,3 @@
+  while (I + 4 <= NumLines) {
+    if ((*(u32*)Shadow & WordValue) != WordValue)
+      *(u32*)Shadow |= WordValue;
----------------
I'd rather just have a `u32 *ShadowWord = Shadow` before this block, and just use that pointer.
It minimizes "line noise" and is more explicit on what we're doing.

================
Comment at: compiler-rt/trunk/lib/esan/working_set.cpp:69
@@ +68,3 @@
+  }
+  // Write any trailing shadow bytes.
+  while (I < NumLines) {
----------------
And remember to update `Shadow` from `ShadowWord`, of course.

================
Comment at: compiler-rt/trunk/lib/esan/working_set.cpp:80
@@ +79,3 @@
+  // The shadow mapping assumes 64 so this cannot be changed.
+  CHECK(getFlags()->cache_line_size == 64);
+}
----------------
Do we want this to be a flag, then?
This seems weird. This should probably be set somewhere close to the mapping, since they're so tied to each other.
And we can just add a VReport to the initialization function to print it for a verbosity level higher than 1.


Repository:
  rL LLVM

http://reviews.llvm.org/D20485