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

[Zequan Wu via Phabricator via llvm-commits]

zequanwu added a comment.

In D81894#2097480 <https://reviews.llvm.org/D81894#2097480>, @jhenderson wrote:

> In D81894#2096899 <https://reviews.llvm.org/D81894#2096899>, @zequanwu wrote:
>
> > In D81894#2094879 <https://reviews.llvm.org/D81894#2094879>, @jhenderson wrote:
> >
> > > 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.
> >
> >
> > Those error paths in `printCGProfile` are about reading integers from `SectionData`, and only raise errors if the content of `SectionData` is not well formatted. I think it's probably not necessary. Since the `.llvm.call-graph-profile` section is written by `llvm-mc`. Shouldn't we assume the input is always valid? Otherwise, it's a problem of `llvm-mc`.
>
>
> It might well be caused by a problem in `llvm-mc` (or whatever other tool happens to be creating the section, e.g. the integrated assembler). If we don't test the error paths however, we could end up with bugs in handling of broken output, leading to crashes from llvm-readobj, or incorrect output results. The user will not know that these problems are caused by a malformed input, unless we tell them. In the worst case, a broken error handling path might produce incorrect results, which don't look unreasonable, so a user may not even notice that it's bad. Finally, if you don't test the error paths, you may find your messages are incorrect/not printed as you'd expect, which could be confusing to the user.
>
> In the ELF portion of llvm-readobj at least, we have gone to extensive lengths to ensure as many error handling code paths are tested as possible, and @grimar found several crashes along the way. I'm sure you'll agree that a crash is always bad, no matter the cause.


By error paths, do you mean the error path of `reportError` inside `printCGProfile` or `reportError` inside `getSymbolName`? The latter already exists in the codebase before, I just refactored those parts out and put them inside a function.



================
Comment at: llvm/test/tools/llvm-readobj/COFF/call-graph-profile.test:33
+    Alignment:       1
+    SectionData:     0400000008000000200000000000000009000000040000000B0000000000000005000000060000001400000000000000
+symbols:
----------------
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. 


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1973
+    else
+      consumeError(NameOrErr.takeError());
+
----------------
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. 


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1988
+                               CGProfileContents.bytes_end());
+  BinaryStreamReader Reader(BinaryData, llvm::support::little);
+
----------------
grimar wrote:
> Is COFF always little endian?
By looking at other parts of this file, I believe so. (only `llvm::support::little` is used throughout this file)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81894