[lldb] Update dwim-print to support limited variable expression paths

[kastiglione]

frame variable supports nested variable access, which the API calls "variable expression paths". This change updates dwim-print to support a subset of supported variable expression paths.

Consider the expression a->b. In C++, the arrow operator can be overloaded, and where that is the case, expression evaluation must be used to evaluate it, not frame variable. Likewise, the subscript operator can be overloaded.

To avoid those cases, this change introduces a limited support for variable expression paths. Use of the dot operator is allowed.

Additionally, this change allows dwim-print to directly access children of this and self (see AllowDirectIVarAccess). This functionality is also provided by the same GetValueForVariableExpressionPath method.

rdar://104348908

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)

Changes

frame variable supports nested variable access, which the API calls "variable
expression paths". This change updates dwim-print to support a subset of supported
variable expression paths.

Consider the expression a->b. In C++, the arrow operator can be overloaded, and where
that is the case, expression evaluation must be used to evaluate it, not frame variable.
Likewise, the subscript operator can be overloaded.

To avoid those cases, this change introduces a limited support for variable expression
paths. Use of the dot operator is allowed.

Additionally, this change allows dwim-print to directly access children of this and
self (see AllowDirectIVarAccess). This functionality is also provided by the same
GetValueForVariableExpressionPath method.

rdar://104348908

---

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

5 Files Affected:

• (modified) lldb/source/Commands/CommandObjectDWIMPrint.cpp (+17-4)
• (modified) lldb/test/API/commands/dwim-print/Makefile (+1-1)
• (modified) lldb/test/API/commands/dwim-print/TestDWIMPrint.py (+36-7)
• (removed) lldb/test/API/commands/dwim-print/main.c (-14)
• (added) lldb/test/API/commands/dwim-print/main.cpp (+22)

diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index 62c4e74d853ad1..842ff6e1466d63 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -151,10 +151,23 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
     result.SetStatus(eReturnStatusSuccessFinishResult);
   };
 
