[PATCH] D60122: [yaml2obj][obj2yaml] - Change how symbol's binding is descibed when parsing/dumping.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/broken-group.test:79
+    Section:         .group
+    Binding:         STB_GLOBAL
+  - Name:            zed
----------------
jhenderson wrote:
> This doesn't match up with their old binding. Was that deliberate?
No. Thanks for catching!


================
Comment at: test/tools/yaml2obj/dynsym-dynstr-addr.yaml:39
 DynamicSymbols:
-  Global:
-    - Name: foo
+  - Name: foo
----------------
jhenderson wrote:
> Nit: no binding here? Probably harmless though...
It was not my intention to change any bindings. Fixed, thanks!


================
Comment at: test/tools/yaml2obj/elf-symbols-binding-order.yaml:6
+# RUN: not yaml2obj %s -o %t 2>&1 | FileCheck %s
+# CHECK: error: Has a local symbol 'bar' after global.
+
----------------
jhenderson wrote:
> Nit: I'd get rid of "Has a". I don't think it's needed. I also think the message should disambiguate between dynamic and static symbols, e.g. "local symbol 'bar' after global in Symbols list".
Done. `buildSymbolIndex` is not called for dynamic symbols. Hence the error message now always mentions the "Symbols" list.

I thought about renaming `buildSymbolIndex` to `checkSymbolsAndBuildIndex` and pass both `Symbols`
and `DynamicSymbols` there. Since `.dynsym` normally do not contain local symbols, for `DynamicSymbols`
we should just probably report any local symbol occurrence. Since it would break the `dynamic-symbols.yaml`,
I would probably do such a change in an independent review.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:343
   // One greater than symbol table index of the last local symbol.
-  SHeader.sh_info = Symbols.Local.size() + 1;
+  SHeader.sh_info = findLocalsNum(Symbols) + 1;
   SHeader.sh_entsize = sizeof(Elf_Sym);
----------------
jhenderson wrote:
> Is this behaviour tested anywhere?
I think no, I posted a patch: D60192 for that.


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

https://reviews.llvm.org/D60122