[PATCH] D42122: Make GlobalValues with non-default visibilility dso_local

[Rafael Avila de Espindola via llvm-commits]

The underlying issue should be fixed by r323361. Can you give that a
try?

Thanks,
Rafael

Andres Freund <andres at anarazel.de> writes:

> On 2018-01-16 19:51:03 +0000, Rafael Avila de Espindola via Phabricator via llvm-commits wrote:
>> espindola created this revision.
>> espindola added reviewers: rnk, sfertile.
>> Herald added subscribers: hiraditya, eraman, emaste.
>> 
>> This is similar to r322317, but for visibility. It is not as neat because we have to special case extern_weak.
>> 
>> The idea is the same as the previous change, make the transition to explicit dso_local easier for the frontends. With this they only have to add dso_local to symbols where we need some external information to decide if it is dso_local (like it being part of an ELF executable.
>
> I just got a crash that seems related to this (or one of the sibling)
> changes:
> #0  (anonymous namespace)::Verifier::visitGlobalValue (this=0x7ffd79187170, GV=...) at /home/andres/src/llvm-master/lib/IR/Verifier.cpp:574
> #1  0x00007f5fe82f7465 in (anonymous namespace)::Verifier::visitFunction (this=0x7ffd79187170, F=...)
>     at /home/andres/src/llvm-master/lib/IR/Verifier.cpp:2002
> #2  0x00007f5fe8309732 in llvm::InstVisitor<(anonymous namespace)::Verifier, void>::visit (this=0x7ffd79187170, F=...)
>     at /home/andres/src/llvm-master/include/llvm/IR/InstVisitor.h:102
> #3  0x00007f5fe82ec823 in (anonymous namespace)::Verifier::verify (this=0x7ffd79187170, F=...)
>     at /home/andres/src/llvm-master/lib/IR/Verifier.cpp:349
> #4  0x00007f5fe8307beb in llvm::verifyModule (M=..., OS=0x7f5fe7e9ea40 <llvm::errs()::S>, BrokenDebugInfo=0x7ffd7918789a)
>     at /home/andres/src/llvm-master/lib/IR/Verifier.cpp:4665
> #5  0x00007f5fe80dc3c9 in llvm::UpgradeDebugInfo (M=...) at /home/andres/src/llvm-master/lib/IR/AutoUpgrade.cpp:2513
> #6  0x00007f5fe895f17b in (anonymous namespace)::BitcodeReader::materializeModule (this=0x558cf686ac30)
>     at /home/andres/src/llvm-master/lib/Bitcode/Reader/BitcodeReader.cpp:4776
> #7  0x00007f5fe829b37a in llvm::Module::materializeAll (this=0x558cf686b570) at /home/andres/src/llvm-master/lib/IR/Module.cpp:397
> #8  0x00007f5fe8965711 in llvm::BitcodeModule::getModuleImpl (this=0x7ffd79187e70, Context=..., MaterializeAll=true, 
>     ShouldLazyLoadMetadata=false, IsImporting=false) at /home/andres/src/llvm-master/lib/Bitcode/Reader/BitcodeReader.cpp:5647
> #9  0x00007f5fe89661ad in llvm::BitcodeModule::parseModule (this=0x7ffd79187e70, Context=...)
>     at /home/andres/src/llvm-master/lib/Bitcode/Reader/BitcodeReader.cpp:5761
> #10 0x00007f5fe8966248 in llvm::parseBitcodeFile (Buffer=..., Context=...)
>     at /home/andres/src/llvm-master/lib/Bitcode/Reader/BitcodeReader.cpp:5772
> #11 0x00007f5fe89393df in LLVMParseBitcodeInContext2 (ContextRef=0x558cf66bfee0, MemBuf=0x558cf6b12220, OutModule=0x7ffd79188070)
>     at /home/andres/src/llvm-master/lib/Bitcode/Reader/BitReader.cpp:66
> #12 0x00007f5fe8939157 in LLVMParseBitcode2 (MemBuf=0x558cf6b12220, OutModule=0x7ffd79188070)
>     at /home/andres/src/llvm-master/lib/Bitcode/Reader/BitReader.cpp:33
>
> (gdb) p GV
> $6 = (const llvm::GlobalValue &) @0x561f82ef0388: {<llvm::Constant> = {<llvm::User> = {<llvm::Value> = {VTy = 0x561f82dbb120, 
>         UseList = 0x561f82f269d0, SubclassID = 0 '\000', HasValueHandle = 1 '\001', SubclassOptionalData = 0 '\000', SubclassData = 0, 
>         NumUserOperands = 0, IsUsedByMD = 0, HasName = 1, HasHungOffUses = 1, HasDescriptor = 0, static MaxAlignmentExponent = 29, 
>         static MaximumAlignment = 536870912}, <No data fields>}, <No data fields>}, ValueType = 0x561f82dbb0e8, 
>   static GlobalValueSubClassDataBits = 17, Linkage = 7, Visibility = 0, UnnamedAddrVal = 0, DllStorageClass = 0, ThreadLocal = 0, 
>   HasLLVMReservedName = 0, IsDSOLocal = 0, SubClassData = 32, IntID = llvm::Intrinsic::not_intrinsic, Parent = 0x561f82646400}
>
> The loaded bitcode file was created by clang-5.0, the crash was at
> revision https://llvm.org/svn/llvm-project/llvm/trunk@323068, while
> https://llvm.org/svn/llvm-project/llvm/trunk@322241 still worked.
>
> That doesn't seem ok?
>
> If I add a rountrip to text IR representation using the *old* llvm-dis,
> everything works.
>
> It seems to me that the problem is that setVisibility() doesn't
> setDSOLocal(true) when default visiblity is used:
>   void setVisibility(VisibilityTypes V) {
>     assert((!hasLocalLinkage() || V == DefaultVisibility) &&
>            "local linkage requires default visibility");
>     Visibility = V;
>     if (!hasExternalWeakLinkage() && V != DefaultVisibility)
>       setDSOLocal(true);
>   }
>
> but the triggering Assert doesn't except DefaultVisibility:
>   if (GV.hasLocalLinkage())
>     Assert(GV.isDSOLocal(),
>            "GlobalValue with private or internal linkage must be dso_local!",
>            &GV);
>
> Greetings,
>
> Andres Freund