Refactor string conversion for InstructionControlFlowKind enum

[jj10306]

Summary

Refactor the string conversion of the lldb::InstructionControlFlowKind enum out
of Instruction::Dump to enable reuse of this logic by the
JSON TraceDumper (to be implemented in separate diff).

Will coordinate the landing of this change with D130320 since there will be a minor merge conflict between
these changes.

Test Plan:
Run unittests

> ninja check-lldb-unittests
[4/5] Running lldb unit test suite

Testing Time: 10.13s
  Passed: 1084

Verify '-k' flag's output

(lldb) thread trace dump instructions -k
thread #1: tid = 1375377
  libstdc++.so.6`std::ostream::flush() + 43
    7048: 0x00007ffff7b54dab    return      retq
    7047: 0x00007ffff7b54daa    other       popq   %rbx
    7046: 0x00007ffff7b54da7    other       movq   %rbx, %rax
    7045: 0x00007ffff7b54da5    cond jump   je     0x11adb0                  ; <+48>
    7044: 0x00007ffff7b54da2    other       cmpl   $-0x1, %eax
  libc.so.6`_IO_fflush + 249
    7043: 0x00007ffff7161729    return      retq
    7042: 0x00007ffff7161728    other       popq   %rbp
    7041: 0x00007ffff7161727    other       popq   %rbx
    7040: 0x00007ffff7161725    other       movl   %edx, %eax
    7039: 0x00007ffff7161721    other       addq   $0x8, %rsp
    7038: 0x00007ffff7161709    cond jump   je     0x87721                   ; <+241>
    7037: 0x00007ffff7161707    other       decl   (%rsi)
    7036: 0x00007ffff71616fe    cond jump   je     0x87707                   ; <+215>
    7035: 0x00007ffff71616f7    other       cmpl   $0x0, 0x33de92(%rip)      ; __libc_multiple_threads
    7034: 0x00007ffff71616ef    other       movq   $0x0, 0x8(%rsi)
    7033: 0x00007ffff71616ed    cond jump   jne    0x87721                   ; <+241>
    7032: 0x00007ffff71616e9    other       subl   $0x1, 0x4(%rsi)
    7031: 0x00007ffff71616e2    other       movq   0x88(%rbx), %rsi
    7030: 0x00007ffff71616e0    cond jump   jne    0x87721                   ; <+241>
    7029: 0x00007ffff71616da    other       testl  $0x8000, (%rbx)           ; imm = 0x8000

[jj10306]

remove accidentily included change to trace dumper

lldb/source/Target/TraceDumper.cpp

221

ignore this accidentally included change, will delete this and update the diff

[jj10306]

lldb/source/Core/Disassembler.cpp

930

Thoughts on adding a default case with a string that indicates that no string conversion was implemented for the provided variant?
This would potentially make it easier for someone to realize they didn't add a case here for a newly added variant of InstructionControlFlowKind. Otherwise undefined data would be returned if a new case isn't added for a new variant.
wdyt?

[persona0220]

lldb/source/Core/Disassembler.cpp

930

I agree to add a default here!

[wallace]

just remove that comment. I've just pushed Sujin's diff as well, so you can rebase

lldb/include/lldb/lldb-enumerations.h

975–976

you can safely remove this comment because the compiler will complain if a new enum value is added and the switch case in GetNameForInstructionControlFlowKind is not updated

lldb/source/Core/Disassembler.cpp

930

no defaults for enums, otherwise the compiler won't complain if a new enum value is added and a case is not added here

[jj10306]

Rebase, fix merge conflicts with D130320

[jj10306]

Remove unnecessary comment

lldb/include/lldb/Core/Disassembler.h

public:
   };
 
   virtual bool ParseOperands(llvm::SmallVectorImpl<Operand> &operands) {
     return false;
   }
 
   virtual bool IsCall() { return false; }
 
+  static const char *GetNameForInstructionControlFlowKind(
+      lldb::InstructionControlFlowKind instruction_control_flow_kind);
+
 protected:
   Address m_address; // The section offset address of this instruction
                      // We include an address class in the Instruction class to
                      // allow the instruction specify the
                      // AddressClass::eCodeAlternateISA (currently used for
                      // thumb), and also to specify data (AddressClass::eData).
                      // The usual value will be AddressClass::eCode, but often
                      // when disassembling memory, you might run into data.

lldb/include/lldb/lldb-enumerations.h

enum ExpressionEvaluationPhase {
   eExpressionEvaluationExecution,
   eExpressionEvaluationComplete
 };
 
 /// Architecture-agnostic categorization of instructions for traversing the
 /// control flow of a trace.
 ///
 /// A single instruction can match one or more of these categories.
