[PATCH] D66532: [yaml2obj] - Lookup relocation symbols in dynamic symbol when .dynsym referenced.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with my suggested changes. Thanks for picking this up. I've been too busy doing other things to get back around to it.



================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:1001
       WithColor::error() << "Local symbol '" + Name +
                                 "' after global in Symbols list.\n";
       return false;
----------------
grimar wrote:
> btw, since this code is now used for dynamic symbols and yaml2obj is
> generally a tool that makes possible to produce the broken binaries,
> perhaps I'd try to remove this restriction logic for both regular and dynamic symbols
> in a follow up. 
Seems like a reasonable plan to me.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:1015
 
+template <class ELFT> bool ELFState<ELFT>::buildSymbolIndex() {
+  return buildSymbolsMap(Doc.Symbols, SymN2I) &&
----------------
Maybe rename this to `buildSymbolIndexes`, since there are more than one.


================
Comment at: test/tools/yaml2obj/dynamic-relocations.yaml:62
+      Binding: STB_GLOBAL
+    - Name: both
+      Section: .data
----------------
Minor point, but it might make sense to have `both` first in one of the two tables so that it doesn't have the same index. If I'm not mistaken, in the currents situation, the unpatched yaml2obj would actually generate the right thing as things stand for `both`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66532/new/

https://reviews.llvm.org/D66532