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

[Rafael Auler]

Hi Shankar,
Glad you made a second round of suggestions, thanks. I answered some of your concerns below, but will also work on your suggestions and update the patch soon.

Best regards,
Rafael Auler

================
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,
----------------
shankarke wrote:
> 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
I just kept the coding style I found in this file, whose original author is not me. For me, it's no problem to convert this to the official LLVM naming convention.

This enum is already sorted according to Rui's suggestion, by using the ASCII table order.

Correct me if I'm wrong, but I think we can drop the prefix here, according to official LLVM naming rules. If the enum is inside a class, the prefix is not necessary.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:276
@@ +275,3 @@
+
+  const StringRef getEntryName() const { return _entryName; }
+
----------------
shankarke wrote:
> const StringRef => StringRef everywhere.
You're right! Thanks for spotting this!

================
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;
----------------
shankarke wrote:
> explicit Constant ? There are other classes that would need explicit as well.
I agree, will do, thanks.

================
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,
----------------
shankarke wrote:
> 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
I guess the criteria was to use enum classes when the enum is defined outside a class, like the one Rui pointed out.

Correct me if I'm wrong, but I think this enum already follows the LLVM standard -- when the enum is inside a class, we may drop the prefix, i.e.:  And instead of O_And.

================
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) {}
+
----------------
shankarke wrote:
> 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.
Will do.

================
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"; }
+};
----------------
shankarke wrote:
> From your tests, do you see any that uses Overlay command ?
None. Do you think it's better to remove this, for now?

================
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;
+}
+
----------------
shankarke wrote:
> How will this perform ? For large linker scripts drop_front is going to be repeatedly called for every token. 
This should be inlined, in fact, it was just a refactoring suggested by Rui that would enable us to write simpler code in the next lines. You're right though, it will be called a lot. On the other hand, I don't think that this code is bad-performing: substr() will just update the size of in the StringRef object and drop_front() will directly compute a new StringRef pointer and return it (pointer arithmetic) and update size as well. But I can profile this and discover if this is a limitation.

================
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
----------------
shankarke wrote:
> move this inside the class itself.
Should we leave these as virtual anchors in the C++ code or is this not a concern for the LLD project?

http://reviews.llvm.org/D5852