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

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

let's try to have tests soon. It seems that the code can be simplified and tests will be very handy



================
Comment at: lldb/include/lldb/lldb-enumerations.h:976
+  eInstructionControlFlowKindUnknown = 0,
+  /// The instruction is something not listed below, i.e. it's sequential
+  /// instruction that doesn't affect the control flow of the program.
----------------



================
Comment at: lldb/source/Core/Disassembler.cpp:730-740
+/// \param[out] opcode
+///    Primary opcode of the instruction.
+///
+/// \param[out] modrm
+///    ModR/M byte of the instruction.
+///    Bit[7:6] indicates MOD. Bit[5:3] specifies a register and R/M bit[2:0]
+///    may contain a register or specify an addressing mode, depending on MOD.
----------------
I'm noticing that MapOpcodeIntoControlFlowKind only uses PTI_MAP_0 and PTI_MAP_1, which are opcodes of 1 and 2 bytes. Any opcode of 3 bytes and even amd3dnow is not used at all. That means that you don't need that enum, so please delete it. Instead, use the length of the opcode throughout the code.



================
Comment at: lldb/source/Core/Disassembler.cpp:882
+
+  if (m_opcode.GetOpcodeBytes() == nullptr || m_opcode.GetByteSize() <= 0) {
+    return lldb::eInstructionControlFlowKindUnknown;
----------------
does this mean that all x86 and x86_64 instructions are categorized as Opcode::Type::eTypeBytes?
I'm asking because after reading GetOpcodeBytes, it only returns data if the type is eTypeBytes. Did you check that?


================
Comment at: lldb/source/Core/Disassembler.cpp:885
+  }
+  memcpy(inst_bytes, m_opcode.GetOpcodeBytes(), m_opcode.GetByteSize());
+
----------------
you don't need to copy GetOpcodeBytes, you can just pass it directly to InstructionLengthDecode


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

https://reviews.llvm.org/D128477