[PATCH] D89712: [CSSPGO][llvm-profgen] Disassemble text sections

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/disassemble.s:23
+
+.text
+foo:
----------------
wmi wrote:
> Can we have a test with multiple .text sections and some of the sections have suffixes like ".text.hot" or ".text.unlikely"? 
@wmi Thanks for your helpful suggestions.
Modified this test with multiple .text sections and ".text.hot" and ".text.unlikely"


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:52
+
+static bool InstIsCall(const MCInst &Inst, Triple TheTriple) {
+  if (TheTriple.isX86()) {
----------------
wmi wrote:
> Can we use MCInstrDesc::isCall()?
fixed, thanks!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:72
+
+static bool InstIsReturn(const MCInst &Inst, Triple TheTriple) {
+  if (TheTriple.isX86()) {
----------------
wmi wrote:
> Can we use MCInstrDesc::isReturn()?
fixed!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:126
+  Binary &Binary = *OBinary.getBinary();
+  if (auto Obj = dyn_cast<ELFObjectFileBase>(&Binary)) {
+    TheTriple = Obj->makeTriple();
----------------
wmi wrote:
> Can we reverse the condition here?
> auto Obj = dyn_cast<ELFObjectFileBase>(&Binary)
> if (!Obj)
>   exitWithError("not a valid Elf image", Path);
> ...
Good catch, fixed!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:260-291
+      if (ShowDisassembly)
+        outs() << '<' << SymbolName << ">:\n";
+
+      uint64_t RVA = StartRVA;
+      while (RVA < EndRVA) {
+        MCInst Inst;
+        uint64_t Size;
----------------
wmi wrote:
> Can we wrap it in a function like disassembleSymbol, to make the disassemble function smaller?
Yeah, It seems there are so many arguments after the refactoring, so I changed the scope of those MC ptrs, move into the class scope, also wrap them into `setUpDisassembler` function.

cc: @hoy 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89712