[PATCH] D39511: [ELF] Support expressions with -defsym option

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: ELF/ScriptLexer.cpp:206
+  if (E == StringRef::npos) {
+    Ret.push_back(S);
+    return Ret;
----------------
grimar wrote:
> What is this code for ? I tried following and no testcases failed for me:
> 
> ```
>   if (E == StringRef::npos) {
>     assert(false);
>     Ret.push_back(S);
>     return Ret;
>   }
> ```
It's the `-defsym=foo` case, I've added test case for this.


================
Comment at: ELF/ScriptParser.cpp:274
+void ScriptParser::readDefsym() {
+  bool Orig = InDefsym;
+  InDefsym = true;
----------------
ruiu wrote:
> You create a new instance of ScriptParser for each -defsym string, so Orig is always the default value which is false.
I've changed this to use enum to represent the context.


================
Comment at: ELF/ScriptParser.cpp:1342
+
+void elf::readDefsym(MemoryBufferRef MB) {
+  ScriptParser(MB).readDefsym();
----------------
ruiu wrote:
> Actually, I think a better way of doing this is to pass two StringRefs, one for symbol name and the other for expression to this function.  Introducing a different tokenization scheme only to separate "<sym>=<expr>" into <sym> and <expr> seems a bit too much. You probably should split a -defsym argument into two StringRefs in the driver.
That's what I've done in the first version: https://reviews.llvm.org/D39511?id=121190. Would you prefer that approach? The current approach is more akin to what gold and BFD.ld does. The main advantage of this approach is that the error messages that come out of script parser have the right offset, i.e. for `-defsym=foo=bar`, the error at offset 1 points to `f` while with the original approach it'd be `b`.


Repository:
  rL LLVM

https://reviews.llvm.org/D39511