[PATCH] D20578: [esan|wset] Iterate all memory to compute the total working set

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.

================
Comment at: lib/esan/working_set.cpp:85
@@ +84,3 @@
+  // Get word aligned start.
+  ShadowStart = RoundDownTo(ShadowStart, 4);
+  for (u32 *Ptr = (u32 *)ShadowStart; Ptr < (u32 *)ShadowEnd; ++Ptr) {
----------------
RoundDown? We'll be looking at some shadow bytes before `ShadowStart`. Won't that be a problem?

================
Comment at: lib/esan/working_set.cpp:89
@@ +88,3 @@
+      byte *BytePtr = (byte *)Ptr;
+      for (u32 j = 0; j < 4; ++j) {
+        if ((BytePtr[j] & ByteValue) == ByteValue) {
----------------
`sizeof(u32)`

================
Comment at: lib/esan/working_set.cpp:90
@@ +89,3 @@
+      for (u32 j = 0; j < 4; ++j) {
+        if ((BytePtr[j] & ByteValue) == ByteValue) {
+          ++WorkingSetSize;
----------------
ByteValue has only one bit set. No need to test for equality.

================
Comment at: lib/esan/working_set.cpp:129
@@ +128,3 @@
+static u32 getSizeForPrinting(u32 NumOfCachelines, const char *&Unit) {
+  // We need a constant to avoid software divide support:
+  static const u32 line_size = 64;
----------------
We don't really care, since we only do this at finalize time (and we're printing stuff anyway), no?

================
Comment at: lib/esan/working_set.cpp:131
@@ +130,3 @@
+  static const u32 line_size = 64;
+  CHECK(line_size == getFlags()->cache_line_size);
+  const u32 KilobyteCachelines = (0x1 << 10) / line_size;
----------------
No sense, again. Just make a global `kDefaultCacheLineSize`, and document next to it that we only support that size.

================
Comment at: lib/esan/working_set_posix.cpp:69
@@ -68,5 +68,3 @@
   if (SigNum == SIGSEGV) {
-
-    // TODO: Add shadow memory fault detection and handling.
-
-    if (AppSigAct.sigaction) {
+    // We rely on si_addr being filled in (thus we do not support old kernels).
+    siginfo_t *SigInfo = (siginfo_t *)Info;
----------------
aizatsky wrote:
> Should this be working_set_linux.cpp then?
or at least the _posix variant.

================
Comment at: test/esan/TestCases/workingset-simple.cpp:18
@@ +17,3 @@
+  bufA[0] = 0;
+  bufA[1] = 0; // Make sure no additional cache line.
+  bufB[33] = 1;
----------------
What do you mean?


http://reviews.llvm.org/D20578