[PATCH] D29703: [XRay] [compiler-rt] Allow logging the first argument of a function call.

[Martin Pelikán via Phabricator via llvm-commits]

pelikan marked an inline comment as done.
pelikan added inline comments.


================
Comment at: lib/xray/xray_interface.cc:217
+{
+  XRayArgLogger.store(Handler, std::memory_order_relaxed);
+}
----------------
dberris wrote:
> pelikan wrote:
> > dberris wrote:
> > > pelikan wrote:
> > > > dberris wrote:
> > > > > I think it would be better to make this store a release, for a stronger guarantee. 
> > > > Stronger guarantee of what?  There's nothing we're synchronizing *with*, we only care about atomicity (which this provides).  When I see a release/consume I automatically look for an acquire, and in this case there isn't one.
> > > In x86-64 the acquire is in the assembly (spelled `movq`) because of the stronger memory model. That would change in weaker memory models, and since this is in a cross-platform part of the code, we have to be a bit more careful about this.
> > Why do we care which order do the store and load happen?  I can't think of a scenario where I'm enabling arg1 logging on one CPU and worrying about what happens on other CPUs at the same time with respect to me just enabling it.  How is eventual consistency not enough?
> > 
> > Similarly, due to the strong memory model, you can look at the MOV as a relaxed operation that just got unnecessarily expensive because of Intel ;-)
> Because on non-x86_64, these operations will really be relaxed. And because this isn't x86-specific code, we need to be careful about it.
> 
> For instance, the load in arm and ppc will need to be stronger versions (as it already is) to avoid having to deal with the issue of some threads not seeing the pointer update for potentially longer than when logging is on -- or they can stay relaxed but the write has to be strong so the appropriate caches are invalidated. A relaxed store here will mean that there's no strong guarantee that the data will show up as soon as the logging handler is installed, and threads can keep executing without seeing the handlers updated even if the sleds have been patched appropriately. We don't want this to happen because it affects data quality.
> 
> Does that make more sense?
> 
> 
None of this answers the question "why do we care".  The test behaviour depending on this, however, does.  I've added a comment about why maintaining this is important.


https://reviews.llvm.org/D29703