[GlobalMerge] Preserve symbol visibility when merging globals

[mspang]

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.

[mspang]

https://bugs.llvm.org/show_bug.cgi?id=39413

[efriedma]

Makes sense.

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

19

Missing CHECK lines to verify the globals were actually merged.

[mspang]

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

19

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.

[efriedma]

LGTM

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

19

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

[mspang]

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

[mspang]

In D73235#1837126, @mspang wrote:

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

Author: Michael Spang <[e-mail]>

[xbolva00]

Worth to backport @hans

[mspang]

Anything left to do?

[efriedma]

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

[hans]

In D73235#1839317, @xbolva00 wrote:

Worth to backport @hans

Okay, cherry-picked as 425198bf1f98e93be37b8675e29ac6d37529dc68

[vvereschaka]

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]

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

llvm/lib/CodeGen/GlobalMerge.cpp

while (i != -1) {
 
     MergedGV->setAlignment(MaxAlign);
     MergedGV->setSection(Globals[i]->getSection());
 
     const StructLayout *MergedLayout = DL.getStructLayout(MergedTy);
     for (ssize_t k = i, idx = 0; k != j; k = GlobalSet.find_next(k), ++idx) {
       GlobalValue::LinkageTypes Linkage = Globals[k]->getLinkage();
       std::string Name = Globals[k]->getName();
+      GlobalValue::VisibilityTypes Visibility = Globals[k]->getVisibility();
       GlobalValue::DLLStorageClassTypes DLLStorage =
           Globals[k]->getDLLStorageClass();
 
       // Copy metadata while adjusting any debug info metadata by the original
       // global's offset within the merged global.
       MergedGV->copyMetadata(Globals[k],
                              MergedLayout->getElementOffset(StructIdxs[idx]));
 
for (ssize_t k = i, idx = 0; k != j; k = GlobalSet.find_next(k), ++idx) {
       // When the linkage is not internal we must emit an alias for the original
       // variable name as it may be accessed from another object. On non-Mach-O
       // we can also emit an alias for internal linkage as it's safe to do so.
       // It's not safe on Mach-O as the alias (and thus the portion of the
       // MergedGlobals variable) may be dead stripped at link time.
       if (Linkage != GlobalValue::InternalLinkage || !IsMachO) {
         GlobalAlias *GA = GlobalAlias::create(Tys[StructIdxs[idx]], AddrSpace,
                                               Linkage, Name, GEP, &M);
+        GA->setVisibility(Visibility);
         GA->setDLLStorageClass(DLLStorage);
       }
 
       NumMerged++;
     }
     Changed = true;
     i = j;
   }

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

+; RUN: llc %s -mtriple=arm-none-linux-gnu -o - | FileCheck %s
+; RUN: llc %s -mtriple=aarch64-none-linux-gnu -o - | FileCheck %s
+
+@x = hidden global i32 0, align 4
+@y = hidden global i32 0, align 4
+
+define hidden void @f() #0 {
+  store i32 0, i32* @x, align 4
+  store i32 0, i32* @y, align 4
+  ret void
+}
+
+attributes #0 = { minsize optsize }
+
+; CHECK: .local .L_MergedGlobals
+; CHECK: .comm .L_MergedGlobals,8,4
+
+; CHECK: .globl x
+; CHECK: .hidden x
+; CHECK: .set x, .L_MergedGlobals
+; CHECK: .size x, 4
+
+; CHECK: .globl y
+; CHECK: .hidden y
+; CHECK: .set y, .L_MergedGlobals+4
+; CHECK: .size y, 4