[PATCH] D73788: [yaml2obj/obj2yaml] - Add support for the SHT_LLVM_CALL_GRAPH_PROFILE sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1015-1016
+  for (const ELFYAML::CallGraphEntry &E : *Section.Entries) {
+    unsigned From = toSymbolIndex(E.From, Section.Name, /*IsDynamic=*/ false);
+    unsigned To = toSymbolIndex(E.To, Section.Name, /*IsDynamic=*/ false);
+
----------------
Nit: I think there shouldn't be a space before the "false" in these two lines.


================
Comment at: llvm/test/tools/obj2yaml/call-graph-profile-section.yaml:111
+
+## Check how we handle broken cases.
+
----------------
I think this is missing a non-16 sh_entsize test case.


================
Comment at: llvm/test/tools/obj2yaml/call-graph-profile-section.yaml:202-203
+    Content: "00112233445566778899AABBCCDDEEFF"
+## Case 3: Check we use the "Content" property when unable to match a
+##         symbol index to a symbol.
+  - Name: .unknown.symbol.1
----------------
Why do we do this? Can't we write the invalid index as  it is in the YAML below?


================
Comment at: llvm/test/tools/obj2yaml/call-graph-profile-section.yaml:216-217
+        Weight: 3
+## Case 4: Check we use the "Content" property when a linked section
+##         is not a symbol table.
+  - Name: .link.to.symtable
----------------
Again, I'm not sure about this: can't we just record the literal index value in this case, like in the YAML below?


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:22
+## Check that we link SHT_LLVM_CALL_GRAPH_PROFILE section with .symtab by default.
+# BASIC-NEXT:   Link: 2
+# BASIC-NEXT:   Info: 0
----------------
Use a regex capture here, in case the symbol table index ever changes.


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:121
+
+## Check we can set an arbitrary sh_link and sh_entsize values.
+## Check we can specify neither "Content" nor "Entries" tags.
----------------
Delete "an"


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:140
+    Link: 0x0
+    EntSize: 0
+  - Name: .llvm.bar
----------------
Indentation of values in this YAML doesn't line up.


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:162-163
+
+## Check we can describe SHT_LLVM_CALL_GRAPH_PROFILE sections using the "Entries" tag.
+## We can specify symbols either using names or indexes.
+# RUN: yaml2obj --docnum=7 %s -o %t.sym
----------------
Isn't this test case at least partially a duplicate of the "main" test case? Could the two be combined?


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:702
+
+  // Dump the section with use of Content key when it is empty or truncated.
+  if (Content.empty() || Content.size() % 16 != 0) {
----------------
with use of -> by using the

Surely if the section is empty, we don't need either Content or Entries?


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

https://reviews.llvm.org/D73788