[lldb] Add assembly syntax highlighting

[JDevlieghere]

Summary

Add support for syntax highlighting assembly. The current patch uses a different color to highlight registers, immediate and addresses.

[JDevlieghere]

Similar to the LLVM patch this depends on, this patch is a WIP to get some early feedback. This definitely needs a test. Most of the patch deals with plumbing the value of Debugger::GetUseColor to the Disassembler.

[jasonmolenda]

LGTM this is just mechanical passing around of the user's setting though the layers.

lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h

77

Most of this patch uses use_color, but in this class there's a couple use of colors. I don't think it was an intentional difference, might as well make them all the same. In llvm the MCInstPrinter has a setUseColor but llvm::raw_string_ostream has a enable_colors, maybe that's where the plural came in.

[JDevlieghere]

• Fix inconsistencies and use use_color everywhere

[clayborg]

We shouldn't need to pass in "bool use_color" to the Disassembler creation functions as the only reason to pass something to the creation function for any plug-in is if that data would exclude a disassembler if it didn't support color. But we always want to see some disassembly rather than none if a plug-in is the only one that handles a certain architecture and doesn't support color. We should either add accessors to enable colorization to the Disassembler virtual plug-in interface _or_ add "bool use_color" to the needed functions. Looks DisassembleBytes added a "bool use_color" argument to the call already, so we shouldn't need a "bool use_color" for the plug-in selection. Are there other places that need to know about the "use_color" setting that do not results from a direct function call that can have an argument added to it?

Even if colorization is enabled, It would still expect any APIs that return information in individual instructions, like:

const char *SBInstruction::GetMnemonic(lldb::SBTarget target);
const char *SBInstruction::GetOperands(lldb::SBTarget target);
const char *SBInstruction::GetComment(lldb::SBTarget target);

To not have colorization attached to the returned string.

lldb/include/lldb/Core/Disassembler.h

410

We shouldn't need to pass "use_color" in when finding the disassembler plug-in. We should add an accessor so the Disassembler virtual function list like:

bool Disassembler::SetUseColor(bool enable);

And the plug-in can return true if it supports colorization and false if not. The selection of the disassembler plug-in shouldn't be predicated on color support right?

415

remove and use accessor suggested above?

lldb/include/lldb/lldb-private-interfaces.h

33 ↗

