[PATCH] [lld] [LinkerScript] Implement linker script expression evaluation

[Rafael Auler]

Hi all,

Thank you for your fast replies. Comments inline.


================
Comment at: include/lld/ReaderWriter/ELFLinkingContext.h:304-306
@@ -303,1 +303,5 @@
 
+  const std::vector<std::unique_ptr<script::Parser>> &scripts() const {
+    return _scripts;
+  }
+
----------------
shankarke wrote:
> do you want to add a boolean hasLinkerScript that checks the size and returns ?
Well, I added this function just to make this patch self-contained and allow us to properly unit-test the evaluator. It gets deleted in the follow-up patch because script::Sema is introduced to manage Parser copies.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:366
@@ +365,3 @@
+  // here is an overkill.
+  typedef llvm::StringMap<int64_t, llvm::BumpPtrAllocator> SymbolTableTy;
+
----------------
shankarke wrote:
> you could replace this with a llvm::DenseMap<StringRef, int64_t> and use a common StringRefMappingInfo.
I used this type because that is what Clang uses for its symbol table, but added the comment to make it easier to refactor this later in favour of other data structures, if needed.

But If we're gonna use std::map<std::string, T>, I'd rather stick with llvm::StringMap, since it is available in the LLVM library :)

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:794-795
@@ -767,2 +793,4 @@
   void dump(raw_ostream &os) const override;
+  const_iterator begin() const { return _sectionsCommands.begin(); }
+  const_iterator end() const { return _sectionsCommands.end(); }
 
----------------
shankarke wrote:
> Use lld::range,once you replace it with range, you can remove the typedef for const_iterator.
Thanks for the tip. Now that I'm thinking about this, there is also little reason to also use the typedef here, since we typically iterate through these elements with "auto", so we don't need to type the full name anyway. I used this mostly to keep code consistent with other classes and to make function signature cleaner. But if we're changing this, I'd rather do it in the follow-up patch, which introduces a whole lot more of iterators for accessing linker script AST nodes.


================
Comment at: lib/ReaderWriter/LinkerScript.cpp:545
@@ +544,3 @@
+    return LinkerScriptReaderError::unknown_symbol_in_expr;
+  return symbolTable[_name];
+}
----------------
ruiu wrote:
> Maybe you want to use find and compare the return value with end, so that you don't look up the table twice.
Thanks, will do.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:581
@@ +580,3 @@
+  switch (_op) {
+  case Unary::Minus:
+    return 0 - childRes;
----------------
shankarke wrote:
> Do you want to handle Unary::Plus ?? 
I guess this is currently not supported by the parser. Pardon my ignorance, but what is the semantic for this?

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:582
@@ +581,3 @@
+  case Unary::Minus:
+    return 0 - childRes;
+  case Unary::Not:
----------------
ruiu wrote:
> nit: return -childRes;
Fixed

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:584
@@ +583,3 @@
+  case Unary::Not:
+    return ~childRes;
+  }
----------------
shankarke wrote:
> I thought linker script uses only a boolean value of a Unary Not, no ?? 
That would be the token "!", but currently Unary::Not comes from parsing "~" tokens. I will update the parser to recognize "!" as a unary operator in a separate patch.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:588
@@ +587,3 @@
+  llvm_unreachable("");
+  return 0;
+}
----------------
ruiu wrote:
> Remove this line. I believe llvm_unreachable has __attribute__((noreturn)), so no warning would be issued.
Ok

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:703
@@ +702,3 @@
+    return trueExprRes;
+  else
+    return falseExprRes;
----------------
shankarke wrote:
> no need of else.
Fixed

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:705
@@ +704,3 @@
+    return falseExprRes;
+}
+
----------------
ruiu wrote:
> Is it okay to compute both "then" and "else" clauses regardless of a condition value? In a normal programming language, we don't do that -- for example, if (true) {} else { 1 / 0; } doesn't fail.
I previously thought that:

if (true) {} else { undefined_symbol + 1 }

should return an error, even though the else branch is not used. But now I see that this is just wrong, since, if we want to generate diagnostics for my example above, we should do this in the semantic analysis, not in the evaluator. I will fix this.

================
Comment at: unittests/DriverTests/GnuLdDriverTest.cpp:262
@@ +261,3 @@
+  EXPECT_EQ(script::SymbolAssignment::Simple, sa1->assignmentKind());
+  EXPECT_EQ(script::SymbolAssignment::Normal, sa1->assignmentVisibility());
+
----------------
shankarke wrote:
> Can we change SymbolAssignment::Normal to SymbolAssignment::Default ? 
> 
> Looks like you are using the same SymboAssignment to name two different things, assignmentKind and assignmentVisibilty. I would recommend separating them.
Sure, I changed the names.

About separating the concept of visibility, I'll do it in a separate patch when appropriate, thanks for the heads up.

http://reviews.llvm.org/D8156

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/