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

[Andres Freund via llvm-commits]

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