Page MenuHomePhabricator

ThinLTOBitcodeWriter: drop dso_local when a GlobalVariable is converted to a declaration
ClosedPublic

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

Details

Summary

If we infer the dso_local flag for -fpic, dso_local should be dropped
when we convert a GlobalVariable a declaration. dso_local causes the
generation of direct access (e.g. R_X86_64_PC32). Such relocations referencing
STB_GLOBAL STV_DEFAULT objects are not allowed in a -shared link.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 17 2020, 6:40 PM

Should there be a verifier check that there is no dso_local on declarations (after this and D74751 go in)?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
186

Better to do this inside convertToDeclaration, since there are other places we use that to drop definitions. Also logically it belongs there.

llvm/test/Transforms/ThinLTOBitcodeWriter/split-dsolocal.ll
8

This checking seems unnecessary in this test.

MaskRay updated this revision to Diff 245998.Feb 21 2020, 2:40 PM
MaskRay marked 3 inline comments as done.

Address comments

tejohnson added inline comments.Feb 21 2020, 3:57 PM
llvm/lib/Transforms/IPO/FunctionImport.cpp
993 ↗(On Diff #245998)

Don't we also need it here (alias case)?

MaskRay marked an inline comment as done.Feb 21 2020, 4:22 PM
MaskRay added inline comments.
llvm/lib/Transforms/IPO/FunctionImport.cpp
993 ↗(On Diff #245998)

GlobalAlias does not need it, but I'll add a test case that an alias works.

I will also add a hidden GlobalVariable case. If we don't check if (!GV.isImplicitDSOLocal()), setting a hidden GlobalVariable declaration dso_preemptable will trigger a verifier error (IR/Verifier.cpp:visitGlobalValue).

MaskRay updated this revision to Diff 246039.Feb 21 2020, 4:22 PM

Improve tests

MaskRay updated this revision to Diff 248558.Mar 5 2020, 11:28 AM

Fix not-prevailing-variables.ll (this patch fixes a copy relocation)

tejohnson accepted this revision.Mar 5 2020, 5:45 PM

LGTM

llvm/lib/Transforms/IPO/FunctionImport.cpp
993 ↗(On Diff #245998)

Oh ic why, because we create a new decl without it in the alias case. Nevermind.

This revision is now accepted and ready to land.Mar 5 2020, 5:45 PM
This revision was automatically updated to reflect the committed changes.
hoyFB added a subscriber: hoyFB.Mar 6 2020, 12:13 PM

Hello, It looks like this change is breaking the test llvm/test/tools/gold/X86/thinlto_weak_library.ll . Can you please verify? Thanks!

Hello, It looks like this change is breaking the test llvm/test/tools/gold/X86/thinlto_weak_library.ll . Can you please verify? Thanks!

My other llvm-objdump -d change also caused a disassembly test to fail.

Fixed by c3de1d0b1f99926f4a2665756b29b2d35a8359b8

hoyFB added a comment.Mar 6 2020, 2:06 PM

Hello, It looks like this change is breaking the test llvm/test/tools/gold/X86/thinlto_weak_library.ll . Can you please verify? Thanks!

My other llvm-objdump -d change also caused a disassembly test to fail.

Fixed by c3de1d0b1f99926f4a2665756b29b2d35a8359b8

Thanks for the fast turnaround!