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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/disassemble.s:23
+
+.text
+foo:
----------------
Can we have a test with multiple .text sections and some of the sections have suffixes like ".text.hot" or ".text.unlikely"? 


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


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


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


================
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;
----------------
Can we wrap it in a function like disassembleSymbol, to make the disassemble function smaller?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89712