Page MenuHomePhabricator

[llvm-readobj][test] - Add a test for --elf-cg-profile option.
ClosedPublic

Authored by grimar on Thu, Feb 6, 3:18 AM.

Details

Summary

This adds a test to document --elf-cg-profile option we have.
I am going to refactor this area, and this patch is mostly to
create a base for a follow-up change.

Diff Detail

Event Timeline

grimar created this revision.Thu, Feb 6, 3:18 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay accepted this revision.Thu, Feb 6, 12:26 PM
This revision is now accepted and ready to land.Thu, Feb 6, 12:26 PM

Aside: why is this switch prefixed with "elf"? I wouldn't think this section is ELF-specific in principle...

llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test
2

handle the --elf-cg-profile...

33

Maybe refer to the symbols by name for clarity/robustness?

grimar added a comment.EditedFri, Feb 7, 1:26 AM

Aside: why is this switch prefixed with "elf"? I wouldn't think this section is ELF-specific in principle...

Hmmm. Should we rename to --llvm-cg-profile? Its a LLVM extention first of all.
And if we want to implement the GNU version (that is what I am working on), perhaps it is good idea to add this suffix to emphasise that is not a common GNU option?

Aside: why is this switch prefixed with "elf"? I wouldn't think this section is ELF-specific in principle...

Hmmm. Should we rename to --llvm-cg-profile? Its a LLVM extention first of all.
And if we want to implement the GNU version (that is what I am working on), perhaps it is good idea to all this suffix to emphasise that is not a common GNU option?

Seems reasonable to me. If the section does get adopted by GNU, it likely will need to go through some standardisation (e.g. getting a new SHT_* type), so the more generic "--cg-profile" switch can be saved for then.

grimar added a comment.Fri, Feb 7, 2:16 AM

Aside: why is this switch prefixed with "elf"? I wouldn't think this section is ELF-specific in principle...

Hmmm. Should we rename to --llvm-cg-profile? Its a LLVM extention first of all.
And if we want to implement the GNU version (that is what I am working on), perhaps it is good idea to all this suffix to emphasise that is not a common GNU option?

Seems reasonable to me. If the section does get adopted by GNU, it likely will need to go through some standardisation (e.g. getting a new SHT_* type), so the more generic "--cg-profile" switch can be saved for then.

OK. I'll leave it for a one more follow-up then (+ lets see what others think too).

grimar updated this revision to Diff 243499.Mon, Feb 10, 3:12 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test
33

Done.

--llvm-cg-profile sounds good.

(I heard that .llvm.call-graph-profile is not that useful.. It also conflicts with lld --symbol-ordering-file=, which may be more useful..

This revision was automatically updated to reflect the committed changes.