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

almost there! I like the tests btw :)



================
Comment at: lldb/source/Commands/Options.td:304
+  def disassemble_options_kind : Option<"kind", "k">,
+    Desc<"Show instruction control flow kind. Refer enum "
+    "`InstructionControlFlowKind` for a list of control flow kind. "
----------------



================
Comment at: lldb/source/Commands/Options.td:1159
+  def thread_trace_dump_instructions_show_kind : Option<"kind", "k">, Group<1>,
+    Desc<"Show instruction control flow kind. Refer enum "
+    "`InstructionControlFlowKind` for a list of control flow kind. "
----------------
same


================
Comment at: lldb/source/Core/Disassembler.cpp:578
+/// instruction, which are represented as the following parameters.
+/// Refer http://ref.x86asm.net/coder.html to see the full list of
+/// opcode and instruction set.
----------------



================
Comment at: lldb/source/Core/Disassembler.cpp:588
+/// \param[in] opcode_len
+///    The length of opcode. Valid opcode lengths are 1, 2, or 3.
+///
----------------



================
Comment at: lldb/source/Core/Disassembler.cpp:592-594
+///   eInstructionControlFlowKindOther if the instruction is not interesting.
+///   i.e. a sequential instruction that doesn't affect the control flow of
+///   the program.
----------------



================
Comment at: lldb/source/Core/Disassembler.cpp:705
+/// Decode an instruction into opcode, modrm and opcode_len.
+/// Refer http://ref.x86asm.net/coder.html for the instruction bytes layout.
+/// Opcodes in x86 are generally the first byte of instruction, though two-byte
----------------



================
Comment at: lldb/source/Core/Disassembler.cpp:734-736
+bool InstructionLengthDecode(const uint8_t *inst_bytes, bool is_exec_mode_64b,
+                             int bytes_len, uint8_t *primary_opcode,
+                             uint8_t *modrm, uint8_t *opcode_len) {
----------------
We try not to use out parameters this way. Instead, create a new struct InstructionOpcodeAndModrm with the three values that are important to you. Then, change this function to return Optional<InstructionOpcodeAndModrm>, where the None case means that decoding failed


================
Comment at: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp:28-29
+
+  //  virtual void SetUp() override { }
+  //  virtual void TearDown() override { }
+
----------------
delete if not used


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

https://reviews.llvm.org/D128477