[PATCH] D115302: GlobalsModRef should treat functions w/o nosync conservatively.

Thu Dec 9 10:25:41 PST 2021

tra added a comment.

In D115302#3181470 <https://reviews.llvm.org/D115302#3181470>, @jdoerfert wrote:

> In D115302#3181440 <https://reviews.llvm.org/D115302#3181440>, @efriedma wrote:
>
>> In general, GlobalsAA follows the call graph, and therefore conservatively assumes a call to an external function can call back into the current module.
>>
>> The callgraph code special-cases intrinsics, though.  Except for a few specific intrinsics, we assume intrinsics can't call back into the current module. See Intrinsic::isLeaf.

Sounds like `isLeaf` is not not completely correct, either. Earlier comment mentioned that "refcounting on objc objects can run arbitrary code".

While poking at the Intrinsics.td I've noticed that `NoSync` is supposed to be set on intrinsics by default. The fact that `int_objc_autoreleasePoolPush`  was affected by this patch suggests that we somehow failed to propagate it.

> 1. Then the intrinsics should be marked with the attribute that says they won't call back into the module: `nocallback` (which has no lang ref entry... https://reviews.llvm.org/D90275#2454912, anyway).

Nor does it have a matching intrinsic property.

> 2. Not calling back into the module is not sufficient. Nosync is necessary, as noted here. I'm still somewhat unsure why it is checked for a call but OK, that might be the interface. I fear we forget to check it in other places too.

I am way out of my depth here, so it's quite possible that I'm doing it wrong. If there's a better way/place to do it, I'm open to suggestions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115302