A few updates around "transcript" #92843

royitaqi · 2024-05-21T02:45:55Z

Changes

Changes to the structured transcript.
1. Add fields commandName and commandArguments. They will hold the name and the arguments string of the expanded/executed command (e.g. breakpoint set and -f main.cpp -l 4). This is not to be confused with the command field, which holds the user input (e.g. br s -f main.cpp -l 4).
2. Add field timestampInEpochSeconds. It will hold the timestamp when the command is executed.
3. Rename field seconds to durationInSeconds, to improve readability, especially since timestampInEpochSeconds is added.
When transcript is available and the newly added option --transcript is present, add the transcript to the output of statistics dump, as a JSON array under a new field transcript.
A few test name and comment changes.

Test

bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py

bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py

llvmbot · 2024-05-21T02:46:21Z

@llvm/pr-subscribers-lldb

Author: None (royitaqi)

Changes

Add resolvedCommand as a new field to (structured) transcript. This field will hold the expanded/executed command (e.g. "breakpoint set ..."). This is not to be confused with the "command" field, which holds the user input (e.g. "br s ...").
When transcript is available, add it to the output of statistics dump.
A few minor comment and test name changes.

Test

bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py

bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py

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

6 Files Affected:

(modified) lldb/include/lldb/API/SBCommandInterpreter.h (+2-1)
(modified) lldb/include/lldb/Interpreter/CommandInterpreter.h (+2-1)
(modified) lldb/source/Interpreter/CommandInterpreter.cpp (+6)
(modified) lldb/source/Target/Statistics.cpp (+31)
(modified) lldb/test/API/commands/statistics/basic/TestStats.py (+30)
(modified) lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py (+14-6)

diff --git a/lldb/include/lldb/API/SBCommandInterpreter.h b/lldb/include/lldb/API/SBCommandInterpreter.h
index 8ac36344b3a79..8eb4a71cb7f88 100644
--- a/lldb/include/lldb/API/SBCommandInterpreter.h
+++ b/lldb/include/lldb/API/SBCommandInterpreter.h
@@ -320,7 +320,8 @@ 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 executed.
+  /// - "command" (string): The command that was given by the user.
+  /// - "resolvedCommand" (string): The expanded 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.
   /// - "seconds" (float): The time it took to execute the command.
diff --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h b/lldb/include/lldb/Interpreter/CommandInterpreter.h
index ccc30cf4f1a82..6e21af7180f79 100644
--- a/lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -776,7 +776,8 @@ 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 executed.
+  /// - "command" (string): The command that was given by the user.
+  /// - "resolvedCommand" (string): The expanded 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.
   /// - "seconds" (float): The time it took to execute the command.
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index 811726e30af4d..04820bd7d39f6 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2011,6 +2011,12 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
               wants_raw_input ? "True" : "False");
   }
 
+  // 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("resolvedCommand", command_string);
+
   // Phase 2.
   // Take care of things like setting up the history command & calling the
   // appropriate Execute method on the CommandObject, with the appropriate
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index 7f866ae0ef324..fd31377abd06b 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -16,6 +16,7 @@
 #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;
