[PATCH] D139209: [IRMover] Remove UB implying parameter attributes when necessary

Tue Dec 6 07:17:32 PST 2022

tejohnson added inline comments.

================
Comment at: llvm/lib/Linker/IRMover.cpp:656
   if (auto *F = dyn_cast<Function>(New)) {
+    if (auto *SF = dyn_cast<Function>(SGV)) {
+      ensureFunctionCompatibility(*F, *SF);
----------------
TimNN wrote:
> tejohnson wrote:
> > It seems this can/should only be an issue when the dest module only contains a declaration, and we are only importing a declaration. Should we be checking that F->isDeclaration() (or doing it after that check falls through)? And should we further check that we wouldn't invoke linkGlobalValueBody() further below? Wondering if the change to ensure function attr compatibility is either unnecessary or would be an error in either of those cases.
> Doing this only when both are declarations makes sense, I think. (Either way, I'll move this to a separate `if` statement because all the cases here handle the "If we already created the body, just return." case).
> 
> While thinking about this I found an (IMO) more severe issue. I was initially thinking that definitions with internal linkage may need to get their attributes updated. Then I realized that reusing an existing symbol with internal linkage when importing a function was probably always wrong (because they could have completely different definitions).
> 
> When I experimented with this, I found that the current code actually _does_ (incorrectly, IIUC) reuse symbols with internal linkage, see https://github.com/llvm/llvm-project/issues/59347 for a repro. I feel like that should be fixed first before continuing this patch.
We promote to global scope and rename any local functions that are either exported by reference or definition to another module. I can see why that didn't happen with the test case you created, as you didn't do any post thin-link updates on the other module. Will make a comment on the github issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139209