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

Wed Jun 17 00:30:18 PDT 2020

jhenderson added a subscriber: grimar.
jhenderson added a comment.

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.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81894