[ELF] - Linkerscript: better diagnostic for INPUT/GROUP commands.

[grimar]

Summary

Imagine we have script with following line:

INPUT(no_such_file)

Previously we would report next error: "cannot open no_such_file: No such file
or directory". It provides no information that file was tried to be opened from
linker script. And it is confusing, because some times we have regular inputs that
looks like libraries, for example libm.so, but they are linker scripts inside.
It is hard to find where error is coming from without debugging.

Patch adds reporting of script file name.

[ruiu]

I think it makes more sense to print out the error message if Error becomes true.

[grimar]

In D35945#823283, @ruiu wrote:

I think it makes more sense to print out the error message if Error becomes true.

That will be less intrusive but probably less straightforward as well. Will update the diff.

[grimar]

• Addressed review comment.

[grimar]

Ping.

[ruiu]

ELF/ScriptParser.cpp

285–289

I think you can only check ErrorCount. Remove Error.

[grimar]

• Addressed review comment.

[ruiu]

ELF/ScriptParser.cpp

311

Ditto

[grimar]

ELF/ScriptParser.cpp

311

Why ? I think readAsNeeded() can set Error.

[ruiu]

ELF/ScriptParser.cpp

311

Whenever error is true, ErrorCount should be greater than 0.

[grimar]

ELF/ScriptParser.cpp

311

I do not think so. What if we will have:

AS_NEEDED#valid_file_name)

'readAsNeeded()' expects '(', but see # and triggers Error, then adds file correctly and we have Error set, but ErrorCount == 0.
So looks readGroup() is fine.

Honestly I think watching these flags separatelly starting to be complicated. May be better to leave initial version, where both 'Error' and 'ErrorCount' were checked in readAsNeeded() ? There is no need to perform attemp to open file in case I showed above.

[grimar]

ELF/ScriptParser.cpp

311

Ah, I missed that setError also calls error() which sets ErrorCount to be > 0. Please ignore this comment.

[grimar]

ELF/ScriptParser.cpp

311

I suggest to switch to always use ErrorCount: D36266

[grimar]

• Reimplemented in a more generic way.

[ruiu]

ELF/ScriptParser.cpp

269

I don't think this is enough. You want to check if ErrorCount is incremented in this function.

[grimar]

ELF/ScriptParser.cpp

269

I think it is not possible to call this function if we have ErrorCount > 0.
(we call it only from readAsNeeded() and readGroup() and both check the ErrorCount before call)

And also it is not recursive.

I do not know why I would want to check it is incremented. Am I missing something ?

[grimar]

ping

[grimar]

ELF/ScriptParser.cpp

251–255

Will it be better if I add following check here ?

if (ErrorCount)
  return;

It still will be excessive check currently I believe (like I pointed in my previous comment),
but if code outside will change so that this function be called with ErrorCount > 0,
such check will protect error reporting logic this patch introduces.

[ruiu]

ELF/ScriptParser.cpp

251–255

If that shouldn't happen, add it as an assert.

[grimar]

• Added assert.

[ruiu]

ELF/ScriptParser.cpp

270

Do not call error twice for one error message.

Okay, so I think we need some better infrastructure for diagnostics if we really want to improve linker script diagnostics. That's not that high priority, but it is worth to try. I'll try to create one. Until then, please hold on with and other changes for diagnostics.

