This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Respect ClearDSOLocalOnDeclarations for unimported functions
ClosedPublic

Authored by MaskRay on Jun 26 2021, 4:50 PM.

Details

Summary

D74751 added ClearDSOLocalOnDeclarations and dropped dso_local for
isDeclarationForLinker GlobalValues. It missed a case for imported
declarations (doImportAsDefinition is false while isPerformingImport is
true). This can lead to a linker error for a default visibility symbol in
ld.lld -shared.

When ClearDSOLocalOnDeclarations is true, we check
isPerformingImport() && !doImportAsDefinition(&GV) along with
GV.isDeclarationForLinker(). The new condition checks an imported declaration.

This patch fixes a LLVMPolly.so link error using a trunk clang -DLLVM_ENABLE_LTO=Thin.

Diff Detail

Event Timeline

MaskRay created this revision.Jun 26 2021, 4:50 PM
MaskRay requested review of this revision.Jun 26 2021, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2021, 4:50 PM
tambre added a subscriber: tambre.Jun 27 2021, 2:07 AM

Ping:)

13.0.0 will be branched this month and I hope this can be addressed before that. (I could also drop -fno-semantic-interposition for Clang and keep it just for GCC but this issue should be addressed anyway)

tejohnson added inline comments.Jul 2 2021, 2:20 PM
llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
130

I don't follow what is happening with and without this change. If we aren't planning to import something as a definition, why do we need to mark it available_externally in the source module? How does this change result in dropping the dso local flag?

From your summary:

i.e. when isPerformingImport, an unimported define dso_local @foo should be treated as an available_externally function, instead of an ExternalLinkage definition.

It should eventually end up as an external declaration. Is that not happening? Or is the available_externally marking just a temporary adjustment to trigger subsequent dso_local clearing?

llvm/test/ThinLTO/X86/import-dsolocal.ll
63

Should this have linkonce_odr linkage?

68

Ditto

MaskRay updated this revision to Diff 356282.Jul 2 2021, 3:05 PM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)

Address comments

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
130

I got confused. The few cases where getLinkage returns ExternalLinkage means this is an imported declaration (I thought it means an unimported definition).

Returning AvailableExternallyLinkage is not good - we will get declare available_externally and available_externally shouldn't be used on declarations. This is benign because bitcode writer treats available_externally declaration like an external declaration.

I've reverted the part.

MaskRay updated this revision to Diff 356284.Jul 2 2021, 3:17 PM

Test weak/linkonce

tejohnson added inline comments.Jul 2 2021, 4:06 PM
llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
134

Is the change of the return linkage type needed for this fix, or is it just the returning of IsImportedDeclaration?

Why not just invoke doImportAsDefinition() in the caller to get this info rather than setting IsImportedDeclaration here? There are also other cases below for other linkage types where we are importing as a declaration but the new var isn't set.

MaskRay updated this revision to Diff 356295.Jul 2 2021, 4:23 PM
MaskRay edited the summary of this revision. (Show Details)

use doImportAsDefinition

lgtm with test fix noted below.

llvm/test/ThinLTO/X86/import-dsolocal.ll
34

Missing checks for linkonceodr[_aux].

tejohnson accepted this revision.Jul 2 2021, 4:50 PM
This revision is now accepted and ready to land.Jul 2 2021, 4:50 PM
MaskRay updated this revision to Diff 356303.Jul 2 2021, 5:04 PM
MaskRay marked 4 inline comments as done.

improve tests

MaskRay added inline comments.Jul 2 2021, 5:06 PM
llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
134

I think invoking doImportAsDefinition is superior. Switched the implementation.

MaskRay edited the summary of this revision. (Show Details)Jul 2 2021, 5:08 PM
This revision was landed with ongoing or failed builds.Jul 2 2021, 5:08 PM
This revision was automatically updated to reflect the committed changes.