Initialize global vectors with invalid members to catch uninitialization errors

[ruiu]

Summary

If you are using lld as a library, and if you are calling lld more than
once in your process, you may encounter a problem that lld runs correctly
only on the first run. This is often caused by global vectors that are
not emptied on each linker invocation.

This patch initializes global vectors with nullptr elements so that
we need to reset them even for the first run of the linker to make
it easy to catch errors.

[MaskRay]

We have other non-vector global variables that cannot be checked this way: SyntheticSections.h InStruct in, OutputSections.h Out::*, LinkerScript.h script, Config.h config, Relocations.cpp undefs, etc. My feeling is that if we want to detect all potential problems, we can hack a ld.lld server, let ld.lld commands invoked by lit send linking requests to the server. Many tests run several ld.lld commands. This can probably catch most problems. The idea is simple but it may not be worthwhile implemented in the code base, though.

[ruiu]

The other way of testing this is actually invoking the linker twice from main(), only in a test mode (specified by some environment variable), to see if it works. It is doable, and it should be able to detect all errors, but I'm not sure if we want to run tests twice.

[blackhole12]

I am not particularly convinced that this will fix the problem, as it simply moves the problem around - instead of having to remember to clean up global variables, now programmers have to remember to initialize them with invalid values. This would certainly make it easier to detect errors once you realize there is one, but if someone silently adds a global variable and forgets to initialize it, you're back at square one. The only reliable way to fix the problem is to create an object whose constructor or destructor is automatically called when necessary, which is why I suggested a global container object. Adding anything to the global container object doesn't require the programmer to remember anything, because the destructor will automatically destruct everything inside of it. If this is simply not possible, some sort of baseline test as suggested by MaskRay is likely your only option.

[peter.smith]

One thing that might work better than potentially random problems if the containers aren't cleared, is some kind of assert only, or self-diagnosis mode that could give an assert or error message that a data-structure hadn't been cleared. For example assert(inputSections.empty()); assert(In.bss == nullptr);. With asserts checking I think we could get away with a small set of tests to run twice with some --self-diagnostic-option to check the majority of the cases.

I agree with blackhole12 that this doesn't solve the problem for new global variables, it only mitigates refactoring of existing ones although it does provide a strong hint as to what should be done. A RAII container for the existing containers could help, although it again suffers from the problem that we'd need to remember to use one for new globals. I think it would need to be combined with some kind of testing.

[grimar]

Does it make sense to accumulate all global variables in the one place? Like place them to a separate file with initGlobals method?
I think one of their problems is that they spreaded everywhere and initialized from many places.

[ruiu]

In D70421#1751289, @grimar wrote:

Does it make sense to accumulate all global variables in the one place? Like place them to a separate file with initGlobals method?
I think one of their problems is that they spreaded everywhere and initialized from many places.

Yeah, that might be both practical and effective, let me try that to see how it will look like.

lld/COFF/Driver.cpp

errorHandler().errorLimitExceededMsg =
       "too many errors emitted, stopping now"
       " (use /errorlimit:0 to see all errors)";
   errorHandler().exitEarly = canExitEarly;
   enableColors(stderrOS.has_colors());
 
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
+  ObjFile::instances.clear();
+  ImportFile::instances.clear();
+  BitcodeFile::instances.clear();
+  memset(MergeChunk::instances, 0, sizeof(MergeChunk::instances));
+
   config = make<Configuration>();
   symtab = make<SymbolTable>();
   driver = make<LinkerDriver>();
 
   driver->link(args);
 
   // Call exit() if we can to avoid calling destructors.
   if (canExitEarly)
     exitLld(errorCount() ? 1 : 0);
 
   freeArena();