ELF/Driver.h

 namespace lld {
 namespace elf {
 
 extern class LinkerDriver *Driver;
 
 class LinkerDriver {
 public:
   void main(ArrayRef<const char *> Args, bool CanExitEarly);
-  void addFile(StringRef Path, bool WithLOption);
-  void addLibrary(StringRef Name);
+  bool addFile(StringRef Path, bool WithLOption);
+  bool addLibrary(StringRef Name);
 
 private:
   void readConfigs(llvm::opt::InputArgList &Args);
   void createFiles(llvm::opt::InputArgList &Args);
   void inferMachineType();
   template <class ELFT> void link(llvm::opt::InputArgList &Args);
 
   // True if we are in --whole-archive and --no-whole-archive.

ELF/Driver.cpp

std::vector<std::pair<MemoryBufferRef, uint64_t>> static getArchiveMembers(
   // Take ownership of memory buffers created for members of thin archives.
   for (std::unique_ptr<MemoryBuffer> &MB : File->takeThinBuffers())
     make<std::unique_ptr<MemoryBuffer>>(std::move(MB));
 
   return V;
 }
 
 // Opens a file and create a file object. Path has to be resolved already.
-void LinkerDriver::addFile(StringRef Path, bool WithLOption) {
+bool LinkerDriver::addFile(StringRef Path, bool WithLOption) {
   using namespace sys::fs;
 
   Optional<MemoryBufferRef> Buffer = readFile(Path);
   if (!Buffer.hasValue())
-    return;
+    return false;
   MemoryBufferRef MBRef = *Buffer;
 
   if (InBinary) {
     Files.push_back(make<BinaryFile>(MBRef));
-    return;
+    return true;
   }
 
   switch (identify_magic(MBRef.getBuffer())) {
   case file_magic::unknown:
     readLinkerScript(MBRef);
-    return;
+    return true;
   case file_magic::archive: {
     // Handle -whole-archive.
     if (InWholeArchive) {
       for (const auto &P : getArchiveMembers(MBRef))
         Files.push_back(createObjectFile(P.first, Path, P.second));
-      return;
+      return true;
     }
 
     std::unique_ptr<Archive> File =
         check(Archive::create(MBRef), Path + ": failed to parse archive");
 
     // If an archive file has no symbol table, it is likely that a user
     // is attempting LTO and using a default ar command that doesn't
     // understand the LLVM bitcode file. It is a pretty common error, so
     // we'll handle it as if it had a symbol table.
     if (!File->isEmpty() && !File->hasSymbolTable()) {
       for (const auto &P : getArchiveMembers(MBRef))
         Files.push_back(make<LazyObjFile>(P.first, Path, P.second));
-      return;
+      return true;
     }
 
     // Handle the regular case.
     Files.push_back(make<ArchiveFile>(std::move(File)));
-    return;
+    return true;
   }
   case file_magic::elf_shared_object:
     if (Config->Relocatable) {
       error("attempted static link of dynamic object " + Path);
-      return;
+      return true;
     }
 
     // DSOs usually have DT_SONAME tags in their ELF headers, and the
     // sonames are used to identify DSOs. But if they are missing,
     // they are identified by filenames. We don't know whether the new
     // file has a DT_SONAME or not because we haven't parsed it yet.
     // Here, we set the default soname for the file because we might
     // need it later.
     //
     // If a file was specified by -lfoo, the directory part is not
     // significant, as a user did not specify it. This behavior is
     // compatible with GNU.
     Files.push_back(
         createSharedFile(MBRef, WithLOption ? path::filename(Path) : Path));
-    return;
+    return true;
   default:
     if (InLib)
       Files.push_back(make<LazyObjFile>(MBRef, "", 0));
     else
       Files.push_back(createObjectFile(MBRef));
   }
+
+  return true;
 }
 
 // Add a given library by searching it from input search paths.
-void LinkerDriver::addLibrary(StringRef Name) {
+bool LinkerDriver::addLibrary(StringRef Name) {
   if (Optional<std::string> Path = searchLibrary(Name))
-    addFile(*Path, /*WithLOption=*/true);
-  else
-    error("unable to find library -l" + Name);
+    return addFile(*Path, /*WithLOption=*/true);
+  return false;
 }
 
 // This function is called on startup. We need this for LTO since
 // LTO calls LLVM functions to compile bitcode files to native code.
 // Technically this can be delayed until we read bitcode files, but
 // we don't bother to do lazily because the initialization is fast.
 static void initLLVM(opt::InputArgList &Args) {
   InitializeAllTargets();

ELF/ScriptParser.cpp

ScriptParser(MemoryBufferRef MB)
       : ScriptLexer(MB),
         IsUnderSysroot(isUnderSysroot(MB.getBufferIdentifier())) {}
 
   void readLinkerScript();
   void readVersionScript();
   void readDynamicList();
 
 private:
-  void addFile(StringRef Path);
+  bool addFile(StringRef Path);
   OutputSection *checkSection(OutputSectionCommand *Cmd, StringRef Loccation);
 
   void readAsNeeded();
   void readEntry();
   void readExtern();
   void readGroup();
   void readInclude();
   void readMemory();
while (!atEOF()) {
     } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok)) {
       Script->Opt.Commands.push_back(Cmd);
     } else {
       setError("unknown directive: " + Tok);
     }
   }
 }
 
-void ScriptParser::addFile(StringRef S) {
+bool ScriptParser::addFile(StringRef S) {
   if (IsUnderSysroot && S.startswith("/")) {
     SmallString<128> PathData;
     StringRef Path = (Config->Sysroot + S).toStringRef(PathData);
-    if (sys::fs::exists(Path)) {
-      Driver->addFile(Saver.save(Path), /*WithLOption=*/false);
-      return;
-    }
+    if (sys::fs::exists(Path))
+      return Driver->addFile(Saver.save(Path), /*WithLOption=*/false);
   }
 
-  if (S.startswith("/")) {
-    Driver->addFile(S, /*WithLOption=*/false);
-  } else if (S.startswith("=")) {
+  if (S.startswith("/"))
+    return Driver->addFile(S, /*WithLOption=*/false);
+
+  if (S.startswith("=")) {
     if (Config->Sysroot.empty())
-      Driver->addFile(S.substr(1), /*WithLOption=*/false);
-    else
-      Driver->addFile(Saver.save(Config->Sysroot + "/" + S.substr(1)),
+      return Driver->addFile(S.substr(1), /*WithLOption=*/false);
+    return Driver->addFile(Saver.save(Config->Sysroot + "/" + S.substr(1)),
                            /*WithLOption=*/false);
-  } else if (S.startswith("-l")) {
-    Driver->addLibrary(S.substr(2));
-  } else if (sys::fs::exists(S)) {
-    Driver->addFile(S, /*WithLOption=*/false);
-  } else {
-    if (Optional<std::string> Path = findFromSearchPaths(S))
-      Driver->addFile(Saver.save(*Path), /*WithLOption=*/true);
-    else
-      setError("unable to find " + S);
   }
+
+  if (S.startswith("-l"))
+    return Driver->addLibrary(S.substr(2));
+
+  if (sys::fs::exists(S))
+    return Driver->addFile(S, /*WithLOption=*/false);
+
+  if (Optional<std::string> Path = findFromSearchPaths(S))
+    return Driver->addFile(Saver.save(*Path), /*WithLOption=*/true);
+
+  return false;
 }
 
 void ScriptParser::readAsNeeded() {
   expect("(");
   bool Orig = Config->AsNeeded;
   Config->AsNeeded = true;
-  while (!Error && !consume(")"))
-    addFile(unquote(next()));
+  while (!Error && !consume(")")) {
+    StringRef File = unquote(next());
+    if (!addFile(File))
+      setError("unable to find " + File);
+  }
   Config->AsNeeded = Orig;
 }
 
 void ScriptParser::readEntry() {
   // -e <symbol> takes predecence over ENTRY(<symbol>).
   expect("(");
   StringRef Tok = next();
   if (Config->Entry.empty())
     Config->Entry = Tok;
   expect(")");
 }
 
 void ScriptParser::readExtern() {
   expect("(");
   while (!Error && !consume(")"))
     Config->Undefined.push_back(next());
 }
 
 void ScriptParser::readGroup() {
   expect("(");
   while (!Error && !consume(")")) {
-    if (consume("AS_NEEDED"))
+    if (consume("AS_NEEDED")) {
       readAsNeeded();
-    else
-      addFile(unquote(next()));
+      continue;
+    }
+
+    StringRef File = unquote(next());
+    if (!addFile(File))
+      setError("unable to find " + File);
   }
 }
 
 void ScriptParser::readInclude() {
   StringRef Tok = unquote(next());
 
   // https://sourceware.org/binutils/docs/ld/File-Commands.html:
   // The file will be searched for in the current directory, and in any

test/ELF/linkerscript/group.s

 # RUN: echo "GROUP(\"%t\")" > %t.script2
 # RUN: ld.lld -o %t2 %t.script1
 # RUN: llvm-readobj %t2 > /dev/null
 
 # RUN: echo "GROUP(AS_NEEDED(\"%t\"))" > %t.script
 # RUN: ld.lld -o %t2 %t.script
 # RUN: llvm-readobj %t2 > /dev/null
 
+# RUN: echo "INPUT(/no_such_file)" > %t.script
+# RUN: not ld.lld -o %t2 %t.script 2>&1 | FileCheck -check-prefix=ERR %s
+# ERR: cannot open {{.*}}no_such_file: {{[Nn]}}o such file or directory
+# ERR: {{.*}}.script:1: unable to find /no_such_file
+
+# RUN: echo "INPUT(AS_NEEDED(/no_such_file))" > %t.script
+# RUN: not ld.lld -o %t2 %t.script 2>&1 | FileCheck -check-prefix=ERR %s
+
+# RUN: echo "GROUP(/no_such_file)" > %t.script
+# RUN: not ld.lld -o %t2 %t.script 2>&1 | FileCheck -check-prefix=ERR %s
+
+# RUN: echo "GROUP(AS_NEEDED(/no_such_file))" > %t.script
+# RUN: not ld.lld -o %t2 %t.script 2>&1 | FileCheck -check-prefix=ERR %s
+
 .globl _start
 _start:
   ret