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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

> 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.

>> 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.)

>> 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.)

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.



================
Comment at: llvm/lib/Linker/LinkModules.cpp:390
+      if (!F->doesNotAccessMemory() && F->onlyAccessesInaccessibleMemory())
+        F->removeFnAttr(llvm::Attribute::Memory);
+      if (F->hasFnAttribute(llvm::Attribute::NoCallback))
----------------
I don't think we should include it in this patch, but if we do, this is not the right way: What you want to do is always add inaccessiblemem as an accessible location. You would have to do something like this:

```
F->setMemoryEffects(F->getMemoryEffects() | MemoryEffects::inaccessibleMemOnly());
```


================
Comment at: llvm/lib/Linker/LinkModules.cpp:392
+      if (F->hasFnAttribute(llvm::Attribute::NoCallback))
+        F->removeFnAttr(llvm::Attribute::NoCallback);
+    }
----------------
You don't need to check hasFnAttribute() first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137360