[ELF] Support for parsing OUTPUT command in LinkerScript

[davide]

Updated comment.

[rafaelauler]

LGTM.

Next time, could you please upload your diffs with "git diff -U999999 <other-branch>"? This way, we get more context here on Phabricator. Thanks for the patch.

[shankarke]

include/lld/ReaderWriter/LinkerScript.h

190

I have one main comment generally with the linker script parser as to we store all the data from linker scripts as StringRef's. We dont need to keep the linker script file open IMO, so using std::string would be better.

What do you think ?

[rafaelauler]

It's an option, but why dup every string that is already in a memory buffer?

[shankarke]

IMO, We could close the linker script after reading.

This discussion is unrelated to the patch, LGTM.

[rafaelauler]

Well, I think no file is left open. After MemoryBuffer::getFileOrSTDIN(), the memory buffer is simply filled in with the file contents, and the file is closed right before this call finishes and the lexer starts its work. So, my original point was that, since we already copied the entire file to a separate memory buffer, we would be duplicating work by creating new strings and duplicating the contents of this memory buffer in several small string buffers.

[ruiu]

It basically looks good, but we already have a large piece of code as a part of the linker script parser which don't have any consumer. This patch adds a parser for OUTPUT() but you didn't make any change to use or interpret that directive. So, the new code doesn't actually provide value to the user by itself. It's also a little bit concerning -- there might be a design flaw that's only noticeable when we try to use the parser.

Could you update the GNU driver to consume OUTPUT directive?

[davide]

In D7326#116598, @ruiu wrote:

It basically looks good, but we already have a large piece of code as a part of the linker script parser which don't have any consumer. This patch adds a parser for OUTPUT() but you didn't make any change to use or interpret that directive. So, the new code doesn't actually provide value to the user by itself. It's also a little bit concerning -- there might be a design flaw that's only noticeable when we try to use the parser.

Could you update the GNU driver to consume OUTPUT directive?

My plan was to split parsing and action, i.e. to implement the latter in a subsequent commit (was planning to do this tonight or tomorrow). If you have any concern I can incorporate the driver update in this change but I don't see a strong reason why they can't be committed as standalone changes. Let me know what you think about it.

[ruiu]

Could you upload your patch to the driver to Phab, so that we can review producer side and consumer side at once? I prefer small patches too, so it's totally fine to split them up, my concern was just that I couldn't see the entire picture.

[davide]

D7328 contains the other bits. Sorry for the lack of context in the diff. The server where the tree sits is currently unreachable. I'll update with a version with context tomorrow, if needed.

[ruiu]

LGTM

include/lld/ReaderWriter/LinkerScript.h

     kw_provide_hidden,
     kw_only_if_ro,
     kw_only_if_rw,
+    kw_output,
     kw_output_arch,
     kw_output_format,
     kw_overlay,

     Entry,
     Group,
     InputSectionsCmd,
+    Output,
     OutputArch,
     OutputFormat,
     OutputSectionDescription,

   Kind _kind;
 };
 
+class Output : public Command {
+public:
+  explicit Output(StringRef outputFileName)
+      : Command(Kind::Output), _outputFileName(outputFileName) {}
+
+  static bool classof(const Command *c) { return c->getKind() == Kind::Output; }
+
+  void dump(raw_ostream &os) const override {
+    os << "OUTPUT(" << _outputFileName << ")\n";
+  }
+
+  StringRef getOutputFileName() const { return _outputFileName; }
+
+private:
+  StringRef _outputFileName;
+};
+
 class OutputFormat : public Command {
 public:
   explicit OutputFormat(StringRef format) : Command(Kind::OutputFormat) {

 
   // ==== High-level commands parsing ====
 
+  /// Parse the OUTPUT linker script command.
+  /// Example:
+  /// OUTPUT(/path/to/file)
+  /// ^~~~> parseOutput()
+  ///
+  Output *parseOutput();
+
   /// Parse the OUTPUT_FORMAT linker script command.
   /// Example:
   ///

lib/ReaderWriter/LinkerScript.cpp

     CASE(kw_provide_hidden)
     CASE(kw_only_if_ro)
     CASE(kw_only_if_rw)
+    CASE(kw_output)
     CASE(kw_output_arch)
     CASE(kw_output_format)
     CASE(kw_overlay)

             .Case("KEEP", Token::kw_keep)
             .Case("ONLY_IF_RO", Token::kw_only_if_ro)
             .Case("ONLY_IF_RW", Token::kw_only_if_rw)
+            .Case("OUTPUT", Token::kw_output)
             .Case("OUTPUT_ARCH", Token::kw_output_arch)
             .Case("OUTPUT_FORMAT", Token::kw_output_format)
             .Case("OVERLAY", Token::kw_overlay)

     case Token::semicolon:
       consumeToken();
       break;
+    case Token::kw_output: {
+      auto output = parseOutput();
+      if (!output)
+        return nullptr;
+      _script._commands.push_back(output);
+      break;
+    }
     case Token::kw_output_format: {
       auto outputFormat = parseOutputFormat();
       if (!outputFormat)

   return new (_alloc) TernaryConditional(lhs, trueExpr, falseExpr);
 }
 
+// Parse OUTPUT(ident)
+Output *Parser::parseOutput() {
+  assert(_tok._kind == Token::kw_output && "Expected OUTPUT");
+  consumeToken();
+  if (!expectAndConsume(Token::l_paren, "expected ("))
+    return nullptr;
+
+  if (_tok._kind != Token::identifier) {
+    error(_tok, "Expected identifier in OUTPUT.");
+    return nullptr;
+  }
+
+  auto ret = new (_alloc) Output(_tok._range);
+  consumeToken();
+
+  if (!expectAndConsume(Token::r_paren, "expected )"))
+    return nullptr;
+
+  return ret;
+}
+
 // Parse OUTPUT_FORMAT(ident)
 OutputFormat *Parser::parseOutputFormat() {
   assert(_tok._kind == Token::kw_output_format && "Expected OUTPUT_FORMAT!");

test/LinkerScript/linker-script.test

 
 OUTPUT_ARCH(i386:x86_64)
 OUTPUT_FORMAT("elf64-x86-64", "elf64-x86-64", "elf64-x86-64")
+OUTPUT("/out/foo")
 GROUP ( /lib/x86_64-linux-gnu/libc.so.6 /usr/lib/x86_64-linux-gnu/libc_nonshared.a  AS_NEEDED ( /lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 ) -lm -l:libgcc.a )
 ENTRY(init)
 

 CHECK: comma: ,
 CHECK: identifier: elf64-x86-64
 CHECK: r_paren: )
+CHECK: kw_output: OUTPUT
+CHECK: l_paren: (
+CHECK: identifier: /out/foo
+CHECK: r_paren: )
 CHECK: kw_group: GROUP
 CHECK: l_paren: (
 CHECK: identifier: /lib/x86_64-linux-gnu/libc.so.6