Add warning message to session save when transcript isn't saved.
#109020
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: Tom Yang (zhyty) ChangesSomewhat recently, we made the change to hide the behavior to save LLDB session history to the transcript buffer behind the flag I'm making a small update here to our TestsFull diff: https://github.com/llvm/llvm-project/pull/109020.diff 4 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectSession.cpp b/lldb/source/Commands/CommandObjectSession.cpp
index c381ba4f74f120..3f714cec414695 100644
--- a/lldb/source/Commands/CommandObjectSession.cpp
+++ b/lldb/source/Commands/CommandObjectSession.cpp
@@ -19,7 +19,8 @@ class CommandObjectSessionSave : public CommandObjectParsed {
: CommandObjectParsed(interpreter, "session save",
"Save the current session transcripts to a file.\n"
"If no file if specified, transcripts will be "
- "saved to a temporary file.",
+ "saved to a temporary file.\n"
+ "Note: transcripts will only be saved if interpreter.save-transcript is true.\n",
"session save [file]") {
AddSimpleArgumentList(eArgTypePath, eArgRepeatOptional);
}
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index b93f47a8a8d5ec..05426771ba0335 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -3306,6 +3306,8 @@ bool CommandInterpreter::SaveTranscript(
result.SetStatus(eReturnStatusSuccessFinishNoResult);
result.AppendMessageWithFormat("Session's transcripts saved to %s\n",
output_file->c_str());
+ if (!GetSaveTranscript())
+ result.AppendError("Note: the setting interpreter.save-transcript is set to false, so the transcript might not have been recorded.");
if (GetOpenTranscriptInEditor() && Host::IsInteractiveGraphicSession()) {
const FileSpec file_spec;
diff --git a/lldb/source/Interpreter/InterpreterProperties.td b/lldb/source/Interpreter/InterpreterProperties.td
index a5fccbbca091cf..834f7be17480c6 100644
--- a/lldb/source/Interpreter/InterpreterProperties.td
+++ b/lldb/source/Interpreter/InterpreterProperties.td
@@ -16,7 +16,7 @@ let Definition = "interpreter" in {
def SaveSessionOnQuit: Property<"save-session-on-quit", "Boolean">,
Global,
DefaultFalse,
- Desc<"If true, LLDB will save the session's transcripts before quitting.">;
+ Desc<"If true, LLDB will save the session's transcripts before quitting. Note: transcripts will only be saved if interpreter.save-transcript is true.">;
def OpenTranscriptInEditor: Property<"open-transcript-in-editor", "Boolean">,
Global,
DefaultTrue,
diff --git a/lldb/test/API/commands/session/save/TestSessionSave.py b/lldb/test/API/commands/session/save/TestSessionSave.py
index aa99bcd56aed46..c81ff645d9d3b8 100644
--- a/lldb/test/API/commands/session/save/TestSessionSave.py
+++ b/lldb/test/API/commands/session/save/TestSessionSave.py
@@ -85,6 +85,8 @@ def test_session_save(self):
interpreter.HandleCommand("session save", res)
self.assertTrue(res.Succeeded())
raw += self.raw_transcript_builder(cmd, res)
+ # Also check that we don't print an error message about an empty transcript.
+ self.assertNotIn("interpreter.save-transcript is set to false", res.GetError())
with open(os.path.join(td.name, os.listdir(td.name)[0]), "r") as file:
content = file.read()
@@ -93,6 +95,33 @@ def test_session_save(self):
for line in lines:
self.assertIn(line, content)
+ @no_debug_info_test
+ def test_session_save_no_transcript_warning(self):
+ interpreter = self.dbg.GetCommandInterpreter()
+
+ self.runCmd("settings set interpreter.save-transcript false")
+
+ # These commands won't be saved, so are arbitrary.
+ commands = [
+ "p 1",
+ "settings set interpreter.save-session-on-quit true",
+ "fr v",
+ "settings set interpreter.echo-comment-commands true",
+ ]
+
+ for command in commands:
+ res = lldb.SBCommandReturnObject()
+ interpreter.HandleCommand(command, res)
+
+ output_file = self.getBuildArtifact('my-session')
+
+ res = lldb.SBCommandReturnObject()
+ interpreter.HandleCommand("session save " + output_file, res)
+ self.assertTrue(res.Succeeded())
+ # We should warn about the setting being false.
+ self.assertIn("interpreter.save-transcript is set to false", res.GetError())
+ self.assertTrue(os.path.getsize(output_file) == 0, "Output file should be empty since we didn't save the transcript.")
+
@no_debug_info_test
def test_session_save_on_quit(self):
raw = ""
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jeffreytan81
approved these changes
Sep 18, 2024
| @@ -3306,6 +3306,8 @@ bool CommandInterpreter::SaveTranscript( | |||
| result.SetStatus(eReturnStatusSuccessFinishNoResult); | |||
| result.AppendMessageWithFormat("Session's transcripts saved to %s\n", | |||
| output_file->c_str()); | |||
| if (!GetSaveTranscript()) | |||
| result.AppendError("Note: the setting interpreter.save-transcript is set to false, so the transcript might not have been recorded."); | |||
There was a problem hiding this comment.
I am not sure why we are using might not here. Since GetSaveTranscript() is false, isn't the transcript definitely won't be saved? Maybe rewording to:
Note: the interpreter.save-transcript setting is disabled (false), so the transcript will not be saved.
There was a problem hiding this comment.
Here's an example:
(lldb) settings set interpreter.save-transcript true
(lldb) p 1
(int) 1
(lldb) hello
error: 'hello' is not a valid command.
(lldb) settings set interpreter.save-transcript false
(lldb) p 2
(int) 2
(lldb) session save
Session's transcripts saved to /var/folders/dh/nv6c_38j4tq3crp6r6f69sdc0000gn/T/lldb/lldb_session_2024-09-20_17-46-03.936239000.log
error: Note: the setting interpreter.save-transcript is set to false, so the transcript might not have been recorded.
In this case, the text in between the two interpreter.save-transcript settings will be saved, including the actual command to disable it. So here's what's saved:
(lldb) settings set interpreter.save-transcript true
(lldb) p 1
(int) 1
(lldb) hello
�[0;1;31merror: �[0m'hello' is not a valid command.
(lldb) settings set interpreter.save-transcript false
This is why I worded it like this. session save will save whatever's found in the transcript buffer, but what was actually saved in the transcript buffer depends if users did something like this.
There was a problem hiding this comment.
Hmm, one would argue that when interpreter.save-transcript is set to false, it should clear any internal buffer because the name interpreter.save-transcript = false implies it should not save transcript.
For the current behavior, interpreter.save-transcript should be renamed to "stop-record-commands" or something like this.
Not say it is important or I care though.
medismailben
requested changes
Sep 18, 2024
Comment on lines
3309
to
3310
| if (!GetSaveTranscript()) | ||
| result.AppendError("Note: the setting interpreter.save-transcript is set to false, so the transcript might not have been recorded."); |
There was a problem hiding this comment.
We should do this at the beginning of the function, before doing the work and return early if save-transcript is disabled.
There was a problem hiding this comment.
See my comment above.
I hate to be so pedantic about it, but I would be worried if users used this flow of
(lldb) settings set interpreter.save-transcript true
... do various things they want to save ...
(lldb) settings set interpreter.save-transcript false
... do various things they don't want to save ...
(lldb) session save
This wouldn't work anymore if we early returned.
To your point though, I guess they could always re-toggle it before that last session save, and maybe it would be better for the average user.
Anyway, that's why I decided to make this commit so conservative.
There was a problem hiding this comment.
@medismailben friendly ping on any followup comments. If not, I'll go ahead and merge.
Although that could be useful but I'm concerned of the performance aspect of it. Not everyone cares to save transcripts so I'm not sure it should be on by default. I like this patch because it explains to the user what they need to do to save the transaction (enable the settings), may be that's good enough. |
zhyty
force-pushed
the
session-save-warning
branch
from
ad7f50d to
943b350
Compare
added 2 commits
October 5, 2024 00:28
Somewhat recently, we made the change to hide the behavior to save LLDB session history to the transcript buffer behind the flag `interpreter.save-transcript`.
zhyty
force-pushed
the
session-save-warning
branch
from
943b350 to
ac5bc70
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/4965 Here is the relevant piece of the build log for the reference |
Kyvangka1610
pushed a commit
to Kyvangka1610/llvm-project
that referenced
this pull request
Oct 5, 2024
…lvm#109020) Somewhat recently, we made the change to hide the behavior to save LLDB session history to the transcript buffer behind the flag `interpreter.save-transcript`. By default, `interpreter.save-transcript` is false. See llvm#90703 for context. I'm making a small update here to our `session save` messaging and some help docs to clarify for users that aren't aware of this change. Maybe `interpreter.save-transcript` could be true by default as well. Any feedback welcome. # Tests ``` bin/lldb-dotest -p TestSessionSave ``` --------- Co-authored-by: Tom Yang <[email protected]>
Kyvangka1610
added a commit
to Kyvangka1610/llvm-project
that referenced
this pull request
Oct 5, 2024
* commit 'FETCH_HEAD': [clang][bytecode] Handle UETT_OpenMPRequiredSimdAlign (llvm#111259) [mlir][polynomial] Add and verify constraints of coefficientModulus for ringAttr (llvm#111016) [clang][bytecode] Save a per-Block IsWeak bit (llvm#111248) [analyzer] Fix wrong `builtin_*_overflow` return type (llvm#111253) [SelectOpt] Don't convert constant selects to branches. (llvm#110858) [InstCombine] Update and-or-icmps.ll after 574266c. NFC [Instcombine] Test for more gep canonicalization [NFC][TableGen] Change `CodeGenIntrinsics` to use const references (llvm#111219) Add warning message to `session save` when transcript isn't saved. (llvm#109020) [RISCV][TTI] Recognize CONCAT_VECTORS if a shufflevector mask is multiple insert subvector. (llvm#110457) Revert "[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)`" (llvm#111236) [NFC][lsan] Add SuspendAllThreads traces [lsan] Add `thread_suspend_fail` flag Signed-off-by: kyvangka1610 <[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.
Somewhat recently, we made the change to hide the behavior to save LLDB session history to the transcript buffer behind the flag
interpreter.save-transcript. By default,interpreter.save-transcriptis false. See #90703 for context.I'm making a small update here to our
session savemessaging and some help docs to clarify for users that aren't aware of this change. Maybeinterpreter.save-transcriptcould be true by default as well. Any feedback welcome.Tests