@@ -362,6 +363,36 @@ llvm::json::Value DebuggerStats::ReportStatistics(
     global_stats.try_emplace("modules", std::move(json_modules));
     global_stats.try_emplace("memory", std::move(json_memory));
     global_stats.try_emplace("commands", std::move(cmd_stats));
+
+    // 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.
+    // - The returned statistics *will* be written to the internal transcript
+    //   buffer as the output of the said LLDB command. 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 fb6fc07d2d443..a536ee2c9bbdc 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -623,3 +623,33 @@ 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(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 target.save-transcript true")
+        self.runCmd("version")
+
+        # Verify the output of a first "statistics dump"
+        debug_stats = self.get_stats()
+        self.assertIn("transcript", debug_stats)
+        transcript = debug_stats["transcript"]
+        self.assertEqual(len(transcript), 2)
+        self.assertEqual(transcript[0]["command"], "version")
+        self.assertEqual(transcript[1]["command"], "statistics dump")
+        self.assertEqual(transcript[1]["output"], "")
+
+        # Verify the output of a second "statistics dump"
+        debug_stats = self.get_stats()
+        self.assertIn("transcript", debug_stats)
+        transcript = debug_stats["transcript"]
+        self.assertEqual(len(transcript), 3)
+        self.assertEqual(transcript[0]["command"], "version")
+        self.assertEqual(transcript[1]["command"], "statistics dump")
+        self.assertNotEqual(transcript[1]["output"], "")
+        self.assertEqual(transcript[2]["command"], "statistics dump")
+        self.assertEqual(transcript[2]["output"], "")
diff --git a/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py b/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py
index 95643eef0d344..157594b4aefa1 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_structured_transcript(self):
+    def test_get_transcript(self):
         """Test structured transcript generation and retrieval."""
         ci = self.buildAndCreateTarget()
 
@@ -118,7 +118,7 @@ def test_structured_transcript(self):
         res = lldb.SBCommandReturnObject()
         ci.HandleCommand("version", res)
         ci.HandleCommand("an-unknown-command", res)
-        ci.HandleCommand("breakpoint set -f main.c -l %d" % self.line, res)
+        ci.HandleCommand("br s -f main.c -l %d" % self.line, res)
         ci.HandleCommand("r", res)
         ci.HandleCommand("p a", res)
         ci.HandleCommand("statistics dump", res)
@@ -130,6 +130,7 @@ def test_structured_transcript(self):
         # All commands should have expected fields.
         for command in transcript:
             self.assertIn("command", command)
+            self.assertIn("resolvedCommand", command)
             self.assertIn("output", command)
             self.assertIn("error", command)
             self.assertIn("seconds", command)
@@ -146,6 +147,7 @@ def test_structured_transcript(self):
 
         # (lldb) version
         self.assertEqual(transcript[0]["command"], "version")
+        self.assertEqual(transcript[0]["resolvedCommand"], "version")
         self.assertIn("lldb version", transcript[0]["output"])
         self.assertEqual(transcript[0]["error"], "")
 
@@ -153,18 +155,21 @@ def test_structured_transcript(self):
         self.assertEqual(transcript[1],
             {
                 "command": "an-unknown-command",
+                "resolvedCommand": "an-unknown-command",
                 "output": "",
                 "error": "error: 'an-unknown-command' is not a valid command.\n",
             })
 
-        # (lldb) breakpoint set -f main.c -l <line>
-        self.assertEqual(transcript[2]["command"], "breakpoint set -f main.c -l %d" % self.line)
+        # (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]["resolvedCommand"], "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]["resolvedCommand"], "process launch -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"])
@@ -174,11 +179,15 @@ def test_structured_transcript(self):
         self.assertEqual(transcript[4],
             {
                 "command": "p a",
+                "resolvedCommand": "dwim-print -- a",
                 "output": "(int) 123\n",
                 "error": "",
             })
 
         # (lldb) statistics dump
+        self.assertEqual(transcript[5]["command"], "statistics dump")
+        self.assertEqual(transcript[5]["resolvedCommand"], "statistics dump")
+        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)
@@ -194,7 +203,6 @@ 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()
@@ -220,7 +228,7 @@ def test_save_transcript_setting_on(self):
         self.assertEqual(len(transcript), 1)
         self.assertEqual(transcript[0]["command"], "version")
 
