[ELF] - Add additional information about location when emiting linkerscript's parsing errors.

[grimar]

Summary

This is PR31128 which complains that we show unclear message when
parsing script like

.= foo

(note: there is no space between `.=')

When LLD see .= foo; it starts reading output section declaration.
It reads section name .=, addess expression foo and expects to see : right after that
all and fails with : expected, but got ; message, which is fully correct in this case,
but not very useful for user who writes simple assignment expression.

FWIW scripts like SECTIONS { .= : { *(.text*) } } are correct, both GNU linkers
would create output section .= in that case, and LLD do the same.

What I suggest is to provide additional error context if available, like this patch do.

[peter.smith]

I think that this is definitely an improvement, I've made some suggestions but I haven't got a strong opinion.

ELF/ScriptLexer.h

42

It might be better to call this LocationContext if it is specifically going to be used within setLocationContext() and dropLocationContex(). This might help prevent it being used/overwritten in other non LocationContext situations by mistake.

ELF/ScriptParser.cpp

607

Is it worth an RAII wrapper so that you don't have to explicitly remember to call dropLocationContext()?

607

Would it be worth using "section definition for output section" rather than "description for output section" to match the Linker Script manual?

[grimar]

ELF/ScriptLexer.h

42

May be, or I was thinking about following names now:
ErrorContext, setErrorContext, dropErrorContext

ELF/ScriptParser.cpp

607

Is it worth an RAII wrapper so that you don't have to explicitly remember to call dropLocationContext()?

I tried 2 more ways here:

• Simple class that sets/drops context in constructor/destructor.
• Storing std::weak_ptr<std::string> Context. That way we can do something like:

std::shared_ptr<std::string> setErrorContext(const Twine& Msg) {
  auto Ret = make_shared<std::string>(Msg.cstr());
  Context = Ret;
  return Ret;
}
...
auto C = setErrorContext("blah blah blah");
<and while this shared pointer is alive, following code will print context>
...

if (auto C = Context.lock())
  error(*C);

But I am not sure it worth to do now, when there is only single place where context is used.

Would it be worth using "section definition for output section" rather than "description for output section" to match the > Linker Script manual?

I'll change it, thanks. My wording was based on method name here.

[grimar]

• Renamed new methods/field, changed error text to suggested.

[peter.smith]

Thanks very much for the update, I've no further comments.

[ruiu]

ELF/ScriptLexer.cpp

81–87

In general, do not call error multiple times for an error message. Call error only once per an error instead. This is important because we have the -error-limit option which manages the number of errors (and that is different from number of lines of error messages.)

I think you can now see why our error message format is something like cannot parse file: /foo/bar. It's because you can concatenate multiple error messages by joining with ":" like this:

error while reading linker script foo.ls: cannot parse file: /foo/bar

So, concatenate error strings into one string and print it out using error just like above.

ELF/ScriptLexer.h

42

I'd name ErrorPrefix as this string is printed as a prefix for an error message.

ELF/ScriptParser.cpp

607

RAII is a good idea, but if you don't want to do that, you don't want to create a set of functions just to set Context local varaible. You could just directly assign values to the variable.

[grimar]

• Addressed review comments.

[ruiu]

ELF/ScriptParser.cpp

608

Don't include ">>>" in a prefix. Just end it with ": ".

[grimar]

ELF/ScriptParser.cpp

608

I did that because message printed was huge for me:

error: {{.*}}.script.inc:4: unable to parse description for output section 'boom': malformed number: .temp

where {{.*}}.script was a long path itself.

[ruiu]

Yes, I understand that, but as I wrote in a previous comment, this kind of style

error: foo bar
>>> A
>>> B
>>> C

is odd unless A, B and C are list items for the same list. In other words, you cannot always simply concatenate all error messages with ">>>\n".

ELF/ScriptLexer.h

public:
   void expect(StringRef Expect);
   bool consumeLabel(StringRef Tok);
   std::string getCurrentLocation();
 
   std::vector<MemoryBufferRef> MBs;
   std::vector<StringRef> Tokens;
   bool InExpr = false;
   size_t Pos = 0;
+  std::string ErrorPrefix;
 
 private:
   void maybeSplitExpr();
   StringRef getLine();
   size_t getLineNumber();
   size_t getColumnNumber();
 
   MemoryBufferRef getCurrentMB();
 };
 
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/ScriptLexer.cpp

 
 ScriptLexer::ScriptLexer(MemoryBufferRef MB) { tokenize(MB); }
 
 // We don't want to record cascading errors. Keep only the first one.
 void ScriptLexer::setError(const Twine &Msg) {
   if (ErrorCount)
     return;
 
-  std::string S = (getCurrentLocation() + ": " + Msg).str();
+  std::string S = getCurrentLocation() + ": " + ErrorPrefix + Msg.str();
   if (Pos)
     S += "\n>>> " + getLine().str() + "\n>>> " +
          std::string(getColumnNumber(), ' ') + "^";
   error(S);
 }
 
 // Split S into linker script tokens.
 void ScriptLexer::tokenize(MemoryBufferRef MB) {
   std::vector<StringRef> Vec;
   MBs.push_back(MB);
   StringRef S = MB.getBuffer();
   StringRef Begin = S;
 
   for (;;) {

ELF/ScriptParser.cpp

void ScriptParser::readSectionAddressType(OutputSection *Cmd) {
   if (consume("(")) {
     expect("NOLOAD");
     expect(")");
     Cmd->Noload = true;
   }
 }
 
 OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
+  ErrorPrefix = "unable to parse description for output section '" +
+                OutSec.str() + "':\n>>> ";
   OutputSection *Cmd =
       Script->createOutputSection(OutSec, getCurrentLocation());
 
   if (peek() != ":")
     readSectionAddressType(Cmd);
   expect(":");
 
   if (consume("AT"))
OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
   if (consume("="))
     Cmd->Filler = parseFill(next());
   else if (peek().startswith("="))
     Cmd->Filler = parseFill(next().drop_front());
 
   // Consume optional comma following output section command.
   consume(",");
 
+  ErrorPrefix = "";
   return Cmd;
 }
 
 // Parses a given string as a octal/decimal/hexadecimal number and
 // returns it as a big-endian number. Used for `=<fillexp>`.
 // https://sourceware.org/binutils/docs/ld/Output-Section-Fill.html
 //
 // When reading a hexstring, ld.bfd handles it as a blob of arbitrary

test/ELF/linkerscript/assert.s

 ## Test assertions inside of output section decriptions.
 # RUN: echo "SECTIONS { .foo : { *(.foo) ASSERT(SIZEOF(.foo) == 8, \"true\"); } }" > %t6.script
 # RUN: ld.lld -shared -o %t6 --script %t6.script %t1.o
 # RUN: llvm-readobj %t6 > /dev/null
 
 # RUN: echo "SECTIONS { .foo : { ASSERT(1, \"true\") } }" > %t7.script
 # RUN: not ld.lld -shared -o %t7 --script %t7.script %t1.o > %t.log 2>&1
 # RUN: FileCheck %s -check-prefix=CHECK-SEMI < %t.log
-# CHECK-SEMI: error: {{.*}}.script:1: ; expected, but got }
+# CHECK-SEMI: error: {{.*}}.script:1: unable to parse description for output section '.foo':
+# CHECK-SEMI-NEXT: >>> ; expected, but got }
 
 .section .foo, "a"
  .quad 0

test/ELF/linkerscript/diagnostic.s

 
 ## One more check that text of lines and pointer to 'bad' token are working ok.
 # RUN: echo "SECTIONS {" > %t.script
 # RUN: echo ".text : { *(.text) }" >> %t.script
 # RUN: echo ".keep : { *(.keep) }" >> %t.script
 # RUN: echo "boom .temp : { *(.temp) } }" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR7 -strict-whitespace %s
-# ERR7:      error: {{.*}}.script:4: malformed number: .temp
+# ERR7:      error: {{.*}}.script:4: unable to parse description for output section 'boom':
+# ERR7-NEXT: >>> malformed number: .temp
 # ERR7-NEXT: >>> boom .temp : { *(.temp) } }
 # ERR7-NEXT: >>>      ^
 
 ## Check tokenize() error
 # RUN: echo "SECTIONS {}" > %t.script
 # RUN: echo "\"" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR8 -strict-whitespace %s

 ## Check error reporting correctness for included files.
 # RUN: echo "SECTIONS {" > %t.script.inc
 # RUN: echo ".text : { *(.text) }" >> %t.script.inc
 # RUN: echo ".keep : { *(.keep) }" >> %t.script.inc
 # RUN: echo "boom .temp : { *(.temp) } }" >> %t.script.inc
 # RUN: echo "INCLUDE \"%t.script.inc\"" > %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR10 -strict-whitespace %s
-# ERR10:      error: {{.*}}.script.inc:4: malformed number: .temp
+# ERR10:      error: {{.*}}.script.inc:4: unable to parse description for output section 'boom':
+# ERR10-NEXT: >>> malformed number: .temp
 # ERR10-NEXT: >>> boom .temp : { *(.temp) } }
 # ERR10-NEXT: >>>      ^
 
 ## Check error reporting in script with INCLUDE directive.
 # RUN: echo "SECTIONS {" > %t.script.inc
 # RUN: echo ".text : { *(.text) }" >> %t.script.inc
 # RUN: echo ".keep : { *(.keep) }" >> %t.script.inc
 # RUN: echo ".temp : { *(.temp) } }" >> %t.script.inc
 # RUN: echo "/* One line before INCLUDE */" > %t.script
 # RUN: echo "INCLUDE \"%t.script.inc\"" >> %t.script
 # RUN: echo "/* One line ater INCLUDE */" >> %t.script
 # RUN: echo "Error" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR11 -strict-whitespace %s
 # ERR11: error: {{.*}}.script:4: unexpected EOF