[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

291

I think you can only check ErrorCount. Remove Error.

[grimar]

• Addressed review comment.

[ruiu]

ELF/ScriptParser.cpp

313

Ditto

[grimar]

ELF/ScriptParser.cpp

313

Why ? I think readAsNeeded() can set Error.

[ruiu]

ELF/ScriptParser.cpp

313

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

[grimar]

ELF/ScriptParser.cpp

313

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

313

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

[grimar]

ELF/ScriptParser.cpp

313

I suggest to switch to always use ErrorCount: D36266

[grimar]

• Reimplemented in a more generic way.

[ruiu]

ELF/ScriptParser.cpp

282

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

[grimar]

ELF/ScriptParser.cpp

282

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

253–254

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

253–254

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

[grimar]

• Added assert.

[ruiu]

ELF/ScriptParser.cpp

283

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/ScriptParser.cpp

while (!atEOF()) {
     } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok)) {
       Script->Opt.Commands.push_back(Cmd);
     } else {
       setError("unknown directive: " + Tok);
     }
   }
 }
 
 void ScriptParser::addFile(StringRef S) {
-  if (IsUnderSysroot && S.startswith("/")) {
+  assert(!ErrorCount);
+
+  if (S.startswith("/")) {
+    if (IsUnderSysroot) {
       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))
+        S = Saver.save(Path);
     }
 
-  if (S.startswith("/")) {
     Driver->addFile(S, /*WithLOption=*/false);
   } else if (S.startswith("=")) {
     if (Config->Sysroot.empty())
       Driver->addFile(S.substr(1), /*WithLOption=*/false);
     else
       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 (ErrorCount)
+    error(">>> location:" + getCurrentLocation());
 }
 
 void ScriptParser::readAsNeeded() {
   expect("(");
   bool Orig = Config->AsNeeded;
   Config->AsNeeded = true;
   while (!ErrorCount && !consume(")"))
     addFile(unquote(next()));
   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 (!ErrorCount && !consume(")"))
     Config->Undefined.push_back(next());
 }
 
 void ScriptParser::readGroup() {
   expect("(");
   while (!ErrorCount && !consume(")")) {
     if (consume("AS_NEEDED"))
       readAsNeeded();
     else
       addFile(unquote(next()));
   }
 }
 
 void ScriptParser::readInclude() {
   StringRef Tok = unquote(next());

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: >>> location:{{.*}}.script:1
+
+# 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