recommend using llvm-ar when finding undefined references and empty archives

[inglorion]

Summary

When we perform LTO builds with a version of ar that does not
understand LLVM bitcode objects, we end up with undefined references,
because our archive files do not list the bitcode symbols in their
indices. The error messages do not make it clear what the real problem
is. This change adds a note that points out the likely problem and
solution. It is similar in spirit to r282633, but aims to avoid false
positives by only triggering when we see both undefined references and
archives without symbols in their indices.

Fixes PR32281.

[inglorion]

ELF/Error.cpp

87 ↗

(On Diff #91954)

Since we can't lock a mutex we already hold, exitLld must be called without holding the lock on Mu. This makes the structure of the code here quite unfortunate. Is there a more elegant way to get this to work?

[ruiu]

I think this patch changes too many files. You could simplify this by constructing an error message string in ArchiveFile::parse (to briefly say something like that "foo.a: no symbol table found. If you are attempting LTO, you need to use llvm-ar instead of ar to create static archives.") and store that to Config. So the code to add parse would be something like

Config->EmptyArchiveErrors.push_back(toString(*this) + ":  symbol table missing ...");

where Config->EmptyArchiveErrors is of std::vector<std::string>.

And then print out these messages in reportUndefined.

[inglorion]

@ruiu: Yes, that would be a lot simpler. Let me explain why I did it the way I did it, and then I'll be happy to simplify it if you still feel that's better.

I thought about emitting the message from Relocations.cpp, but then I thought about ErrorLimit. If I emit the note about empty archives after the undefined symbols, it is quite likely that it would not be emitted because the error limit is reached before getting to that point. If I emit the note first, it's easily missed because there may be a whole bunch of undefined reference errors after it. Thinking about that some, I thought it would be most useful if the note is emitted just before lld exits - and that this is actually something that may be more generically useful. That's why I added support for this to Errors. Using this new mechanism, the note is not affected by ErrorLimit and is emitted last, where it should be most effective.

I also thought about reporting the names of the archives in which we found no or empty indices, but my guess is that there isn't a lot of value to that; the most important thing is that this situation may be affecting your build, and switching ar implementations will fix the problem if that's the case. Having the list of file names is more text on the screen, and more memory usage to carry them around, and I didn't feel the benefits outweigh the cost, so I went with a simple boolean.

[ruiu]

ErrorLimit is 20. Isn't it enough to print out both unresolved symbols and suspicious archive files, is it? If it is not enough, you can print out an error only for the first archive file that doesn't contain a symbol table.

[inglorion]

@ruiu, how is this?

[ruiu]

ELF/InputFiles.h

253 ↗

(On Diff #91960)

In this way, you need to reset the value in main because LLD should be able to be invoked more than once in the same process if it's used as a library. You want to store this boolean variable to Config.

ELF/Relocations.cpp

619 ↗

(On Diff #91960)

You only want to print out this message only once, no?

(nit: remove trailing "\n". I believe message() automatically appends that.)

[inglorion]

@ruiu's comments

[davide]

Reid (@rnk) pointed out this comes up during Chromium build so please make sure to verify the output is still reasonable with this patch (if I recall correctly libtool emits something similar (but it's more verbose) and the Chromium folks have to filter it out).

[inglorion]

Yes, I'll check Chromium builds once we're happy with the implementation.

[ruiu]

Let's try this. LGTM.

ELF/InputFiles.cpp

565 ↗

(On Diff #91963)

Format.

Let's eliminate the local varaible. You could replace NoSymbols with File->symbols().empty(). File->symbols() is cheap so you don't need to save time here.

ELF/Relocations.cpp

623 ↗

(On Diff #91963)

Remove the space before ".

[davide]

ELF/InputFiles.cpp

559–565 ↗

(On Diff #91963)

Why do you need two different flags?

565 ↗

(On Diff #91963)

should live on two different lines, no? (in general, clang-format).

ELF/Relocations.cpp

621 ↗

(On Diff #91963)

I'd say "the bitcode files".

[davide]

Other than the comments, this looks good

[inglorion]

ELF/InputFiles.cpp

559–565 ↗

(On Diff #91963)

By "two different flags" do you mean NoSymbols and ArchiveWithoutSymbolsSeen? I'll get rid of that by following ruiu's suggestion.

565 ↗

(On Diff #91963)

W.r.t. "two different lines": that's what I thought, too, but clang-format actually changed it to be one line. Shall I just go ahead and change it back to two lines?

ELF/Relocations.cpp

621 ↗

(On Diff #91963)

Hmm, yeah. I added the message because I ran into trouble with bitcode files, but it really is a more general situation, which is why I went with "object files" in the message. Is it ok to leave that as-is or shall I reword the message to something like "This can happen when creating archives with bitcode files (such as created by -flto) using a version of ar that does not know how to index those"?

[ruiu]

ELF/InputFiles.cpp

565 ↗

(On Diff #91963)

You want to make sure that you are using the LLVM style when formatting it with clang-format. Clang-format reflowed this line to use two lines for me.

[inglorion]

clang-format done right and removed NoSymbols local

[ruiu]

Still LGTM

[davide]

In D31011#704298, @ruiu wrote:

Still LGTM

Same.

lld/trunk/ELF/Config.h

struct Configuration {
   std::vector<llvm::StringRef> AuxiliaryList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> SymbolOrderingFile;
   std::vector<llvm::StringRef> Undefined;
   std::vector<SymbolVersion> VersionScriptGlobals;
   std::vector<SymbolVersion> VersionScriptLocals;
   std::vector<uint8_t> BuildIdVector;
   bool AllowMultipleDefinition;
+  bool ArchiveWithoutSymbolsSeen = false;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
   bool ColorDiagnostics = false;
   bool DefineCommon;
   bool Demangle = true;
   bool DisableVerify;
   bool EhFrameHdr;

lld/trunk/ELF/InputFiles.cpp

SymbolBody *elf::ObjectFile<ELFT>::createSymbolBody(const Elf_Sym *Sym) {
   }
 }
 
 template <class ELFT> void ArchiveFile::parse() {
   File = check(Archive::create(MB),
                MB.getBufferIdentifier() + ": failed to parse archive");
 
   // Read the symbol table to construct Lazy objects.
-  for (const Archive::Symbol &Sym : File->symbols())
+  for (const Archive::Symbol &Sym : File->symbols()) {
     Symtab<ELFT>::X->addLazyArchive(this, Sym);
   }
 
+  if (File->symbols().begin() == File->symbols().end())
+    Config->ArchiveWithoutSymbolsSeen = true;
+}
+
 // Returns a buffer pointing to a member file containing a given symbol.
 std::pair<MemoryBufferRef, uint64_t>
 ArchiveFile::getMember(const Archive::Symbol *Sym) {
   Archive::Child C =
       check(Sym->getMember(),
             "could not get the member for symbol " + Sym->getName());
 
   if (!Seen.insert(C.getChildOffset()).second)

lld/trunk/ELF/Relocations.cpp

static void reportUndefined(SymbolBody &Sym, InputSectionBase &S,
   if (Config->UnresolvedSymbols == UnresolvedPolicy::IgnoreAll ||
       (Config->UnresolvedSymbols == UnresolvedPolicy::Ignore && CanBeExternal))
     return;
 
   std::string Msg = S.getLocation<ELFT>(Offset) + ": undefined symbol '" +
                     toString(Sym) + "'";
 
   if (Config->UnresolvedSymbols == UnresolvedPolicy::WarnAll ||
-      (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal))
+      (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal)) {
     warn(Msg);
-  else
+  } else {
     error(Msg);
+    if (Config->ArchiveWithoutSymbolsSeen) {
+      message("At least one archive listed no symbols in its index."
+              " This can happen when creating archives with a version"
+              " of ar that does not understand the object files in"
+              " the archive. For example, if you are using LLVM"
+              " bitcode objects (such as created by -flto), you may"
+              " need to use llvm-ar or GNU ar with a plugin.");
+      // Reset to false so that we print the message only once.
+      Config->ArchiveWithoutSymbolsSeen = false;
+    }
+  }
 }
 
 template <class RelTy>
 static std::pair<uint32_t, uint32_t>
 mergeMipsN32RelTypes(uint32_t Type, uint32_t Offset, RelTy *I, RelTy *E) {
   // MIPS N32 ABI treats series of successive relocations with the same offset
   // as a single relocation. The similar approach used by N64 ABI, but this ABI
   // packs all relocations into the single relocation record. Here we emulate