[L] Change in dart/sdk[main]: [vm] Record stack for JIT TSAN.
0 views
Skip to first unread message
Ryan Macnak (Gerrit)
Oct 15, 2025, 7:03:30 PM (6 days ago) Oct 15
to Alexander Markov, Commit Queue, dart-vm-compil...@google.com, rev...@dartlang.org, vm-...@dartlang.org
Attention needed from Alexander Markov
Ryan Macnak added 1 comment![]( "Open in Gerrit")
File runtime/vm/compiler/backend/flow_graph_compiler.cc
Line 3115, Patchset 23:
__ CallRuntime(runtime_entry_, num_args, /*tsan_enter_exit=*/false);Ryan Macnak . resolved
Most uses of CallRuntime are like
```
EnterStubFrame
Push arg
Push arg
CallRuntime
Pop result
LeaveStubFrame
```
so the default is for CallRuntime to emit the tsan_func_entry/exit, but here we're calling the runtime directly from the function's frame that already did it.
Related details
Attention is currently required from:
- Alexander Markov
Submit Requirements:
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)
Oct 16, 2025, 1:14:13 PM (5 days ago) Oct 16
to Ryan Macnak, Alexander Markov, Commit Queue, dart-vm-compil...@google.com, rev...@dartlang.org, vm-...@dartlang.org
Attention needed from Ryan Macnak
Alexander Markov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Code-Review | +1 |
3 comments
File runtime/vm/compiler/backend/flow_graph.cc
Line 2737, Patchset 24 (Latest):
if (block->IsFunctionEntry() || block->IsNativeEntry()) {Alexander Markov . unresolved
`NativeEntryInstr` extends `FunctionEntryInstr`, so `IsFunctionEntry()` should cover both.
Line 2742, Patchset 24 (Latest):
for (ForwardInstructionIterator it(block); !it.Done(); it.Advance()) {Alexander Markov . unresolved
`DartReturn`, `TailCall` and `NativeReturn` instructions are always last in their basic blocks. So you can just check `block->last_instruction()` instead of iterating all instructions in the block.
File runtime/vm/compiler/stub_code_compiler_x64.cc
Line 1201, Patchset 24 (Latest):
__ cmpq(CALLEE_SAVED_TEMP, Immediate(0));Alexander Markov . unresolved
Nit: `sub` sets Z flag according to the result, so there is no need to `cmp` with 0. If you omit `cmp`, `j(NOT_ZERO, &loop)` would be more readable.
Related details
Attention is currently required from:
- Ryan Macnak
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Macnak (Gerrit)
Oct 20, 2025, 12:35:04 PM (yesterday) Oct 20
to Alexander Markov, Commit Queue, dart-vm-compil...@google.com, rev...@dartlang.org, vm-...@dartlang.org
Ryan Macnak voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ryan Macnak
| Commit-Queue | +1 |
3 comments
File runtime/vm/compiler/backend/flow_graph.cc
Line 2737, Patchset 24:
if (block->IsFunctionEntry() || block->IsNativeEntry()) {Alexander Markov . resolved
`NativeEntryInstr` extends `FunctionEntryInstr`, so `IsFunctionEntry()` should cover both.
Ryan Macnak
Done
Line 2742, Patchset 24:
for (ForwardInstructionIterator it(block); !it.Done(); it.Advance()) {Alexander Markov . resolved
`DartReturn`, `TailCall` and `NativeReturn` instructions are always last in their basic blocks. So you can just check `block->last_instruction()` instead of iterating all instructions in the block.
Ryan Macnak
Done
File runtime/vm/compiler/stub_code_compiler_x64.cc
Nit: `sub` sets Z flag according to the result, so there is no need to `cmp` with 0. If you omit `cmp`, `j(NOT_ZERO, &loop)` would be more readable.
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Macnak (Gerrit)
Oct 20, 2025, 1:49:11 PM (23 hours ago) Oct 20
to Alexander Markov, Commit Queue, dart-vm-compil...@google.com, rev...@dartlang.org, vm-...@dartlang.org
Ryan Macnak voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Oct 20, 2025, 1:49:24 PM (23 hours ago) Oct 20
to Ryan Macnak, Alexander Markov, dart-vm-compil...@google.com, rev...@dartlang.org, vm-...@dartlang.org
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
24 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: runtime/vm/compiler/backend/flow_graph.cc
Insertions: 9, Deletions: 11.
@@ -2734,21 +2734,19 @@
for (BlockIterator block_it = reverse_postorder_iterator(); !block_it.Done();
block_it.Advance()) {
BlockEntryInstr* block = block_it.Current();
- if (block->IsFunctionEntry() || block->IsNativeEntry()) {
+ if (block->IsFunctionEntry()) {
auto* tsan_entry = new (Z) TsanFuncEntryExitInstr(
TsanFuncEntryExitInstr::kEntry, block->source());
InsertAfter(block, tsan_entry, /*env=*/nullptr, kEffect);
}
- for (ForwardInstructionIterator it(block); !it.Done(); it.Advance()) {
- Instruction* current = it.Current();
- if ((current->IsDartReturn() &&
- !parsed_function_.function().IsAsyncFunction() &&
- !parsed_function_.function().IsAsyncGenerator()) ||
- current->IsTailCall() || current->IsNativeReturn()) {
- auto* tsan_exit = new (Z) TsanFuncEntryExitInstr(
- TsanFuncEntryExitInstr::kExit, current->source());
- InsertBefore(current, tsan_exit, /*env=*/nullptr, kEffect);
- }
+ Instruction* last = block->last_instruction();
+ if ((last->IsDartReturn() &&
+ !parsed_function_.function().IsAsyncFunction() &&
+ !parsed_function_.function().IsAsyncGenerator()) ||
+ last->IsTailCall() || last->IsNativeReturn()) {
+ auto* tsan_exit = new (Z)
+ TsanFuncEntryExitInstr(TsanFuncEntryExitInstr::kExit, last->source());
+ InsertBefore(last, tsan_exit, /*env=*/nullptr, kEffect);
}
}
}
```
```
The name of the file: runtime/vm/compiler/stub_code_compiler_x64.cc
Insertions: 1, Deletions: 2.
@@ -1198,8 +1198,7 @@
__ movq(CallingConventions::kArg1Reg, Immediate(42));
__ TsanFuncEntry(/*preserve_registers=*/false); // Unoptimized frames.
__ subq(CALLEE_SAVED_TEMP, Immediate(1));
- __ cmpq(CALLEE_SAVED_TEMP, Immediate(0));
- __ j(NOT_EQUAL, &loop);
+ __ j(NOT_ZERO, &loop);
}
if (kind == kLazyDeoptFromReturn) {
// Restore result into RBX.
```
Change information
Commit message:
[vm] Record stack for JIT TSAN.
The main interesting thing is that deopt can expand one optimized frame to multiple unoptimized frames, so deopt needs to call __tsan_func_entry enough times to rebalance the stack.
Also fixes stack balance for throw error slow paths for AOT.
TEST=tsan
Bug: https://github.com/dart-lang/sdk/issues/61352
Change-Id: I3fdc8481bc8db7a3aec0fa1938ac3e0e96ac13a5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/452943
Commit-Queue: Ryan Macnak <rma...@google.com>
Reviewed-by: Alexander Markov <alexm...@google.com>
Files:
- M runtime/platform/thread_sanitizer.h
- M runtime/tests/vm/dart/tsan/typed_data_race_test.dart
- M runtime/tests/vm/vm.status
- M runtime/vm/compiler/assembler/assembler_arm64.cc
- M runtime/vm/compiler/assembler/assembler_riscv.cc
- M runtime/vm/compiler/assembler/assembler_x64.cc
- M runtime/vm/compiler/backend/flow_graph.cc
- M runtime/vm/compiler/backend/flow_graph_compiler.cc
- M runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc
- M runtime/vm/compiler/backend/flow_graph_compiler_riscv.cc
- M runtime/vm/compiler/backend/flow_graph_compiler_x64.cc
- M runtime/vm/compiler/backend/il_arm64.cc
- M runtime/vm/compiler/backend/il_riscv.cc
- M runtime/vm/compiler/backend/il_x64.cc
- M runtime/vm/compiler/stub_code_compiler.cc
- M runtime/vm/compiler/stub_code_compiler_arm64.cc
- M runtime/vm/compiler/stub_code_compiler_riscv.cc
- M runtime/vm/compiler/stub_code_compiler_x64.cc
- M runtime/vm/deopt_instructions.cc
- M runtime/vm/deopt_instructions.h
- M runtime/vm/object_test.cc
- M runtime/vm/runtime_entry.cc
- M runtime/vm/runtime_entry_list.h
- M runtime/vm/simulator_riscv.cc
- M runtime/vm/tsan_symbolize.cc
- M runtime/vm/type_testing_stubs_test.cc
- M tools/bots/test_matrix.json
Change size: L
Delta: 27 files changed, 318 insertions(+), 196 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Alexander Markov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages