[PATCH] D68333: [yaml2obj/obj2yaml] - Add support for SHT_LLVM_ADDRSIG sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:260-262
+  AddrsigSymbol(StringRef N) : Name(N), Index(None){};
+  AddrsigSymbol(llvm::yaml::Hex32 Ndx) : Name(None), Index(Ndx){};
+  AddrsigSymbol() : Name(None), Index(None){};
----------------
clang-format?


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:263
+  AddrsigSymbol() : Name(None), Index(None){};
+  Optional<StringRef> Name;
+  Optional<llvm::yaml::Hex32> Index;
----------------
I'd find it easier to read if there were a blank line between the methods and member variables.


================
Comment at: test/tools/obj2yaml/elf-llvm-addrsig-section.yaml:4
+## Check that when possible obj2yaml tries to produce the "Name" tag when
+## dumps entries of the SHT_LLVM_ADDRSIG section. It falls back to producing
+## the "Index" tag when can't match a symbol index found with a symbol table entry.
----------------
dumps -> dumping


================
Comment at: test/tools/obj2yaml/elf-llvm-addrsig-section.yaml:5
+## dumps entries of the SHT_LLVM_ADDRSIG section. It falls back to producing
+## the "Index" tag when can't match a symbol index found with a symbol table entry.
+
----------------
when it can't match

I'd delete "found"


================
Comment at: test/tools/obj2yaml/elf-llvm-addrsig-section.yaml:56
+
+## Check that malformed obj2yaml dumps the SHT_LLVM_ADDRSIG section
+## data using the "Content" tag when at least one of the entries is broken.
----------------
Delete "malformed"


================
Comment at: test/tools/obj2yaml/elf-llvm-addrsig-section.yaml:57-58
+## Check that malformed obj2yaml dumps the SHT_LLVM_ADDRSIG section
+## data using the "Content" tag when at least one of the entries is broken.
+## E.g. contains a mailformed uleb128 value.
+
----------------
broken, e.g. because the entry contains a malformed


================
Comment at: test/tools/obj2yaml/elf-llvm-addrsig-section.yaml:61
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: obj2yaml %t2 | FileCheck %s --check-prefix=INVALID-INDEX
+
----------------
I might use the prefix "INVALID-ENTRY"


================
Comment at: test/tools/obj2yaml/elf-llvm-addrsig-section.yaml:79
+
+## obj2yaml uses "Symbols" tag to describe an empty SHT_LLVM_ADDRSIG section.
+
----------------
obj2yaml produces a "Symbols" tag when describing an ...


================
Comment at: test/tools/yaml2obj/elf-llvm-addrsig-section.yaml:3
+
+## Check we can describe SHT_LLVM_ADDRSIG using "Symbols" tag. We can define
+## symbols either using names or indexes.
----------------
using the "Symbols" tag


================
Comment at: test/tools/yaml2obj/elf-llvm-addrsig-section.yaml:117
+
+## Check that maximum "Symbol"->"Index" size is 32 bits.
+
----------------
I'm not really sure what `"Symbol"->"Index"` is trying to say. Perhaps it could be written as follows: "Check that the maximum symbol index size is 32 bits."

This actually shows that an error is produced if the value is out of range. You should have a separate case without the error, using the maximum possible value.


================
Comment at: test/tools/yaml2obj/elf-llvm-addrsig-section.yaml:135
+
+## Check we can use "Content" tag to specify any data for the SHT_LLVM_ADDRSIG section.
+
----------------
the "Content" tag


================
Comment at: test/tools/yaml2obj/elf-llvm-addrsig-section.yaml:158
+
+## Either "Content" or "Symbols" must be specifed for the SHT_LLVM_ADDRSIG section.
+
----------------
"for SHT_LLVM_ADDRSIG sections." here and elsewhere.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:865
+
+  // Get symbol with index sh_info which name is the signature of the group.
+  Expected<StringRef> SymbolName = getSymbolName(Shdr->sh_link, Shdr->sh_info);
----------------
Whilst you're moving code around, I'd improve this comment slightly:
"Get symbol with index sh_info. This symbol's name is the signature of the group.


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

https://reviews.llvm.org/D68333