[ELF2/LinkerScript] Fix OUTPUT_FORMAT directive parsing

[davide]

Summary

Right now the parser recognizes
OUTPUT_ARCH("foo")
but not OUTPUT_ARCH("foo", "goo", "blah")
This patch is an attempt to fix.

Side note:
Also, the diagnostic we emit is currently very uninformative.
In the future, I think we could improve this aspect showing which directive failed to parse -- or even better, line/column of failure. I would like to discuss design further.

[ruiu]

s/OUTPUT_ARCH/OUTPUT_FORMAT/

Rename linkerscript-outputgroup.s -> linkerscript-outputformat.s.

Regarding better error handling, I don't think now is the right time to do something about that. This parser is a designed to be just enough to read Linux's libc.so, and this should suffice until we link something like a OS kernel, so designing a linker script that is capable more than this one is in my opinion relatively low priority.

ELF/LinkerScript.cpp

222 ↗

(On Diff #37151)

Wrong indentation.

test/elf2/linkerscript-ouputgroup.s

2 ↗

(On Diff #37151)

You also need a test for OUTPUT_FORMAT(default, big, little)-style directive.

[davide]

I'm sorry, I updated a wrong/stale diff. I'll update a new one shortly.

[davide]

In D13668#265235, @ruiu wrote:

s/OUTPUT_ARCH/OUTPUT_FORMAT/

Rename linkerscript-outputgroup.s -> linkerscript-outputformat.s.

Regarding better error handling, I don't think now is the right time to do something about that. This parser is a designed to be just enough to read Linux's libc.so, and this should suffice until we link something like a OS kernel, so designing a linker script that is capable more than this one is in my opinion relatively low priority.

This is exactly my point. In fact, I said "in the future", and not "now". That said, I think it's good to emit meaningful diagnostics for users at some point.

[davide]

Now with the correct diff. I also renamed the test. Sorry for messing up the name at least a couple of times, but apparently the code was the only place where I got it right :)

[davide]

Use a real triple for the test.

[ruiu]

ELF/LinkerScript.cpp

214–215 ↗

(On Diff #37163)

next();
StringRef Tok = next();

test/elf2/linkerscript-ouputformat.s

2–3 ↗

(On Diff #37163)

I'd write OUTPUT_FORMAT(x, y, z) as we don't actually care about the values.

8 ↗

(On Diff #37163)

So is this.

12 ↗

(On Diff #37163)

You also need a test to check OUTPUT_FOMAT(x) is considered valid.

[davide]

test/elf2/linkerscript-ouputformat.s

12 ↗

(On Diff #37165)

There's one already in linkerscript.s

[davide]

Rename identifiers in test.

[ruiu]

LGTM with a nit.

ELF/LinkerScript.cpp

214–215 ↗

(On Diff #37166)

Please discard the result first next() and assign only the second one.

[davide]

Thanks!

lld/trunk/ELF/LinkerScript.cpp

if (Config->OutputFile.empty())
     Config->OutputFile = Tok;
   expect(")");
 }
 
 void LinkerScript::readOutputFormat() {
   // Error checking only for now.
   expect("(");
   next();
+  StringRef Tok = next();
+  if (Tok == ")")
+   return;
+  if (Tok != ",")
+    error("unexpected token: " + Tok);
+  next();
+  expect(",");
+  next();
   expect(")");
 }
 
 void LinkerScript::readSearchDir() {
   expect("(");
   Config->SearchPaths.push_back(next());
   expect(")");
 }
 
 // Entry point. The other functions or classes are private to this file.
 void lld::elf2::readLinkerScript(BumpPtrAllocator *A, MemoryBufferRef MB) {
   LinkerScript(A, MB.getBuffer()).run();
 }

lld/trunk/test/elf2/linkerscript-ouputformat.s

+# REQUIRES: x86
+# RUN: echo "OUTPUT_FORMAT(x, y, z)" > %t.script
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-freebsd %s -o %t1
+# RUN: ld.lld2 -shared -o %t2 %t1 %t.script
+# RUN: llvm-readobj %t2 > /dev/null
+
+# RUN: echo "OUTPUT_FORMAT(x, y)" > %t.script
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-freebsd %s -o %t1
+# RUN: not ld.lld2 -shared -o %t2 %t1 %t.script
+# RUN: llvm-readobj %t2 > /dev/null