[PATCH] D22098: [esan] Add __esan_report for mid-run data

Derek Bruening via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 8 09:09:24 PDT 2016

bruening added inline comments.

Comment at: include/sanitizer/esan_interface.h:26
@@ +25,3 @@
+// data for that point in the run be reported from the tool.
+void __esan_report();
aizatsky wrote:
> do you want this to be weak so that host program could be compiled without esan as well?
> Or do you plan to have ifdefs in the host?
I don't see how it can work without ifdefs simply by being weak?  If it's weak and the esan library is not linked in, the caller's call to __esan_report() will be a call to NULL and will crash.  To avoid that, the caller would have to provide an empty-body implementation of __esan_report that was also weak, and we'd have to hope that the esan library's version was found first.  I don't see any use of weak in the other *san_interface.h headers and I assume they all rely on ifdefs in the caller -- right?  Or am I missing something?

Comment at: lib/esan/esan.cpp:236
@@ +235,3 @@
+  } else if (__esan_which_tool == ESAN_WorkingSet) {
+    return reportWorkingSet();
+  }
aizatsky wrote:
> Do you think it makes sense to have classes with virtual methods now?
> All these functions perform tool-based dispatch.
That is something we have considered and should probably do at some point, but as a separate refactoring from this CL.  We'll put it on our short list for improving the code.

Comment at: lib/esan/working_set.cpp:121
@@ -120,2 +120,3 @@
 // This routine will word-align ShadowStart and ShadowEnd prior to scanning.
+// It does *not* clear for BitIdx==TotalWorkingSetBitIdx.
 static u32 countAndClearShadowValues(u32 BitIdx, uptr ShadowStart,
aizatsky wrote:
> can you explain why is this a desired behavior? It is not clear to me :)
> Maybe making it caller's responsibility is better?
> countAndClearShadowValues(u32 BitIdx, uptr ShadowStart,
>                                                  uptr ShadowEnd, bool ClearBits)
We only want to clear bits that are measuring just part of the execution: one time period.  The top bit is measuring the entire execution, so it's special, and should never be cleared.  In the past we only ever called this routine for the top bit at the very end of the run to report the total, but with this new mid-run report we're calling this at arbitrary times and thus need to avoid the clear.


More information about the llvm-commits mailing list