[M] Change in dart/sdk[main]: [dyn_modules] Check target of dcall from dynamic modules is valid.
Sigmund Cherem (Gerrit)
Sigmund Cherem added 1 comment![]( "Open in Gerrit")
I'm about to be OOO for about a week so I wont respond as quickly. I just wanted to send this out to get preliminary feedback for when I'm back.
Note the related CLs in the chain, including the end-to-end test. Those may provide helpful examples of the semantics we are enforcing with the runtime checks.
Related details
- Alexander Markov
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
1 comment
// Check for dyn-module:dynamically-callable ornit: I find is bit confusing that this attribute change happens here and not when `Function` object is created when loading kernel... but okay.
Related details
- Alexander Markov
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 3 comments![]( "Open in Gerrit")
function.set_is_dynamically_callable(true);Why is this code needed? It seems like we already set a bit on the functions in kernel loader.
function2.set_is_dynamically_callable(true);CreateDynamicInvocationForwarder should propagate the bit to the forwarder automatically. Consider replacing this code with an assertion.
!target_func.is_declared_in_bytecode()) {Can we make these checks once per cache miss instead of repeating them on every call?
We would probably need to split the cache for interface and dynamic calls.
Related details
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 4 comments![]( "Open in Gerrit")
// Check for dyn-module:dynamically-callable ornit: I find is bit confusing that this attribute change happens here and not when `Function` object is created when loading kernel... but okay.
Good point - removed, we are setting it already when loading kernel and this logic was now obsolete and can be removed.
Why is this code needed? It seems like we already set a bit on the functions in kernel loader.
Good point, we can delete this now. I had this on an earlier implementation before I added support in the kernel loader (which was also necessary for JIT) and I forgot to remove it here.
CreateDynamicInvocationForwarder should propagate the bit to the forwarder automatically. Consider replacing this code with an assertion.
Done.
Can we make these checks once per cache miss instead of repeating them on every call?
We would probably need to split the cache for interface and dynamic calls.
Fair, I think it's possible, but I'd like to do that separately. I've added a TODO for now.
Given that this is guarded by `is_dynamic_call` already, we shouldn't be incurring this cost except for the cases where we actually are performing a dcall from bytecoxe, which are less critical.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov voted Code-Review+1![]( "Open in Gerrit")
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 1 comment![]( "Open in Gerrit")
Thank you Alex and Slava for the reviews!
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
if (!target_func.is_dynamically_callable() &&I forgot to mention: we should probably have a runtime flag to disable these checks in order to support case when interpreter is used to run arbitrary Dart code without any restriction (e.g. for debugging/hot reload on iOS), not dynamic module code.
Related details
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted Code-Review+1![]( "Open in Gerrit")
- Alexander Markov
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 2 comments![]( "Open in Gerrit")
if (!target_func.is_dynamically_callable() &&I forgot to mention: we should probably have a runtime flag to disable these checks in order to support case when interpreter is used to run arbitrary Dart code without any restriction (e.g. for debugging/hot reload on iOS), not dynamic module code.
Maybe we should also disable checks in JIT mode until it has correct pragmas / `is_dynamically_callable` bit.
dynamically-callable:We should probably expose all dart:* libraries as dynamically callable (similarly to `callable` above) to allow running tests with arbitrary Dart code.
Also note that this dynamic interface yaml file is only used in AOT mode.
Related details
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 2 comments![]( "Open in Gerrit")
if (!target_func.is_dynamically_callable() &&I forgot to mention: we should probably have a runtime flag to disable these checks in order to support case when interpreter is used to run arbitrary Dart code without any restriction (e.g. for debugging/hot reload on iOS), not dynamic module code.
Makes sense.
The `#if defined(DART_DYNAMIC_MODULES)` at the beginning of the file made me believe that we only used it with dynamic_modules at the moment. I realize now that we already have the `--interpreter` flag which makes use of it in other ways. I expect with time we will also change the `#if` above so the interpreter will be available in modular-aot debug mode even when dynamic modules is not enabled, correct?
For now, what about using `FLAG_interpreter` which already exists and appears to be the only way we use the interpreter from non-dynamic modules?
Long term I don't think a global runtime flag is the right approach, though. It seems to me that we want the behavior to depend on the caller: if the caller is declared in a dynamic module we'd want this behavior regardless of the modality. Right now we can approximating that question with `FLAG_interpreter`, but maybe we need an actual property like `is_declared_in_dynamic_module` instead?
dynamically-callable:We should probably expose all dart:* libraries as dynamically callable (similarly to `callable` above) to allow running tests with arbitrary Dart code.
Also note that this dynamic interface yaml file is only used in AOT mode.
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 3 comments![]( "Open in Gerrit")
if (!target_func.is_dynamically_callable() &&Sigmund CheremI forgot to mention: we should probably have a runtime flag to disable these checks in order to support case when interpreter is used to run arbitrary Dart code without any restriction (e.g. for debugging/hot reload on iOS), not dynamic module code.
Makes sense.
The `#if defined(DART_DYNAMIC_MODULES)` at the beginning of the file made me believe that we only used it with dynamic_modules at the moment. I realize now that we already have the `--interpreter` flag which makes use of it in other ways. I expect with time we will also change the `#if` above so the interpreter will be available in modular-aot debug mode even when dynamic modules is not enabled, correct?
For now, what about using `FLAG_interpreter` which already exists and appears to be the only way we use the interpreter from non-dynamic modules?
Long term I don't think a global runtime flag is the right approach, though. It seems to me that we want the behavior to depend on the caller: if the caller is declared in a dynamic module we'd want this behavior regardless of the modality. Right now we can approximating that question with `FLAG_interpreter`, but maybe we need an actual property like `is_declared_in_dynamic_module` instead?
This is now guarded by `FLAG_interpreter`, I prefer to not disable entirely the checks in JIT since I do have support for them when loading from kernel snapshots which are transformed and annotated.
It appears our test suite is running JIT with FLAG_interpreter anyways, so the use of this flag is letting us keep the test coverage we have.
invocation.set_is_dynamically_callable(true);@alexm...@google.com - investigating bot failures I came across the invoke-field-dispatcher. I'm using this change to set them as dynamically callable by default, but I'm less familiar with their use and whether such default is correct. Thoguhts?
Sigmund CheremWe should probably expose all dart:* libraries as dynamically callable (similarly to `callable` above) to allow running tests with arbitrary Dart code.
Also note that this dynamic interface yaml file is only used in AOT mode.
Thanks - I'll try that instead.
Done
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 1 comment![]( "Open in Gerrit")
@pragma("dyn-module:dynamically-callable")FYI - also added this change for another a couple failures involving closure calls via `get:call`.
Related details
- Alexander Markov
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 2 comments![]( "Open in Gerrit")
if (!target_func.is_dynamically_callable() &&Sigmund CheremI forgot to mention: we should probably have a runtime flag to disable these checks in order to support case when interpreter is used to run arbitrary Dart code without any restriction (e.g. for debugging/hot reload on iOS), not dynamic module code.
Sigmund CheremMakes sense.
The `#if defined(DART_DYNAMIC_MODULES)` at the beginning of the file made me believe that we only used it with dynamic_modules at the moment. I realize now that we already have the `--interpreter` flag which makes use of it in other ways. I expect with time we will also change the `#if` above so the interpreter will be available in modular-aot debug mode even when dynamic modules is not enabled, correct?
For now, what about using `FLAG_interpreter` which already exists and appears to be the only way we use the interpreter from non-dynamic modules?
Long term I don't think a global runtime flag is the right approach, though. It seems to me that we want the behavior to depend on the caller: if the caller is declared in a dynamic module we'd want this behavior regardless of the modality. Right now we can approximating that question with `FLAG_interpreter`, but maybe we need an actual property like `is_declared_in_dynamic_module` instead?
This is now guarded by `FLAG_interpreter`, I prefer to not disable entirely the checks in JIT since I do have support for them when loading from kernel snapshots which are transformed and annotated.
It appears our test suite is running JIT with FLAG_interpreter anyways, so the use of this flag is letting us keep the test coverage we have.
I agree that ideally dynamic call checking should only work if executing bytecode from a dynamic module. Maybe dart2bytecode can generate a different instruction instead of a DynamicCall (CheckedDynamicCall?) to support this, if dynamic module validation is enabled at compile time.
Regarding your other points:
Eventually I plan to change `#if defined(DART_DYNAMIC_MODULES)` to `#if defined(DART_INCLUDE_INTERPRETER)` where appropriate to make it possible to use interpreter without dynamic modules. Of course, DART_DYNAMIC_MODULES would imply DART_INCLUDE_INTERPRETER (but not vice versa).
`--interpreter` flag is orthogonal to dynamic call checking: it just instructs the standalone embedder to compile everything to bytecode rather than kernel. Even without this flag VM can execute bytecode directly (if it has the interpreter). So this doesn't look like a good indication to enable dynamic call checking.
And lastly, annotating with pragmas in JIT mode is very incomplete. It doesn't cover many important cases such as incremental compiler, running Dart scripts via standalone VM and calls to core libraries. It means that these checks can suddenly block users without any possible remedy. This makes me think that at least for now we should have a runtime flag for dynamic call checking, and we should enable it only in AOT mode by default. This would allow running with checks if needed even in JIT mode but not disturb any of the existing workflows. (Alternatively, we could guard checking by `#if defined(DART_PRECOMPILED_RUNTIME)` to disable them in JIT entirely.)
invocation.set_is_dynamically_callable(true);@alexm...@google.com - investigating bot failures I came across the invoke-field-dispatcher. I'm using this change to set them as dynamically callable by default, but I'm less familiar with their use and whether such default is correct. Thoguhts?
This code is used by 2 kinds of dispatchers: InvokeFieldDispatcher and NoSuchMethodDispatcher.
InvokeFieldDispatcher is used when making a dynamic call through getter (e.g. `o.foo(...)` where `o.foo` turns out to be a getter or field). In such a case I think InvokeFieldDispatcher should inherit the dynamically-callable flag from a getter or field (except when it is used for closure calls). Otherwise it would allow unchecked dynamic calls through getters, as InvokeFieldDispatcher would perform a non-dynamic call of the getter.
NoSuchMethodDispatcher is for dispatching to `noSuchMethod` when corresponding method is not found. We should probably always make those dynamically callable as they are used as the last resort and we disallow user-defined `noSuchMethod` along with dynamically-callable members.
Related details
- Sigmund Cherem
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 2 comments![]( "Open in Gerrit")
if (!target_func.is_dynamically_callable() &&Sigmund CheremI forgot to mention: we should probably have a runtime flag to disable these checks in order to support case when interpreter is used to run arbitrary Dart code without any restriction (e.g. for debugging/hot reload on iOS), not dynamic module code.
Sigmund CheremMakes sense.
The `#if defined(DART_DYNAMIC_MODULES)` at the beginning of the file made me believe that we only used it with dynamic_modules at the moment. I realize now that we already have the `--interpreter` flag which makes use of it in other ways. I expect with time we will also change the `#if` above so the interpreter will be available in modular-aot debug mode even when dynamic modules is not enabled, correct?
For now, what about using `FLAG_interpreter` which already exists and appears to be the only way we use the interpreter from non-dynamic modules?
Long term I don't think a global runtime flag is the right approach, though. It seems to me that we want the behavior to depend on the caller: if the caller is declared in a dynamic module we'd want this behavior regardless of the modality. Right now we can approximating that question with `FLAG_interpreter`, but maybe we need an actual property like `is_declared_in_dynamic_module` instead?
Alexander MarkovThis is now guarded by `FLAG_interpreter`, I prefer to not disable entirely the checks in JIT since I do have support for them when loading from kernel snapshots which are transformed and annotated.
It appears our test suite is running JIT with FLAG_interpreter anyways, so the use of this flag is letting us keep the test coverage we have.
I agree that ideally dynamic call checking should only work if executing bytecode from a dynamic module. Maybe dart2bytecode can generate a different instruction instead of a DynamicCall (CheckedDynamicCall?) to support this, if dynamic module validation is enabled at compile time.
Regarding your other points:
Eventually I plan to change `#if defined(DART_DYNAMIC_MODULES)` to `#if defined(DART_INCLUDE_INTERPRETER)` where appropriate to make it possible to use interpreter without dynamic modules. Of course, DART_DYNAMIC_MODULES would imply DART_INCLUDE_INTERPRETER (but not vice versa).
`--interpreter` flag is orthogonal to dynamic call checking: it just instructs the standalone embedder to compile everything to bytecode rather than kernel. Even without this flag VM can execute bytecode directly (if it has the interpreter). So this doesn't look like a good indication to enable dynamic call checking.
And lastly, annotating with pragmas in JIT mode is very incomplete. It doesn't cover many important cases such as incremental compiler, running Dart scripts via standalone VM and calls to core libraries. It means that these checks can suddenly block users without any possible remedy. This makes me think that at least for now we should have a runtime flag for dynamic call checking, and we should enable it only in AOT mode by default. This would allow running with checks if needed even in JIT mode but not disturb any of the existing workflows. (Alternatively, we could guard checking by `#if defined(DART_PRECOMPILED_RUNTIME)` to disable them in JIT entirely.)
Maybe dart2bytecode can generate a different instruction instead of a DynamicCall (CheckedDynamicCall?) to support this, if dynamic module validation is enabled at compile time.
Seems reasonable. That said, we have other situations that require knowing whether a method was declared in a dynamic module (e.g. to implicitly label it as dynamically callable). Rather than checking `is_declared_in_bytecode`, could we use a single mechanism to answer both questions?
Eventually I plan to change #if defined(DART_DYNAMIC_MODULES) to #if defined(DART_INCLUDE_INTERPRETER)
Sounds great
... This makes me think that at least for now we should have a runtime flag for dynamic call checking, and we should enable it only in AOT mode by default. This would allow running with checks if needed even in JIT mode but not disturb any of the existing workflows. (Alternatively, we could guard checking by #if defined(DART_PRECOMPILED_RUNTIME) to disable them in JIT entirely.)
Thanks, that's completely fair. I modified this CL for now to entirely disable it in JIT and we can add the runtime flag as a follow up (logic is below in the DynamicCall block)
invocation.set_is_dynamically_callable(true);Alexander Markov@alexm...@google.com - investigating bot failures I came across the invoke-field-dispatcher. I'm using this change to set them as dynamically callable by default, but I'm less familiar with their use and whether such default is correct. Thoguhts?
This code is used by 2 kinds of dispatchers: InvokeFieldDispatcher and NoSuchMethodDispatcher.
InvokeFieldDispatcher is used when making a dynamic call through getter (e.g. `o.foo(...)` where `o.foo` turns out to be a getter or field). In such a case I think InvokeFieldDispatcher should inherit the dynamically-callable flag from a getter or field (except when it is used for closure calls). Otherwise it would allow unchecked dynamic calls through getters, as InvokeFieldDispatcher would perform a non-dynamic call of the getter.
NoSuchMethodDispatcher is for dispatching to `noSuchMethod` when corresponding method is not found. We should probably always make those dynamically callable as they are used as the last resort and we disallow user-defined `noSuchMethod` along with dynamically-callable members.
In such a case I think InvokeFieldDispatcher should inherit the dynamically-callable flag from a getter or field
👍 - added this to the CL. It required also tracking the pragma on fields, which we hadn't done until now.
(except when it is used for closure calls)
Like no-such-method, I assume that for closure calls we also want to default to true, correct?
NoSuchMethodDispatcher is for dispatching to noSuchMethod when corresponding method is not found. We should probably always make those dynamically callable as they are used as the last resort and we disallow user-defined noSuchMethod along with dynamically-callable members.
👍
- Alexander Markov
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
4 comments
field.set_is_dynamically_callable(Nit: this is probably not needed as this code reads top-level (static) fields.
return untag()->kind_bits_.Read<NoSanitizeThreadBit>();IsDynamicallyCallableBit
untag()->kind_bits_.UpdateBool<NoSanitizeThreadBit>(value);IsDynamicallyCallableBit
FYI - also added this change for another a couple failures involving closure calls via `get:call`.
Can we special case closure calls when creating InvokeFieldDispatcher instead of adding this pragma (which can affect builds without dynamic modules)?
Related details
- Sigmund Cherem
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem voted and added 5 comments![]( "Open in Gerrit")
Votes added by Sigmund Cherem
| Commit-Queue | +2 |
5 comments
Thank you Alex and Slava for the multiple iterations of review!
Nit: this is probably not needed as this code reads top-level (static) fields.
Done
return untag()->kind_bits_.Read<NoSanitizeThreadBit>();Sigmund CheremIsDynamicallyCallableBit
Done
untag()->kind_bits_.UpdateBool<NoSanitizeThreadBit>(value);Sigmund CheremIsDynamicallyCallableBit
🤦 Thank you, done
@pragma("dyn-module:dynamically-callable")Alexander MarkovFYI - also added this change for another a couple failures involving closure calls via `get:call`.
Can we special case closure calls when creating InvokeFieldDispatcher instead of adding this pragma (which can affect builds without dynamic modules)?
For now, I removed the pragma and instead added this member to the dynamic_interface of the dynamic_modules_runner target.
Not sure if we can completely handle this with InvokeFieldDispatcher - I am seeing cases that are not going through a field dispatcher, but directly finding the getter instead (maybe I'm interpreting this incorrectly, though).
Long term, my guess is that we will need to revisit this so that end-users don't need to do the same thing in their dynamic interface (_Closure.call is a low level implementation detail they shouldn't have to worry about). What I find tricky here is that we don't have something public to anchor on (e.g. we don't have a `Function.call` declaration we can use).
Related details
- Alexander Markov
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sigmund Cherem
| Auto-Submit | +1 |
1 comment
Note that CQ requires another +1 for the last patchset
Slava Egorov (Gerrit)
Slava Egorov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
2 comments
// TODO(sigmund): don't perform this check repeatedly, consider insteadnit: I think we prefer `TODO(dartbug.com/num)` these days. Otherwise TODOs just stay behind.
// splitting the lookup-cacke to separately track dynamic calls.spelling: cache
Related details
- Alexander Markov
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |