Skip to content

[lldb][TypeSystem] Enable colored AST dump #86159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Mar 21, 2024

This patch sets the necessary ASTContext flag
to ensure that the various ASTs in LLDB (i.e., symbolfile-backed ASTs, scratch AST and the expression AST) are dumped using syntax highlighting (i.e., whenever a user runs target modules dump ast or calls the dump() method on a clang::Decl).

Decided to not put this behind a setting because the diagnostics object persists on some AST and gets re-used, so the setting wouldn't consistenly turn off syntax highlighting (unless we made the setter fetch all live ASTs and turn the highlighting off).

This patch sets the necessary `ASTContext` flag
to ensure that the various ASTs (i.e., symbolfile-backed
ASTs, scratch AST and the expression AST) in LLDB are dumped
using syntax highlighting (i.e., whenever a user runs
`target modules dump ast` or calls the `dump() on a `clang::Decl`).

Decided to not put this behind a setting because the diagnostics
object persists on some AST and gets re-used, so the setting wouldn't
consistenly turn off syntax highlighting (unless we made the setter
fetch all live ASTs and turn the highlighting off).
@llvmbot
Copy link
Member

llvmbot commented Mar 21, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch sets the necessary ASTContext flag
to ensure that the various ASTs (i.e., symbolfile-backed ASTs, scratch AST and the expression AST) in LLDB are dumped using syntax highlighting (i.e., whenever a user runs target modules dump ast or calls the dump() on a clang::Decl).

Decided to not put this behind a setting because the diagnostics object persists on some AST and gets re-used, so the setting wouldn't consistenly turn off syntax highlighting (unless we made the setter fetch all live ASTs and turn the highlighting off).


Full diff: https://github.com/llvm/llvm-project/pull/86159.diff

2 Files Affected:

  • (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+3)
  • (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+3)
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 574d661e2a215e..0da497d74ffe86 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -465,6 +465,9 @@ ClangExpressionParser::ClangExpressionParser(
   // A value of 0 means no limit for both LLDB and Clang.
   m_compiler->getDiagnostics().setErrorLimit(target_sp->GetExprErrorLimit());
 
+  // AST nodes will be dumped with color
+  m_compiler->getDiagnostics().setShowColors(true);
+
   auto target_info = TargetInfo::CreateTargetInfo(
       m_compiler->getDiagnostics(), m_compiler->getInvocation().TargetOpts);
   if (log) {
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 51ab13108feb3a..146bfbe965e33e 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -752,6 +752,9 @@ void TypeSystemClang::CreateASTContext() {
   m_diagnostic_consumer_up = std::make_unique<NullDiagnosticConsumer>();
   m_ast_up->getDiagnostics().setClient(m_diagnostic_consumer_up.get(), false);
 
+  // AST nodes will be dumped with color
+  m_ast_up->getDiagnostics().setShowColors(true);
+
   // This can be NULL if we don't know anything about the architecture or if
   // the target for an architecture isn't enabled in the llvm/clang that we
   // built

@bulbazord
Copy link
Member

What happens if you have colors disabled in your terminal? Does this do nothing? Or does it start inserting ANSI escape codes in plain text?

@Michael137
Copy link
Member Author

Michael137 commented Mar 21, 2024

What happens if you have colors disabled in your terminal? Does this do nothing? Or does it start inserting ANSI escape codes in plain text?

Yea good question, was about to try this out. It does whatever clang's ast-dump would do if colors aren't turned off

@JDevlieghere
Copy link
Member

What happens if you have colors disabled in your terminal? Does this do nothing? Or does it start inserting ANSI escape codes in plain text?

Yea good question, was about to try this out. It does whatever clang's ast-dump would do if colors aren't turned off

That keys off of the output stream supporting colors, but LLDB also allows you to disable colors globally. Please also try lldb --no-use-colors and see if that behaves correctly (it might if we set the properties of the stream correctly). If it doesn't you'll need to read debugger.GetUseColor().

@JDevlieghere
Copy link
Member

Either way, we should add a comment explaining that (and why) this works.

@Michael137
Copy link
Member Author

What happens if you have colors disabled in your terminal? Does this do nothing? Or does it start inserting ANSI escape codes in plain text?

Yea good question, was about to try this out. It does whatever clang's ast-dump would do if colors aren't turned off

That keys off of the output stream supporting colors, but LLDB also allows you to disable colors globally. Please also try lldb --no-use-colors and see if that behaves correctly (it might if we set the properties of the stream correctly). If it doesn't you'll need to read debugger.GetUseColor().

It behaves correctly for the target modules dump ast case because that uses the LLDB streams, as you mentioned. For other diagnostics we would have to use the GetUseColor API, though we'd have to plumb the target through to TypeSystemClang::CreateASTContext first, to get access to the debugger object. Will try that instead

@jimingham
Copy link
Collaborator

jimingham commented Mar 25, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants