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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
