[lldb] Make print delegate to synthetic frames.

[bzcheeseman]

Stacked PRs:

• ->[lldb] Make print delegate to synthetic frames. #178602
• [lldb] Add support for ScriptedFrame to provide values/variables. #178575

---

[lldb] Make print delegate to synthetic frames.

This patch is more of a proposal in that it's a pretty dramatic change to the way that print works. It completely delegates getting values to the frame if the frame is synthetic, and does not redirect at all if the frame fails.

For this patch, the main goal was to allow the synthetic frame to bubble up its own errors in expression evaluation, rather than having errors come back with an extra "could not find identifier " or worse, simply get swallowed. If there's a better way to handle this, I'm more than happy to change this as long as the core goals of 'delegate variable/value extraction to the synthetic frame', and 'allow the synthetic frame to give back errors that are displayed to the user' can be met.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Aman LaChapelle (bzcheeseman)

Changes

Stacked PRs:

• ->#178602
• #178575
• #178574
• #178573

---

[lldb] Make print delegate to synthetic frames.

This patch is more of a proposal in that it's a pretty dramatic change to the way that print works. It completely delegates getting values to the frame if the frame is synthetic, and does not redirect at all if the frame fails.

For this patch, the main goal was to allow the synthetic frame to bubble up its own errors in expression evaluation, rather than having errors come back with an extra "could not find identifier <blah>" or worse, simply get swallowed. If there's a better way to handle this, I'm more than happy to change this as long as the core goals of 'delegate variable/value extraction to the synthetic frame', and 'allow the synthetic frame to give back errors that are displayed to the user' can be met.

---

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

1 Files Affected:

• (modified) lldb/source/Commands/CommandObjectDWIMPrint.cpp (+23-2)

diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index 40f00c90bbbfb..68bb87221ed03 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -143,8 +143,7 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
       maybe_add_hint(output);
       result.GetOutputStream() << output;
     } else {
-      llvm::Error error =
-        valobj.Dump(result.GetOutputStream(), dump_options);
+      llvm::Error error = valobj.Dump(result.GetOutputStream(), dump_options);
       if (error) {
         result.AppendError(toString(std::move(error)));
         return;
@@ -155,6 +154,28 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
     result.SetStatus(eReturnStatusSuccessFinishResult);
   };
 
+  // If the frame is synthetic, then we handle all printing through the
+  // GetValueForVariableExpressionPath. This is so that the synthetic frame has
+  // the ability to take over anything it needs to.
+  if (frame && frame->IsSynthetic()) {
+    VariableSP var_sp;
+    Status status;
+    auto valobj_sp = frame->GetValueForVariableExpressionPath(
+        expr, eval_options.GetUseDynamic(),
+        StackFrame::eExpressionPathOptionsAllowDirectIVarAccess, var_sp,
+        status);
+
+    // Something failed, print the error and return immediately.
+    if (!status.Success()) {
+      result.AppendError(status.AsCString());
+      return;
+    }
+
+    // Otherwise, simply print the object.
+    dump_val_object(*valobj_sp);
+    return;
+  }
+
   // First, try `expr` as a _limited_ frame variable expression path: only the
   // dot operator (`.`) is permitted for this case.
   //

[jimingham]

This doesn't seem like the right way to do this to me. What should be happening fundamentally is that when anybody asks "can you find the variable foo" for a synthetic frame, the frame's synthetic provider's variable list should be queried, rather than (or along with depending on what policy we're going to choose for access to synthetic variables) the "native" variable list from the frame. You would want that to work not just in dwim-print but anywhere that variables get looked up in a frame.

Once you've handed out the right root variable, the parsing of child nodes for an expression will happen naturally since that's just querying the root's type, so if you get the right root variable, you get the appropriate children. That shows that properly the task is "hand out the right variables from the frame when asked".

The one thing that dwim-print and expr should probably do is warn or error out if the user tries to running expressions in a frame with synthetic variables. Even that wouldn't be necessary if we take the extra step of materializing the synthetic values into the inferior when the frame makes them. But that's probably going to be tricky and seems like a "version 2" kind of effort. In the case of StackFrame::EvaluateExpression and expr you should be able to do this at the top of the command (except expr --top-level which explicitly forces a different scope). For dwim-print, you would error out if the frame var lookup failed and before it falls back to running expressions.

But note, if you said frame var (Foo *) synthesized_var_name with the new DIL support, you would first want the DIL parser to parse this to the point where it sees a name it has to ask for. If you handed out the right variable when the DIL asked for it by name, then the rest of this would all fall out naturally.

So while we probably want to put some "can't run expressions that have to run in the target in a frame with synthesized variables" warning when in a frame with synthesized variables, the actual lookup into the synthesized variables list needs to happen at a lower layer in the lookup.

[bzcheeseman]

This doesn't seem like the right way to do this to me. What should be happening fundamentally is that when anybody asks "can you find the variable foo" for a synthetic frame, the frame's synthetic provider's variable list should be queried, rather than (or along with depending on what policy we're going to choose for access to synthetic variables) the "native" variable list from the frame. You would want that to work not just in dwim-print but anywhere that variables get looked up in a frame.

Yes that does seem much more correct. I think the issue I was running into was understanding where that change should be made!

Once you've handed out the right root variable, the parsing of child nodes for an expression will happen naturally since that's just querying the root's type, so if you get the right root variable, you get the appropriate children. That shows that properly the task is "hand out the right variables from the frame when asked".

That makes sense. I hope that works with Values as well, since we can't produce variables at this point 😅 My reading of the code is that you're right, it should, but I'm not as confident as I'd like to be.

The one thing that dwim-print and expr should probably do is warn or error out if the user tries to running expressions in a frame with synthetic variables. Even that wouldn't be necessary if we take the extra step of materializing the synthetic values into the inferior when the frame makes them. But that's probably going to be tricky and seems like a "version 2" kind of effort. In the case of StackFrame::EvaluateExpression and expr you should be able to do this at the top of the command (except expr --top-level which explicitly forces a different scope). For dwim-print, you would error out if the frame var lookup failed and before it falls back to running expressions.

Do you mean in the context of something like "I have provided a synthetic variable from my interpreter, and now I want to run an expression on it"? Wouldn't you just have the interpreter itself run that expression? Or not, I guess, depending on the thing being interpreted.

But note, if you said frame var (Foo *) synthesized_var_name with the new DIL support, you would first want the DIL parser to parse this to the point where it sees a name it has to ask for. If you handed out the right variable when the DIL asked for it by name, then the rest of this would all fall out naturally.

Sorry what's the DIL here? That sounds promising, but I don't know enough to evaluate. The main blocker I encountered was finding things/methods that required VariableSP (hard to create) vs ValueObjectSP (easy to create).

So while we probably want to put some "can't run expressions that have to run in the target in a frame with synthesized variables" warning when in a frame with synthesized variables, the actual lookup into the synthesized variables list needs to happen at a lower layer in the lookup.

Got it, makes sense. Do you have a sense for where something like that should be done? It kind of sounds like you're shooting for putting some of this stuff into the expression evaluation machinery? I'm happy to try and put some more time into it, I will just need your guidance :) I would also like to see if there's a good way for the plugin to propagate errors back up to the user through the print/expression evaluation.

In the meantime I'll close this PR since it sounds like, and I agree, there's a better way to do the thing I'm trying to do.