-    def test_save_transcript_returns_copy(self):
+    def test_get_transcript_returns_copy(self):
         """
         Test that the returned structured data is *at least* a shallow copy.

github-actions · 2024-05-21T02:48:37Z

✅ With the latest revision this PR passed the Python code formatter.

royitaqi · 2024-05-21T14:16:37Z

cc @clayborg and @jeffreytan81 about this PR

lldb/source/Target/Statistics.cpp

…python files

lldb/include/lldb/API/SBCommandInterpreter.h

lldb/source/Commands/Options.td

medismailben

LGTM

royitaqi · 2024-05-31T22:03:50Z

This appears to be failing the lldb-x86_64-debian buildbot:
https://lab.llvm.org/buildbot/#/builders/68/builds/75333

I'm testing it on a linux machine.

royitaqi · 2024-05-31T22:32:37Z

Here is the test error:

FAIL: test_get_transcript (TestCommandInterpreterAPI.CommandInterpreterAPICase.test_get_transcript)
   Test structured transcript generation and retrieval.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py", line 180, in test_get_transcript
    self.assertEqual(transcript[3]["commandArguments"], "-X true --")
AssertionError: '-c/bin/bash --' != '-X true --'
- -c/bin/bash --
+ -X true --

I have proposed a fix in #94067
However, I haven't verified it on a Linux machine yet.

gulfemsavrun · 2024-05-31T22:46:55Z

We also see the same test error on our bots.
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64-rbe/b8746387351248919521/overview

gulfemsavrun · 2024-06-01T02:31:47Z

Can you please revert the patch if it's going to take a while to land the fix because I don't want our builders to stay red over the weekend?

This reverts commit ad884d9.

Reverts #92843 because it broke some lldb tests: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8746385730949743489/overview

royitaqi · 2024-06-01T03:13:59Z

@gulfemsavrun

The fix PR is ready, as linked in the above (#94067). I just need someone to approve it and merge it.

I'm quite new to this process. Isn't reverting also require creating a new patch and someone who has the merge privilege to approve and merge it?

FWIW, I have created the revert PR.

If someone can approve-and-merge either the revert PR or the forward fix PR, that will be great:

Revert PR: Revert "A few updates around "transcript"" #94090
Forward fix PR: Re-merge A few updates around "transcript" (#92843) #94067

This reverts commit ad884d9.

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]>

royitaqi added 3 commits May 20, 2024 22:30

Add resolvedCommand to transcript, add transcript to statistics dump

8499f16

Small fixes

1cac4fa

More small fixes

67a1197

royitaqi requested a review from JDevlieghere as a code owner May 21, 2024 02:45

llvmbot added the lldb label May 21, 2024

royitaqi commented May 21, 2024

View reviewed changes

lldb/source/Target/Statistics.cpp Outdated Show resolved Hide resolved

royitaqi added 4 commits May 23, 2024 22:26

Add timestamp into transcript, rename duration field, fix format for …

eadf71a

…python files

Fix test and comment

d2389e3

Fix more format

a2ea0ef

Remove debug prints

83df90b

clayborg reviewed May 28, 2024

View reviewed changes

lldb/include/lldb/API/SBCommandInterpreter.h Outdated Show resolved Hide resolved

JDevlieghere requested a review from medismailben May 29, 2024 16:58

royitaqi added 2 commits May 30, 2024 10:36

Split "resolvedCommand" into "commandName" and "commandArguments"

685771c

Add argument "statistics dump --transcript". Default off.

2132829

clayborg reviewed May 30, 2024

View reviewed changes

lldb/source/Commands/Options.td Outdated Show resolved Hide resolved

medismailben approved these changes May 30, 2024

View reviewed changes

Update description for argument --transcript

47a670c

clayborg approved these changes May 31, 2024

View reviewed changes

Fix python format

2adec16

clayborg merged commit ad884d9 into llvm:main May 31, 2024
5 checks passed

royitaqi mentioned this pull request May 31, 2024

Re-merge A few updates around "transcript" (#92843) #94067

Merged

gulfemsavrun added a commit that referenced this pull request Jun 1, 2024

Revert "A few updates around "transcript" (#92843)"

e6c1256

This reverts commit ad884d9.

gulfemsavrun mentioned this pull request Jun 1, 2024

Revert "A few updates around "transcript"" #94088

Merged

gulfemsavrun added a commit that referenced this pull request Jun 1, 2024

Revert "A few updates around "transcript"" (#94088)

85fd168

Reverts #92843 because it broke some lldb tests: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8746385730949743489/overview

royitaqi mentioned this pull request Jun 1, 2024

Revert "A few updates around "transcript"" #94090

Closed

royitaqi added a commit to royitaqi/llvm-project that referenced this pull request Jun 1, 2024

Revert "A few updates around "transcript" (llvm#92843)"

49ef525

This reverts commit ad884d9.

royitaqi deleted the enhance-transcript-2 branch October 24, 2024 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

A few updates around "transcript" #92843

A few updates around "transcript" #92843

Uh oh!

royitaqi commented May 21, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 21, 2024

Changes

Test

Uh oh!

github-actions bot commented May 21, 2024 •

edited

Loading

Uh oh!

royitaqi commented May 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

medismailben left a comment

Uh oh!

Uh oh!

royitaqi commented May 31, 2024 •

edited

Loading

Uh oh!

royitaqi commented May 31, 2024 •

edited

Loading

Uh oh!

gulfemsavrun commented May 31, 2024

Uh oh!

gulfemsavrun commented Jun 1, 2024

Uh oh!

royitaqi commented Jun 1, 2024 •

edited

Loading

Uh oh!

Uh oh!

A few updates around "transcript" #92843

A few updates around "transcript" #92843

Uh oh!

Conversation

royitaqi commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test

Uh oh!

llvmbot commented May 21, 2024

Changes

Test

Uh oh!

github-actions bot commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

royitaqi commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

royitaqi commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

royitaqi commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gulfemsavrun commented May 31, 2024

Uh oh!

gulfemsavrun commented Jun 1, 2024

Uh oh!

royitaqi commented Jun 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

royitaqi commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024 •

edited

Loading

royitaqi commented May 21, 2024 •

edited

Loading

royitaqi commented May 31, 2024 •

edited

Loading

royitaqi commented May 31, 2024 •

edited

Loading

royitaqi commented Jun 1, 2024 •

edited

Loading