[lld] Make ELFLinkingContext own LinkerScript buffers

[rafaelauler]

Summary

Currently, no one owns script::Parser buffers, but yet ELFLinkingContext gets
updated with StringRef pointers to data inside Parser buffers. Since this buffer
is locally owned inside GnuLdDriver::evalLinkerScript(), as soon as this
function finishes, all pointers in ELFLinkingContext that comes from linker
scripts get invalid. The problem is that we need someone to own linker scripts
data structures and, since ELFLinkingContext transports references to linker
scripts data, we can simply make it also own all linker scripts data.

[ruiu]

Hrm, looks like it's doing too much to solve the issue we have. We basically want the linker script memory buffer to have the same lifespan as the linking context. Instead of adding a ScriptInstance to the linking context, can we add a method to transfer the ownership of a memory buffer to the linking context? Then we modify script::Lexer to not take the ownership of a given memory buffer.

[davide]

Although I don't see major problems with your patch, I like Rui's proposed approach better.

[rafaelauler]

The reason I created a new class that encapsulates the parser is because I
plan to later add code to this class (or to another new class called
ScriptSema that is owned by ScriptInstance) that cooks the AST with
semantic info to answer high level questions about the linker script, such
as "in which output section to put this input section, according to this
linker script?". Anyway, I will need to find a location to put new data
structures to store this data, and I thought of this as a first step. What
do you think?

[ruiu]

I didn't think hard enough about that, but it might be good to have a class to cook linker scripts. But if you have such class, you can instantiate that class in the driver and only set that class instance to the linking context, no? You wouldn't have to add a pointer to linker script ASTs to the linking context, I guess.

I prefer a patch that does one thing; in this case, we should try to fix the unsafe memory access first, so I'd fix that without adding any new feature.

[rafaelauler]