(On Diff #554873)

remove per above comments

lldb/source/Commands/CommandObjectDisassemble.cpp

456–457 ↗

(On Diff #554873)

revert

lldb/source/Core/Disassembler.cpp

59

revert

[JDevlieghere]

In D159164#4632017, @clayborg wrote:

We shouldn't need to pass in "bool use_color" to the Disassembler creation functions as the only reason to pass something to the creation function for any plug-in is if that data would exclude a disassembler if it didn't support color. But we always want to see some disassembly rather than none if a plug-in is the only one that handles a certain architecture and doesn't support color. We should either add accessors to enable colorization to the Disassembler virtual plug-in interface _or_ add "bool use_color" to the needed functions. Looks DisassembleBytes added a "bool use_color" argument to the call already, so we shouldn't need a "bool use_color" for the plug-in selection. Are there other places that need to know about the "use_color" setting that do not results from a direct function call that can have an argument added to it?

Thanks for the suggestion, that's definitely much nicer. I made it part of the create function because I thought we needed to know the value during initialization time, but you're correct that we can set this after the fact with the help of an accessor. This simplifies the patch by a lot.

Even if colorization is enabled, It would still expect any APIs that return information in individual instructions, like:

const char *SBInstruction::GetMnemonic(lldb::SBTarget target);
const char *SBInstruction::GetOperands(lldb::SBTarget target);
const char *SBInstruction::GetComment(lldb::SBTarget target);

To not have colorization attached to the returned string.

Yeah, the updated patch accounts for that and adds a test.

[JDevlieghere]

Address @clayborg's code review feedback

[JDevlieghere]

Discard whitespace changes

[clayborg]

Yes, this simplifies the patch a lot. Looks good after we check the mnemonic and comment for no color when used through the SBInstruction APIs.

lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py

69

Can we check both:

inst.GetMnemonic(target)
inst.GetComment(target)

here too to make sure no colorization gets added?

[JDevlieghere]

Test GetMnemonic and GetComment.

[clayborg]

lgtm. Is there any more things you want to add to this patch test wise, or is this complete?

[JDevlieghere]

As I was adding another test I realized an issue with the previous approach due to the fact that we cache the result. The solution is to cache both the plain and the marked up result, and have the different accessors decide whether they want colored output or not. As a result, I've removed the global Disassembler option to enable color, as it's not really meaningful anymore. I've also added a test to make sure this doesn't break.

[JDevlieghere]

@clayborg let me know if you're happy with this. To answer your previous question, I'm not planning anymore changes.

[clayborg]

lldb/include/lldb/Core/Disassembler.h

156

We might not need "bool show_color" here if we can rely on the "exe_ctx->GetTarget()->GetDebugger()"?

339–340

Ditto here. Should we rely on the exe_ctx having a target and thus we can get to the debugger instead of passing "show_color" as a separate arg?

453–454

should "show_color" be put as a bit into the "options" instead?

lldb/source/API/SBInstruction.cpp

263–264 ↗

(On Diff #555442)

If we rely on the execution context being NULL, then we don't need to pass in color as false here.

299 ↗

(On Diff #555442)

ditto

lldb/source/API/SBInstructionList.cpp

169–170 ↗

(On Diff #555442)

ditto

lldb/source/Core/Disassembler.cpp

605–606

Should we just rely on the "exe_ctx" having a target here instead of adding a new parameter here? Cause we can do "exe_ctx->GetTarget()->GetDebugger()" if needed. If no exe_ctx, then assume no color?

657

The color codes bytes should't contribute to the opcode columns width. We should probably use "m_opcode_name", but we will need to guarantee that both are filled in if we ask for color. Can we modify it so if we ask for the opcode name with color that we always fill in the m_opcode_name in as well?

lldb/source/Core/DumpDataExtractor.cpp

160 ↗

(On Diff #555442)

Here is an example of where we fill the execution context in with the target, but also pass in "target_sp->GetDebugger().GetUseColor()"

[JDevlieghere]

Use the execution context to enable colors.

[clayborg]

Just one last inline comment where we can't use the colorized string to calculate the column width and this is good to go.

lldb/source/Core/Disassembler.cpp

656–657

still need to directly use "m_opcode_name" here instead of "opcode_name" so we don't include color codes as characters.

[JDevlieghere]

Thanks Greg!

[clayborg]

lldb/source/Core/Disassembler.cpp

656–657

lldb/include/lldb/Core/Disassembler.h

public:
   /// \param[in] max_address_text_size
   ///     The length of the longest address string at the start of the
   ///     disassembly line that will be printed (the
   ///     Debugger::FormatDisassemblerAddress() string)
   ///     so this method can properly align the instruction opcodes.
   ///     May be 0 to indicate no indentation/alignment of the opcodes.
   virtual void Dump(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);
 
   virtual bool DoesBranch() = 0;
 
   virtual bool HasDelaySlot();
public:
   /// in the list by returning an index.
   ///
   /// @param[in] start
   ///     The instruction index of the first instruction to check.
   ///
   /// @param[in] ignore_calls
   ///     It true, then fine the first branch instruction that isn't
   ///     a function call (a branch that calls and returns to the next
-  ///     instruction). If false, find the instruction index of any 
+  ///     instruction). If false, find the instruction index of any
   ///     branch in the list.
-  ///     
+  ///
   /// @param[out] found_calls
-  ///     If non-null, this will be set to true if any calls were found in 
+  ///     If non-null, this will be set to true if any calls were found in
   ///     extending the range.
-  ///    
+  ///
   /// @return
   ///     The instruction index of the first branch that is at or past
-  ///     \a start. Returns UINT32_MAX if no matching branches are 
+  ///     \a start. Returns UINT32_MAX if no matching branches are
   ///     found.
   //------------------------------------------------------------------
   uint32_t GetIndexOfNextBranchInstruction(uint32_t start,
                                            bool ignore_calls,
                                            bool *found_calls) const;
 
   uint32_t GetIndexOfInstructionAtLoadAddress(lldb::addr_t load_addr,
                                               Target &target);
 
   uint32_t GetIndexOfInstructionAtAddress(const Address &addr);
 
   void Clear();
 
   void Append(lldb::InstructionSP &inst_sp);
 
   void Dump(Stream *s, bool show_address, bool show_bytes,
             bool show_control_flow_kind, const ExecutionContext *exe_ctx);
 
 private:
   typedef std::vector<lldb::InstructionSP> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
   collection m_instructions;
 };
 
public:
 
   // FindPlugin should be lax about the flavor string (it is too annoying to
   // have various internal uses of the disassembler fail because the global
   // flavor string gets set wrong. Instead, if you get a flavor string you
   // don't understand, use the default.  Folks who care to check can use the
   // FlavorValidForArchSpec method on the disassembler they got back.
   static lldb::DisassemblerSP
   FindPlugin(const ArchSpec &arch, const char *flavor, const char *plugin_name);
 
   // This version will use the value in the Target settings if flavor is NULL;
   static lldb::DisassemblerSP FindPluginForTarget(const Target &target,
                                                   const ArchSpec &arch,
                                                   const char *flavor,
                                                   const char *plugin_name);
 
   struct Limit {
     enum { Bytes, Instructions } kind;
     lldb::addr_t value;
   };
 
   static lldb::DisassemblerSP DisassembleRange(const ArchSpec &arch,
                                                const char *plugin_name,
public:
   // Constructors and Destructors
   Disassembler(const ArchSpec &arch, const char *flavor);
   ~Disassembler() override;
 
   void PrintInstructions(Debugger &debugger, const ArchSpec &arch,
                          const ExecutionContext &exe_ctx,
                          bool mixed_source_and_assembly,
                          uint32_t num_mixed_context_lines, uint32_t options,
                          Stream &strm);
 
   size_t ParseInstructions(Target &target, Address address, Limit limit,
                            Stream *error_strm_ptr,
                            bool force_live_memory = false);
 
   virtual size_t DecodeInstructions(const Address &base_addr,
                                     const DataExtractor &data,
                                     lldb::offset_t data_offset,
                                     size_t num_instructions, bool append,
                                     bool data_from_file) = 0;
 
   InstructionList &GetInstructionList();
 
   const InstructionList &GetInstructionList() const;
 
   const ArchSpec &GetArchitecture() const { return m_arch; }
 
   const char *GetFlavor() const { return m_flavor.c_str(); }
 
   virtual bool FlavorValidForArchSpec(const lldb_private::ArchSpec &arch,
                                       const char *flavor) = 0;
 
+  virtual void SetUseColor(bool use_color) {}
+
 protected:
   // SourceLine and SourceLinesToDisplay structures are only used in the mixed
   // source and assembly display methods internal to this class.
 
   struct SourceLine {
     FileSpec file;
     uint32_t line = LLDB_INVALID_LINE_NUMBER;
     uint32_t column = 0;

lldb/source/Core/Disassembler.cpp

 #include <cassert>
 
 #define DEFAULT_DISASM_BYTE_SIZE 32
 
 using namespace lldb;
 using namespace lldb_private;
 
 DisassemblerSP Disassembler::FindPlugin(const ArchSpec &arch,
                                         const char *flavor,
                                         const char *plugin_name) {
   LLDB_SCOPED_TIMERF("Disassembler::FindPlugin (arch = %s, plugin_name = %s)",
                      arch.GetArchitectureName(), plugin_name);
 
   DisassemblerCreateInstance create_callback = nullptr;
 
   if (plugin_name) {
     create_callback =
void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
   AddressRange current_source_line_range;
   const Address *pc_addr_ptr = nullptr;
   StackFrame *frame = exe_ctx.GetFramePtr();
 
   TargetSP target_sp(exe_ctx.GetTargetSP());
   SourceManager &source_manager =
       target_sp ? target_sp->GetSourceManager() : debugger.GetSourceManager();
 
+  SetUseColor(debugger.GetUseColor());
+
   if (frame) {
     pc_addr_ptr = &frame->GetFrameCodeAddress();
   }
   const uint32_t scope =
       eSymbolContextLineEntry | eSymbolContextFunction | eSymbolContextSymbol;
   const bool use_inline_block_range = false;
 
   const FormatEntity::Entry *disassembly_format = nullptr;
const char *Instruction::GetNameForInstructionControlFlowKind(
   case eInstructionControlFlowKindFarJump:
     return "far jump";
   }
   llvm_unreachable("Fully covered switch above!");
 }
 
 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) {
   size_t opcode_column_width = 7;
   const size_t operand_column_width = 25;
 
   CalculateMnemonicOperandsAndCommentIfNeeded(exe_ctx);
void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
   }
 
   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) {
     opcode_column_width = m_opcode_name.length() + 1;
   }
 
   ss.PutCString(m_opcode_name);
   ss.FillLastLineToColumn(opcode_pos + opcode_column_width, ' ');
   ss.PutCString(m_mnemonics);
 
   if (!m_comment.empty()) {
     ss.FillLastLineToColumn(
         opcode_pos + opcode_column_width + operand_column_width, ' ');

lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h

public:
   static lldb::DisassemblerSP CreateInstance(const lldb_private::ArchSpec &arch,
                                              const char *flavor);
 
   size_t DecodeInstructions(const lldb_private::Address &base_addr,
                             const lldb_private::DataExtractor &data,
                             lldb::offset_t data_offset, size_t num_instructions,
                             bool append, bool data_from_file) override;
 
+  void SetUseColor(bool use_color) override;
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
 protected:
   friend class InstructionLLVMC;
 
   bool FlavorValidForArchSpec(const lldb_private::ArchSpec &arch,
                               const char *flavor) override;
static const char *SymbolLookupCallback(void *DisInfo,
                                           uint64_t *ReferenceType,
                                           uint64_t ReferencePC,
                                           const char **ReferenceName);
 
   const lldb_private::ExecutionContext *m_exe_ctx;
   InstructionLLVMC *m_inst;
   std::mutex m_mutex;
   bool m_data_from_file;
   // Save the AArch64 ADRP instruction word and address it was at,
   // in case the next instruction is an ADD to the same register;
   // this is a pc-relative address calculation and we need both
   // parts to calculate the symbolication.
   lldb::addr_t m_adrp_address;
   std::optional<uint32_t> m_adrp_insn;
 
   // Since we need to make two actual MC Disassemblers for ARM (ARM & THUMB),
   // and there's a bit of goo to set up and own in the MC disassembler world,
   // this class was added to manage the actual disassemblers.
   class MCDisasmInstance;
   std::unique_ptr<MCDisasmInstance> m_disasm_up;
   std::unique_ptr<MCDisasmInstance> m_alternate_disasm_up;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_DISASSEMBLER_LLVMC_DISASSEMBLERLLVMC_H

lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

 //===-- DisassemblerLLVMC.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "DisassemblerLLVMC.h"
 
 #include "llvm-c/Disassembler.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/MC/MCDisassembler/MCExternalSymbolizer.h"
 #include "llvm/MC/MCDisassembler/MCRelocationInfo.h"
 #include "llvm/MC/MCInst.h"
public:
 
   ~MCDisasmInstance() = default;
 
   uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
                      lldb::addr_t pc, llvm::MCInst &mc_inst) const;
   void PrintMCInst(llvm::MCInst &mc_inst, lldb::addr_t pc,
                    std::string &inst_string, std::string &comments_string);
   void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
+  void SetUseColor(bool use_color);
+  bool GetUseColor() const;
   bool CanBranch(llvm::MCInst &mc_inst) const;
   bool HasDelaySlot(llvm::MCInst &mc_inst) const;
   bool IsCall(llvm::MCInst &mc_inst) const;
   bool IsLoad(llvm::MCInst &mc_inst) const;
   bool IsAuthenticated(llvm::MCInst &mc_inst) const;
 
 private:
   MCDisasmInstance(std::unique_ptr<llvm::MCInstrInfo> &&instr_info_up,
if (m_opcode.GetData(data)) {
       if (disasm) {
         DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr;
 
         if (address_class == AddressClass::eCodeAlternateISA)
           mc_disasm_ptr = disasm->m_alternate_disasm_up.get();
         else
           mc_disasm_ptr = disasm->m_disasm_up.get();
 
+        // Disable coloring for the duration of this function.
+        const bool saved_use_color = mc_disasm_ptr->GetUseColor();
+        mc_disasm_ptr->SetUseColor(false);
+        auto on_exit = llvm::make_scope_exit(
+            [=]() { mc_disasm_ptr->SetUseColor(saved_use_color); });
+
         lldb::addr_t pc = m_address.GetFileAddress();
         m_using_file_addr = true;
 
         const bool data_from_file = disasm->m_data_from_file;
         bool use_hex_immediates = true;
         Disassembler::HexImmediateStyle hex_style = Disassembler::eHexStyleC;
 
         if (exe_ctx) {

 }
 
 void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
     llvm::MCInst &mc_inst, lldb::addr_t pc, std::string &inst_string,
     std::string &comments_string) {
   llvm::raw_string_ostream inst_stream(inst_string);
   llvm::raw_string_ostream comments_stream(comments_string);
 
+  inst_stream.enable_colors(m_instr_printer_up->getUseColor());
   m_instr_printer_up->setCommentStream(comments_stream);
   m_instr_printer_up->printInst(&mc_inst, pc, llvm::StringRef(),
                                 *m_subtarget_info_up, inst_stream);
   m_instr_printer_up->setCommentStream(llvm::nulls());
+
   comments_stream.flush();
 
   static std::string g_newlines("\r\n");
 
   for (size_t newline_pos = 0;
        (newline_pos = comments_string.find_first_of(g_newlines, newline_pos)) !=
        comments_string.npos;
        /**/) {
case eHexStyleC:
     m_instr_printer_up->setPrintHexStyle(llvm::HexStyle::C);
     break;
   case eHexStyleAsm:
     m_instr_printer_up->setPrintHexStyle(llvm::HexStyle::Asm);
     break;
   }
 }
 
+void DisassemblerLLVMC::MCDisasmInstance::SetUseColor(bool use_color) {
+  m_instr_printer_up->setUseColor(use_color);
+}
+
+bool DisassemblerLLVMC::MCDisasmInstance::GetUseColor() const {
+  return m_instr_printer_up->getUseColor();
+}
+
 bool DisassemblerLLVMC::MCDisasmInstance::CanBranch(
     llvm::MCInst &mc_inst) const {
   if (m_instr_analysis_up)
     return m_instr_analysis_up->mayAffectControlFlow(mc_inst, *m_reg_info_up);
   return m_instr_info_up->get(mc_inst.getOpcode())
       .mayAffectControlFlow(mc_inst, *m_reg_info_up);
 }
 
lldb::DisassemblerSP DisassemblerLLVMC::CreateInstance(const ArchSpec &arch,
   if (arch.GetTriple().getArch() != llvm::Triple::UnknownArch) {
     auto disasm_sp = std::make_shared<DisassemblerLLVMC>(arch, flavor);
     if (disasm_sp && disasm_sp->IsValid())
       return disasm_sp;
   }
   return lldb::DisassemblerSP();
 }
 
+void DisassemblerLLVMC::SetUseColor(bool use_color) {
+  if (m_disasm_up)
+    m_disasm_up->SetUseColor(use_color);
+  if (m_alternate_disasm_up)
+    m_alternate_disasm_up->SetUseColor(use_color);
+}
+
 size_t DisassemblerLLVMC::DecodeInstructions(const Address &base_addr,
                                              const DataExtractor &data,
                                              lldb::offset_t data_offset,
                                              size_t num_instructions,
                                              bool append, bool data_from_file) {
   if (!append)
     m_instruction_list.Clear();
 

lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py

def test_disassemble_raw_data(self):
                 inst.GetControlFlowKind(target), lldb.eInstructionControlFlowKindUnknown
             )
         elif arch in ("aarch64", "arm64"):
             self.assertEqual(inst.GetMnemonic(target), "mov")
             self.assertEqual(inst.GetOperands(target), "w0, #0x63")
             self.assertEqual(
                 inst.GetControlFlowKind(target), lldb.eInstructionControlFlowKindUnknown
             )
+            # Make sure that using colors doesn't affect the output here.
+            res = lldb.SBCommandReturnObject()
+            ci = self.dbg.GetCommandInterpreter()
+            ci.HandleCommand("settings set use-color true", res)
+            self.assertEqual(inst.GetOperands(target), "w0, #0x63")
+            ci.HandleCommand("settings set use-color false", res)
+
         elif arch == "arm":
             self.assertEqual(inst.GetMnemonic(target), "mov")
             self.assertEqual(inst.GetOperands(target), "r3, #99")
             self.assertEqual(
                 inst.GetControlFlowKind(target), lldb.eInstructionControlFlowKindUnknown
             )
         else:
             self.assertEqual(inst.GetMnemonic(target), "movq")
             self.assertEqual(inst.GetOperands(target), "%" + "rsp, " + "%" + "rbp")
             self.assertEqual(
                 inst.GetControlFlowKind(target), lldb.eInstructionControlFlowKindOther
             )