-  // First, try `expr` as the name of a frame variable.
-  if (frame) {
-    auto valobj_sp = frame->FindVariable(ConstString(expr));
-    if (valobj_sp && valobj_sp->GetError().Success()) {
+  // First, try `expr` as a _limited_ frame variable expression path: only the
+  // dot operator (`.`) is permitted for this case.
+  //
+  // This is limited to support only unambiguous expression paths. Of note,
+  // expression paths are not attempted if the expression contain either the
+  // arrow operator (`->`) or the subscript operator (`[]`). This is because
+  // both operators can be overloaded in C++, and could result in ambiguity in
+  // how the expression is handled. Additionally, `*` and `&` are not supported.
+  bool try_variable_path = expr.find_first_of("*&->[]") == StringRef::npos;
+  if (frame && try_variable_path) {
+    Status status;
+    VariableSP var_sp;
+    auto valobj_sp = frame->GetValueForVariableExpressionPath(
+        expr, eDynamicDontRunTarget,
+        StackFrame::eExpressionPathOptionsAllowDirectIVarAccess, var_sp,
+        status);
+    if (valobj_sp && status.Success() && valobj_sp->GetError().Success()) {
       if (!suppress_result) {
         if (auto persisted_valobj = valobj_sp->Persist())
           valobj_sp = persisted_valobj;
diff --git a/lldb/test/API/commands/dwim-print/Makefile b/lldb/test/API/commands/dwim-print/Makefile
index 10495940055b63..99998b20bcb050 100644
--- a/lldb/test/API/commands/dwim-print/Makefile
+++ b/lldb/test/API/commands/dwim-print/Makefile
@@ -1,3 +1,3 @@
-C_SOURCES := main.c
+CXX_SOURCES := main.cpp
 
 include Makefile.rules
diff --git a/lldb/test/API/commands/dwim-print/TestDWIMPrint.py b/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
index b40924a182e37d..492d49f008a9e4 100644
--- a/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ b/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -16,7 +16,7 @@ def _run_cmd(self, cmd: str) -> str:
         self.ci.HandleCommand(cmd, result)
         return result.GetOutput().rstrip()
 
-    VAR_IDENT = re.compile(r"(?:\$\d+|\w+) = ")
+    VAR_IDENT = re.compile(r"(?:\$\d+|[\w.]+) = ")
 
     def _strip_result_var(self, string: str) -> str:
         """
@@ -121,30 +121,39 @@ def test_empty_expression(self):
     def test_nested_values(self):
         """Test dwim-print with nested values (structs, etc)."""
         self.build()
-        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.cpp")
+        )
         self.runCmd("settings set auto-one-line-summaries false")
         self._expect_cmd(f"dwim-print s", "frame variable")
         self._expect_cmd(f"dwim-print (struct Structure)s", "expression")
 
     def test_summary_strings(self):
-        """Test dwim-print with nested values (structs, etc)."""
+        """Test dwim-print with type summaries."""
         self.build()
-        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.cpp")
+        )
         self.runCmd("settings set auto-one-line-summaries false")
         self.runCmd("type summary add -e -s 'stub summary' Structure")
         self._expect_cmd(f"dwim-print s", "frame variable")
         self._expect_cmd(f"dwim-print (struct Structure)s", "expression")
+        self.runCmd("type summary delete Structure")
 
     def test_void_result(self):
         """Test dwim-print does not surface an error message for void expressions."""
         self.build()
-        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.cpp")
+        )
         self.expect("dwim-print (void)15", matching=False, patterns=["(?i)error"])
 
     def test_preserves_persistent_variables(self):
         """Test dwim-print does not delete persistent variables."""
         self.build()
-        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.cpp")
+        )
         self.expect("dwim-print int $i = 15")
         # Run the same expression twice and verify success. This ensures the
         # first command does not delete the persistent variable.
@@ -154,5 +163,25 @@ def test_preserves_persistent_variables(self):
     def test_missing_type(self):
         """The expected output of po opaque is its address (no error)"""
         self.build()
-        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.cpp")
+        )
         self.expect("dwim-print -O -- opaque", substrs=["0x"])
+
+    def test_variable_expression_path(self):
+        """Test dwim-print supports certain variable expression paths."""
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.cpp")
+        )
+        self.runCmd("settings set auto-one-line-summaries false")
+        self._expect_cmd("dwim-print w.s", "frame variable")
+        self._expect_cmd("dwim-print wp->s", "expression")
+
+    def test_direct_child_access(self):
+        """Test dwim-print supports accessing members/ivars without qualification."""
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "break inside", lldb.SBFileSpec("main.cpp")
+        )
+        self._expect_cmd("dwim-print number", "frame variable")
diff --git a/lldb/test/API/commands/dwim-print/main.c b/lldb/test/API/commands/dwim-print/main.c
deleted file mode 100644
index 6bfe645c7c6e09..00000000000000
--- a/lldb/test/API/commands/dwim-print/main.c
+++ /dev/null
@@ -1,14 +0,0 @@
-struct Structure {
-  int number;
-};
-
-struct Opaque;
-int puts(const char *s);
-
-int main(int argc, char **argv) {
-  struct Structure s;
-  s.number = 30;
-  struct Opaque *opaque = &s;
-  puts("break here");
-  return 0;
-}
diff --git a/lldb/test/API/commands/dwim-print/main.cpp b/lldb/test/API/commands/dwim-print/main.cpp
new file mode 100644
index 00000000000000..d1abb5a85dd450
--- /dev/null
+++ b/lldb/test/API/commands/dwim-print/main.cpp
@@ -0,0 +1,22 @@
+extern "C" int puts(const char *s);
+
+struct Structure {
+  int number = 30;
+  void f() { puts("break inside"); }
+};
+
+struct Wrapper {
+  Structure s;
+};
+
+struct Opaque;
+
+int main(int argc, char **argv) {
+  Structure s;
+  Wrapper w;
+  Wrapper *wp = &w;
+  Opaque *opaque = (Opaque *)(void *)&s;
+  puts("break here");
+  s.f();
+  return 0;
+}

[Michael137]

Makes sense to me!

[Michael137]

Nit:

Suggested change

	bool try_variable_path = expr.find_first_of("*&->[]") == StringRef::npos;
	const bool try_variable_path = expr.find_first_of("*&->[]") == StringRef::npos;

[Michael137]

Should we also add cases for */[]/&? Or are those tested elsewhere?