[XL] Change in dart/sdk[main]: [vm/shared] Ensure stores into shared static variables are checked.
Alexander Markov (Gerrit)
Alexander Markov added 14 comments![]( "Open in Gerrit")
Change-Id: I27416773fd77077018739ea4dbcc6e4695e67be8Please add
```
Cq-Include-Trybots: luci.dart.try:vm-dyn-linux-debug-x64-try,vm-dyn-mac-debug-arm64-try,vm-aot-dyn-linux-debug-x64-try
```
to test interpreter in the debug mode.
__ ldrb(temp, compiler::FieldAddress(Nit: consider loading the whole word to match other architectures (and avoid possible performance penalties).
__ CompareImmediate(temp, kFirstTypedDataCid);
__ b(&allow_store, LT);
__ CompareImmediate(temp, kLastTypedDataCid);
__ b(checked_store_into_shared_slow_path->entry_label(), LT);Why do we need to go to the runtime for `TypedData`? As per https://github.com/dart-lang/language/blob/main/working/333%20-%20shared%20memory%20multithreading/proposal.md, all TypedData instances are allowed.
(also in other `il_<arch>.cc`)
ObjectPtr object_ptr,Consider passing a handle instead of raw object pointer as handle is created anyway. Passing a raw object pointer is unsafe, so we need to ensure that safepoints are not happening for the whole duration of the function call (e.g. caller needs to use `NoSafepointScope`).
(also for other functions)
ObjectPtr offendingValue_;How can we make sure raw pointer is not cached across a safepoint?
auto& object = Object::Handle(zone, object_ptr);Creating a handle for each tested object seems expensive. Consider rewriting this checking as a helper class with a few cached and reused handles.
if (object.IsImmutable()) {Should we also test `IsCanonical()` bit to allow arbitrary compile-time constant objects, as mentioned in https://github.com/dart-lang/language/blob/main/working/333%20-%20shared%20memory%20multithreading/proposal.md?
Consider also doing this in the fast-path before calling runtime.
Context::Handle(zone, Context::RawCast(object.ptr()));Nit: we can continue using the same handle via `Context::Cast(object)`.
CheckIfTriviallyImmutableHelper(zone, context.At(i), visited, result);Consider using a worklist instead of recursion to avoid stack overflows in case of deep chains of closures. This would also make it easier to reuse handles.
const auto& function = Function::Handle(closure.function());Nit: `zone, `
}Consider adding a separate `IsShareable` bit which we can set on closures to avoid repeated checking. Or maybe `IsImmutable` should be split to `IsDeeplyImmutable` and `IsShallowImmutable`, then we can set `IsShallowImmutable` on closures when they are created and `IsDeeplyImmutable` after they are checked.
message ^= String::NewFormatted(This would create a new object which involves a safepoint. We should avoid having raw object pointers (e.g. `object_ptr`) when safepoint can be triggered.
FfiCallbackMetadata::CheckIfTriviallyImmutable(thread->zone(), value,Similarly to compiled code, we should guard this code with `FLAG_experimental_shared_data` and do all fast-path checks (e.g. test ImmutableBit and ClosureCid) before calling into a slower check which may create handles or throw an exception. Also, we should do a proper runtime call via `INVOKE_RUNTIME`. The same runtime entry can be used both for compiled code and the interpreter.
if (is_shared()) {`FLAG_experimental_shared_data && is_shared()` to match behavior of the compiled code.
Related details
- Alexander Aprelev
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 Aprelev (Gerrit)
Alexander Aprelev voted and added 12 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
12 comments
thank you for the comments so far!
Please add
```
Cq-Include-Trybots: luci.dart.try:vm-dyn-linux-debug-x64-try,vm-dyn-mac-debug-arm64-try,vm-aot-dyn-linux-debug-x64-try
```
to test interpreter in the debug mode.
Done
Nit: consider loading the whole word to match other architectures (and avoid possible performance penalties).
Done
__ CompareImmediate(temp, kFirstTypedDataCid);
__ b(&allow_store, LT);
__ CompareImmediate(temp, kLastTypedDataCid);
__ b(checked_store_into_shared_slow_path->entry_label(), LT);Why do we need to go to the runtime for `TypedData`? As per https://github.com/dart-lang/language/blob/main/working/333%20-%20shared%20memory%20multithreading/proposal.md, all TypedData instances are allowed.
(also in other `il_<arch>.cc`)
Done
Consider passing a handle instead of raw object pointer as handle is created anyway. Passing a raw object pointer is unsafe, so we need to ensure that safepoints are not happening for the whole duration of the function call (e.g. caller needs to use `NoSafepointScope`).
(also for other functions)
The use in interpreter.cc is such that it requires no safepoints, so it seems reasonable to have this highlighted via explicit ObjectPtr passing..
How can we make sure raw pointer is not cached across a safepoint?
That's the purpose of this class is - to hold values while we can't allocate during running the interpreter.
Should we also test `IsCanonical()` bit to allow arbitrary compile-time constant objects, as mentioned in https://github.com/dart-lang/language/blob/main/working/333%20-%20shared%20memory%20multithreading/proposal.md?
Consider also doing this in the fast-path before calling runtime.
Done
Context::Handle(zone, Context::RawCast(object.ptr()));Nit: we can continue using the same handle via `Context::Cast(object)`.
Done
const auto& function = Function::Handle(closure.function());Alexander AprelevNit: `zone, `
Done
This would create a new object which involves a safepoint. We should avoid having raw object pointers (e.g. `object_ptr`) when safepoint can be triggered.
Done
FfiCallbackMetadata::CheckIfTriviallyImmutable(thread->zone(), value,Similarly to compiled code, we should guard this code with `FLAG_experimental_shared_data` and do all fast-path checks (e.g. test ImmutableBit and ClosureCid) before calling into a slower check which may create handles or throw an exception. Also, we should do a proper runtime call via `INVOKE_RUNTIME`. The same runtime entry can be used both for compiled code and the interpreter.
What are the reasons to do the checks in the runtime rather than call this CheckIfTriviallyImmutable? That function is reused between the compiled code and interpreter.
Going to runtime seems to be slower, less efficient.
`FLAG_experimental_shared_data && is_shared()` to match behavior of the compiled code.
- 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 added 4 comments![]( "Open in Gerrit")
ObjectPtr offendingValue_;Alexander AprelevHow can we make sure raw pointer is not cached across a safepoint?
That's the purpose of this class is - to hold values while we can't allocate during running the interpreter.
We need to either verify/assert that safepoint doesn't happen or use handles. I added another comment which points to the place where this object holds a raw pointer across safepoint.
FfiCallbackMetadata::CheckIfTriviallyImmutable(thread->zone(), value,Alexander AprelevSimilarly to compiled code, we should guard this code with `FLAG_experimental_shared_data` and do all fast-path checks (e.g. test ImmutableBit and ClosureCid) before calling into a slower check which may create handles or throw an exception. Also, we should do a proper runtime call via `INVOKE_RUNTIME`. The same runtime entry can be used both for compiled code and the interpreter.
What are the reasons to do the checks in the runtime rather than call this CheckIfTriviallyImmutable? That function is reused between the compiled code and interpreter.
Going to runtime seems to be slower, less efficient.
The interpreter implementation directly works with raw pointers and quite restrictive wrt what it can call/use. For example, interpreter should not allocate / leak handles and `CheckIfTriviallyImmutable` currently allocates them (so executing this check in a loop would leak memory). Also, interpreter should not reach safepoints without going to runtime, throw exceptions etc.
Another thing to note is that the overhead of calling a runtime entry from the interpreter is not higher compared to calling a runtime entry from the compiled / optimized code (assuming they are called under the same circumstances) - it could be even lower as we don't need to save all registers. So, in most cases, if a runtime entry created for compiled code works for the interpreter, then it is worth reusing it.
NoSafepointScope no_safepoint;This doesn't look correct as it covers `INVOKE_RUNTIME` below which would reach a safepoint.
CheckTriviallyImmutableResult result;Instance within `CheckTriviallyImmutableResult` is not going to be seen by GC when calling runtime entry `DRT_ThrowTriviallyImmutableViolationError` below.
Related details
- Alexander Aprelev
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 Aprelev (Gerrit)
Alexander Aprelev voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
4 comments
ObjectPtr offendingValue_;Alexander AprelevHow can we make sure raw pointer is not cached across a safepoint?
Alexander MarkovThat's the purpose of this class is - to hold values while we can't allocate during running the interpreter.
We need to either verify/assert that safepoint doesn't happen or use handles. I added another comment which points to the place where this object holds a raw pointer across safepoint.
Done
FfiCallbackMetadata::CheckIfTriviallyImmutable(thread->zone(), value,Alexander AprelevSimilarly to compiled code, we should guard this code with `FLAG_experimental_shared_data` and do all fast-path checks (e.g. test ImmutableBit and ClosureCid) before calling into a slower check which may create handles or throw an exception. Also, we should do a proper runtime call via `INVOKE_RUNTIME`. The same runtime entry can be used both for compiled code and the interpreter.
Alexander MarkovWhat are the reasons to do the checks in the runtime rather than call this CheckIfTriviallyImmutable? That function is reused between the compiled code and interpreter.
Going to runtime seems to be slower, less efficient.
The interpreter implementation directly works with raw pointers and quite restrictive wrt what it can call/use. For example, interpreter should not allocate / leak handles and `CheckIfTriviallyImmutable` currently allocates them (so executing this check in a loop would leak memory). Also, interpreter should not reach safepoints without going to runtime, throw exceptions etc.
Another thing to note is that the overhead of calling a runtime entry from the interpreter is not higher compared to calling a runtime entry from the compiled / optimized code (assuming they are called under the same circumstances) - it could be even lower as we don't need to save all registers. So, in most cases, if a runtime entry created for compiled code works for the interpreter, then it is worth reusing it.
Done
This doesn't look correct as it covers `INVOKE_RUNTIME` below which would reach a safepoint.
Done
Instance within `CheckTriviallyImmutableResult` is not going to be seen by GC when calling runtime entry `DRT_ThrowTriviallyImmutableViolationError` below.
- 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 and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
3 comments
Lgtm, assuming the rest of the comments are addressed.
ObjectPtr object_ptr,Alexander AprelevConsider passing a handle instead of raw object pointer as handle is created anyway. Passing a raw object pointer is unsafe, so we need to ensure that safepoints are not happening for the whole duration of the function call (e.g. caller needs to use `NoSafepointScope`).
(also for other functions)
The use in interpreter.cc is such that it requires no safepoints, so it seems reasonable to have this highlighted via explicit ObjectPtr passing..
Are there still any reasons to pass raw object pointers?
(value->untag()->IsImmutable() && value->IsClosure())))) {Nit: it is superfluous to check IsImmutable once again
Related details
- Alexander Aprelev
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 Aprelev (Gerrit)
Alexander Aprelev voted and added 6 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
6 comments
ObjectPtr object_ptr,Alexander AprelevConsider passing a handle instead of raw object pointer as handle is created anyway. Passing a raw object pointer is unsafe, so we need to ensure that safepoints are not happening for the whole duration of the function call (e.g. caller needs to use `NoSafepointScope`).
(also for other functions)
Alexander MarkovThe use in interpreter.cc is such that it requires no safepoints, so it seems reasonable to have this highlighted via explicit ObjectPtr passing..
Are there still any reasons to pass raw object pointers?
Done
Creating a handle for each tested object seems expensive. Consider rewriting this checking as a helper class with a few cached and reused handles.
Done
CheckIfTriviallyImmutableHelper(zone, context.At(i), visited, result);Alexander AprelevConsider using a worklist instead of recursion to avoid stack overflows in case of deep chains of closures. This would also make it easier to reuse handles.
Done
}Alexander AprelevConsider adding a separate `IsShareable` bit which we can set on closures to avoid repeated checking. Or maybe `IsImmutable` should be split to `IsDeeplyImmutable` and `IsShallowImmutable`, then we can set `IsShallowImmutable` on closures when they are created and `IsDeeplyImmutable` after they are checked.
Will address this in follow-up cl.
(value->untag()->IsImmutable() && value->IsClosure())))) {Nit: it is superfluous to check IsImmutable once again
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |