[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

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

299

Add a blank line after this line.

include/lld/ReaderWriter/LinkerScript.h

729

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

746

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

738

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

742

could we use ErrorOr ?

971

std::error_code instead of bool ?

[rafaelauler]

Thanks for the suggestions. Here is the new version.

[ruiu]

LGTM

include/lld/ReaderWriter/ELFLinkingContext.h

13

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

include/lld/ReaderWriter/LinkerScript.h

738

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.

include/lld/ReaderWriter/ELFLinkingContext.h

 //===- lld/ReaderWriter/ELFLinkingContext.h -------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_READER_WRITER_ELF_LINKER_CONTEXT_H
 #define LLD_READER_WRITER_ELF_LINKER_CONTEXT_H
 
+#include "LinkerScript.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 "llvm/ADT/StringSet.h"
public:
   std::string demangle(StringRef symbolName) const override;
   bool demangleSymbols() const { return _demangle; }
   void setDemangleSymbols(bool d) { _demangle = d; }
 
   /// \brief Align segments.
   bool alignSegments() const { return _alignSegments; }
   void setAlignSegments(bool align) { _alignSegments = align; }
 
+  // We can parse several "implicit" linker scripts, which are all input files
+  // that are not recognized as objects or archives. These are stored in the
+  // ELFLinkingContext with addLinkerScriptInstance(). However, only one
+  // "master" linker script can override the default linker script, specified
+  // with -T in the command line and, here, by calling
+  // addMasterLinkerScript().
+  script::ScriptInstance &
+  addLinkerScriptInstance(std::unique_ptr<script::ScriptInstance> script) {
+    _scripts.push_back(std::move(script));
+    return *_scripts.back().get();
+  }
+  script::ScriptInstance &
+  addMasterLinkerScript(std::unique_ptr<script::ScriptInstance> script) {
+    _scripts.push_back(std::move(script));
+    _masterLinkerScript = _scripts.back().get();
+    return *_masterLinkerScript;
+  }
 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::ScriptInstance>> _scripts;
+  script::ScriptInstance *_masterLinkerScript;
 };
 } // end namespace lld
 
 #endif

include/lld/ReaderWriter/LinkerScript.h

 /// 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) {}
 
+  /// 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;
+
   LinkerScript *parse();
 
 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;
       _peekAvailable = false;
       return;
     }
     _lex.lex(_tok);
   }
 
   /// Returns the token that succeeds the current one without consuming the
   /// current token. This operation will lex an additional token and store it in
   /// a private buffer.
   const Token &peek() {
     if (_peekAvailable)
       return _bufferedToken;
 
     _lex.lex(_bufferedToken);
     _peekAvailable = true;
     return _bufferedToken;
   }
private:
 
   // Current token being analyzed
   Token _tok;
 
   // Annotate whether we buffered the next token to allow peeking
   bool _peekAvailable;
   Token _bufferedToken;
 };
+
+/// Encapsulates a linker script reading session, including lexer, parser and
+/// semantic analysis.
+class ScriptInstance {
+public:
+  ScriptInstance(std::unique_ptr<MemoryBuffer> mb)
+      : _lex(llvm::make_unique<Lexer>(std::move(mb))),
+        _parser(llvm::make_unique<Parser>(*_lex)) {
+    _topLevelNode = _parser->parse();
+  }
+
+  LinkerScript *getTopLevelNode();
+
+private:
+  std::unique_ptr<Lexer> _lex;
+  std::unique_ptr<Parser> _parser;
+  LinkerScript *_topLevelNode;
+};
 } // end namespace script
 } // end namespace lld
 
 #endif

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();
+  script::LinkerScript *script =
+      ctx.addLinkerScriptInstance(llvm::make_unique<script::ScriptInstance>(
+                                      std::move(mb))).getTopLevelNode();
   if (!script)
     return LinkerScriptReaderError::parse_error;
   // Evaluate script commands.
   // Currently we only recognize this subset of linker script commands:
   // - GROUP()
   // - SEARCH_DIR()
   for (const script::Command *c : script->_commands) {
     if (auto *group = dyn_cast<script::Group>(c))

lib/ReaderWriter/ELF/ELFLinkingContext.cpp

ELFLinkingContext::ELFLinkingContext(
     llvm::Triple triple, std::unique_ptr<TargetHandlerBase> targetHandler)
     : _outputELFType(llvm::ELF::ET_EXEC), _triple(triple),
       _targetHandler(std::move(targetHandler)), _baseAddress(0),
       _isStaticExecutable(false), _noInhibitExec(false), _exportDynamic(false),
       _mergeCommonStrings(false), _useShlibUndefines(true),
       _dynamicLinkerArg(false), _noAllowDynamicLibraries(false),
       _mergeRODataToTextSegment(true), _demangle(true), _alignSegments(true),
       _outputMagic(OutputMagic::DEFAULT), _initFunction("_init"),
-      _finiFunction("_fini"), _sysrootPath("") {}
+      _finiFunction("_fini"), _sysrootPath(""), _masterLinkerScript(nullptr) {}
 
 void ELFLinkingContext::addPasses(PassManager &pm) {
   pm.add(std::unique_ptr<Pass>(new elf::OrderPass()));
 }
 
 uint16_t ELFLinkingContext::getOutputMachine() const {
   switch (getTriple().getArch()) {
   case llvm::Triple::x86:

lib/ReaderWriter/LinkerScript.cpp

Sections *Parser::parseSections() {
   if (!expectAndConsume(
           Token::r_brace,
           "expected symbol assignment, entry, overlay or output section name."))
     return nullptr;
 
   return new (_alloc) Sections(sectionsCommands);
 }
 
+// ScriptInstance functions
+LinkerScript *ScriptInstance::getTopLevelNode() {
+  return _topLevelNode;
+}
+
 } // end namespace script
 } // end namespace lld