This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add assembly syntax highlighting
ClosedPublic

Authored by JDevlieghere on Aug 29 2023, 9:42 PM.

Details

Summary

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

Diff Detail

Event Timeline

JDevlieghere created this revision.Aug 29 2023, 9:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 9:42 PM
JDevlieghere requested review of this revision.Aug 29 2023, 9:42 PM

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 accepted this revision.Aug 30 2023, 1:45 PM

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.

This revision is now accepted and ready to land.Aug 30 2023, 1:45 PM
  • Fix inconsistencies and use use_color everywhere

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 marked 6 inline comments as done.Aug 31 2023, 11:09 PM

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.

Address @clayborg's code review feedback

Discard whitespace changes

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 marked an inline comment as done.

Test GetMnemonic and GetComment.

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

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.

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

clayborg added inline comments.Sep 1 2023, 12:43 PM
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 marked 9 inline comments as done.

Use the execution context to enable colors.

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 marked an inline comment as done.Sep 1 2023, 2:20 PM

Thanks Greg!

clayborg added inline comments.Sep 1 2023, 2:21 PM
lldb/source/Core/Disassembler.cpp
656–657
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2023, 2:47 PM