[BitcodeReader] Infer the correct runtime preemption for GlobalValue

[steven_wu]

Summary

To allow bitcode built by old compiler to pass the current verifer,
BitcodeReader needs to auto infer the correct runtime preemption from
linkage and visibility for GlobalValues.

Since llvm-6.0 bitcode already contains the new field but can be
incorrect in some cases, the attribute needs to be recomputed all the
time in BitcodeReader. This will make all the GVs has dso_local marked
correctly if read from bitcode, and it should still allow the verifier
to catch mistakes in optimization passes.

This should fix PR38009.

[vsk]

Thanks for doing this!

Could you add a test? Adding "opt -verify <old-bitcode-file>" run lines to the compatibility tests would be fine.

lib/Bitcode/Reader/BitcodeReader.cpp

2927 ↗

(On Diff #154449)

Could you factor this out into a helper which accepts a GlobalValue &?

[steven_wu]

In D49039#1154801, @vsk wrote:

Thanks for doing this!

Could you add a test? Adding "opt -verify <old-bitcode-file>" run lines to the compatibility tests would be fine.

You will be surprise how many things will break if you run verifier on old bitcode file. The first error you will hit is not about dso_local: Global is marked as dllimport, but not external

[vsk]

In D49039#1154866, @steven_wu wrote:

In D49039#1154801, @vsk wrote:

Thanks for doing this!

Could you add a test? Adding "opt -verify <old-bitcode-file>" run lines to the compatibility tests would be fine.

You will be surprise how many things will break if you run verifier on old bitcode file. The first error you will hit is not about dso_local: Global is marked as dllimport, but not external

In that case, we can enable verification on checked-in bitcode files as a follow-up. How about testing this by round-tripping textual IR through llvm-as and llvm-dis? You just need a function, alias, and global var with local linkage and without dso_local set, right?

[steven_wu]

In that case, we can enable verification on checked-in bitcode files as a follow-up. How about testing this by round-tripping textual IR through llvm-as and llvm-dis? You just need a function, alias, and global var with local linkage and without dso_local set, right?

Nope. dso_local is even implicit in assembly which is a completely different from bitcode reader/writer.

static void PrintDSOLocation(const GlobalValue &GV,
                             formatted_raw_ostream &Out) {
  // GVs with local linkage or non default visibility are implicitly dso_local,
  // so we don't print it.
  bool Implicit = GV.hasLocalLinkage() ||
                  (!GV.hasExternalWeakLinkage() && !GV.hasDefaultVisibility());
  if (GV.isDSOLocal() && !Implicit)
    Out << "dso_local ";
}

I guess this patch is unifying the behavior, sort of.

[steven_wu]

Address review feedback

[vsk]

In D49039#1154893, @steven_wu wrote:

In that case, we can enable verification on checked-in bitcode files as a follow-up. How about testing this by round-tripping textual IR through llvm-as and llvm-dis? You just need a function, alias, and global var with local linkage and without dso_local set, right?

Nope. dso_local is even implicit in assembly which is a completely different from bitcode reader/writer.

static void PrintDSOLocation(const GlobalValue &GV,
                             formatted_raw_ostream &Out) {
  // GVs with local linkage or non default visibility are implicitly dso_local,
  // so we don't print it.
  bool Implicit = GV.hasLocalLinkage() ||
                  (!GV.hasExternalWeakLinkage() && !GV.hasDefaultVisibility());
  if (GV.isDSOLocal() && !Implicit)
    Out << "dso_local ";
}

I guess this patch is unifying the behavior, sort of.

Hm, if there's no simpler option, what about checking in bitcode produced by 6.0 which would have triggered the verifier?

[steven_wu]

Add testcase using 6.0 bitcode.

[sfertile]

Thanks for doing this!

[vsk]

Thanks, LGTM!

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

if (!UseStrtab) {
     Name = OldFormatName;
   }
   Comdat *C = TheModule->getOrInsertComdat(Name);
   C->setSelectionKind(SK);
   ComdatList.push_back(C);
   return Error::success();
 }
 
+static void inferDSOLocal(GlobalValue *GV) {
+  // infer dso_local from linkage and visibility if it is not encoded.
+  if (GV->hasLocalLinkage() ||
+      (!GV->hasDefaultVisibility() && !GV->hasExternalWeakLinkage()))
+    GV->setDSOLocal(true);
+}
+
 Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   // v1: [pointer type, isconst, initid, linkage, alignment, section,
   // visibility, threadlocal, unnamed_addr, externally_initialized,
   // dllstorageclass, comdat, attributes, preemption specifier] (name in VST)
   // v2: [strtab_offset, strtab_size, v1]
   StringRef Name;
   std::tie(Name, Record) = readNameFromStrtab(Record);
 
Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   if (Record.size() > 12) {
     auto AS = getAttributes(Record[12]).getFnAttributes();
     NewGV->setAttributes(AS);
   }
 
   if (Record.size() > 13) {
     NewGV->setDSOLocal(getDecodedDSOLocal(Record[13]));
   }
+  inferDSOLocal(NewGV);
 
   return Error::success();
 }
 
 Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
   // v1: [type, callingconv, isproto, linkage, paramattr, alignment, section,
   // visibility, gc, unnamed_addr, prologuedata, dllstorageclass, comdat,
   // prefixdata,  personalityfn, preemption specifier] (name in VST)
if (Record.size() > 13 && Record[13] != 0)
     FunctionPrefixes.push_back(std::make_pair(Func, Record[13] - 1));
 
   if (Record.size() > 14 && Record[14] != 0)
     FunctionPersonalityFns.push_back(std::make_pair(Func, Record[14] - 1));
 
   if (Record.size() > 15) {
     Func->setDSOLocal(getDecodedDSOLocal(Record[15]));
   }
+  inferDSOLocal(Func);
 
   ValueList.push_back(Func);
 
   // If this is a function with a body, remember the prototype we are
   // creating now, so that we can match up the body with them later.
   if (!isProto) {
     Func->setIsMaterializable(true);
     FunctionsWithBodies.push_back(Func);
else
       upgradeDLLImportExportLinkage(NewGA, Linkage);
     if (OpNum != Record.size())
       NewGA->setThreadLocalMode(getDecodedThreadLocalMode(Record[OpNum++]));
     if (OpNum != Record.size())
       NewGA->setUnnamedAddr(getDecodedUnnamedAddrType(Record[OpNum++]));
   }
   if (OpNum != Record.size())
     NewGA->setDSOLocal(getDecodedDSOLocal(Record[OpNum++]));
+  inferDSOLocal(NewGA);
+
   ValueList.push_back(NewGA);
   IndirectSymbolInits.push_back(std::make_pair(NewGA, Val));
   return Error::success();
 }
 
 Error BitcodeReader::parseModule(uint64_t ResumeBit,
                                  bool ShouldLazyLoadMetadata) {
   if (ResumeBit)

llvm/trunk/test/Bitcode/infer_dso_local.ll

+; RUN: opt -verify %S/Inputs/infer_dso_local.bc | llvm-dis | FileCheck %s
+
+; CHECK: define linkonce_odr hidden void @test()