ThinLTO incorrectly imports available_externally definition to replace weak definition

[smeenai]

$ cat av_ext_def.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
@def = available_externally constant i32 0
define ptr @ret_av_ext_def() {
  ret ptr @def
}

$ cat weak_def.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
@def = weak constant i32 0
define ptr @ret_weak_def() {
  ret ptr @def
}

$ opt -module-summary av_ext_def.ll -o av_ext_def.bc
$ opt -module-summary weak_def.ll -o weak_def.bc
$ llvm-lto2 run -o bad_import -save-temps av_ext_def.bc weak_def.bc -r=av_ext_def.bc,ret_av_ext_def,px -r=av_ext_def.bc,def,x -r=weak_def.bc,ret_weak_def,px -r=weak_def.bc,def,px -debug-only=function-import
Live root: 4762193002289486 (def)
Live root: 4976220419557185952 (ret_weak_def)
Live root: 17495876165006506858 (ret_av_ext_def)
3 symbols Live, and 0 symbols Dead
Computing import for Module 'weak_def.bc'
Initialize import for 4976220419557185952 (ret_weak_def)
 ref -> 4762193002289486 (def)
Computing import for Module 'av_ext_def.bc'
Initialize import for 17495876165006506858 (ret_av_ext_def)
Ref ignored! Target already in destination module.
Import/Export lists for 2 modules:
* Module av_ext_def.bc exports 0 functions and 1 vars. Imports from 0 modules.
* Module weak_def.bc exports 0 functions and 0 vars. Imports from 1 modules.
 - 0 functions imported from av_ext_def.bc
 - 1 global vars imported from av_ext_def.bc
Starting import for Module av_ext_def.bc
Imported 0 functions for Module av_ext_def.bc
Imported 0 global variables for Module av_ext_def.bc
Starting import for Module weak_def.bc
Not importing function 17495876165006506858 ret_av_ext_def from av_ext_def.ll
Is importing global 4762193002289486 def from av_ext_def.ll
Imported 0 functions for Module weak_def.bc
Imported 1 global variables for Module weak_def.bc

We're importing the available_externally definition from av_ext.def.bc into weak_def.bc, and so we end up not emitting a definition for def in the object file corresponding to weak_def.bc:

$ nm bad_import.2
                 U def
0000000000000000 T ret_weak_def

I believe this is incorrect because according to the LangRef, "unreferenced globals with weak linkage may not be discarded". I'm not sure how that interacts with LTO, but I'm also supplying a resolution for weak_def.bc,def that's prevailing and VisibleToRegularObj, which I thought would have been sufficient. This results in link errors down the line from the missing definition.

The relevant logic is here, which mentions a case where the def with interposable linkage is non-prevailing. In this case though, the def with interposable linkage is prevailing.

Does my analysis seem correct? If so, is returning false for a prevailing definition in shouldImportGlobal the best fix, or should we add the check somewhere else (e.g. Index.canImportGlobalVar)?

CC @teresajohnson

[teresajohnson]

Thanks for the detailed write up. This handling was added back in https://reviews.llvm.org/D95943. It was a tricky issue, but it looks like I convinced myself that anything we would decide to import would be either a strong def or odr equivalent. I wasn't thinking about the case where the linkage type is available externally. Rather than try to adjust the behavior based on the linkage type of the copy we want to import, I think it would be best just to pass in info to determine whether the interposable copy is prevailing or not and use that.

I.e. you can change the callsites like in LTO.cpp to pass the isPrevailing callback to ComputeCrossModuleImport (the isPrevailing definition can just be moved up to be above ComputeCrossModuleImport so it is available). Then return false from shouldImportGlobal for a prevailing interposable linkage.

[smeenai]

Thank you for the quick reply!

I started working on threading isPrevailing through to shouldImportGlobal where it's needed, but unfortunately it's a pretty long chain of calls to get there. The one that I'm stuck on is (all inside llvm/lib/Transforms/IPO/FunctionImport.cpp):

FunctionImportPass::run -> doImportingForModule -> ComputeCrossModuleImportForModule -> ComputeImportForModule -> computeImportForReferencedGlobals -> shouldImportGlobal

I can figure out how to get an appropriate isPrevailing for all other call-sites of ComputeImportForModule, but this particular one is originating from the pass run function, which only has access to the Module and ModuleAnalysisManager. doImportingForModule additionally looks up the ModuleSummaryIndex for the Module, but I'm not sure if there's any way to look up the prevailing information similarly. Do you have any suggestions for this?

[teresajohnson]

That's only used when testing the function import pass via the opt tool. I would just conservatively create a callback lambda that says everything is prevailing (just returns true).

[smeenai]

Thanks again :) I put up https://reviews.llvm.org/D146876 for this.