Revert "A few updates around "transcript"" #94088
Merged
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
This reverts commit ad884d9.
|
@llvm/pr-subscribers-lldb Author: None (gulfemsavrun) ChangesReverts llvm/llvm-project#92843 because it broke some lldb tests: Full diff: https://github.com/llvm/llvm-project/pull/94088.diff 9 Files Affected:
diff --git a/lldb/include/lldb/API/SBCommandInterpreter.h b/lldb/include/lldb/API/SBCommandInterpreter.h
index 639309aa32bfc..8ac36344b3a79 100644
--- a/lldb/include/lldb/API/SBCommandInterpreter.h
+++ b/lldb/include/lldb/API/SBCommandInterpreter.h
@@ -320,17 +320,10 @@ class SBCommandInterpreter {
/// Returns a list of handled commands, output and error. Each element in
/// the list is a dictionary with the following keys/values:
- /// - "command" (string): The command that was given by the user.
- /// - "commandName" (string): The name of the executed command.
- /// - "commandArguments" (string): The arguments of the executed command.
+ /// - "command" (string): The command that was executed.
/// - "output" (string): The output of the command. Empty ("") if no output.
/// - "error" (string): The error of the command. Empty ("") if no error.
- /// - "durationInSeconds" (float): The time it took to execute the command.
- /// - "timestampInEpochSeconds" (int): The timestamp when the command is
- /// executed.
- ///
- /// Turn on settings `interpreter.save-transcript` for LLDB to populate
- /// this list. Otherwise this list is empty.
+ /// - "seconds" (float): The time it took to execute the command.
SBStructuredData GetTranscript();
protected:
diff --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h b/lldb/include/lldb/Interpreter/CommandInterpreter.h
index 8863523b2e31f..ccc30cf4f1a82 100644
--- a/lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -776,14 +776,10 @@ class CommandInterpreter : public Broadcaster,
/// Contains a list of handled commands and their details. Each element in
/// the list is a dictionary with the following keys/values:
- /// - "command" (string): The command that was given by the user.
- /// - "commandName" (string): The name of the executed command.
- /// - "commandArguments" (string): The arguments of the executed command.
+ /// - "command" (string): The command that was executed.
/// - "output" (string): The output of the command. Empty ("") if no output.
/// - "error" (string): The error of the command. Empty ("") if no error.
- /// - "durationInSeconds" (float): The time it took to execute the command.
- /// - "timestampInEpochSeconds" (int): The timestamp when the command is
- /// executed.
+ /// - "seconds" (float): The time it took to execute the command.
///
/// Turn on settings `interpreter.save-transcript` for LLDB to populate
/// this list. Otherwise this list is empty.
diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h
index c04d529290fff..c4f17b503a1f9 100644
--- a/lldb/include/lldb/Target/Statistics.h
+++ b/lldb/include/lldb/Target/Statistics.h
@@ -133,7 +133,6 @@ struct ConstStringStats {
struct StatisticsOptions {
bool summary_only = false;
bool load_all_debug_info = false;
- bool include_transcript = false;
};
/// A class that represents statistics for a since lldb_private::Target.
diff --git a/lldb/source/Commands/CommandObjectStats.cpp b/lldb/source/Commands/CommandObjectStats.cpp
index 1935b0fdfadfb..a92bb5d1165ee 100644
--- a/lldb/source/Commands/CommandObjectStats.cpp
+++ b/lldb/source/Commands/CommandObjectStats.cpp
@@ -81,9 +81,6 @@ class CommandObjectStatsDump : public CommandObjectParsed {
case 'f':
m_stats_options.load_all_debug_info = true;
break;
- case 't':
- m_stats_options.include_transcript = true;
- break;
default:
llvm_unreachable("Unimplemented option");
}
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 194902abdce49..62bbfdc117834 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -1425,8 +1425,4 @@ let Command = "statistics dump" in {
Desc<"Dump the total possible debug info statistics. "
"Force loading all the debug information if not yet loaded, and collect "
"statistics with those.">;
- def statistics_dump_transcript: Option<"transcript", "t">, Group<1>,
- Desc<"If the setting interpreter.save-transcript is enabled and this "
- "option is specified, include a JSON array with all commands the user and/"
- "or scripts executed during a debug session.">;
}
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index acd6294cb3f42..6a61882df093d 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -6,7 +6,6 @@
//
//===----------------------------------------------------------------------===//
-#include <chrono>
#include <cstdlib>
#include <limits>
#include <memory>
@@ -1910,11 +1909,6 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
transcript_item = std::make_shared<StructuredData::Dictionary>();
transcript_item->AddStringItem("command", command_line);
- transcript_item->AddIntegerItem(
- "timestampInEpochSeconds",
- std::chrono::duration_cast<std::chrono::seconds>(
- std::chrono::system_clock::now().time_since_epoch())
- .count());
m_transcript.AddItem(transcript_item);
}
@@ -2062,14 +2056,6 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
log, "HandleCommand, command line after removing command name(s): '%s'",
remainder.c_str());
- // To test whether or not transcript should be saved, `transcript_item` is
- // used instead of `GetSaveTrasncript()`. This is because the latter will
- // fail when the command is "settings set interpreter.save-transcript true".
- if (transcript_item) {
- transcript_item->AddStringItem("commandName", cmd_obj->GetCommandName());
- transcript_item->AddStringItem("commandArguments", remainder);
- }
-
ElapsedTime elapsed(execute_time);
cmd_obj->Execute(remainder.c_str(), result);
}
@@ -2086,8 +2072,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
transcript_item->AddStringItem("output", result.GetOutputData());
transcript_item->AddStringItem("error", result.GetErrorData());
- transcript_item->AddFloatItem("durationInSeconds",
- execute_time.get().count());
+ transcript_item->AddFloatItem("seconds", execute_time.get().count());
}
return result.Succeeded();
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index be0848573f812..7f866ae0ef324 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -16,7 +16,6 @@
#include "lldb/Target/Process.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/UnixSignals.h"
-#include "lldb/Utility/StructuredData.h"
using namespace lldb;
using namespace lldb_private;
@@ -226,7 +225,6 @@ llvm::json::Value DebuggerStats::ReportStatistics(
const bool summary_only = options.summary_only;
const bool load_all_debug_info = options.load_all_debug_info;
- const bool include_transcript = options.include_transcript;
json::Array json_targets;
json::Array json_modules;
@@ -366,36 +364,5 @@ llvm::json::Value DebuggerStats::ReportStatistics(
global_stats.try_emplace("commands", std::move(cmd_stats));
}
- if (include_transcript) {
- // When transcript is available, add it to the to-be-returned statistics.
- //
- // NOTE:
- // When the statistics is polled by an LLDB command:
- // - The transcript in the returned statistics *will NOT* contain the
- // returned statistics itself (otherwise infinite recursion).
- // - The returned statistics *will* be written to the internal transcript
- // buffer. It *will* appear in the next statistcs or transcript poll.
- //
- // For example, let's say the following commands are run in order:
- // - "version"
- // - "statistics dump" <- call it "A"
- // - "statistics dump" <- call it "B"
- // The output of "A" will contain the transcript of "version" and
- // "statistics dump" (A), with the latter having empty output. The output
- // of B will contain the trascnript of "version", "statistics dump" (A),
- // "statistics dump" (B), with A's output populated and B's output empty.
- const StructuredData::Array &transcript =
- debugger.GetCommandInterpreter().GetTranscript();
- if (transcript.GetSize() != 0) {
- std::string buffer;
- llvm::raw_string_ostream ss(buffer);
- json::OStream json_os(ss);
- transcript.Serialize(json_os);
- if (auto json_transcript = llvm::json::parse(ss.str()))
- global_stats.try_emplace("transcript",
- std::move(json_transcript.get()));
- }
- }
-
return std::move(global_stats);
}
diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py
index ebe6166eb45e5..fb6fc07d2d443 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -623,50 +623,3 @@ def test_had_frame_variable_errors(self):
# Verify that the top level statistic that aggregates the number of
# modules with debugInfoHadVariableErrors is greater than zero
self.assertGreater(stats["totalModuleCountWithVariableErrors"], 0)
-
- def test_transcript_happy_path(self):
- """
- Test "statistics dump" and the transcript information.
- """
- self.build()
- exe = self.getBuildArtifact("a.out")
- target = self.createTestTarget(file_path=exe)
- self.runCmd("settings set interpreter.save-transcript true")
- self.runCmd("version")
-
- # Verify the output of a first "statistics dump"
- debug_stats = self.get_stats("--transcript")
- self.assertIn("transcript", debug_stats)
- transcript = debug_stats["transcript"]
- self.assertEqual(len(transcript), 2)
- self.assertEqual(transcript[0]["commandName"], "version")
- self.assertEqual(transcript[1]["commandName"], "statistics dump")
- # The first "statistics dump" in the transcript should have no output
- self.assertNotIn("output", transcript[1])
-
- # Verify the output of a second "statistics dump"
- debug_stats = self.get_stats("--transcript")
- self.assertIn("transcript", debug_stats)
- transcript = debug_stats["transcript"]
- self.assertEqual(len(transcript), 3)
- self.assertEqual(transcript[0]["commandName"], "version")
- self.assertEqual(transcript[1]["commandName"], "statistics dump")
- # The first "statistics dump" in the transcript should have output now
- self.assertIn("output", transcript[1])
- self.assertEqual(transcript[2]["commandName"], "statistics dump")
- # The second "statistics dump" in the transcript should have no output
- self.assertNotIn("output", transcript[2])
-
- def test_transcript_should_not_exist_when_not_asked_for(self):
- """
- Test "statistics dump" and the transcript information.
- """
- self.build()
- exe = self.getBuildArtifact("a.out")
- target = self.createTestTarget(file_path=exe)
- self.runCmd("settings set interpreter.save-transcript true")
- self.runCmd("version")
-
- # Verify the output of a first "statistics dump"
- debug_stats = self.get_stats() # Not with "--transcript"
- self.assertNotIn("transcript", debug_stats)
diff --git a/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py b/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py
index 6814ccec16c48..95643eef0d344 100644
--- a/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py
+++ b/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py
@@ -104,7 +104,7 @@ def getTranscriptAsPythonObject(self, ci):
return json.loads(stream.GetData())
- def test_get_transcript(self):
+ def test_structured_transcript(self):
"""Test structured transcript generation and retrieval."""
ci = self.buildAndCreateTarget()
@@ -118,7 +118,7 @@ def test_get_transcript(self):
res = lldb.SBCommandReturnObject()
ci.HandleCommand("version", res)
ci.HandleCommand("an-unknown-command", res)
- ci.HandleCommand("br s -f main.c -l %d" % self.line, res)
+ ci.HandleCommand("breakpoint set -f main.c -l %d" % self.line, res)
ci.HandleCommand("r", res)
ci.HandleCommand("p a", res)
ci.HandleCommand("statistics dump", res)
@@ -130,28 +130,22 @@ def test_get_transcript(self):
# All commands should have expected fields.
for command in transcript:
self.assertIn("command", command)
- # Unresolved commands don't have "commandName"/"commandArguments".
- # We will validate these fields below, instead of here.
self.assertIn("output", command)
self.assertIn("error", command)
- self.assertIn("durationInSeconds", command)
- self.assertIn("timestampInEpochSeconds", command)
+ self.assertIn("seconds", command)
# The following validates individual commands in the transcript.
#
# Notes:
# 1. Some of the asserts rely on the exact output format of the
# commands. Hopefully we are not changing them any time soon.
- # 2. We are removing the time-related fields from each command, so
- # that some of the validations below can be easier / more readable.
+ # 2. We are removing the "seconds" field from each command, so that
+ # some of the validations below can be easier / more readable.
for command in transcript:
- del command["durationInSeconds"]
- del command["timestampInEpochSeconds"]
+ del(command["seconds"])
# (lldb) version
self.assertEqual(transcript[0]["command"], "version")
- self.assertEqual(transcript[0]["commandName"], "version")
- self.assertEqual(transcript[0]["commandArguments"], "")
self.assertIn("lldb version", transcript[0]["output"])
self.assertEqual(transcript[0]["error"], "")
@@ -159,25 +153,18 @@ def test_get_transcript(self):
self.assertEqual(transcript[1],
{
"command": "an-unknown-command",
- # Unresolved commands don't have "commandName"/"commandArguments"
"output": "",
"error": "error: 'an-unknown-command' is not a valid command.\n",
})
- # (lldb) br s -f main.c -l <line>
- self.assertEqual(transcript[2]["command"], "br s -f main.c -l %d" % self.line)
- self.assertEqual(transcript[2]["commandName"], "breakpoint set")
- self.assertEqual(
- transcript[2]["commandArguments"], "-f main.c -l %d" % self.line
- )
+ # (lldb) breakpoint set -f main.c -l <line>
+ self.assertEqual(transcript[2]["command"], "breakpoint set -f main.c -l %d" % self.line)
# Breakpoint 1: where = a.out`main + 29 at main.c:5:3, address = 0x0000000100000f7d
self.assertIn("Breakpoint 1: where = a.out`main ", transcript[2]["output"])
self.assertEqual(transcript[2]["error"], "")
# (lldb) r
self.assertEqual(transcript[3]["command"], "r")
- self.assertEqual(transcript[3]["commandName"], "process launch")
- self.assertEqual(transcript[3]["commandArguments"], "-X true --")
# Process 25494 launched: '<path>/TestCommandInterpreterAPI.test_structured_transcript/a.out' (x86_64)
self.assertIn("Process", transcript[3]["output"])
self.assertIn("launched", transcript[3]["output"])
@@ -187,17 +174,11 @@ def test_get_transcript(self):
self.assertEqual(transcript[4],
{
"command": "p a",
- "commandName": "dwim-print",
- "commandArguments": "-- a",
"output": "(int) 123\n",
"error": "",
})
# (lldb) statistics dump
- self.assertEqual(transcript[5]["command"], "statistics dump")
- self.assertEqual(transcript[5]["commandName"], "statistics dump")
- self.assertEqual(transcript[5]["commandArguments"], "")
- self.assertEqual(transcript[5]["error"], "")
statistics_dump = json.loads(transcript[5]["output"])
# Dump result should be valid JSON
self.assertTrue(statistics_dump is not json.JSONDecodeError)
@@ -213,6 +194,7 @@ def test_save_transcript_setting_default(self):
# The setting's default value should be "false"
self.runCmd("settings show interpreter.save-transcript", "interpreter.save-transcript (boolean) = false\n")
+ # self.assertEqual(res.GetOutput(), )
def test_save_transcript_setting_off(self):
ci = self.buildAndCreateTarget()
@@ -238,7 +220,7 @@ def test_save_transcript_setting_on(self):
self.assertEqual(len(transcript), 1)
self.assertEqual(transcript[0]["command"], "version")
- def test_get_transcript_returns_copy(self):
+ def test_save_transcript_returns_copy(self):
"""
Test that the returned structured data is *at least* a shallow copy.
|
|
Just curious, @gulfemsavrun , how were you able to merge without another person's approval of the PR? I'm noob to github pull request - thought revert PR are the same as other PRs and need an approval, but reading this PR it seems such step isn't required. I see the following on my revert PR. I'm guess maybe you have write access, so you can merge yourself. Is my understanding correct? |
clayborg
pushed a commit
that referenced
this pull request
Jun 3, 2024
Problematic PR: #92843 Reverted by: #94088 The first PR added a test which fails in Linux builds (see the last few comments there). This PR contains all the changes in the first PR, plus the fix to the said test. --------- Co-authored-by: Roy Shi <[email protected]>
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.
Reverts #92843 because it broke some lldb tests:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8746385730949743489/overview