[PATCH] D101701: [nofree] Refine concurrency requirements

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:1282
+
+  if (I.hasReleaseOrdering()) {
+    if (auto *FI = dyn_cast<FenceInst>(&I))
----------------
nhaehnle wrote:
> reames wrote:
> > This is wrong.  A thread can coordinate using only acquire ordering within 'f'.
> > 
> > Example:
> > g = o; // the object being potentially freed
> > if (g_acquire)
> >   return; // then *caller* does release store
> > use o;
> > 
> > The other thread does:
> > while (!g) {
> >   g_acquire = true;
> >   while (!g_release) {}
> >   free(g);
> > }
> > 
> > Please exactly match the existing nosync logic in this file by using isOrderedAtomic helper in this file.  We can be more aggressive later if desired.
> This is an interesting example, but I don't think it shows what you want it to show? Without the release store in the caller, the other thread cannot proceed to `free(g)`. In other words, the point at which `g`/`o` stops being dereferenceable is the release store in the caller, not the acquire load in the callee.
> 
> The acquire load is part of some coordination with the other thread, but it's redundant as far as the free concerned. At least it looks that way to me.
> 
> To attack this from a different angle, consider a slight variation of the example where the caller does a g_release store **before** calling f. In that case, putting `dereferenceable` on `o` is incorrect, because the other thread could free `o` before f is even entered.
> 
> From yet another different angle, which part of the reasoning in the comment in the AANoFreeImpl is wrong?
I'm not going to debate concurrency in this review.  My (blocking, mandatory) request is that you exactly match existing behavior in this review.

If you wish to open a follow up review to refine the concurrency logic, we can do so.  Warning: Concurrency is *hard* and we will need to loop in some experts who are not on this thread, and should not have to care about this review.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101701/new/

https://reviews.llvm.org/D101701