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

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem added a comment.

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

>> Drop memory(inaccessiblemem) attributes as well, and added test cases for them
>
> I'd personally recommend to keep inaccessiblemem out of this patch. Yes, it has a similar issue as nocallback, but it's also a much higher impact change (fixing it properly basically means declaring that every single call can have a side-effect after linking) and that case definitely needs to deal with inference and cannot be limited to declarations. I think once we have a solution for nocallback, we should tackle inaccessiblemem as a second step.

I agree with that. Our first goal is to agree on the `nocallback` semantics, add the description into LVM Language Reference Manual.

>>> A nocallback intrinsic, where the attribute should never be dropped.
>>
>> Why is the case? @nikic and @jdoerfert can you explain the reason behind this? Why should we drop nocallback for user declared functions, but keep it for intrinsics?
>> We need to clarify 1).
>
> Intrinsic attributes are ... intrinsic to the intrinsic :) They must be legal regardless of where and how the intrinsic is used. If the intrinsic would not be nocallback in some circumstance, it cannot be nocallback at all. (This doesn't apply to call-site attributes, which raises the question of whether nocallback can be used as a call-site attribute -- if it can, then your patch would have to clear the attribute at call-sites as well, not just declarations. If it can't, then we have no problem.)

I'm ok to keep `nocallback` on intrinsics. Is there any chance that an existing intrinsic violates the restricted module-level `nocallback` semantics?
`nocallback` is a `function` attribute, not a `call site` attribute.

>>> A nocallback definition.
>>
>> We are not going to allow nocallback on definitions, and I think we are all on the same page for 2.
>
> I would be okay with this limitation, but if it exists, we need to enforce it. We need to add a check to the IR verifier that the attribute does not occur on definitions, and we need to make sure that Clang does not produce invalid IR. This means either diagnosing if the attribute would be placed on a definition or silently dropping it. (For the purposes of this patch, we probably don't strongly care about having the limitation -- we could also omit the declaration check and clear the attribute on all functions. I think the question of call-site attributes is more problematic.)

In the recent revision, I dropped `nocallback` attribute from the definitions as well.

> On that note, does someone have an example of how `__attribute__((nocallback))` is getting used in the wild? Do people use the preprocessor to add the nocallback attribute in the header only for external users of the library, but omit it when including the header in the implementation (as no callback is likely not valid there?)
>
>> Whether the definition is being linked in or not, we will drop the nocallback attribute. I think, we are all on the same page for 3 and 4 as well.
>
> Yes, I think this is right.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137360