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


================
Comment at: lldb/include/lldb/Core/Disassembler.h:82-86
+  lldb::InstructionControlFlowType
+  instruction_decode(uint8_t opcode, uint8_t modrm, uint8_t map);
+
+  lldb::InstructionControlFlowType
+  GetControlFlowInstructionKind(const ExecutionContext *exe_ctx);
----------------
as this code is new, please write documentation and function names should be in PascalCase. Try to have a more descriptive name as well, because `instruction_decode` is very vague. 


================
Comment at: lldb/include/lldb/Core/Disassembler.h:152
   virtual void Dump(Stream *s, uint32_t max_opcode_byte_size, bool show_address,
-                    bool show_bytes, const ExecutionContext *exe_ctx,
+                    bool show_bytes, bool show_kind,
+                    const ExecutionContext *exe_ctx,
----------------
let's try to be more explicit with the naming


================
Comment at: lldb/include/lldb/Core/Disassembler.h:226-233
+  enum {
+    PTI_MAP_0,
+    PTI_MAP_1,
+    PTI_MAP_2,
+    PTI_MAP_3,
+    PTI_MAP_AMD3DNOW,
+    PTI_MAP_INVALID
----------------
add documentation. Also, if this mapping is intel-specific, I recommend moving this to the cpp file so that it's not exposed publicly. The Disassembler is architecture agnostic, so its API should try to remain that way


================
Comment at: lldb/include/lldb/Core/Disassembler.h:341
 
-  void Dump(Stream *s, bool show_address, bool show_bytes,
+  void Dump(Stream *s, bool show_address, bool show_bytes, bool show_kind,
             const ExecutionContext *exe_ctx);
----------------
name


================
Comment at: lldb/include/lldb/Core/Disassembler.h:398
+        (1u << 3), // Mark the disassembly line the contains the PC
+    eOptionShowKind = (1u << 4),
   };
----------------



================
Comment at: lldb/include/lldb/Target/TraceCursor.h:35
 ///  point at them. The consumer should invoke \a TraceCursor::GetError() to
 ///  check if the cursor is pointing to either a valid instruction or an error.
 ///
----------------
this file has changed, could you rebase from upstream/main. 


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:230-234
   /// \return
-  ///     The \a lldb::TraceInstructionControlFlowType categories the
+  ///     The \a lldb::InstructionControlFlowType categories the
   ///     instruction the cursor is pointing at falls into. If the cursor points
   ///     to an error in the trace, return \b 0.
+  virtual lldb::InstructionControlFlowType GetInstructionControlFlowType() = 0;
----------------
when you rebase, could you delete any references to this method and its implementation as well? We won't use it anymore


================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:38
+  /// For each instruction, print the instruction type.
+  bool show_kind = false;
   /// Optional custom id to start traversing from.
----------------
name


================
Comment at: lldb/include/lldb/lldb-enumerations.h:973
 /// A single instruction can match one or more of these categories.
-FLAGS_ENUM(TraceInstructionControlFlowType){
-    /// Any instruction.
-    eTraceInstructionControlFlowTypeInstruction = (1u << 1),
-    /// A conditional or unconditional branch/jump.
-    eTraceInstructionControlFlowTypeBranch = (1u << 2),
-    /// A conditional or unconditional branch/jump that changed
-    /// the control flow of the program.
-    eTraceInstructionControlFlowTypeTakenBranch = (1u << 3),
-    /// A call to a function.
-    eTraceInstructionControlFlowTypeCall = (1u << 4),
-    /// A return from a function.
-    eTraceInstructionControlFlowTypeReturn = (1u << 5)};
-
-LLDB_MARK_AS_BITMASK_ENUM(TraceInstructionControlFlowType)
+FLAGS_ENUM(InstructionControlFlowType){
+    /// The instruction could not be classified.
----------------
this doesn't need to be a FLAGS_ENUM anymore (i.e. bitmask), you can use a simple enum instead, like the `ExpressionEvaluationPhase` above


================
Comment at: lldb/source/API/SBInstruction.cpp:244
     FormatEntity::Parse("${addr}: ", format);
-    inst_sp->Dump(&s.ref(), 0, true, false, nullptr, &sc, nullptr, &format, 0);
+    inst_sp->Dump(&s.ref(), 0, true, false, false, nullptr, &sc, nullptr,
+                  &format, 0);
----------------
include this kind of comment wherever you have added this parameter


================
Comment at: lldb/source/Commands/CommandObjectDisassemble.h:49
     bool show_bytes;
+    bool show_kind;
     uint32_t num_lines_context = 0;
----------------
ame


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

https://reviews.llvm.org/D128477