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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for doing this!



================
Comment at: test/Object/X86/yaml2obj-elf-x86-rel.yaml:41
+    Binding:         STB_GLOBAL
\ No newline at end of file

----------------
Nit: missing new line at EOF.


================
Comment at: test/tools/llvm-objdump/verneed-elf.test:48
+    Binding: STB_GLOBAL
\ No newline at end of file

----------------
Nit: No new line at EOF.


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


================
Comment at: test/tools/llvm-readobj/elf-section-types.test:223
+    Binding: STB_GLOBAL
\ No newline at end of file

----------------
Nit: no new line at EOF.


================
Comment at: test/tools/obj2yaml/verneed-section.yaml:70
+    Binding: STB_GLOBAL
\ No newline at end of file

----------------
Not: no new line at EOF.


================
Comment at: test/tools/yaml2obj/dynamic-symbols.yaml:25-28
+  - Name: dynlocal
+    Type: STT_OBJECT
+    Section: .data
+    Binding: STB_LOCAL
----------------
Why doesn't this cause an error?


================
Comment at: test/tools/yaml2obj/dynsym-dynstr-addr.yaml:39
 DynamicSymbols:
-  Global:
-    - Name: foo
+  - Name: foo
----------------
Nit: no binding here? Probably harmless though...


================
Comment at: test/tools/yaml2obj/elf-symbols-binding-order.yaml:3
+## We might want to change it later to allow doing that
+## for producing the broken outputs.
+
----------------
Nit: delete "the"


================
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.
+
----------------
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".


================
Comment at: test/tools/yaml2obj/elf-symtab-shtype.yaml:22
+    Binding: STB_GLOBAL
\ No newline at end of file

----------------
Nit: no new line at EOF.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:324
 
+static size_t findLocalsNum(ArrayRef<ELFYAML::Symbol> Symbols) {
+  for (size_t I = 0; I < Symbols.size(); ++I)
----------------
Perhaps better to call this "findLastLocal" or something along those lines. It's not clear what "LocalsNum" is.


================
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);
----------------
Is this behaviour tested anywhere?


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

https://reviews.llvm.org/D60122