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

[Zequan Wu via Phabricator via llvm-commits]

zequanwu marked an inline comment as done.
zequanwu added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/call-graph-profile.test:33
+    Alignment:       1
+    SectionData:     0400000008000000200000000000000009000000040000000B0000000000000005000000060000001400000000000000
+symbols:
----------------
jhenderson wrote:
> zequanwu wrote:
> > grimar wrote:
> > > zequanwu wrote:
> > > > grimar wrote:
> > > > > You should probably put a comment about how to recreate this bytes.
> > > > > (e.g. a piece of asm)
> > > > This is generated from a child patch (D81775) and I reduced it by only keeping symbols referred in `SectionData`. Those bytes represent the symbol indexes (which are not stable) and weight. 
> > > > 
> > > > Or I could use the previous version of the test, which includes all symbols and sections generated.
> > > > 
> > > > I don't if there is a better way to write the test. In `tools/llvm-readobj/ELF/call-graph-profile.test`, `yaml2obj` could generate the `SectionData` from `Entryies` for ELF.  For COFF, it lacks the functionalities. 
> > > Ok. Lets leave it as is for now then. I am not sure it is reasonable to reference the functionality of D81775 until it is not landed.
> > Actually, D81775 depends on this patch. Because the test case `cgprofile.s` in D81775 replies on the functionality of `llvm-readobj` to dump `CGProfile`.
> > 
> > Before, these two patch were in one patch. I split it into two. 
> Not sure if it's a good idea in this case or not, but an alternative could be to write this in assembly, but use `.byte`, `.short` etc to write the section content.
> 
> It might be helpful to label the section contents here with comments so that it is clear what each byte/byte range represents. A vaguely similar thing is done in D82073, with regards to the output data, which you might want to consider applying. Something like:
> ```
>     SectionData:     0400000008000000200000000000000009000000040000000B0000000000000005000000060000001400000000000000
>                      ^- something
>                        ^-------- something else
> ```
> etc
Since section name cannot have token `-`, I cannot create `.llvm.call-graph-profile` section from assembly. So, I added comment to explain what byte range represents.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1973
+    else
+      consumeError(NameOrErr.takeError());
+
----------------
jhenderson wrote:
> zequanwu wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > zequanwu wrote:
> > > > > jhenderson wrote:
> > > > > > Why `consumeError` rather than report the `Error` in some form?
> > > > > Because some section may not have name, and we can just skip them. This for loop is used to find the section with the name `.llvm.call-graph-profile`, similar to the part of `.llvm_addrsig` above.
> > > > Hmmm... okay. Personally, I think that if there's a problem reading the name, we should tell the user that. What if the intended section is the one missing the name due to some malformed output? In that case, the user will simply get no output, and wonder why.
> > > Each unobvious `consumeError` should have a comment to expain why it is acceptable to ignore an error.
> > By second look at it, I should use `unwrapOrError` to report error. 
> Thanks. I'm not sure what the prevailing behaviour in the COFF side of llvm-readobj is, but we try these days to emit warnings rather than errors in the ELF side, so that parsing can continue, and users can get as much useful output as possible. In this case, you'd report the `Error` as a warning, and then `continue` to the next section. It's up to you either way though.
Reporting error is the prevailing behavior in the COFF side of llvm-readobj, so I will stick with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81894