Default transcript dumping in "statistics dump" to false #145436
+83
−17
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: None (qxy11) ChangesSummaryCurrently, if the setting These changes default the option to log transcripts in the TestingManual testingTested with Unit testsChanged unit tests to account for new expected default behavior to Full diff: https://github.com/llvm/llvm-project/pull/145436.diff 4 Files Affected:
diff --git a/lldb/include/lldb/API/SBStatisticsOptions.h b/lldb/include/lldb/API/SBStatisticsOptions.h
index 74bea03eff9c9..bfff9dc926432 100644
--- a/lldb/include/lldb/API/SBStatisticsOptions.h
+++ b/lldb/include/lldb/API/SBStatisticsOptions.h
@@ -57,8 +57,7 @@ class LLDB_API SBStatisticsOptions {
/// a JSON array with all commands the user and/or scripts executed during a
/// debug session.
///
- /// Defaults to true, unless the `SummaryOnly` mode is enabled, in which case
- /// this is turned off unless specified.
+ /// Defaults to false.
void SetIncludeTranscript(bool b);
bool GetIncludeTranscript() const;
diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h
index 42f03798c219e..68194cf216a1a 100644
--- a/lldb/include/lldb/Target/Statistics.h
+++ b/lldb/include/lldb/Target/Statistics.h
@@ -193,9 +193,8 @@ struct StatisticsOptions {
bool GetIncludeTranscript() const {
if (m_include_transcript.has_value())
return m_include_transcript.value();
- // `m_include_transcript` has no value set, so return a value based on
- // `m_summary_only`.
- return !GetSummaryOnly();
+ // Default to false in both default mode and summary mode.
+ return false;
}
void SetIncludePlugins(bool value) { m_include_plugins = value; }
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 75bdffe16c52e..5327647d65cc4 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -1482,13 +1482,15 @@ let Command = "statistics dump" in {
"this is turned off unless specified. "
"In default mode, if both '--targets' and '--modules' are 'true', a list "
"of module identifiers will be added to the 'targets' section.">;
- def statistics_dump_transcript: Option<"transcript", "t">, Group<1>,
- Arg<"Boolean">,
- Desc<"If the setting interpreter.save-transcript is enabled and this "
- "option is 'true', include a JSON array with all commands the user and/or "
- "scripts executed during a debug session. "
- "Defaults to true, unless the '--summary' mode is enabled, in which case "
- "this is turned off unless specified.">;
+ def statistics_dump_transcript
+ : Option<"transcript", "t">,
+ Group<1>,
+ Arg<"Boolean">,
+ Desc<"If the setting interpreter.save-transcript is enabled and this "
+ "option is 'true', include a JSON array with all commands the "
+ "user and/or "
+ "scripts executed during a debug session. "
+ "Defaults to false. ">;
def statistics_dump_plugins
: Option<"plugins", "p">,
Group<1>,
diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py
index 5281bde4c6479..890ceb1f5fd57 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -892,7 +892,7 @@ def get_test_cases_for_sections_existence(self):
"targets.frameVariable": True,
"targets.totalSharedLibraryEventHitCount": True,
"modules": True,
- "transcript": True,
+ "transcript": False,
},
},
{ # Summary mode
@@ -979,6 +979,24 @@ def get_test_cases_for_sections_existence(self):
"transcript": False,
},
},
+ { # Default mode without modules and with transcript
+ "command_options": " --modules=false --transcript=true",
+ "api_options": {
+ "SetIncludeModules": False,
+ "SetIncludeTranscript": True,
+ },
+ "expect": {
+ "commands": True,
+ "targets": True,
+ "targets.moduleIdentifiers": False,
+ "targets.breakpoints": True,
+ "targets.expressionEvaluation": True,
+ "targets.frameVariable": True,
+ "targets.totalSharedLibraryEventHitCount": True,
+ "modules": False,
+ "transcript": True,
+ },
+ },
{ # Default mode without modules
"command_options": " --modules=false",
"api_options": {
@@ -993,6 +1011,23 @@ def get_test_cases_for_sections_existence(self):
"targets.frameVariable": True,
"targets.totalSharedLibraryEventHitCount": True,
"modules": False,
+ "transcript": False,
+ },
+ },
+ { # Default mode with transcript
+ "command_options": " --transcript=true",
+ "api_options": {
+ "SetIncludeTranscript": True,
+ },
+ "expect": {
+ "commands": True,
+ "targets": True,
+ "targets.moduleIdentifiers": True,
+ "targets.breakpoints": True,
+ "targets.expressionEvaluation": True,
+ "targets.frameVariable": True,
+ "targets.totalSharedLibraryEventHitCount": True,
+ "modules": True,
"transcript": True,
},
},
|
|
cc @jeffreytan81 @clayborg @dmpots |
Michael137
reviewed
Jun 24, 2025
Michael137
approved these changes
Jun 24, 2025
Michael137
approved these changes
Jun 25, 2025
|
Added a unit test around the warning message as well. I'll probably need some help with landing this. |
|
✅ With the latest revision this PR passed the Python code formatter. |
Summary: Add warning to 'statistics dump --transcript=true' when interpreter.save-transcript is disabled, helping users understand why transcript data is empty. Also refactor StatisticsOptions::GetIncludeTranscript() to use value_or(false) instead of the verbose has_value()/value() pattern to address PR comments.
qxy11
force-pushed
the
no-dump-transcript
branch
from
4bd8089 to
105e48f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Currently, if the setting
interpreter.save-transcriptis enabled, whenever we call "statistics dump", it'll default to reporting a huge list of transcripts which can be a bit noisy. This is because the current checkGetIncludeTranscriptreturns!GetSummaryOnly()by default if no specific transcript-setting option is given in the statistics dump command (ie.statistics dump --transcripts=falseorstatistics dump --transcripts=true). Then wheninterpreter.save-transcriptis enabled, this saves a list of transcripts, and the transcript list ends up getting logged by default.These changes default the option to log transcripts in the
statistics dumpcommand to "false". This can still be enabled via the--transcriptsoption if users want to see a transcript. Sinceinterpreter.save-transcriptis false by default, the main delta is that ifinterpreter.save-transcriptis true and summary mode is false, we now disable saving the transcript.This also adds a warning to 'statistics dump --transcript=true' when
interpreter.save-transcript is disabled, which should help users understand
why transcript data is empty.
Testing
Manual testing
Tested with
settings set interpreter.save-transcript trueenabled at startup on a toy hello-world program:Without
settings set interpreter.save-transcript true:Unit tests
Changed unit tests to account for new expected default behavior to
false, and added a couple new tests around expected behavior with--transcript=true.