[PATCH] [lld] Teach LLD how to parse complete linker scripts

[Shankar Kalpathi Easwaran]

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:42-68
@@ -40,1 +41,29 @@
     r_paren,
+    star,
+    starequal,
+    plus,
+    plusequal,
+    comma,
+    minus,
+    minusequal,
+    slash,
+    slashequal,
+    number,
+    colon,
+    semicolon,
+    less,
+    lessequal,
+    lessless,
+    lesslessequal,
+    equal,
+    equalequal,
+    greater,
+    greaterequal,
+    greatergreater,
+    greatergreaterequal,
+    question,
+    identifier,
+    libname,
+    kw_align,
+    kw_align_with_input,
+    kw_as_needed,
----------------
All of the above need to have a prefix.Also you may need to sort all the enumerated constants like others to be consistent.

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:276
@@ +275,3 @@
+
+  const StringRef getEntryName() const { return _entryName; }
+
----------------
const StringRef => StringRef everywhere.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:295
@@ -201,3 +294,3 @@
 
-  const StringRef getEntryName() const {
-    return _entryName;
+  const StringRef getSearchPath() const { return _searchPath; }
+
----------------
remove const StringRef, StringRef is const already

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:335
@@ +334,3 @@
+public:
+  Constant(uint64_t num) : Expression(Kind::Constant), _num(num) {}
+  void dump(raw_ostream &os) const override;
----------------
explicit Constant ? There are other classes that would need explicit as well.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:400-412
@@ +399,15 @@
+public:
+  enum Operation {
+    And,
+    CompareDifferent,
+    CompareEqual,
+    CompareGreater,
+    CompareGreaterEqual,
+    CompareLess,
+    CompareLessEqual,
+    Div,
+    Mul,
+    Or,
+    Shl,
+    Shr,
+    Sub,
----------------
There are enums which are not classes, and some enums which are plain enums. We would need to be consistent.

Rename the enumeration names as documented in the convention.

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:432-437
@@ +431,8 @@
+
+class TernaryConditional : public Expression {
+public:
+  TernaryConditional(const Expression *conditional, const Expression *trueExpr,
+                     const Expression *falseExpr)
+      : Expression(Kind::TernaryConditional), _conditional(conditional),
+        _trueExpr(trueExpr), _falseExpr(falseExpr) {}
+
----------------
Can you add comment on whats the depth of the ternary condition thats supported as well as an example like how you document other commands.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:434
@@ +433,3 @@
+public:
+  TernaryConditional(const Expression *conditional, const Expression *trueExpr,
+                     const Expression *falseExpr)
----------------
explicit again.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:464-465
@@ +463,4 @@
+public:
+  enum AssignmentKind { Simple, Sum, Sub, Mul, Div, Shl, Shr, And, Or };
+  enum AssignmentVisibility { Normal, Hidden, Provide, ProvideHidden };
+
----------------
look at previous comments on enums.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:657-667
@@ +656,13 @@
+
+/// Represents an Overlay structure as documented in
+/// https://sourceware.org/binutils/docs/ld/Overlay-Description.html#Overlay-Description
+class Overlay : public Command {
+public:
+  Overlay() : Command(Kind::Overlay) {}
+
+  static bool classof(const Command *c) {
+    return c->getKind() == Kind::Overlay;
+  }
+
+  void dump(raw_ostream &os) const override { os << "Overlay description\n"; }
+};
----------------
>From your tests, do you see any that uses Overlay command ?

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:92-139
@@ -41,1 +91,50 @@
 
+static llvm::ErrorOr<uint64_t> parseDecimal(StringRef str) {
+  uint64_t res = 0;
+  for (auto &c : str) {
+    res *= 10;
+    if (c < '0' || c > '9')
+      return std::errc::io_error;
+    res += c - '0';
+  }
+  return res;
+}
+
+static llvm::ErrorOr<uint64_t> parseOctal(StringRef str) {
+  uint64_t res = 0;
+  for (auto &c : str) {
+    res <<= 3;
+    if (c < '0' || c > '7')
+      return std::errc::io_error;
+    res += c - '0';
+  }
+  return res;
+}
+
+static llvm::ErrorOr<uint64_t> parseBinary(StringRef str) {
+  uint64_t res = 0;
+  for (auto &c : str) {
+    res <<= 1;
+    if (c != '0' && c != '1')
+      return std::errc::io_error;
+    res += c - '0';
+  }
+  return res;
+}
+
+static llvm::ErrorOr<uint64_t> parseHex(StringRef str) {
+  uint64_t res = 0;
+  for (auto &c : str) {
+    res <<= 4;
+    if (c >= '0' && c <= '9')
+      res += c - '0';
+    else if (c >= 'a' && c <= 'f')
+      res += c - 'a' + 10;
+    else if (c >= 'A' && c <= 'F')
+      res += c - 'A' + 10;
+    else
+      return std::errc::io_error;
+  }
+  return res;
+}
+
----------------
Could you move all the above functions to lld/Support ? These functions may be useful for even command line parsing for example to support --section-start as well as for other flavors.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:308-312
@@ +307,7 @@
+/// characters.
+static StringRef drop(StringRef &s, int n) {
+  StringRef res = s.substr(0, n);
+  s = s.drop_front(n);
+  return res;
+}
+
----------------
How will this perform ? For large linker scripts drop_front is going to be repeatedly called for every token. 

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:577-582
@@ -204,1 +576,8 @@
 
+// Constant functions
+void Constant::dump(raw_ostream &os) const { os << _num; }
+
+// Symbol functions
+void Symbol::dump(raw_ostream &os) const { os << _name; }
+
+// FunctionCall functions
----------------
move this inside the class itself.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:754-757
@@ +753,6 @@
+
+// InputSectionName functions
+void InputSectionName::dump(raw_ostream &os) const {
+  os << _name;
+}
+
----------------
move this inside the class itself.

http://reviews.llvm.org/D5852