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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D81894#2096899 <https://reviews.llvm.org/D81894#2096899>, @zequanwu wrote:

> 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`.


We can never assume that the input is valid. llvm-readobj is a tool for inspecting objects, which is often used to investigate what is broken.
That is why it has so many places where errors are reported.



================
Comment at: llvm/test/tools/llvm-readobj/COFF/call-graph-profile.test:1
+# RUN: yaml2obj %s | llvm-readobj --cg-profile | FileCheck %s
+
----------------
I'd ask not to do in this way, but do:

```
# RUN: yaml2obj %s -o %t
# RUN: llvm-readobj %t --cg-profile | FileCheck %s
```

It is very inconvenient to debug failing tests which do not create temporary outputs.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/call-graph-profile.test:33
+    Alignment:       1
+    SectionData:     0400000008000000200000000000000009000000040000000B0000000000000005000000060000001400000000000000
+symbols:
----------------
You should probably put a comment about how to recreate this bytes.
(e.g. a piece of asm)


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


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1521
 
-      Expected<COFFSymbolRef> Linked = Obj->getSymbol(Aux->TagIndex);
-      if (!Linked)
-        reportError(Linked.takeError(), Obj->getFileName());
-
-      Expected<StringRef> LinkedName = Obj->getSymbolName(*Linked);
-      if (!LinkedName)
-        reportError(LinkedName.takeError(), Obj->getFileName());
+      StringRef LinkedName = getSymbolName(Aux->TagIndex);
 
----------------
You can inline `LinkedName`.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1569
 
-      Expected<COFFSymbolRef> ReferredSym =
-          Obj->getSymbol(Aux->SymbolTableIndex);
-      if (!ReferredSym)
-        reportError(ReferredSym.takeError(), Obj->getFileName());
-
-      Expected<StringRef> ReferredName = Obj->getSymbolName(*ReferredSym);
-      if (!ReferredName)
-        reportError(ReferredName.takeError(), Obj->getFileName());
+      StringRef ReferredName = getSymbolName(Aux->SymbolTableIndex);
 
----------------
Also can be inlined.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1961
 
-    W.printNumber("Sym", *SymName, SymIndex);
+    W.printNumber("Sym", SymName, SymIndex);
     Cur += Size;
----------------
You can inline `SymName`:
W.printNumber("Sym", getSymbolName(SymIndex), SymIndex);


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1986
+      unwrapOrError(Obj->getFileName(), CGProfileSection.getContents());
+  ArrayRef<uint8_t> BinaryData(CGProfileContents.bytes_begin(),
+                               CGProfileContents.bytes_end());
----------------
Do you need this? `BinaryStreamReader` can take a `StringRef`:

`explicit BinaryStreamReader(StringRef Data, llvm::support::endianness Endian);`


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1988
+                               CGProfileContents.bytes_end());
+  BinaryStreamReader Reader(BinaryData, llvm::support::little);
+
----------------
Is COFF always little endian?


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:2001
+    StringRef From = getSymbolName(FromIndex);
+    StringRef To = getSymbolName(ToIndex);
+    DictScope D(W, "CGProfileEntry");
----------------
Probably `From` and `To` also will look better inlined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81894