[lldb] Fix dwim-print to not delete non-result persistent variables #85152
Conversation
|
@llvm/pr-subscribers-lldb Author: Dave Lee (kastiglione) ChangesFull diff: https://github.com/llvm/llvm-project/pull/85152.diff 2 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index b183cb423111fb..5c043dfd101be6 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -170,6 +170,14 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
ExpressionResults expr_result = target.EvaluateExpression(
expr, exe_scope, valobj_sp, eval_options, &fixed_expression);
+ auto persistent_name = valobj_sp->GetName();
+ // EvaluateExpression doesn't generate a new persistent result (`$0`) when
+ // the expression is already just a persistent variable (`$var`). Instead,
+ // the same persistent variable is reused. Take note of when a persistent
+ // result is created, to prevent unintentional deletion of a user's
+ // persistent variable.
+ bool did_persist_result = persistent_name != expr;
+
// Only mention Fix-Its if the expression evaluator applied them.
// Compiler errors refer to the final expression after applying Fix-It(s).
if (!fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
@@ -199,9 +207,9 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
}
}
- if (suppress_result)
+ if (did_persist_result && suppress_result)
if (auto result_var_sp =
- target.GetPersistentVariable(valobj_sp->GetName())) {
+ target.GetPersistentVariable(persistent_name)) {
auto language = valobj_sp->GetPreferredDisplayLanguage();
if (auto *persistent_state =
target.GetPersistentExpressionStateForLanguage(language))
diff --git a/lldb/test/API/commands/dwim-print/TestDWIMPrint.py b/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
index 040632096c70e7..c650b1e3533e08 100644
--- a/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ b/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -146,3 +146,15 @@ def test_void_result(self):
self, "// break here", lldb.SBFileSpec("main.c")
)
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")
+ )
+ 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.
+ for _ in range(2):
+ self.expect("dwim-print $i", startstr="(int) 15")
|
| // the same persistent variable is reused. Take note of when a persistent | ||
| // result is created, to prevent unintentional deletion of a user's | ||
| // persistent variable. | ||
| bool did_persist_result = persistent_name != expr; |
There was a problem hiding this comment.
This seems like a really fragile way to test what you want to test, which is whether the result of the expression was a new expression result variable. It might be better to add a "PeekNextPersistentVariableName" that just prints what the next persistent variable would be called but doesn't increment the counter, then assert that the variable you produced is NOT called that?
There was a problem hiding this comment.
I agree that it's not ideal, but I felt it was sufficient.
what you want to test, which is whether the result of the expression was a new expression result variable
exactly. However I think peeking at the result variables is also not ideal. Both my initial stab at this, and your suggestion are deducing whether a new expression result variable was created. If I'm to change the API, I think it'd be ideal to make the API have some means to communicate this explicitly.
There's another approach I considered taking. Before calling EvaluateExpression, try the expression as a persistent variable and check in the target to see if it exists, and if it does exist, use it (and avoid EvaluateExpression altogether). This is similar to how expressions are first treated as frame variables (and as @felipepiovezan has requested, we should try target variable too).
There was a problem hiding this comment.
It would be good to have EvaluateExpression tell you whether it created a result variable or not. For instance, calling a void function produces no result. It would be nice to know that happened directly rather than attempting to guess from the returned ValueObject. We definitely know this...
That would definitely be a better way to do this.
There was a problem hiding this comment.
Jim, I have updated the PR to try the expression as a persistent variable – prior to expression evaluation. My thinking is, instead of evaluating first, then asking if a persistent variable was created, it's more straight to the point to first ask "is this a persistent variable".
I like the idea of trying the various kinds of variables in sequence: frame, target, persistent.
|
That's an okay approach if the only time an expression produces a result variable dwim-print should not delete is when a persistent variable is explicitly mentioned. But anyway, since that's the case we're fixing, and checking as a persistent variable as a short-cut early on isn't a bad idea, it is fine for these purposes. I think you need to get the persistent variables for whatever the language of the expression happens to be, not just get all the persistent variables. Otherwise, if you define a C persistent variable, then stop in a swift frame and do: (lldb) p $works_in_c which would be confusing. |
|
I'll change this to scope lookup of persistent variables to the expected language. However, there's an argument to be made that |
|
The counter to this is that `dwim-print` differs from `frame var` in that it supports complex expressions. So having it work for simple statements of a variable name (what frame var and target var already do just fine) but then fail when the SAME name is used in a slightly more complicated expression is a serious failure to do its particular job.
So if you start doing cross-language lookups for simple identifiers, you're also going to have to keep trying the expression with all the supported ExpressionParsers to avoid this problem. Probably if you start with the current frame's language, then do the other ones if that fails, this will "do what people mean".
Jim
… On Mar 15, 2024, at 2:28 PM, Dave Lee ***@***.***> wrote:
I'll change this to scope lookup of persistent variables to the expected language.
However, there's an argument to be made that dwim-print $my_var should work anywhere. I think that follows the spirit of "do what I mean". Maybe in the future I'll propose that in a separate change.
—
Reply to this email directly, view it on GitHub <#85152 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZKCVFMQ7535FPIMULYYNRYRAVCNFSM6AAAAABEVCQEZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBQGQ3DEMZSGU>.
You are receiving this because you are on a team that was mentioned.
|
|
I don't consider it a failure to not be able to perform mixed language expressions. But it would have to be presented to the user in a way that is understandable, which could be a challenge. Anyway, I pushed the change that scopes the lookup to the expected language. thank you for the reviews |
I'm not sure what you mean by "mixed language expressions". What I'm trying to avoid is, if stopped in a swift frame, you get: That's not a mixed language expression, that's a perfectly good C++ expression. I don't think it makes sense to present simple variables of all language types, but not evaluate expressions using all the available expression parsers, starting with the obvious one first. |
I was thinking about the case where a user attempts |
kastiglione
deleted the
lldb-Fix-dwim-print-to-not-delete-non-result-persistent-variables
branch
…lvm#85152) `EvaluateExpression` does not always create a new persistent result. If the expression is a bare persistent variable, then a new persistent result is not created. This means the caller can't assume a new persistent result is created for each evaluation. However, `dwim-print` was doing exactly that: assuming a new persistent result for each evaluation. This resulted in a bug: ``` (lldb) p int $j = 23 (lldb) p $j (lldb) p $j ``` The first `p $j` would not create a persistent result, and so `dwim-print` would inadvertently delete `$j`. The second `p $j` would fail. The fix is to try `expr` as a persistent variable, after trying `expr` as a frame variable. For persistent variables, this avoids calling `EvaluateExpression`. Resolves llvm#84806 rdar://124688427 (cherry picked from commit 4da2b54)
The important thing is not whether this is abstractly a valid swift expression, but rather that is an expression that would execute correctly in the Swift Expression parser. That won't happen with this expression because the swift compiler can't find My take is that if I do
|
…lvm#85152) `EvaluateExpression` does not always create a new persistent result. If the expression is a bare persistent variable, then a new persistent result is not created. This means the caller can't assume a new persistent result is created for each evaluation. However, `dwim-print` was doing exactly that: assuming a new persistent result for each evaluation. This resulted in a bug: ``` (lldb) p int $j = 23 (lldb) p $j (lldb) p $j ``` The first `p $j` would not create a persistent result, and so `dwim-print` would inadvertently delete `$j`. The second `p $j` would fail. The fix is to try `expr` as a persistent variable, after trying `expr` as a frame variable. For persistent variables, this avoids calling `EvaluateExpression`. Resolves llvm#84806 rdar://124688427 (cherry picked from commit 4da2b54)
…lvm#85152) `EvaluateExpression` does not always create a new persistent result. If the expression is a bare persistent variable, then a new persistent result is not created. This means the caller can't assume a new persistent result is created for each evaluation. However, `dwim-print` was doing exactly that: assuming a new persistent result for each evaluation. This resulted in a bug: ``` (lldb) p int $j = 23 (lldb) p $j (lldb) p $j ``` The first `p $j` would not create a persistent result, and so `dwim-print` would inadvertently delete `$j`. The second `p $j` would fail. The fix is to try `expr` as a persistent variable, after trying `expr` as a frame variable. For persistent variables, this avoids calling `EvaluateExpression`. Resolves llvm#84806 rdar://124688427 (cherry picked from commit 4da2b54) (cherry-picked from commit 38b55e1)
…lvm#85152) (#8428) `EvaluateExpression` does not always create a new persistent result. If the expression is a bare persistent variable, then a new persistent result is not created. This means the caller can't assume a new persistent result is created for each evaluation. However, `dwim-print` was doing exactly that: assuming a new persistent result for each evaluation. This resulted in a bug: ``` (lldb) p int $j = 23 (lldb) p $j (lldb) p $j ``` The first `p $j` would not create a persistent result, and so `dwim-print` would inadvertently delete `$j`. The second `p $j` would fail. The fix is to try `expr` as a persistent variable, after trying `expr` as a frame variable. For persistent variables, this avoids calling `EvaluateExpression`. Resolves llvm#84806 rdar://124688427 (cherry picked from commit 4da2b54) (cherry-picked from commit 38b55e1)
…lvm#85152) `EvaluateExpression` does not always create a new persistent result. If the expression is a bare persistent variable, then a new persistent result is not created. This means the caller can't assume a new persistent result is created for each evaluation. However, `dwim-print` was doing exactly that: assuming a new persistent result for each evaluation. This resulted in a bug: ``` (lldb) p int $j = 23 (lldb) p $j (lldb) p $j ``` The first `p $j` would not create a persistent result, and so `dwim-print` would inadvertently delete `$j`. The second `p $j` would fail. The fix is to try `expr` as a persistent variable, after trying `expr` as a frame variable. For persistent variables, this avoids calling `EvaluateExpression`. Resolves llvm#84806 rdar://124688427 (cherry picked from commit 4da2b54) (cherry-picked from commit 38b55e1)
…m-print-to-not-delete-non-result-persistent-variables-85152 [lldb] Fix dwim-print to not delete non-result persistent variables (llvm#85152)
EvaluateExpressiondoes not always create a new persistent result. If the expression is a bare persistent variable, then a new persistent result is not created. This means the caller can't assume a new persistent result is created for each evaluation. However,dwim-printwas doing exactly that: assuming a new persistent result for each evaluation. This resulted in a bug:The first
p $jwould not create a persistent result, and sodwim-printwould inadvertently delete$j. The secondp $jwould fail.The fix is to try
expras a persistent variable, after tryingexpras a frame variable. For persistent variables, this avoids callingEvaluateExpression.Resolves #84806
rdar://124688427