This is an archive of the discontinued LLVM Phabricator instance.

[GlobalMerge] Preserve symbol visibility when merging globals
ClosedPublic

Authored by mspang on Jan 22 2020, 2:40 PM.

Details

Summary

Symbols created for merged external global variables have default
visibility. This can break programs when compiling with -Oz -fvisibility=hidden
as symbols that should be hidden will be exported at link time.

Diff Detail

Event Timeline

mspang created this revision.Jan 22 2020, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 2:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Makes sense.

llvm/test/CodeGen/AArch64/global-merge-hidden-minsize.ll
18

Missing CHECK lines to verify the globals were actually merged.

mspang updated this revision to Diff 239951.Jan 23 2020, 11:29 AM
mspang marked an inline comment as done.
mspang added inline comments.
llvm/test/CodeGen/AArch64/global-merge-hidden-minsize.ll
18

Added them. I wasn't sure if we needed to, since the command line is not asking for this specific optimization and the bug is just that we end up with wrong visibility. But I think I agree with you, better to fail if this case isn't merged so we can update the test.

mspang marked an inline comment as done.Jan 23 2020, 11:45 AM
efriedma accepted this revision.Jan 23 2020, 1:38 PM

LGTM

llvm/test/CodeGen/AArch64/global-merge-hidden-minsize.ll
18

Yes, the point of the extra CHECK lines is to ensure the test continues to test the transform in question.

This revision is now accepted and ready to land.Jan 23 2020, 1:38 PM

FYI, I don't have commit access so someone will have to commit on my behalf. Thanks.

FYI, I don't have commit access so someone will have to commit on my behalf. Thanks.

Author: Michael Spang <spang@google.com>

Worth to backport @hans

Anything left to do?

Oh, sorry, I lost track of this; I'll merge it today.

This revision was automatically updated to reflect the committed changes.
hans added a comment.Jan 29 2020, 12:39 PM

Worth to backport @hans

Okay, cherry-picked as 425198bf1f98e93be37b8675e29ac6d37529dc68

Hi @mspang, @hans,

the CodeGen/AArch64/global-merge-hidden-minsize.ll test gets failed on aarch64 builder
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/3704/steps/test-check-llvm/logs/FAIL%3A%20LLVM%3A%3Aglobal-merge-hidden-minsize.ll

with the following error:

error: unable to get target for 'arm-none-linux-gnu', see --version and --triple.

This is Aarch64 only builder without supporting for Arm32 triples. The test is inside Aarch64 specific test folder, but it also uses Arm32 triple.
Would you split this test and put the Arm32 specific part into CodeGen/ARM?

Thanks.
Vlad.

hans added a comment.Jan 30 2020, 6:03 AM

I've droped the arm triple from the test in 6be9acdfa814dee6c57833d5351137c72c11fbd3 and will cherry-pick that to 10.x as well.