https://github.com/gulfemsavrun created https://github.com/llvm/llvm-project/pull/94088
Reverts llvm/llvm-project#92843 because it broke some lldb tests: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8746385730949743489/overview >From e6c1256867d6992d5affca539a8dab6541f8db9e Mon Sep 17 00:00:00 2001 From: gulfemsavrun <gul...@google.com> Date: Fri, 31 May 2024 20:05:03 -0700 Subject: [PATCH] Revert "A few updates around "transcript" (#92843)" This reverts commit ad884d97288c752ba9088d01cf7ab80b20e4d2a6. --- lldb/include/lldb/API/SBCommandInterpreter.h | 11 +---- .../lldb/Interpreter/CommandInterpreter.h | 8 +--- lldb/include/lldb/Target/Statistics.h | 1 - lldb/source/Commands/CommandObjectStats.cpp | 3 -- lldb/source/Commands/Options.td | 4 -- .../source/Interpreter/CommandInterpreter.cpp | 17 +------ lldb/source/Target/Statistics.cpp | 33 ------------- .../commands/statistics/basic/TestStats.py | 47 ------------------- .../interpreter/TestCommandInterpreterAPI.py | 38 ++++----------- 9 files changed, 15 insertions(+), 147 deletions(-) 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. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
