[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

nice job!!



================
Comment at: lldb/source/Commands/Options.td:304
+  def disassemble_options_kind : Option<"kind", "k">,
+    Desc<"Show instruction control flow type.">;
   def disassemble_options_context : Option<"context", "C">, Arg<"NumLines">,
----------------
mention here the enum to inspect for documentation. Besides that, you can quickly mention here that far jumps and far returns often indicate a kernel calls and returns. That might be enough because the other categories are pretty intuitive


================
Comment at: lldb/source/Commands/Options.td:1156
+  def thread_trace_dump_instructions_show_kind : Option<"kind", "k">, Group<1>,
+    Desc<"For each instruction, print the instruction type">;
   def thread_trace_dump_instructions_show_tsc : Option<"tsc", "t">, Group<1>,
----------------
same here


================
Comment at: lldb/source/Core/Disassembler.cpp:680
+Instruction::GetControlFlowInstructionKind(const ExecutionContext *exe_ctx) {
+  uint8_t *opcode_bytes = (uint8_t *)m_opcode.GetOpcodeBytes();
+  uint8_t opcode, modrm, map;
----------------
you have to check here that the triple of the current architecture of the current target (you can get that from exe_ctx) is x86, otherwise directly return eInstructionControlFlowTypeUnknown. You can just rely on the check you are doing in line 685 because that check might fail in the future


================
Comment at: lldb/source/Core/Disassembler.cpp:681
+  uint8_t *opcode_bytes = (uint8_t *)m_opcode.GetOpcodeBytes();
+  uint8_t opcode, modrm, map;
+  int op_idx = 0;
----------------
from this point on, the code is specifically meant for x86, so I recommend creating a x86 namespace in this file and include a GetControlFlowInstructionKind method along with the instruction_decode method there. That way we organize the code in a more cleaner manner


================
Comment at: lldb/source/Core/Disassembler.cpp:681
+  uint8_t *opcode_bytes = (uint8_t *)m_opcode.GetOpcodeBytes();
+  uint8_t opcode, modrm, map;
+  int op_idx = 0;
----------------
wallace wrote:
> from this point on, the code is specifically meant for x86, so I recommend creating a x86 namespace in this file and include a GetControlFlowInstructionKind method along with the instruction_decode method there. That way we organize the code in a more cleaner manner
add a comment explaining what these variables are


================
Comment at: lldb/source/Core/Disassembler.cpp:692
+  // Get opcode and modrm
+  map = PTI_MAP_INVALID;
+  while (!prefix_done) {
----------------
add a comment explaining what this prefix handling is actually doing


================
Comment at: lldb/source/Core/Disassembler.cpp:715
+    case 0x40 ... 0x4f:
+      if (exe_ctx->GetTargetRef().GetArchitecture().GetMachine() ==
+          llvm::Triple::ArchType::x86_64)
----------------
you can move this to the beginning of the function and assume that this mapping is x86 only, which is how it should be


================
Comment at: lldb/source/Core/Disassembler.cpp:773
+
+  if (opcode == 0x0F) {
+    if (opcode_bytes[op_idx + 1] == 0x38) {
----------------
you can also mention here how this mapping is actually generated and were you got inspiration from, like libipt


================
Comment at: lldb/source/Core/Disassembler.cpp:846
+  if (show_kind) {
+    // TODO-SUJIN
+    switch (GetControlFlowInstructionKind(exe_ctx)) {
----------------
should you remove this?


================
Comment at: lldb/source/Core/Disassembler.cpp:849
+    case eInstructionControlFlowTypeUnknown:
+      ss.Printf("%-12s", "unknwon");
+      break;
----------------
unknown


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:63
+InstructionControlFlowType
 DecodedThread::GetInstructionControlFlowType(size_t insn_index) const {
   if (IsInstructionAnError(insn_index))
----------------
remove this, because we won't use it anymore


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

https://reviews.llvm.org/D128477