-  ObjFile::instances.clear();
-  ImportFile::instances.clear();
-  BitcodeFile::instances.clear();
-  memset(MergeChunk::instances, 0, sizeof(MergeChunk::instances));
   return !errorCount();
 }
 
 // Parse options of the form "old;new".
 static std::pair<StringRef, StringRef> getOldNewOptions(opt::InputArgList &args,
                                                         unsigned id) {
   auto *arg = args.getLastArg(id);
   if (!arg)

lld/COFF/InputFiles.cpp

std::string toString(const coff::InputFile *file) {
 
   return (getBasename(file->parentName) + "(" + getBasename(file->getName()) +
           ")")
       .str();
 }
 
 namespace coff {
 
-std::vector<ObjFile *> ObjFile::instances;
-std::vector<ImportFile *> ImportFile::instances;
-std::vector<BitcodeFile *> BitcodeFile::instances;
+std::vector<ObjFile *> ObjFile::instances = {nullptr};
+std::vector<ImportFile *> ImportFile::instances = {nullptr};
+std::vector<BitcodeFile *> BitcodeFile::instances = {nullptr};
 
 /// Checks that Source is compatible with being a weak alias to Target.
 /// If Source is Undefined and has no weak alias set, makes it a weak
 /// alias to Target.
 static void checkAndSetWeakAlias(SymbolTable *symtab, InputFile *f,
                                  Symbol *source, Symbol *target) {
   if (auto *u = dyn_cast<Undefined>(source)) {
     if (u->weakAlias && u->weakAlias != target) {

lld/COFF/Writer.cpp

 
 static const int dosStubSize = sizeof(dos_header) + sizeof(dosProgram);
 static_assert(dosStubSize % 8 == 0, "DOSStub size must be multiple of 8");
 
 static const int numberOfDataDirectory = 16;
 
 // Global vector of all output sections. After output sections are finalized,
 // this can be indexed by Chunk::getOutputSection.
-static std::vector<OutputSection *> outputSections;
+static std::vector<OutputSection *> outputSections = {nullptr};
 
 OutputSection *Chunk::getOutputSection() const {
   return osidx == 0 ? nullptr : outputSections[osidx - 1];
 }
 
 namespace {
 
 class DebugDirectoryChunk : public NonSectionChunk {
while (true) {
     pass++;
   }
 }
 
 // The main function of the writer.
 void Writer::run() {
   ScopedTimer t1(codeLayoutTimer);
 
+  outputSections.clear();
   createImportTables();
   createSections();
   createMiscChunks();
   appendImportThunks();
   createExportTable();
   mergeSections();
   removeUnusedSections();
   finalizeAddresses();

lld/ELF/Driver.cpp

bool link(ArrayRef<const char *> args, bool canExitEarly, raw_ostream &stdoutOS,
 
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
   inputSections.clear();
   outputSections.clear();
   binaryFiles.clear();
   bitcodeFiles.clear();
+  lazyObjFiles.clear();
   objectFiles.clear();
   sharedFiles.clear();
 
   config = make<Configuration>();
   driver = make<LinkerDriver>();
   script = make<LinkerScript>();
   symtab = make<SymbolTable>();
 

lld/ELF/InputFiles.cpp

else
       f->toStringCache = (f->archiveName + "(" + f->getName() + ")").str();
   }
   return f->toStringCache;
 }
 
 namespace elf {
 bool InputFile::isInGroup;
 uint32_t InputFile::nextGroupId;
-std::vector<BinaryFile *> binaryFiles;
-std::vector<BitcodeFile *> bitcodeFiles;
-std::vector<LazyObjFile *> lazyObjFiles;
-std::vector<InputFile *> objectFiles;
-std::vector<SharedFile *> sharedFiles;
+
+// These containers are initialized with invalid values to make sure
+// that we clear them before use. Clearing them on every linker
+// invocation is needed to allow users call lld's main function more
+// than once.
+std::vector<BinaryFile *> binaryFiles = {nullptr};
+std::vector<BitcodeFile *> bitcodeFiles = {nullptr};
+std::vector<LazyObjFile *> lazyObjFiles = {nullptr};
+std::vector<InputFile *> objectFiles = {nullptr};
+std::vector<SharedFile *> sharedFiles = {nullptr};
 
 std::unique_ptr<TarWriter> tar;
 
 static ELFKind getELFKind(MemoryBufferRef mb, StringRef archiveName) {
   unsigned char size;
   unsigned char endian;
   std::tie(size, endian) = getElfArchType(mb.getBuffer());
 

lld/ELF/InputSection.cpp

 
 namespace lld {
 // Returns a string to construct an error message.
 std::string toString(const elf::InputSectionBase *sec) {
   return (toString(sec->file) + ":(" + sec->name + ")").str();
 }
 
 namespace elf {
-std::vector<InputSectionBase *> inputSections;
+std::vector<InputSectionBase *> inputSections = {nullptr};
 
 template <class ELFT>
 static ArrayRef<uint8_t> getSectionContents(ObjFile<ELFT> &file,
                                             const typename ELFT::Shdr &hdr) {
   if (hdr.sh_type == SHT_NOBITS)
     return makeArrayRef<uint8_t>(nullptr, hdr.sh_size);
   return check(file.getObj().getSectionContents(&hdr));
 }