[LLD] [COFF] Fix automatic export of symbols from LTO objects

[mstorsjo]

Summary

This isn't quite pretty, but it seems to work at least. Suggestions
for how to do things better are very welcome. I'll make a few inline
comments about a few surprising bits.

[mstorsjo]

lld/COFF/Driver.cpp

1223

It turns out we can't actually call def->getChunk() on a DefinedRegular from a bitcode object file, because that symbol has got the SectionChunk **data; member set to nullptr, and getChunk() dereferences *data, so we have to do this dance so we won't crash on calling def->getChunk().

1232

Here we end up iterating over all symbols in the input bitcode file, to find the one we're looking at, to figure out whether it's data or a function. Not pretty...

2125

Here we need to decide what symbols to export before we run addCombinedLTOObjects().

lld/COFF/MinGW.cpp

126 ↗

(On Diff #341632)

The ->getChunk() call from here stemmed from the original iplementation of this feature, where it was added just as a guesstimate of what would be a decent condition - for LTO symbols we must tolerate quite different symbols.

[rnk]

lld/COFF/Driver.cpp

1223

Exciting. I took a look at LTO support a while back, and I wanted to rearrange how some of this stuff worked, but I never got around to it. I think I wanted to make some kind of DefinedBitcode symbol or something like that so that the fields would be less overloaded and more uniform.

1232

Yeah, it would be nice to figure out how to avoid this, since it's O(n^2)-ish.

One way would be to store this bit directly on the DefinedRegular symbol object.

Another way would be to start making fake, synthetic SectionChunk objects for symbols from bitcode files. We could have the standard text, data, rdata, bss sections, for example. This would make the previous conditional unnecessary. That seems better.

The least invasive way would be to separately iterate over the bitcode input files and look their symbols up in the symbol table and use that to create exports. However, symbol resolution is already expensive, and this is basically a second symbol resolution phase.

I kind of lean towards the second option. It leads to less null checks in the long run.

2125

Yes, that makes sense: we can't really do effective LTO without knowing what the entry points are going to be.

[mstorsjo]

lld/COFF/Driver.cpp

1232

You mean your preference is synthetic section chunks?

That does sound nice as it'd avoid null checks, but wouldn't it be a huge waste (both mem and time) to do that for all bitcode object file symbols, for the odd chance of wanting to inspect some of them for exporting - just for the short period of time until we replace them with the LTO compiled output?

Or should there be like 1 common fake section chunk used for all LTO symbols, for each symbol type (func/data)?

[rnk]

lld/COFF/Driver.cpp

1232

I was thinking we could have one fake section chunk for the standard types of LLVM IR GlobalValues: .text for Function, .data for GlobalVariable, .rdata for Constant, etc. Something like that. I think it's just a matter of passing extra parameters to addRegular in BitcodeFile::parse.

[mstorsjo]

Simplified (somewhat) by adding fake SectionChunks for LTO symbols until they're compiled.

There's still some amount of ugliness here; the SectionChunk ctor needs to be made to cope with a null ObjFile, and that this runs before config is initialized. Any ideas for how to make that bit neater?

[rnk]

Nice, I think this is a reasonable direction.

lld/COFF/InputFiles.cpp

1045

Please put this in an anonymous namespace and add a comment that this is just adding a constructor to help stamp out COFF section headers. For some reason I thought it was adding fields. Actually, maybe it would be less surprising if the coff_section was a plain field, and then we can take its address to construct the SectionChunk objects.

1047

Can this be marked constexpr? We want to avoid any dynamic initialization if possible.

1052

Please mark the globals static.

1054

The SectionChunk constructor is non-trivial, so this will definitely create dynamic initialization. I guess that's OK, LLD isn't really structured as a library anyway.

[mstorsjo]

lld/COFF/InputFiles.cpp

1047

Hmm, I'll have to see.

1054

Yeah this bit isn't ideal. I guess it's still tolerable for a library, even if it isn't optimal.

[mstorsjo]

lld/COFF/InputFiles.cpp

1047

Doesn't seem like it's doable to make it constexpr in this form, it'd have to be e.g. constexpr FakeSection(int c) : section(<initializer expression>) {}, it seems (at least in C++11).

The most straightforward would be C99 style designated intializers, i.e coff_section fakeSection = { .Characteristics = 42; };, but that's only available in C++20. And I'd rather not enumerate the members by hand to create something like coff_section fakeSection = { 0, 0, 0, 0, 0, 0, importantValue }.

Also, coff_section consists of support::ulittle16_t, which don't seem to be constexpr'able, so I'm not sure if it'd even work like that in C++20?

(This is all kinda roundabout, when what we really want just is a couple static bytes laid out in a specific way, but accessible for the caller via the coff_section class.)

[mstorsjo]

Added a comment about the fake section class, made the coff_section a member instead of subclassing it, wrapped the globals in an anonymous namespace.

[mstorsjo]

Apply clang-format suggestions.

[mstorsjo]

Ping @rnk

[rnk]

lgtm, with a nit

lld/COFF/InputFiles.cpp

1047

Oh well.

1050

Wouldn't it be simpler to make the field public, and take the address of the field in the constructor below?

[mstorsjo]

lld/COFF/InputFiles.cpp

1050

I guess that would work just as well, and be clearer. Will change before pushing.

lld/COFF/Chunks.cpp

 using llvm::support::ulittle32_t;
 
 namespace lld {
 namespace coff {
 
 SectionChunk::SectionChunk(ObjFile *f, const coff_section *h)
     : Chunk(SectionKind), file(f), header(h), repl(this) {
   // Initialize relocs.
+  if (file)
     setRelocs(file->getCOFFObj()->getRelocations(header));
 
   // Initialize sectionName.
   StringRef sectionName;
+  if (file) {
     if (Expected<StringRef> e = file->getCOFFObj()->getSectionName(header))
       sectionName = *e;
+  }
   sectionNameData = sectionName.data();
   sectionNameSize = sectionName.size();
 
   setAlignment(header->getAlignment());
 
   hasData = !(header->Characteristics & IMAGE_SCN_CNT_UNINITIALIZED_DATA);
 
   // If linker GC is disabled, every chunk starts out alive.  If linker GC is
   // enabled, treat non-comdat sections as roots. Generally optimized object
   // files will be built with -ffunction-sections or /Gy, so most things worth
   // stripping will be in a comdat.
+  if (config)
     live = !config->doGC || !isCOMDAT();
+  else
+    live = true;
 }
 
 // SectionChunk is one of the most frequently allocated classes, so it is
 // important to keep it as compact as possible. As of this writing, the number
 // below is the size of this class on x64 platforms.
 static_assert(sizeof(SectionChunk) <= 88, "SectionChunk grew unexpectedly");
 
 static void add16(uint8_t *p, int16_t v) { write16le(p, read16le(p) + v); }

lld/COFF/Driver.cpp

symtab->forEachSymbol([&](Symbol *s) {
     }
 
     Export e;
     e.name = def->getName();
     e.sym = def;
     if (Chunk *c = def->getChunk())
       if (!(c->getOutputCharacteristics() & IMAGE_SCN_MEM_EXECUTE))
         e.data = true;
+    s->isUsedInRegularObj = true;
     config->exports.push_back(e);
   });
 }
 
 // lld has a feature to create a tar file containing all input files as well as
 // all command line options, so that other people can run lld again with exactly
 // the same inputs. This feature is accessible via /linkrepro and /reproduce.
 //
 // /linkrepro and /reproduce are very similar, but /linkrepro takes a directory
 // name while /reproduce takes a full path. We have /linkrepro for compatibility
 // with Microsoft link.exe.
 Optional<std::string> getReproduceFile(const opt::InputArgList &args) {
   if (auto *arg = args.getLastArg(OPT_reproduce))
     return std::string(arg->getValue());
 
   if (auto *arg = args.getLastArg(OPT_linkrepro)) {
     SmallString<64> path = StringRef(arg->getValue());
void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   // If we are going to do codegen for link-time optimization, check for
   // unresolvable symbols first, so we don't spend time generating code that
   // will fail to link anyway.
   if (!BitcodeFile::instances.empty() && !config->forceUnresolved)
     symtab->reportUnresolvable();
   if (errorCount())
     return;
 
+  config->hadExplicitExports = !config->exports.empty();
+  if (config->mingw) {
+    // In MinGW, all symbols are automatically exported if no symbols
+    // are chosen to be exported.
+    maybeExportMinGWSymbols(args);
+  }
+
   // Do LTO by compiling bitcode input files to a set of native COFF files then
   // link those files (unless -thinlto-index-only was given, in which case we
   // resolve symbols and write indices, but don't generate native code or link).
   symtab->addCombinedLTOObjects();
 
   // If -thinlto-index-only is given, we should create only "index
   // files" and not object files. Index file creation is already done
   // in addCombinedLTOObject, so we are done if that's the case.
   if (config->thinLTOIndexOnly)
     return;
 
   // If we generated native object files from bitcode files, this resolves
   // references to the symbols we use from them.
   run();
 
   // Apply symbol renames for -wrap.
   if (!wrapped.empty())
     wrapSymbols(wrapped);
 
   // Resolve remaining undefined symbols and warn about imported locals.
   symtab->resolveRemainingUndefines();
   if (errorCount())
     return;
 
-  config->hadExplicitExports = !config->exports.empty();
   if (config->mingw) {
-    // In MinGW, all symbols are automatically exported if no symbols
-    // are chosen to be exported.
-    maybeExportMinGWSymbols(args);
-
     // Make sure the crtend.o object is the last object file. This object
     // file can contain terminating section chunks that need to be placed
     // last. GNU ld processes files and static libraries explicitly in the
     // order provided on the command line, while lld will pull in needed
     // files from static libraries only after the last object file on the
     // command line.
     for (auto i = ObjFile::instances.begin(), e = ObjFile::instances.end();
          i != e; i++) {

lld/COFF/InputFiles.cpp

MemoryBufferRef mbref(
                                      : archiveName + sys::path::filename(path) +
                                            utostr(offsetInArchive)));
 
   obj = check(lto::InputFile::create(mbref));
 }
 
 BitcodeFile::~BitcodeFile() = default;
 
+namespace {
+// Convenience class for initializing a coff_section with specific flags.
+class FakeSection {
+public:
+  FakeSection(int c) { section.Characteristics = c; }
+
+  coff_section section;
+};
+
+FakeSection ltoTextSection(IMAGE_SCN_MEM_EXECUTE);
+FakeSection ltoDataSection(IMAGE_SCN_CNT_INITIALIZED_DATA);
+SectionChunk ltoTextSectionChunk(nullptr, &ltoTextSection.section);
+SectionChunk ltoDataSectionChunk(nullptr, &ltoDataSection.section);
+} // namespace
+
 void BitcodeFile::parse() {
   std::vector<std::pair<Symbol *, bool>> comdat(obj->getComdatTable().size());
   for (size_t i = 0; i != obj->getComdatTable().size(); ++i)
     // FIXME: lto::InputFile doesn't keep enough data to do correct comdat
     // selection handling.
     comdat[i] = symtab->addComdat(this, saver.save(obj->getComdatTable()[i]));
   for (const lto::InputFile::Symbol &objSym : obj->symbols()) {
     StringRef symName = saver.save(objSym.getName());
     int comdatIndex = objSym.getComdatIndex();
     Symbol *sym;
+    SectionChunk *fakeSC = nullptr;
+    if (objSym.isExecutable())
+      fakeSC = &ltoTextSectionChunk;
+    else
+      fakeSC = &ltoDataSectionChunk;
     if (objSym.isUndefined()) {
       sym = symtab->addUndefined(symName, this, false);
     } else if (objSym.isCommon()) {
       sym = symtab->addCommon(this, symName, objSym.getCommonSize());
     } else if (objSym.isWeak() && objSym.isIndirect()) {
       // Weak external.
       sym = symtab->addUndefined(symName, this, true);
       std::string fallback = std::string(objSym.getCOFFWeakExternalFallback());
       Symbol *alias = symtab->addUndefined(saver.save(fallback));
       checkAndSetWeakAlias(symtab, this, sym, alias);
     } else if (comdatIndex != -1) {
       if (symName == obj->getComdatTable()[comdatIndex])
         sym = comdat[comdatIndex].first;
       else if (comdat[comdatIndex].second)
-        sym = symtab->addRegular(this, symName);
+        sym = symtab->addRegular(this, symName, nullptr, fakeSC);
       else
         sym = symtab->addUndefined(symName, this, false);
     } else {
-      sym = symtab->addRegular(this, symName);
+      sym = symtab->addRegular(this, symName, nullptr, fakeSC);
     }
     symbols.push_back(sym);
     if (objSym.isUsed())
       config->gcroot.push_back(sym);
   }
   directives = obj->getCOFFLinkerOpts();
 }
 

lld/test/COFF/export-all-lto.ll

+; REQUIRES: x86
+
+; RUN: llvm-as %s -o %t.bc
+
+; RUN: lld-link -lldmingw -dll -out:%t.dll %t.bc -noentry -output-def:%t.def
+; RUN: llvm-readobj --coff-exports %t.dll | grep Name: | FileCheck %s
+; RUN: cat %t.def | FileCheck --check-prefix=IMPLIB %s
+
+; CHECK: Name: MyExtData
+; CHECK: Name: MyLibFunc
+
+; IMPLIB: MyExtData @1 DATA
+; IMPLIB: MyLibFunc @2{{$}}
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-w64-windows-gnu"
+
+@MyExtData = dso_local global i32 42, align 4
+
+define dso_local void @MyLibFunc() {
+entry:
+  ret void
+}