-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dart VM Crash: Trying to evaluate expression on optimized out variable crashes the VM #53996
Comments
/cc @aam could you look at this? |
This crash is caused by |
Oh also, implementing #53687 is an alternative way to fix this specific crash, but I imagine we'll still need the checks described above to handle cases unrelated to pruning dead locals. |
In order to support expression evaluation involving extension types, CFE will use static types of variables at given source position instead of runtime types of variables. That would make dynamic (runtime) types of variables optional. After that, we can change the VM to provide only a subset of runtime types of variables, filtering out all optimized out. If subset of available runtime variables would not be enough, front-end would fail compiling the expression and issue a nice readable error. If certain optimized out variable is not used in the expression, then it would be compiled and executed successfully. We can also change the Procedure which is created by the front-end for the evaluated expression to receive all variable values as named parameters, so VM can match their names against available non-optimized variables and issue an error if used variable is not available/optimized out. @johnniwinther WDYT? |
Our plan (at least currently) is to (somehow) make use of both the runtime type and the static type.
Is anything stopping us from doing that now? If a variable is not actually available (for whatever reason) it seems to me the compiler shouldn't be told it exists. |
@jensjoha Yes, VM could filter optimized out variables even now. At least, we would avoid crash in this case. However, that would not make expression evaluation correct - we would get either 1) a compile-time error about not found variable, which is somewhat misleading; 2) a silent incorrect behavior when missing local variable shadows another variable/getter from outer scope, which is silently used if variable is missing. Currently this already happens inside closures for variables which are not captured, but it would be nice to fix that too. |
So it should communicate that these variables do exist in scope but is unavailable --- say have a special "_OptimizedOut" type, or be in a separate list so the expression compiler could define them as constants of a certain type or something. Was that basically what you suggested but I didn't understand in #53996 (comment)? |
No. VM currently doesn't have information about variables inside closures which were not captured. If needed, CFE should be able to figure out the complete set of variables in scope by given source position, in order to get their static types (and maybe even promoted types) and correctly distinguish between references to local variables and getters from outer scope. VM would provide runtime types for a subset of variables which are actually available at run time (which were not optimized out and not omitted from closure context). CFE can issue an error if variable value is not available at run time (or generate a Procedure which would take those values as named parameters, and VM can issue such error). |
…ctivationFrame::BuildParameters TEST=pkg/vm_service/test/evaluate_optimized_out_variable_test.dart Issue: #53996 Change-Id: I5e6f0b2c02455af73c2108e6996039c95d3f1f31 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347940 Reviewed-by: Alexander Markov <alexmarkov@google.com> Commit-Queue: Derek Xu <derekx@google.com>
The commit above filters out optimized out variables when building the scope in
I spoke with @alexmarkov and we think that the error should be made more specialized and user-readable. |
…ope in ActivationFrame::BuildParameters" This reverts commit e727f4e. Reason for revert: breaks some tests in debug mode Original change's description: > [VM/Debugger] Ignore optimized out variables when building scope in ActivationFrame::BuildParameters > > TEST=pkg/vm_service/test/evaluate_optimized_out_variable_test.dart > > Issue: #53996 > Change-Id: I5e6f0b2c02455af73c2108e6996039c95d3f1f31 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347940 > Reviewed-by: Alexander Markov <alexmarkov@google.com> > Commit-Queue: Derek Xu <derekx@google.com> Issue: #53996 Change-Id: I0d5c94b206ea31e82240f17e9304bec1b01e3580 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347942 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: Derek Xu <derekx@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>
…ope in ActivationFrame::BuildParameters" This is a reland of commit e727f4e We allow the arguments array in EvaluateCompiledExpressionHelper to contain Object::optimized_out().ptr() when we have determined that the receiver has been optimized out but isn't used by the compiled expression. So, I have moved the assertion that checks for optimized out arguments from EvaluateCompiledExpressionHelper to Instance::EvaluateCompiledExpression. TEST=vm-linux-debug-x64 tryjob Original change's description: > [VM/Debugger] Ignore optimized out variables when building scope in ActivationFrame::BuildParameters > > TEST=pkg/vm_service/test/evaluate_optimized_out_variable_test.dart > > Issue: #53996 > Change-Id: I5e6f0b2c02455af73c2108e6996039c95d3f1f31 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347940 > Reviewed-by: Alexander Markov <alexmarkov@google.com> > Commit-Queue: Derek Xu <derekx@google.com> Change-Id: Id6df0b0b3e0d26239068041126b034e9469b87af Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347945 Reviewed-by: Alexander Markov <alexmarkov@google.com> Commit-Queue: Derek Xu <derekx@google.com>
I guess the remaining work is in the front end so updated issue accordingly. //cc @johnniwinther |
@jensjoha Can you take a look at this? |
Reproduction:
Start it, e.g. like this
open e.g. observatory and set a breakpoint on the
print("set a breakpoint here!");
line and continue execution.Go to the "foo" frame (frame 1 in observatory; "foo" in the "call stack" in devtools) and evaluate
data.length
:I'm on 0be8cb2 but it also happens in e.g. Dart 3.0.0.
The text was updated successfully, but these errors were encountered: