This is an archive of the discontinued LLVM Phabricator instance.

LTO: Don't internalize available_externally globals.
ClosedPublic

Authored by pcc on Dec 3 2018, 3:28 PM.

Details

Summary

This breaks C and C++ semantics because it can cause the address
of the global inside the module to differ from the address outside
of the module.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Dec 3 2018, 3:28 PM
pirama added inline comments.Dec 3 2018, 3:32 PM
llvm/lib/LTO/LTO.cpp
363 ↗(On Diff #176495)

Use !GLobalValue::isAvailableExternallyLinkage(S->linkage()) instead?

Also, we should update the earlier comment.

pcc updated this revision to Diff 176503.Dec 3 2018, 3:44 PM
pcc marked an inline comment as done.

Add comment

llvm/lib/LTO/LTO.cpp
363 ↗(On Diff #176495)

I'd prefer not to since it boils down to a comparison and it's slightly clearer to be explicit. Added comment and changed the other condition to be explicit as well.

srhines added a subscriber: srhines.Dec 3 2018, 5:25 PM
tejohnson accepted this revision.Dec 4 2018, 4:05 PM

Lgtm

llvm/lib/LTO/LTO.cpp
362 ↗(On Diff #176503)

This change seems unnecessary?

This revision is now accepted and ready to land.Dec 4 2018, 4:05 PM
pcc added inline comments.Dec 4 2018, 4:09 PM
llvm/lib/LTO/LTO.cpp
362 ↗(On Diff #176503)

Just making it consistent with the code I'm adding below.

This revision was automatically updated to reflect the committed changes.