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

[Derek Bruening via llvm-commits]

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:
> bruening wrote:
> > 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?
> > 
> No, with weak function the caller could be written as:
> 
> if (esan_report)
>   esan_report();
> 
> And compile/link/run fine with and without esan and without the use of preprocessor.
Right, that would work.  I don't see any precedent for this, as no other sanitizer does it in their public interfaces, but I don't see any downside to making it weak (unintended symbol collision is quite unlikely) so I'll go ahead and do so.

================
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:
> bruening wrote:
> > 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.
> If you could put this into one-line comment next to clear that would be awesome.
> 
> // high bit measure access during entire execution, should never be cleared.
Will do.


http://reviews.llvm.org/D22098