[lldb] Store the return SBValueList in the CommandReturnObject #127566
Conversation
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThere are a lot of lldb commands whose result is really a ValueObject that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (#125006), we can store the resultant ValueObject in the return object, allowing an IDE to access the SBValue and do its own rich formatting. rdar://143965453 Full diff: https://github.com/llvm/llvm-project/pull/127566.diff 8 Files Affected:
diff --git a/lldb/include/lldb/API/SBCommandReturnObject.h b/lldb/include/lldb/API/SBCommandReturnObject.h
index 9a63c1f96aa70..96dda239d3c97 100644
--- a/lldb/include/lldb/API/SBCommandReturnObject.h
+++ b/lldb/include/lldb/API/SBCommandReturnObject.h
@@ -136,6 +136,8 @@ class LLDB_API SBCommandReturnObject {
void SetError(const char *error_cstr);
+ lldb::SBValue GetReturnValue(lldb::DynamicValueType use_dynamic);
+
protected:
friend class SBCommandInterpreter;
friend class SBOptions;
diff --git a/lldb/include/lldb/API/SBValue.h b/lldb/include/lldb/API/SBValue.h
index 46ef6daa95264..75d20a4378f09 100644
--- a/lldb/include/lldb/API/SBValue.h
+++ b/lldb/include/lldb/API/SBValue.h
@@ -442,6 +442,7 @@ class LLDB_API SBValue {
protected:
friend class SBBlock;
+ friend class SBCommandReturnObject;
friend class SBFrame;
friend class SBModule;
friend class SBTarget;
diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h
index 803bcd76995ed..20600ff667bb6 100644
--- a/lldb/include/lldb/Interpreter/CommandReturnObject.h
+++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -14,6 +14,7 @@
#include "lldb/Utility/StreamString.h"
#include "lldb/Utility/StreamTee.h"
#include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-forward.h"
#include "lldb/lldb-private.h"
#include "llvm/ADT/StringRef.h"
@@ -165,6 +166,12 @@ class CommandReturnObject {
return m_diagnostic_indent;
}
+ lldb::ValueObjectSP GetValueObjectSP() const { return m_value_object_sp; }
+
+ void SetValueObjectSP(lldb::ValueObjectSP value_object_sp) {
+ m_value_object_sp = value_object_sp;
+ }
+
lldb::ReturnStatus GetStatus() const;
void SetStatus(lldb::ReturnStatus status);
@@ -197,6 +204,9 @@ class CommandReturnObject {
lldb::ReturnStatus m_status = lldb::eReturnStatusStarted;
+ /// An optional ValueObjectSP if the command created one.
+ lldb::ValueObjectSP m_value_object_sp;
+
bool m_did_change_process_state = false;
bool m_suppress_immediate_output = false;
diff --git a/lldb/source/API/SBCommandReturnObject.cpp b/lldb/source/API/SBCommandReturnObject.cpp
index 6f54581e64ef4..b0de4adc6e241 100644
--- a/lldb/source/API/SBCommandReturnObject.cpp
+++ b/lldb/source/API/SBCommandReturnObject.cpp
@@ -12,6 +12,7 @@
#include "lldb/API/SBFile.h"
#include "lldb/API/SBStream.h"
#include "lldb/API/SBStructuredData.h"
+#include "lldb/API/SBValue.h"
#include "lldb/Core/StructuredDataImpl.h"
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Utility/ConstString.h"
@@ -62,8 +63,8 @@ SBCommandReturnObject::SBCommandReturnObject(const SBCommandReturnObject &rhs) {
m_opaque_up = clone(rhs.m_opaque_up);
}
-SBCommandReturnObject &SBCommandReturnObject::
-operator=(const SBCommandReturnObject &rhs) {
+SBCommandReturnObject &
+SBCommandReturnObject::operator=(const SBCommandReturnObject &rhs) {
LLDB_INSTRUMENT_VA(this, rhs);
if (this != &rhs)
@@ -356,3 +357,12 @@ void SBCommandReturnObject::SetError(const char *error_cstr) {
if (error_cstr)
ref().AppendError(error_cstr);
}
+
+SBValue
+SBCommandReturnObject::GetReturnValue(lldb::DynamicValueType use_dynamic) {
+ LLDB_INSTRUMENT_VA(this, use_dynamic);
+
+ SBValue sb_value;
+ sb_value.SetSP(ref().GetValueObjectSP(), use_dynamic);
+ return sb_value;
+}
diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index d4d038d28f675..aab99441c63a3 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -205,6 +205,9 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
ExpressionResults expr_result = target.EvaluateExpression(
expr, exe_scope, valobj_sp, eval_options, &fixed_expression);
+ if (valobj_sp)
+ result.SetValueObjectSP(valobj_sp);
+
// Record the position of the expression in the command.
std::optional<uint16_t> indent;
if (fixed_expression.empty()) {
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index 7e26381c92405..b0c92d9391b13 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -434,6 +434,8 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
}
if (result_valobj_sp) {
+ result.SetValueObjectSP(result_valobj_sp);
+
Format format = m_format_options.GetFormat();
if (result_valobj_sp->GetError().Success()) {
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index a5709b36f52ee..69bce205ddccc 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -152,6 +152,7 @@ class CommandObjectFrameDiagnose : public CommandObjectParsed {
return;
}
+ result.SetValueObjectSP(valobj_sp);
DumpValueObjectOptions::DeclPrintingHelper helper =
[&valobj_sp](ConstString type, ConstString var,
const DumpValueObjectOptions &opts,
@@ -317,9 +318,9 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
} else if (*m_options.relative_frame_offset > 0) {
// I don't want "up 20" where "20" takes you past the top of the stack
// to produce an error, but rather to just go to the top. OTOH, start
- // by seeing if the requested frame exists, in which case we can avoid
+ // by seeing if the requested frame exists, in which case we can avoid
// counting the stack here...
- const uint32_t frame_requested = frame_idx
+ const uint32_t frame_requested = frame_idx
+ *m_options.relative_frame_offset;
StackFrameSP frame_sp = thread->GetStackFrameAtIndex(frame_requested);
if (frame_sp)
diff --git a/lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py b/lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py
index 2193b7270d0b4..0fee54eb5fe1d 100644
--- a/lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py
+++ b/lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py
@@ -33,3 +33,16 @@ def test_get_command(self):
ci.HandleCommand("help help", res)
self.assertTrue(res.Succeeded())
self.assertEqual(res.GetCommand(), "help help")
+
+ value = res.GetReturnValue(lldb.eNoDynamicValues)
+ self.assertFalse(value)
+
+ def test_get_value(self):
+ res = lldb.SBCommandReturnObject()
+ ci = self.dbg.GetCommandInterpreter()
+ ci.HandleCommand("p 1 + 1", res)
+ self.assertTrue(res.Succeeded())
+
+ value = res.GetReturnValue(lldb.eNoDynamicValues)
+ self.assertTrue(value)
+ self.assertEqual(value.GetValue(), "2")
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There are a lot of lldb commands whose result is really a ValueObject that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (llvm#125006), we can store the resultant ValueObject in the return object, allowing an IDE to access the SBValue and do its own rich formatting. rdar://143965453
JDevlieghere
force-pushed
the
command-return-value
branch
from
9c290f5 to
6f59b11
Compare
There was a problem hiding this comment.
Two questions:
- what about commands (like frame/target variable) which can return more than variable. I guess I'm wondering if this should be a SBValueList instead?
- what about commands (I don't know if we have any) whose output consists of more than formatting a single value (which, I guess would be lost if the IDE decides to ignore the output). Are we fine with not supporting that? (we could say that if a command wants to print some extra output, it should use the error stream for that)
|
Would be great if we could integrate this with Note that lldb-dap already exposes SBValues via the protocol when evaluating an expression. Afaict, the same logic only could also be wired with |
That makes sense.
In the longer term, we can also add an optional SBStructuredData for commands that return structured data, like I think that would be better than trying to dump some of the output to stderr. |
|
I think Pavel is right, having this be an SBValueList would allow us to support more commands. That should be just as easy to implement and use, and more flexible. |
JDevlieghere
changed the title
| @@ -22,8 +22,6 @@ class ValueObject; | |||
| /// A collection of ValueObject values that. | |||
| class ValueObjectList { | |||
| public: | |||
| const ValueObjectList &operator=(const ValueObjectList &rhs); | |||
There was a problem hiding this comment.
Essentially a no-op, but necessary to silence:
warning: definition of implicit copy constructor for 'ValueObjectList' is deprecated because it has a user-provided copy assignment operator [-Wdeprecated-copy-with-user-provided-copy]
That sort of makes sense, but it doesn't sound like you're planning to do that soon, so I'm asking about what should we do until (and if) that happens. The reason I suggested stderr is because that made sense for the commands I'm thinking of. For example, the expression command can produce a result and some warnings and fixit-applied notes. I don't know if these currently go to stderr, but I think it would be okay if they did... |
I don't think it's a good idea for commands to put any of their output anywhere but the command result. Anything that goes to stderr is lost, and particularly if is trying to run their own command loop, so that they are by hand printing the command result, there will be no way to associate that output with the command that produced it. We don't dump warnings on successful expression evaluation, though we do print fixits. I would suggest for the near term that if a command that produces a ValueObject returns successfully but with "notes" we should put those notes in the Error object - but still have the state be Success. That would make sense for the warnings and fixits since Adrian has been working on getting the error output into a structured form to better hold compiler warnings and errors. |
|
Please keep the discussion going. I'm going to go ahead and merge this as I don't expect the outcome to affect this particular PR. |
…127566) There are a lot of lldb commands whose result is really one or more ValueObjects that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (llvm#125006), we can store the resultant ValueObjects in the return object, allowing an IDE to access the SBValues and do its own rich formatting. rdar://143965453 Conflicts: lldb/include/lldb/Interpreter/CommandReturnObject.h
…62f13d5db21 [🍒 stable/20240723] [lldb] Store the return SBValueList in the CommandReturnObject (llvm#127566)
I see that there's been a misunderstanding. By "stderr" I meant the "stderr of the CommandReturnObject" (i.e., I'm sorry about the confusion. Does that sound better?
I sort of like that, particularly as it lets you obtain the diagnostics in a structured form, but it also feels kinda wrong because -- if I'm not mistaken -- we are currently only setting the error field if we failed to retrieve the value. It sounds like it could confuse some code which is not expecting it. I should also add that I don't really have a horse in this race. I have no plan on using this feature any time soon. I'm just thinking about the questions I would have if I was trying to implement it (either from the consumer or producer end). So it's possible this isn't something that's going to matter in practice... |
|
On Feb 20, 2025, at 1:52 AM, Pavel Labath ***@***.***> wrote:
labath
left a comment
(llvm/llvm-project#127566)
That sort of makes sense, but it doesn't sound like you're planning to do that soon, so I'm asking about what should we do until (and if) that happens. The reason I suggested stderr is because that made sense for the commands I'm thinking of. For example, the expression command can produce a result and some warnings and fixit-applied notes. I don't know if these currently go to stderr, but I think it would be okay if they did...
I don't think it's a good idea for commands to put any of their output anywhere but the command result. Anything that goes to stderr is lost, and particularly if is trying to run their own command loop, so that they are by hand printing the command result, there will be no way to associate that output with the command that produced it.
I see that there's been a misunderstanding. By "stderr" I meant the "stderr of the CommandReturnObject" (i.e., CommandReturnObject::m_err_stream, returned by SBCommandReturnObject::GetError), not the actual stderr of the debugger. So the convention would be that if CommandReturnObject contains a variable, then its output (GetOutput) would contain a rendering of rendering of that variable (and it can be ignored when doing your own printing), but the error output (GetError) would contain any additional data (like the fixits) that can/should be anyway.
<#127566 (comment)> <https://github.com/notifications/unsubscribe-auth/ADUPVW2LCZ4HORI2JUMJNFD2QWQWLAVCNFSM6AAAAABXKWF5M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZQHE4TEOBZG4>
labath
left a comment
(llvm/llvm-project#127566)
<#127566 (comment)>
That sort of makes sense, but it doesn't sound like you're planning to do that soon, so I'm asking about what should we do until (and if) that happens. The reason I suggested stderr is because that made sense for the commands I'm thinking of. For example, the expression command can produce a result and some warnings and fixit-applied notes. I don't know if these currently go to stderr, but I think it would be okay if they did...
I don't think it's a good idea for commands to put any of their output anywhere but the command result. Anything that goes to stderr is lost, and particularly if is trying to run their own command loop, so that they are by hand printing the command result, there will be no way to associate that output with the command that produced it.
I see that there's been a misunderstanding. By "stderr" I meant the "stderr of the CommandReturnObject" (i.e., CommandReturnObject::m_err_stream, returned by SBCommandReturnObject::GetError), not the actual stderr of the debugger. So the convention would be that if CommandReturnObject contains a variable, then its output (GetOutput) would contain a rendering of rendering of that variable (and it can be ignored when doing your own printing), but the error output (GetError) would contain any additional data (like the fixits) that can/should be anyway.
I'm sorry about the confusion. Does that sound better?
That would work.
We don't dump warnings on successful expression evaluation, though we do print fixits. I would suggest for the near term that if a command that produces a ValueObject returns successfully but with "notes" we should put those notes in the Error object - but still have the state be Success. That would make sense for the warnings and fixits since Adrian has been working on getting the error output into a structured form to better hold compiler warnings and errors.
I sort of like that, particularly as it lets you obtain the diagnostics in a structured form, but it also feels kinda wrong because -- if I'm not mistaken -- we are currently only setting the error field if we failed to retrieve the value. It sounds like it could confuse some code which is not expecting it.
We always have a Status object in the ValueObject. Up to now, if the Status object is in a Success state, then the contents of the Status object is undefined. So I was suggesting giving a meaning to when the Status object is in a "Success" state for a ValueObject that is retrieved from a CommandReturnObject. This shouldn't cause problems with other clients. No one should be checking whether the current command returned an error by checking whether the status object has text in it or not, you should be calling Success() and seeing whether it is true or false. After all, a failing command is not obligated to put error text in the Status.
OTOH, SBError (lldb_private::Status) doesn't have a "set text w/o setting the error state" method, so maybe it's better not to add that for just this purpose. Something specific to the command interpreter makes more sense.
In the long run, we need something more structured, because there's no requirement that you either "do all your warnings and then call ValueObjectPrinter" or vice versa. So if we're handing the UI the pieces of the output with the intention that it reconstruct the console output but more interactively, we need to mark up the text like:
<text before first VO>
<text before second VO>
...
<text after all VO's>
Then the UI could interleave these text fields with the interactive field that holds each local.
Jim
… I should also add that I don't really have a horse in this race. I have no plan on using this feature any time soon. I'm just thinking about the questions I would have if I was trying to implement it (either from the consumer or producer end). So it's possible this isn't something that's going to matter in practice...
—
Reply to this email directly, view it on GitHub <#127566 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2LCZ4HORI2JUMJNFD2QWQWLAVCNFSM6AAAAABXKWF5M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZQHE4TEOBZG4>.
You are receiving this because your review was requested.
|
There are a lot of lldb commands whose result is really one or more ValueObjects that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (#125006), we can store the resultant ValueObjects in the return object, allowing an IDE to access the SBValues and do its own rich formatting.
rdar://143965453