I also want to solve the problem of this patch (make ELFLinkingContext own
the buffers) without mixing with future directions on cooking linkerscript
AST. However, we will essentially end up with a vector of BumpPtrAllocators
in ELFLinkingContext (one for each linker script), which makes the code, in
my opinion, much harder to understand (it's better to have "hey, these are
a bunch of linkerscripts" than "hey, here are a bunch of unnamed buffers
that need to be anchored somewhere"). What are the benefits of this? We
avoid including LinkerScript.h into ELFLinkingContext.h, but that is
something that we will eventually need to do to avoid rewriting all classes
in LinkerScript.h in their "cooked" version, don't you think?

For example, if you look specifically at the problem of mapping input
sections to output sections, we already have classes that encode this same
information in the script namespace (script::OutputSectionDescription in
the raw script AST) and in SectionChunks.h (template class
lld::OutputSection<>), used by DefaultLayout. I wanted to avoid creating
yet another class such as "CookedOutputSectionInfo", and that's why I had
an intent on preserving the original linker script AST structure in the
LinkingContext -- mostly to avoid rewriting the same info in different data
structures that stores the same information that is already exposed in the
AST.

Let me know your thoughts and thanks for reviewing this.

[shankarke]

Adding a test might give more information on the functionality that you are trying to add too.

include/lld/ReaderWriter/ELFLinkingContext.h

295–311 ↗

(On Diff #19115)

Why do you recognize the master linker script from the rest ? The linker can use more than one -T option to specify more than one linker script too.

Input files that are linker scripts are positional too.

For example :

ld 1.o -lc

Where libc.so is a linker script that needs to be handled in command line order.

[rafaelauler]

Changed the implementation according to suggestions from Rui and Shankar.

[ruiu]

include/lld/ReaderWriter/ELFLinkingContext.h

304 ↗

(On Diff #19168)

Add a blank line after this line.

include/lld/ReaderWriter/LinkerScript.h

748 ↗

(On Diff #19168)

Maybe we can write "= delete" instead of LLVM_DELETED_FUNCTION? I believe the macro is for pre-C++11 compilers.

765 ↗

(On Diff #19168)

This function allows the user to lazy-evaluate linker scripts, but I think lazy evaluation is not a good idea here. I'd like get errors as soon as possible if there's an error in a linker script. How about this.

Change the signature of parse to std::error_code parse(). We call this function after instantiating the class. If it returns an error, it means there's an error in the linker script. If it doesn't return an error, subsequent getTopLevelNode will always succeed.

Also, getTopLevelNode seems a bit too long. Maybe get is enough?

[shankarke]

include/lld/ReaderWriter/LinkerScript.h

757 ↗

(On Diff #19168)

We could just use one variable like how the File Parser does using llvm::Optional.

761 ↗

(On Diff #19168)

could we use ErrorOr ?

1016 ↗

(On Diff #19168)

std::error_code instead of bool ?

[rafaelauler]

Thanks for the suggestions. Here is the new version.

[ruiu]

LGTM

include/lld/ReaderWriter/ELFLinkingContext.h

13 ↗

(On Diff #19171)

Add "lld/ReaderWriter/" for consistency with other headers.

include/lld/ReaderWriter/LinkerScript.h

755 ↗

(On Diff #19171)

Indentation. (You could actually write this method in one line instead of three lines.)

[rafaelauler]

I committed with LLVM_DELETED_FUNCTION because "= delete" breaks my MSVC 2012 box. After we move on to officially only support MSVC >= 2013, we can drop this macro.

lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h

 
 #include "lld/Core/LinkingContext.h"
 #include "lld/Core/Pass.h"
 #include "lld/Core/PassManager.h"
 #include "lld/Core/STDExtras.h"
 #include "lld/Core/range.h"
 #include "lld/Core/Reader.h"
 #include "lld/Core/Writer.h"
+#include "lld/ReaderWriter/LinkerScript.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Object/ELF.h"
 #include "llvm/Support/ELF.h"
 #include <map>
 #include <memory>
 
 namespace lld {
public:
   bool alignSegments() const { return _alignSegments; }
   void setAlignSegments(bool align) { _alignSegments = align; }
 
   /// \brief add platform specific search directories.
   virtual void addDefaultSearchDirs(llvm::Triple & /*triple*/) {
     addSearchPath("=/usr/lib");
   }
 
+  // We can parse several linker scripts via command line whose ASTs are stored
+  // in the current linking context via addLinkerScript().
+  void addLinkerScript(std::unique_ptr<script::Parser> script) {
+    _scripts.push_back(std::move(script));
+  }
+
 private:
   ELFLinkingContext() LLVM_DELETED_FUNCTION;
 
 protected:
   ELFLinkingContext(llvm::Triple, std::unique_ptr<TargetHandlerBase>);
 
   Writer &writer() const override;
 
protected:
   StringRef _initFunction;
   StringRef _finiFunction;
   StringRef _sysrootPath;
   StringRef _soname;
   StringRefVector _rpathList;
   StringRefVector _rpathLinkList;
   std::map<std::string, uint64_t> _absoluteSymbols;
   llvm::StringSet<> _dynamicallyExportedSymbols;
+  std::vector<std::unique_ptr<script::Parser>> _scripts;
 };
 } // end namespace lld
 
 #endif

lld/trunk/include/lld/ReaderWriter/LinkerScript.h

 /// \file
 /// \brief Linker script parser.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_READER_WRITER_LINKER_SCRIPT_H
 #define LLD_READER_WRITER_LINKER_SCRIPT_H
 
+#include "lld/Core/Error.h"
 #include "lld/Core/LLVM.h"
 #include "lld/Core/range.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"
 #include <memory>

 
 /// Recognizes syntactic constructs of a linker script using a predictive
 /// parser/recursive descent implementation.
 ///
 /// Based on the linker script documentation available at
 /// https://sourceware.org/binutils/docs/ld/Scripts.html
 class Parser {
 public:
-  explicit Parser(Lexer &lex) : _lex(lex), _peekAvailable(false) {}
+  explicit Parser(std::unique_ptr<MemoryBuffer> mb)
+      : _lex(std::move(mb)), _peekAvailable(false) {}
 
-  LinkerScript *parse();
+  /// Let's not allow copying of Parser class because it would be expensive
+  /// to update all the AST pointers to a new buffer.
+  Parser(const Parser &instance) LLVM_DELETED_FUNCTION;
+
+  /// Lex and parse the current memory buffer to create a linker script AST.
+  std::error_code parse();
+
+  /// Returns a reference to the top level node of the linker script AST.
+  LinkerScript *get() { return &_script; }
 
 private:
   /// Advances to the next token, either asking the Lexer to lex the next token
   /// or obtaining it from the look ahead buffer.
   void consumeToken() {
     // First check if the look ahead buffer cached the next token
     if (_peekAvailable) {
       _tok = _bufferedToken;

 
 private:
   // Owns the entire linker script AST nodes
   llvm::BumpPtrAllocator _alloc;
 
   // The top-level/entry-point linker script AST node
   LinkerScript _script;
 
-  Lexer &_lex;
+  Lexer _lex;
 
   // Current token being analyzed
   Token _tok;
 
   // Annotate whether we buffered the next token to allow peeking
   bool _peekAvailable;
   Token _bufferedToken;
 };
 } // end namespace script
 } // end namespace lld
 
 #endif

lld/trunk/lib/Driver/GnuLdDriver.cpp

 }
 
 std::error_code
 GnuLdDriver::evalLinkerScript(ELFLinkingContext &ctx,
                               std::unique_ptr<MemoryBuffer> mb,
                               raw_ostream &diag) {
   // Read the script file from disk and parse.
   StringRef path = mb->getBufferIdentifier();
-  auto lexer = llvm::make_unique<script::Lexer>(std::move(mb));
-  auto parser = llvm::make_unique<script::Parser>(*lexer);
-  script::LinkerScript *script = parser->parse();
+  auto parser = llvm::make_unique<script::Parser>(std::move(mb));
+  if (std::error_code ec = parser->parse())
+    return ec;
+  script::LinkerScript *script = parser->get();
   if (!script)
     return LinkerScriptReaderError::parse_error;
   // Evaluate script commands.
   // Currently we only recognize this subset of linker script commands.
   for (const script::Command *c : script->_commands) {
     if (auto *group = dyn_cast<script::Group>(c))
       if (std::error_code ec = evaluateLinkerScriptGroup(ctx, path, group, diag))
         return ec;
     if (auto *searchDir = dyn_cast<script::SearchDir>(c))
       ctx.addSearchPath(searchDir->getSearchPath());
     if (auto *entry = dyn_cast<script::Entry>(c))
       ctx.setEntrySymbolName(entry->getEntryName());
     if (auto *output = dyn_cast<script::Output>(c))
       ctx.setOutputPath(output->getOutputFileName());
   }
+  // Transfer ownership of the script to the linking context
+  ctx.addLinkerScript(std::move(parser));
   return std::error_code();
 }
 
 bool GnuLdDriver::applyEmulation(llvm::Triple &triple,
                                  llvm::opt::InputArgList &args,
                                  raw_ostream &diag) {
   llvm::opt::Arg *arg = args.getLastArg(OPT_m);
   if (!arg)

lld/trunk/lib/ReaderWriter/LinkerScript.cpp

void Sections::dump(raw_ostream &os) const {
   for (auto &command : _sectionsCommands) {
     command->dump(os);
     os << "\n";
   }
   os << "}\n";
 }
 
 // Parser functions
-LinkerScript *Parser::parse() {
+std::error_code Parser::parse() {
   // Get the first token.
   _lex.lex(_tok);
   // Parse top level commands.
   while (true) {
     switch (_tok._kind) {
     case Token::eof:
-      return &_script;
+      return std::error_code();
     case Token::semicolon:
       consumeToken();
       break;
     case Token::kw_output: {
       auto output = parseOutput();
       if (!output)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(output);
       break;
     }
     case Token::kw_output_format: {
       auto outputFormat = parseOutputFormat();
       if (!outputFormat)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(outputFormat);
       break;
     }
     case Token::kw_output_arch: {
       auto outputArch = parseOutputArch();
       if (!outputArch)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(outputArch);
       break;
     }
     case Token::kw_group: {
       auto group = parseGroup();
       if (!group)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(group);
       break;
     }
     case Token::kw_as_needed:
       // Not allowed at top level.
       error(_tok, "AS_NEEDED not allowed at top level.");
-      return nullptr;
+      return LinkerScriptReaderError::parse_error;
     case Token::kw_entry: {
       Entry *entry = parseEntry();
       if (!entry)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(entry);
       break;
     }
     case Token::kw_search_dir: {
       SearchDir *searchDir = parseSearchDir();
       if (!searchDir)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(searchDir);
       break;
     }
     case Token::kw_sections: {
       Sections *sections = parseSections();
       if (!sections)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(sections);
       break;
     }
     case Token::identifier:
     case Token::kw_hidden:
     case Token::kw_provide:
     case Token::kw_provide_hidden: {
       const Command *cmd = parseSymbolAssignment();
       if (!cmd)
-        return nullptr;
+        return LinkerScriptReaderError::parse_error;
       _script._commands.push_back(cmd);
       break;
     }
     default:
       // Unexpected.
       error(_tok, "expected linker script command");
-      return nullptr;
+      return LinkerScriptReaderError::parse_error;
     }
   }
-
-  return nullptr;
+  return LinkerScriptReaderError::parse_error;
 }
 
 const Expression *Parser::parseFunctionCall() {
   assert((_tok._kind == Token::identifier || _tok._kind == Token::kw_align) &&
          "expected function call first tokens");
   std::vector<const Expression *> params;
   StringRef name = _tok._range;
 

lld/trunk/utils/linker-script-test/linker-script-test.cpp

int main(int argc, const char **argv) {
   }
   {
     ErrorOr<std::unique_ptr<MemoryBuffer>> mb =
         MemoryBuffer::getFileOrSTDIN(argv[1]);
     if (std::error_code ec = mb.getError()) {
       llvm::errs() << ec.message() << "\n";
       return 1;
     }
-    Lexer l(std::move(mb.get()));
-    Parser p(l);
-    LinkerScript *ls = p.parse();
-    if (ls)
+    Parser p(std::move(mb.get()));
+    if (!p.parse()) {
+      LinkerScript *ls = p.get();
       ls->dump(llvm::outs());
     }
   }
+}