Add support for syntax highlighting assembly. The current patch uses a different color to highlight registers, immediate and addresses.
Details
Diff Detail
Event Timeline
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.
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. |
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 |
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.
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 | ||
---|---|---|
70 | Can we check both: inst.GetMnemonic(target) inst.GetComment(target) here too to make sure no colorization gets added? |
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.
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()" |
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. |
lldb/source/Core/Disassembler.cpp | ||
---|---|---|
656–657 |
We might not need "bool show_color" here if we can rely on the "exe_ctx->GetTarget()->GetDebugger()"?