Page MenuHomePhabricator

[ThinLTO] Drop dso_local if a GlobalVariable satisfies isDeclarationForLinker()
ClosedPublic

Authored by MaskRay on Feb 17 2020, 6:41 PM.

Details

Summary

dso_local leads to direct access even if the definition is not within this compilation unit (it is
still in the same linkage unit). On ELF, such a relocation (e.g. R_X86_64_PC32) referencing a
STB_GLOBAL STV_DEFAULT object can cause a linker error in a -shared link.

If the linkage is changed to available_externally, the dso_local flag should be dropped, so that no
direct access will be generated.

The current behavior is benign, because -fpic does not assume dso_local
(clang/lib/CodeGen/CodeGenModule.cpp:shouldAssumeDSOLocal).
If we do that for -fno-semantic-interposition (D73865), there will be an
R_X86_64_PC32 linker error without this patch.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 17 2020, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 6:41 PM
tejohnson added inline comments.Feb 21 2020, 2:18 PM
llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h
42

Needs comment. Also, maybe something more specific like ClearDSOLocalOnDeclarations ? Better yet, something that communicates what the condition is like IsPic.

127–128

Is a consistent name for the variable.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
218

It seems like we'd want to do the same thing here in the old LTO API whenever we have a TM (some of the callers of these routines do).

llvm/lib/Transforms/IPO/FunctionImport.cpp
1306–1307

document constant parameter, here and elsewhere (I see that is missing for the existing nullptr param, but better to not add more cases like that).

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

Needs comment.

MaskRay updated this revision to Diff 246020.Feb 21 2020, 3:30 PM
MaskRay marked 5 inline comments as done.

Address comments

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
218

Added some comments.

May I ask what is the old LTO API? Are most functions in ThinLTOCodeGenerator.cpp related to the old LTO API?

I try passing ClearDSOLocalOnDeclarations as long as the information is available. llvm-lto.cpp:707 ThinGenerator.crossModuleImport(*TheModule, *Index, *Input); does not pass the information.

MaskRay edited the summary of this revision. (Show Details)Feb 21 2020, 3:32 PM

Needs tests (both llvm-lto and llvm-lto2 to cover both old and new LTO APIs, respectively. For llvm-lto you can trigger the modified code via the -thinlto-action=run option.

llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h
127–128

That should have been "Use" a consistent name for variable btw. Please use "ClearDSOLocalOnDeclarations" for consistency.

llvm/lib/LTO/LTOBackend.cpp
556–557

Clang format

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
218

The old LTO API is used by linkers such as ld64 and Sony's proprietary linker and possibly others. I'm not sure if there are any that are ELF, but good to keep this up to date there as well since we still support both.

A few of these methods are only used for testing by llvm-lto, where we don't have a TM, so it's fine to skip it there.

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

s/is GV is/if GV is/

MaskRay updated this revision to Diff 251862.Mar 21 2020, 4:14 PM
MaskRay marked 4 inline comments as done.

Address review comments

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
218

It seems that the D35702 logic does not apply to the old API. GlobalValueSummary::setDSOLocal is never called.

// lib/LTO/LTO.cpp
      if (Res.FinalDefinitionInLinkageUnit) {
        if (auto S = ThinLTO.CombinedIndex.findSummaryInModule(
                GUID, BM.getModuleIdentifier())) {
          S->setDSOLocal(true);
        }
      }

Nevertheless, I added

; RUN: llvm-lto -thinlto-action=run %t1.bc %t2.bc -thinlto-save-temps=%t5.
; RUN: llvm-dis < %t5.1.3.imported.bc | FileCheck %s --check-prefix=OLDAPI_SRC
MaskRay updated this revision to Diff 251863.Mar 21 2020, 4:26 PM

Add proper old API tests to index-const-prop-gvref.ll
I can add dso_local to Inputs/index-const-prop-gvref.ll to exercise the code path for old API.

tejohnson accepted this revision.Apr 7 2020, 2:13 PM

LGTM

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
218

I see. Good to have the fix / test in place in case it ever gets supported there.

This revision is now accepted and ready to land.Apr 7 2020, 2:13 PM
This revision was automatically updated to reflect the committed changes.