+/// The enum -> string conversion is in Disassembler.cpp, don't change
+/// this enum without updating that code as well.
 enum InstructionControlFlowKind {
   /// The instruction could not be classified.
   eInstructionControlFlowKindUnknown = 0,
   /// The instruction is something not listed below, i.e. it's a sequential
   /// instruction that doesn't affect the control flow of the program.
   eInstructionControlFlowKindOther,
   /// The instruction is a near (function) call.
   eInstructionControlFlowKindCall,

lldb/source/Core/Disassembler.cpp

 }
 
 AddressClass Instruction::GetAddressClass() {
   if (m_address_class == AddressClass::eInvalid)
     m_address_class = m_address.GetAddressClass();
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+    lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+    return "unknown";
+  case eInstructionControlFlowKindOther:
+    return "other";
+  case eInstructionControlFlowKindCall:
+    return "call";
+  case eInstructionControlFlowKindReturn:
+    return "return";
+  case eInstructionControlFlowKindJump:
+    return "jump";
+  case eInstructionControlFlowKindCondJump:
+    return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+    return "far call";
+  case eInstructionControlFlowKindFarReturn:
+    return "far return";
+  case eInstructionControlFlowKindFarJump:
+    return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
                        bool show_address, bool show_bytes,
                        bool show_control_flow_kind,
                        const ExecutionContext *exe_ctx,
                        const SymbolContext *sym_ctx,
                        const SymbolContext *prev_sym_ctx,
                        const FormatEntity::Entry *disassembly_addr_format,
                        size_t max_address_text_size) {
if (m_opcode.GetType() == Opcode::eTypeBytes) {
       if (max_opcode_byte_size > 0)
         m_opcode.Dump(&ss, max_opcode_byte_size * 3 + 1);
       else
         m_opcode.Dump(&ss, 12);
     }
   }
 
   if (show_control_flow_kind) {
-    switch (GetControlFlowKind(exe_ctx->GetTargetRef().GetArchitecture())) {
-    case eInstructionControlFlowKindUnknown:
-      ss.Printf("%-12s", "unknown");
-      break;
-    case eInstructionControlFlowKindOther:
-      ss.Printf("%-12s", "other");
-      break;
-    case eInstructionControlFlowKindCall:
-      ss.Printf("%-12s", "call");
-      break;
-    case eInstructionControlFlowKindReturn:
-      ss.Printf("%-12s", "return");
-      break;
-    case eInstructionControlFlowKindJump:
-      ss.Printf("%-12s", "jump");
-      break;
-    case eInstructionControlFlowKindCondJump:
-      ss.Printf("%-12s", "cond jump");
-      break;
-    case eInstructionControlFlowKindFarCall:
-      ss.Printf("%-12s", "far call");
-      break;
-    case eInstructionControlFlowKindFarReturn:
-      ss.Printf("%-12s", "far return");
-      break;
-    case eInstructionControlFlowKindFarJump:
-      ss.Printf("%-12s", "far jump");
-      break;
-    }
+    lldb::InstructionControlFlowKind instruction_control_flow_kind =
+        GetControlFlowKind(exe_ctx->GetTargetRef().GetArchitecture());
+    ss.Printf("%-12s", GetNameForInstructionControlFlowKind(
+                           instruction_control_flow_kind));
   }
 
   const size_t opcode_pos = ss.GetSizeOfLastLine();
 
   // The default opcode size of 7 characters is plenty for most architectures
   // but some like arm can pull out the occasional vqrshrun.s16.  We won't get
   // consistent column spacing in these cases, unfortunately.
   if (m_opcode_name.length() >= opcode_column_width) {

lldb/source/Target/TraceDumper.cpp

if (item.symbol_info) {
       m_j.attribute(
           "symbol",
           ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
 
       if (item.symbol_info->instruction) {
         m_j.attribute("mnemonic",
                       ToOptionalString(item.symbol_info->instruction->GetMnemonic(
                           &item.symbol_info->exe_ctx)));
+
+        m_j.attribute(
+            "mnemonic",
+            ToOptionalString(item.symbol_info->instruction->GetMnemonic(
+                &item.symbol_info->exe_ctx)));
       }
 
       if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {
         m_j.attribute(
             "source",
             ToOptionalString(
                 item.symbol_info->sc.line_entry.file.GetPath().c_str()));
         m_j.attribute("line", item.symbol_info->sc.line_entry.line);