[PATCH] D81894: [llvm-readobj][COFF] add .llvm.call-graph-profile section dump

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for splitting this up. The new test doesn't cover several of the error paths by the looks of it. You'll need to add additional test cases for each of these. I'd also expect a test case for no call graph section, to cover that code path, and possibly also for when there is more than one.



================
Comment at: llvm/test/tools/llvm-readobj/COFF/cgprofile.test:2
+# RUN: yaml2obj %s | llvm-readobj --cg-profile | FileCheck %s
+
+
----------------
Nit: get rid of the second blank line.

As the ELF test is named `call-graph-profile.test` it probably makes sense to name this test the same.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/cgprofile.test:27
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
----------------
Are all these sections actually necessary to test all the code paths in the new function? Could you cut it down to one section containing all the symbols?


================
Comment at: llvm/test/tools/llvm-readobj/COFF/cgprofile.test:163
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
\ No newline at end of file

----------------
Nit: no new line at end of file - please add one.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1968
+  object::SectionRef CGProfileSection;
+  for (auto Sec : Obj->sections()) {
+    StringRef Name;
----------------
Could you change this `auto` to the concrete type please. It's not obvious to me what the return type of `sections()` is.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1973
+    else
+      consumeError(NameOrErr.takeError());
+
----------------
Why `consumeError` rather than report the `Error` in some form?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81894