[PATCH] D137360: [Linker] Remove nocallback attribute while linking

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

> In D137360#3907031 <https://reviews.llvm.org/D137360#3907031>, @jdoerfert wrote:
>
>> I somewhat assume we could remove the attribute whenever we actually move a function (lazy linking) rather than for all in the module.
>> Other than that I'm happy with dropping nocallback *and* inacc* (see https://reviews.llvm.org/D131628#3724307 for the latter).
>
> There is a recent change that drops `argmemonly`, `inaccessiblememonly` and `inaccessiblemem_or_argmemonly` attributes (https://reviews.llvm.org/D135780). So, we do not need to apply this change to them anymore.

But it just replaces `inaccessiblememonly` with `memoru(inaccessiblememonly)`, right? It doesn't address the underlying problem, which is that the linking potentially invalidates `memory(inaccessiblememonly)`.

In D137360#3909886 <https://reviews.llvm.org/D137360#3909886>, @nikic wrote:

> I don't think this implementation is correct, but I'm also not entirely sure in which cases you want to drop the attribute and in which you want to retain it. I'd suggest to have the following test cases:
>
> - A nocallback intrinsic, where the attribute should never be dropped.
> - A nocallback definition.
> - A nocallback declaration with a definition that is being linked in.
> - A nocallback declaration without a definition being linked in.
>
> Additionally, how does this interact with inference? Say we originally had a call to a nocallback declaration in a function, then inference determines that that function is nocallback as well (that would be legal, right?) In that case we might have to clear that nocallback attribute as well. So I think at point we have to drop all (non-intrinsic) nocallback attributes in the module?

Definitions should never be nocallback, IMHO. I thought the proposed lang-ref change explicitly mentioned that. Which leaves three cases above to be dealt with:
 `nocallback` intrinsic and `nocallback` declaration w/ and w/o definition being linked in.
We should not touch intrinsics, here, as noted.
For the declaration case, we always need to drop it because it is either a definition now (for which nocallback is meaningless) or we might have imported code that is called by the (former) nocallback declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137360