[lldb-dap] Adjust variable display values. #146754
Conversation
This corrects a mistake I made when I previously tried to add support for obj-c/swift variables in lldb-dap. This should call into `GetObjectDescription` if there is no summary of the type available and the description is not empty. I also added a unit test to verify this.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis corrects a mistake I made when I previously tried to add support for obj-c/swift variables in lldb-dap. This should call into I also added a unit test to verify this. Full diff: https://github.com/llvm/llvm-project/pull/146754.diff 5 Files Affected:
diff --git a/lldb/test/API/tools/lldb-dap/variables/objc/Makefile b/lldb/test/API/tools/lldb-dap/variables/objc/Makefile
new file mode 100644
index 0000000000000..71d7ec417633f
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/variables/objc/Makefile
@@ -0,0 +1,9 @@
+OBJC_SOURCES := main.m
+
+CFLAGS_EXTRAS := -w -fobjc-arc
+
+USE_SYSTEM_STDLIB := 1
+
+LD_EXTRAS := -framework Foundation
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/variables/objc/TestDAP_variables_objc.py b/lldb/test/API/tools/lldb-dap/variables/objc/TestDAP_variables_objc.py
new file mode 100644
index 0000000000000..9dcbed50987b9
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/variables/objc/TestDAP_variables_objc.py
@@ -0,0 +1,31 @@
+"""
+Test 'variables' requests for obj-c types.
+"""
+
+import lldbdap_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class TestDAP_variables_objc(lldbdap_testcase.DAPTestCaseBase):
+ @skipUnlessDarwin
+ def test_objc_description(self):
+ """Test that we can get the description of an Objective-C object."""
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(
+ program,
+ )
+ source = "main.m"
+ breakpoint_ids = self.set_source_breakpoints(
+ source, [line_number(source, "// breakpoint")]
+ )
+ self.continue_to_breakpoints(breakpoint_ids)
+
+ greeter_var = self.dap_server.get_local_variable(name="greeter")
+ self.assertIsNotNone(greeter_var, "greeter variable should not be None")
+ self.assertEqual(greeter_var["type"], "Greeter *")
+ self.assertEqual(greeter_var["evaluateName"], "greeter")
+ self.assertRegexpMatches(
+ greeter_var["value"], r"<Greeter 0x[0-9A-Fa-f]+ name=Bob debugDescription>"
+ )
+ self.continue_to_exit()
diff --git a/lldb/test/API/tools/lldb-dap/variables/objc/main.m b/lldb/test/API/tools/lldb-dap/variables/objc/main.m
new file mode 100644
index 0000000000000..aeafa0019af8c
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/variables/objc/main.m
@@ -0,0 +1,45 @@
+#import <Foundation/Foundation.h>
+
+@interface Greeter : NSObject
+
+@property(nonatomic, strong) NSString *name;
+
+- (void)greet:(NSString *)other;
+
+@end
+
+@implementation Greeter
+
+- (instancetype)initWithName:(NSString *)name {
+ if ((self = [super init])) {
+ _name = [name copy];
+ }
+ return self;
+}
+
+- (void)greet:(NSString *)other {
+ NSLog(@"Hello %@, from %@", other, _name);
+}
+
+- (NSString *)description {
+ return
+ [NSString stringWithFormat:@"<Greeter %p name=%@>", (void *)self, _name];
+}
+
+- (NSString *)debugDescription {
+ return [NSString stringWithFormat:@"<Greeter %p name=%@ debugDescription>",
+ (void *)self, _name];
+}
+
+@end
+
+int main(int argc, char *argv[]) {
+ Greeter *greeter = [[Greeter alloc] initWithName:@"Bob"];
+ if (argc > 1) {
+ [greeter greet:@(argv[1])];
+ } else {
+ [greeter greet:@"World"];
+ }
+
+ return 0; // breakpoint
+}
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 08e65ab835a57..a42b4af05d782 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -836,6 +836,7 @@ VariableDescription::VariableDescription(lldb::SBValue v,
os_display_value << "<error: " << error << ">";
} else {
value = llvm::StringRef(v.GetValue()).str();
+ object_description = llvm::StringRef(v.GetObjectDescription()).str();
summary = llvm::StringRef(v.GetSummary()).str();
if (summary.empty() && auto_variable_summaries)
auto_summary = TryCreateAutoSummary(v);
@@ -843,7 +844,10 @@ VariableDescription::VariableDescription(lldb::SBValue v,
std::optional<std::string> effective_summary =
!summary.empty() ? summary : auto_summary;
- if (!value.empty()) {
+ if (!object_description.empty() &&
+ (!effective_summary || effective_summary->empty())) {
+ os_display_value << object_description;
+ } else if (!value.empty()) {
os_display_value << value;
if (effective_summary)
os_display_value << " " << *effective_summary;
@@ -901,18 +905,18 @@ llvm::json::Object VariableDescription::GetVariableExtensionsJSON() {
std::string VariableDescription::GetResult(llvm::StringRef context) {
// In repl context, the results can be displayed as multiple lines so more
// detailed descriptions can be returned.
- if (context != "repl")
+ if (context != "repl" || !v.IsValid())
return display_value;
- if (!v.IsValid())
- return display_value;
+ // First, try the SBValue::GetObjectDescription(), which may call into
+ // language runtime specific formatters (see ValueObjectPrinter).
+ if (!object_description.empty())
+ return object_description;
- // Try the SBValue::GetDescription(), which may call into language runtime
- // specific formatters (see ValueObjectPrinter).
+ // Fallback to the default description for the value.
lldb::SBStream stream;
v.GetDescription(stream);
- llvm::StringRef description = stream.GetData();
- return description.trim().str();
+ return llvm::StringRef(stream.GetData()).trim().str();
}
bool ValuePointsToCode(lldb::SBValue v) {
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index fd9a06931ebff..0a660d04d02d8 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -329,6 +329,9 @@ struct VariableDescription {
std::string evaluate_name;
// The output of SBValue.GetValue() if it doesn't fail. It might be empty.
std::string value;
+ // The output of SBValue.GetObjectDescription() if it doesn't fail. It might
+ // be empty.
+ std::string object_description;
// The summary string of this variable. It might be empty.
std::string summary;
// The auto summary if using `enableAutoVariableSummaries`.
|
|
I'm not sure that you really always want to do this. Fetching the ObjectDescription for a variable means running code in the target to do so. And because we can't tell whether code in an ObjectDescription will require locks that other threads might hold, we do our usual "try on one thread for a bit, then run all threads". So if you run ObjectDescription for every variable in a locals view, you can pretty easily get into the situation where you stop on a breakpoint in Thread A, switch to Thread B to see a frame and its locals and then when you switch back to Thread A it is no longer at the breakpoint - because running all the ObjectDescriptions for the frame in Thread B required allowing Thread A to run. That's a pretty bad user experience. If you can ensure that the expression you are running can safely be executed by running only the current thread, it's only a performance problem to run expressions behind the user's back. But otherwise, we really should only run expressions when the user explicitly asks us to. |
I thought, but didn't check, that this wouldn't run the target if the Maybe we could use this if only for If you try to print something and it just prints |
|
dynamic value and object description are orthogonal. Dynamic value is the system that allows you to discover, for instance, the "most specific class of an object" when you are passed a pointer typed as one of the base classes of that most specific class. TTTT, I don't think we currently have to run code to get dynamic types anymore. It used to be that in some cases in ObjC we had to do that, but we haven't had to do that for a while. |
|
Object Description is always code-running. It works in languages that have a convention for printing a user-facing string representation of an object. I guess you could have a language with a static way to do this but I haven't seen one. |
|
The way Xcode deals with this is that the Locals view's Right-Click menu offers a "Print Description" action. That way users can very easily dial up the object description, but doing so remains under the user's explicit control. |
|
IIRC, C# has a similar object description mechanism, and they had to add some annotations to the typesystem to allow you to specify "don't run this one automatically in VisualStudio" because they kept getting into trouble with object descriptions that did too much work, or forced lazily evaluated entities to get evaluated, or whatever. |
|
Also note that for ObjC in particular the "Object Description" is NOT a summary of the object. Print the Object Description of an NSDictionary with 1000 objects and you'll get pages and pages of output with all the elements (and their Descriptions inline IIRC). There is nothing in the contract that says Object Descriptions should be brief. |
|
Remember, running code is NOT a risk free operation. If I am stopped on a thread in code that has acquired a non-recursive lock A, and I run an expression by hand that ends up first successfully acquiring a lock B and then trying to acquire A, that expression will deadlock. The only thing lldb can do is interrupt it and tear down the expression stack. But that means that now lock B is permanently locked, nothing is ever going to release it, and your debug session is pretty much toast. So it is a feature we should use judiciously, and the best way to ensure that is to always make it an explicitly user-directed action. |
|
There are exceptions to this, for instance we run functions automatically to gather the various Sanitizer report results. But those functions have been written with this use in mind, so they don't take locks or do anything that might be unsafe. |
|
We can add a button to the variables UI or a context menu entry for running the description. I can switch to that instead. |
I think that's the better way to go. If you do this, the View that's going to display the description has to be expandable and scrollable, since these descriptions are often quite long. |
This corrects a mistake I made when I previously tried to add support for obj-c/swift variables in lldb-dap. This should call into
GetObjectDescriptionif there is no summary of the type available and the description is not empty.I also added a unit test to verify this.