[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/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
----------------
grimar wrote:
> jhenderson wrote:
> > Isn't this test case at least partially a duplicate of the "main" test case? Could the two be combined?
> > Could the two be combined?
> I would not combine them. The intention of the main test is to check we encode 32/64/LE/BE bytes correctly.
> That is why I used raw data check there.
> 
> Here I check how we can use "Entires" and I use `--elf-cg-profile`. Using of `--elf-cg-profile` would be unreasonable
> until we know that encode section data correctly. 
> 
> Probably we can remove the "Case 2" from here as the same case was tested in the main case.
> I would leave it as is though as it complements the rest of this test case which tries to test all valid cases
> related to "Entries" at once.
I don't think there's strong reasoning for not adding an --elf-cg-profile check in addition to section data in the first test case, if you're concerned about the raw bytes matching the expected format.

That being said, testing the use of "Entries" in general terms is not needed in this case, since you are testing it in the first case. A set of tests that focuses purely on the symbol references is fine and appropriate. I think it's okay therefore to keep this set of tests here, but the headline comment really needs to emphasise that its testing that, not the "Entries" tag.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:702
+
+  // Dump the section by using the Content key when it is empty or truncated.
+  if (Content.empty() || Content.size() % 16 != 0) {
----------------
Given the behaviour change "when it is empty" is no longer valid.


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

https://reviews.llvm.org/D73788