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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: ELF/ScriptParser.cpp:1342
+
+void elf::readDefsym(MemoryBufferRef MB) {
+  ScriptParser(MB).readDefsym();
----------------
ruiu wrote:
> phosek wrote:
> > 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`.
> Yes, I think I prefer that over this patch. (A reason why you added defsym'ed symbols to the symbol table using addSymbolAlias in that patch isn't clear to me though.)
That was just a leftover code, that should be fixed now.


Repository:
  rL LLVM

https://reviews.llvm.org/D39511