[PATCH] D81775: [COFF] Add .llvm.call-graph-profile and `--coff-cg-profile` dumpping

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'd expect a dedicated test for the new llvm-readobj dumping behaviour in "test/tools/llvm-readobj/COFF", probably using yaml as input if the format is supported or the binary content isn't ridiculous. We need to be careful not to create circular testing to ensure that llvm-readobj doesn't share the same bug as MC.

It probably makes sense to split the llvm-readobj dumping changes into a separate patch, that can be done separately, probably in addition to a third patch as mentioned inline for the option.



================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1974
+
+void COFFDumper::printCGProfile() {
+  object::SectionRef CGProfileSection;
----------------
I've not looked into whether this is possible, but if it is, it might be nice to share at least some of the COFF and ELF code, since presumably the section format is identical in both cases.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:349-352
+  cl::opt<bool> ELFCGProfile("elf-cg-profile", cl::desc("Display callgraph profile section"));
+
+// --coff-cg-profile
+  cl::opt<bool> COFFCGProfile("coff-cg-profile", cl::desc("Display callgraph profile section"));
----------------
zequanwu wrote:
> Probably, these two should be combined into one option `--cg-profile`.
Yeah, rather than adding a coff-specific option, I'd add `--cg-profile` and make `--elf-cg-profile` an alias for backwards compatibility (or possibly even delete it, but I'd be somewhat reluctant to do that). You can do that in a separate patch before this one.

Also, the new option, in whatever form it takes, needs adding to the CommandGuide doc for llvm-readobj and llvm-readelf (the latter only for the generic `--cg-profile` option, and only if applicable).


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:509
       Dumper->printHashHistograms();
-    if (opts::CGProfile)
+    if (opts::ELFCGProfile)
       Dumper->printCGProfile();
----------------
By using the new name, you can avoid this code change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81775