Page MenuHomePhabricator

[yaml2obj/obj2yaml] - Add support for the SHT_LLVM_CALL_GRAPH_PROFILE sections.
ClosedPublic

Authored by grimar on Fri, Jan 31, 8:03 AM.

Details

Summary

This is a LLVM specific section that is well described here:
https://llvm.org/docs/Extensions.html#sht-llvm-call-graph-profile-section-call-graph-profile

This patch teaches yaml2obj and obj2yaml about how to work with it.

Diff Detail

Event Timeline

grimar created this revision.Fri, Jan 31, 8:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Fri, Jan 31, 10:17 PM
llvm/test/tools/obj2yaml/call-graph-profile-section.yaml
8

This is verbose. Intention is to test that the section has the same representation with different EI_CLASS.

If https://reviews.llvm.org/D73821 looks good, we can add -D EI_CLASS=ELFCLASS32 and -D EI_DATA=ELFDATA2LSB to represent a similar object file.

jhenderson added inline comments.Mon, Feb 3, 2:06 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
1015–1016

Nit: I think there shouldn't be a space before the "false" in these two lines.

llvm/test/tools/obj2yaml/call-graph-profile-section.yaml
112

I think this is missing a non-16 sh_entsize test case.

203–204

Why do we do this? Can't we write the invalid index as it is in the YAML below?

217–218

Again, I'm not sure about this: can't we just record the literal index value in this case, like in the YAML below?

llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml
23

Use a regex capture here, in case the symbol table index ever changes.

122

Delete "an"

141

Indentation of values in this YAML doesn't line up.

163–164

Isn't this test case at least partially a duplicate of the "main" test case? Could the two be combined?

llvm/tools/obj2yaml/elf2yaml.cpp
702

with use of -> by using the

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

grimar marked 4 inline comments as done.Mon, Feb 3, 2:58 AM
grimar added inline comments.
llvm/test/tools/obj2yaml/call-graph-profile-section.yaml
8

This is verbose.

Yes, that is why I placed the "TODO" comment below.

203–204

Technially it is possible, but there is a problem behind:
When we read a binary (in obj2yaml) we read the From/To fields as an integers.
If we can't map them to a symbol and want to store them as StringRef, we need to convert them
to strings and save those strings somewhere
(the case with yaml2obj is different as it has strings representation from the begining).

The solution I used for AddrsigSymbol was:

struct AddrsigSymbol {
  AddrsigSymbol(StringRef N) : Name(N), Index(None) {}
  AddrsigSymbol(llvm::yaml::Hex32 Ndx) : Name(None), Index(Ndx) {}
  AddrsigSymbol() : Name(None), Index(None) {}

  Optional<StringRef> Name;
  Optional<llvm::yaml::Hex32> Index;
};

i.e. we can convert AddrsigSymbol to SymbolOrName and reuse probably.
But I'd like to investigate another options first, perhaps it is possible to eliminate AddrsigSymbol.

Given all above I supposed that it should be done separatelly as it should
change/refactor another code and hence probably deserves a separate follow-up patch.
What do you think?

jhenderson added inline comments.Mon, Feb 3, 3:08 AM
llvm/test/tools/obj2yaml/call-graph-profile-section.yaml
203–204

Given all above I supposed that it should be done separatelly as it should

change/refactor another code and hence probably deserves a separate follow-up patch.
What do you think?

That sounds fair. Thanks for the explanation!

grimar updated this revision to Diff 242020.Mon, Feb 3, 4:24 AM
grimar marked 11 inline comments as done.
  • Addressed review comments.
llvm/test/tools/obj2yaml/call-graph-profile-section.yaml
112

Reasonable. Done.

llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml
163–164

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.

jhenderson added inline comments.Mon, Feb 3, 4:49 AM
llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml
163–164

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.

llvm/tools/obj2yaml/elf2yaml.cpp
702

Given the behaviour change "when it is empty" is no longer valid.

grimar updated this revision to Diff 242058.Mon, Feb 3, 6:37 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Tue, Feb 4, 1:51 AM

LGTM, with nit.

llvm/tools/obj2yaml/elf2yaml.cpp
703

neither/nor -> either/or (in this context - the "no" provides the negation, making it unnecessary here).

This revision is now accepted and ready to land.Tue, Feb 4, 1:51 AM
grimar marked an inline comment as done.Tue, Feb 4, 2:12 AM
grimar added inline comments.
llvm/tools/obj2yaml/elf2yaml.cpp
703

Thanks!

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.