Skip to content

Commit 284b51f

Browse files
LeszekSwirskiV8 LUCI CQ
LeszekSwirski
authored and
V8 LUCI CQ
committed
Merged: [compiler] Ensure old SFIs stay alive for merging
When merging two scripts, we decide whether to use new or old SFIs in one pass over the script info list, and patch old SFIs (in bytecode constant pools) with new ones in another pass. If there is a GC that clears some of the weak script info list pointers between these two passes, then various assumptions break, leading to SFIs in the published result pointing at the wrong script. Make sure that any old SFIs that were encountered in the first pass stay alive for the second pass. Bug: 355575275 (cherry picked from commit b2b082c) Change-Id: Id15a6c6e67544bbaad561312ccf8a9816f8d98f2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5797603 Reviewed-by: Patrick Thier <[email protected]> Commit-Queue: Patrick Thier <[email protected]> Auto-Submit: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/branch-heads/12.8@{#32} Cr-Branched-From: 70cbb39-refs/heads/12.8.374@{#1} Cr-Branched-From: 451b63e-refs/heads/main@{#95151}
1 parent b4ac74b commit 284b51f

6 files changed

Lines changed: 242 additions & 6 deletions

File tree

‎src/codegen/background-merge-task.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ class V8_EXPORT_PRIVATE BackgroundMergeTask {
5656
return state_ == kPendingForegroundWork;
5757
}
5858

59+
static void ForceGCDuringNextMergeForTesting();
60+
5961
private:
6062
std::unique_ptr<PersistentHandles> persistent_handles_;
6163

‎src/codegen/compiler.cc‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2254,6 +2254,12 @@ void BackgroundMergeTask::SetUpOnMainThread(
22542254
cached_script_ = persistent_handles_->NewHandle(*cached_script);
22552255
}
22562256

2257+
static bool force_gc_during_next_merge_for_testing_ = false;
2258+
2259+
void BackgroundMergeTask::ForceGCDuringNextMergeForTesting() {
2260+
force_gc_during_next_merge_for_testing_ = true;
2261+
}
2262+
22572263
void BackgroundMergeTask::BeginMergeInBackground(
22582264
LocalIsolate* isolate, DirectHandle<Script> new_script) {
22592265
DCHECK_EQ(state_, kPendingBackgroundWork);
@@ -2295,20 +2301,22 @@ void BackgroundMergeTask::BeginMergeInBackground(
22952301
// this function literal.
22962302
Tagged<SharedFunctionInfo> old_sfi =
22972303
Cast<SharedFunctionInfo>(maybe_old_info.GetHeapObjectAssumeWeak());
2304+
// Make sure to allocate a persistent handle to the old sfi whether or
2305+
// not it or the new sfi have bytecode -- this is necessary to keep the
2306+
// old sfi reference in the old script list alive, so that pointers to
2307+
// the new sfi are redirected to the old sfi.
2308+
Handle<SharedFunctionInfo> old_sfi_handle =
2309+
local_heap->NewPersistentHandle(old_sfi);
22982310
if (old_sfi->HasBytecodeArray()) {
22992311
// Reset the old SFI's bytecode age so that it won't likely get
23002312
// flushed right away. This operation might be racing against
23012313
// concurrent modification by another thread, but such a race is not
23022314
// catastrophic.
23032315
old_sfi->set_age(0);
2304-
// Make sure we'll keep the old sfi alive so it'll be installed in the
2305-
// new bytecode by the forwarder.
2306-
local_heap->NewPersistentHandle(old_sfi);
23072316
} else if (new_sfi->HasBytecodeArray()) {
23082317
// Also push the old_sfi to make sure it stays alive / isn't replaced.
23092318
new_compiled_data_for_cached_sfis_.push_back(
2310-
{local_heap->NewPersistentHandle(old_sfi),
2311-
local_heap->NewPersistentHandle(new_sfi)});
2319+
{old_sfi_handle, local_heap->NewPersistentHandle(new_sfi)});
23122320
// Pick up existing scope infos from the old sfi. The new sfi will be
23132321
// copied over the old sfi later. This will ensure that we'll keep
23142322
// using the old sfis. This will also allow us check later whether new
@@ -2341,6 +2349,17 @@ void BackgroundMergeTask::BeginMergeInBackground(
23412349
}
23422350
}
23432351

2352+
// Since we are walking the script infos weak list both when figuring out
2353+
// which SFIs to merge above, and actually merging them below, make sure that
2354+
// a GC here which clears any dead weak refs or flushes any bytecode doesn't
2355+
// break anything.
2356+
if (V8_UNLIKELY(force_gc_during_next_merge_for_testing_)) {
2357+
// This GC is only synchronous on the main thread at the moment.
2358+
DCHECK(isolate->is_main_thread());
2359+
local_heap->AsHeap()->CollectAllAvailableGarbage(
2360+
GarbageCollectionReason::kTesting);
2361+
}
2362+
23442363
if (forwarder.HasAnythingToForward()) {
23452364
for (DirectHandle<SharedFunctionInfo> new_sfi : used_new_sfis_) {
23462365
forwarder.UpdateScopeInfo(*new_sfi);

‎src/codegen/compiler.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ class V8_EXPORT_PRIVATE BackgroundCompileTask {
647647

648648
// Contains all data which needs to be transmitted between threads for
649649
// background parsing and compiling and finalizing it on the main thread.
650-
struct ScriptStreamingData {
650+
struct V8_EXPORT_PRIVATE ScriptStreamingData {
651651
ScriptStreamingData(
652652
std::unique_ptr<ScriptCompiler::ExternalSourceStream> source_stream,
653653
ScriptCompiler::StreamedSource::Encoding encoding);

‎src/heap/factory.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2525,6 +2525,12 @@ Handle<FixedArray> Factory::CopyFixedArrayAndGrow(
25252525
return CopyArrayAndGrow(array, grow_by, allocation);
25262526
}
25272527

2528+
Handle<WeakFixedArray> Factory::CopyWeakFixedArray(
2529+
DirectHandle<WeakFixedArray> src) {
2530+
DCHECK(!IsTransitionArray(*src)); // Compacted by GC, this code doesn't work
2531+
return CopyArrayWithMap(src, weak_fixed_array_map(), AllocationType::kOld);
2532+
}
2533+
25282534
Handle<WeakFixedArray> Factory::CopyWeakFixedArrayAndGrow(
25292535
DirectHandle<WeakFixedArray> src, int grow_by) {
25302536
DCHECK(!IsTransitionArray(*src)); // Compacted by GC, this code doesn't work

‎src/heap/factory.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,8 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
568568
Handle<WeakArrayList> NewWeakArrayList(
569569
int capacity, AllocationType allocation = AllocationType::kYoung);
570570

571+
Handle<WeakFixedArray> CopyWeakFixedArray(DirectHandle<WeakFixedArray> array);
572+
571573
Handle<WeakFixedArray> CopyWeakFixedArrayAndGrow(
572574
DirectHandle<WeakFixedArray> array, int grow_by);
573575

‎test/unittests/compiler/compiler-unittest.cc‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "src/objects/allocation-site-inl.h"
2121
#include "src/objects/objects-inl.h"
2222
#include "src/objects/shared-function-info.h"
23+
#include "test/unittests/heap/heap-utils.h" // For ManualGCScope.
2324
#include "test/unittests/test-utils.h"
2425
#include "testing/gtest/include/gtest/gtest.h"
2526

@@ -928,5 +929,211 @@ TEST_F(CompilerTest, ProfilerEnabledDuringBackgroundCompile) {
928929
cpu_profiler->StopProfiling(profile);
929930
}
930931

932+
using BackgroundMergeTest = TestWithNativeContext;
933+
934+
// Tests that a GC during merge doesn't break the merge.
935+
TEST_F(BackgroundMergeTest, GCDuringMerge) {
936+
v8_flags.verify_code_merge = true;
937+
938+
HandleScope scope(isolate());
939+
const char* source =
940+
// f is compiled eagerly thanks to the IIFE hack.
941+
"f = (function f(x) {"
942+
" let b = x;"
943+
// f is compiled eagerly, so g's SFI exists. But, it is not compiled.
944+
" return function g() {"
945+
// g isn't compiled, so h's SFI does not exist.
946+
" return function h() {"
947+
" return b;"
948+
" }"
949+
" }"
950+
"})";
951+
Handle<String> source_string =
952+
isolate()
953+
->factory()
954+
->NewStringFromUtf8(base::CStrVector(source))
955+
.ToHandleChecked();
956+
957+
const int kTopLevelId = 0;
958+
const int kFId = 1;
959+
const int kGId = 2;
960+
const int kHId = 3;
961+
962+
// Compile the script once to warm up the compilation cache.
963+
Handle<JSFunction> old_g;
964+
IsCompiledScope old_g_bytecode_keepalive;
965+
{
966+
// Compile in a new handle scope, so that the script can die while the inner
967+
// functions stay alive.
968+
HandleScope scope(isolate());
969+
ScriptCompiler::CompilationDetails compilation_details;
970+
Handle<SharedFunctionInfo> top_level_sfi =
971+
Compiler::GetSharedFunctionInfoForScript(
972+
isolate(), source_string, ScriptDetails(),
973+
v8::ScriptCompiler::kNoCompileOptions,
974+
ScriptCompiler::kNoCacheNoReason, NOT_NATIVES_CODE,
975+
&compilation_details)
976+
.ToHandleChecked();
977+
978+
{
979+
Tagged<Script> script = Cast<Script>(top_level_sfi->script());
980+
CHECK(!script->infos()->get(kTopLevelId).IsCleared());
981+
CHECK(!script->infos()->get(kFId).IsCleared());
982+
CHECK(!script->infos()->get(kGId).IsCleared());
983+
// h in the script infos list was never initialized by the compilation, so
984+
// it's the default value for a WeakFixedArray, which is `undefined`.
985+
CHECK(Is<Undefined>(script->infos()->get(kHId)));
986+
}
987+
988+
Handle<JSFunction> top_level =
989+
Factory::JSFunctionBuilder{isolate(), top_level_sfi,
990+
isolate()->native_context()}
991+
.Build();
992+
993+
Handle<JSObject> global(isolate()->context()->global_object(), isolate());
994+
Execution::CallScript(isolate(), top_level, global,
995+
isolate()->factory()->empty_fixed_array())
996+
.Check();
997+
998+
Handle<JSFunction> f = Cast<JSFunction>(
999+
JSObject::GetProperty(isolate(), global, "f").ToHandleChecked());
1000+
1001+
CHECK(f->is_compiled(isolate()));
1002+
1003+
// Execute f to get g's SFI (no g bytecode yet)
1004+
Handle<JSFunction> g = Cast<JSFunction>(
1005+
Execution::Call(isolate(), f, global, 0, nullptr).ToHandleChecked());
1006+
CHECK(!g->is_compiled(isolate()));
1007+
1008+
// Execute g's SFI to initialize g's bytecode, and to get h.
1009+
Handle<JSFunction> h = Cast<JSFunction>(
1010+
Execution::Call(isolate(), g, global, 0, nullptr).ToHandleChecked());
1011+
CHECK(g->is_compiled(isolate()));
1012+
CHECK(!h->is_compiled(isolate()));
1013+
1014+
CHECK_EQ(top_level->shared()->function_literal_id(), kTopLevelId);
1015+
CHECK_EQ(f->shared()->function_literal_id(), kFId);
1016+
CHECK_EQ(g->shared()->function_literal_id(), kGId);
1017+
CHECK_EQ(h->shared()->function_literal_id(), kHId);
1018+
1019+
// Age everything so that subsequent GCs can pick it up if possible.
1020+
SharedFunctionInfo::EnsureOldForTesting(top_level->shared());
1021+
SharedFunctionInfo::EnsureOldForTesting(f->shared());
1022+
SharedFunctionInfo::EnsureOldForTesting(g->shared());
1023+
SharedFunctionInfo::EnsureOldForTesting(h->shared());
1024+
old_g = scope.CloseAndEscape(g);
1025+
}
1026+
Handle<Script> old_script(Cast<Script>(old_g->shared()->script()), isolate());
1027+
1028+
// Make sure bytecode is cleared...
1029+
for (int i = 0; i < 3; ++i) {
1030+
InvokeMajorGC();
1031+
}
1032+
CHECK(!old_g->is_compiled(isolate()));
1033+
1034+
// The top-level script should now be dead.
1035+
CHECK(old_script->infos()->get(kTopLevelId).IsCleared());
1036+
// f should still be alive by global reference.
1037+
CHECK(!old_script->infos()->get(kFId).IsCleared());
1038+
// g should be kept alive by our old_g handle.
1039+
CHECK(!old_script->infos()->get(kGId).IsCleared());
1040+
// h should be dead since g's bytecode was flushed.
1041+
CHECK(old_script->infos()->get(kHId).IsCleared());
1042+
1043+
// Copy the old_script_infos WeakFixedArray, so that we can inspect it after
1044+
// the merge mutated the original.
1045+
Handle<WeakFixedArray> unmutated_old_script_list =
1046+
isolate()->factory()->CopyWeakFixedArray(
1047+
direct_handle(old_script->infos(), isolate()));
1048+
1049+
{
1050+
HandleScope scope(isolate());
1051+
ScriptStreamingData streamed_source(
1052+
std::make_unique<DummySourceStream>(source),
1053+
v8::ScriptCompiler::StreamedSource::UTF8);
1054+
ScriptCompiler::CompilationDetails details;
1055+
streamed_source.task = std::make_unique<i::BackgroundCompileTask>(
1056+
&streamed_source, isolate(), ScriptType::kClassic,
1057+
ScriptCompiler::CompileOptions::kNoCompileOptions, &details);
1058+
1059+
streamed_source.task->RunOnMainThread(isolate());
1060+
1061+
Handle<SharedFunctionInfo> top_level_sfi;
1062+
{
1063+
// Use a manual GC scope, because we want to test a GC in a very precise
1064+
// spot in the merge.
1065+
ManualGCScope manual_gc(isolate());
1066+
// There's one more reference to the old_g -- clear it so that nothing is
1067+
// keeping it alive
1068+
CHECK(!old_script->infos()->get(kGId).IsCleared());
1069+
CHECK(!unmutated_old_script_list->get(kGId).IsCleared());
1070+
old_g.PatchValue({});
1071+
CHECK(!old_script->infos()->get(kFId).IsCleared());
1072+
1073+
BackgroundMergeTask::ForceGCDuringNextMergeForTesting();
1074+
1075+
top_level_sfi = streamed_source.task
1076+
->FinalizeScript(isolate(), source_string,
1077+
ScriptDetails(), old_script)
1078+
.ToHandleChecked();
1079+
CHECK(!old_script->infos()->get(kFId).IsCleared());
1080+
}
1081+
1082+
CHECK_EQ(top_level_sfi->script(), *old_script);
1083+
1084+
Handle<JSFunction> top_level =
1085+
Factory::JSFunctionBuilder{isolate(), top_level_sfi,
1086+
isolate()->native_context()}
1087+
.Build();
1088+
1089+
Handle<JSObject> global(isolate()->context()->global_object(), isolate());
1090+
1091+
Handle<JSFunction> f = Cast<JSFunction>(
1092+
JSObject::GetProperty(isolate(), global, "f").ToHandleChecked());
1093+
1094+
// f should normally be compiled (with the old shared function info but the
1095+
// new bytecode). However, the extra GCs in finalization might cause it to
1096+
// be flushed, so we can't guarantee this check.
1097+
// CHECK(f->is_compiled(isolate()));
1098+
1099+
// Execute f to get g's SFI (no g bytecode yet)
1100+
Handle<JSFunction> g = Cast<JSFunction>(
1101+
Execution::Call(isolate(), f, global, 0, nullptr).ToHandleChecked());
1102+
CHECK(!g->is_compiled(isolate()));
1103+
1104+
// Execute g's SFI to initialize g's bytecode, and to get h.
1105+
Handle<JSFunction> h = Cast<JSFunction>(
1106+
Execution::Call(isolate(), g, global, 0, nullptr).ToHandleChecked());
1107+
CHECK(g->is_compiled(isolate()));
1108+
CHECK(!h->is_compiled(isolate()));
1109+
1110+
CHECK_EQ(top_level->shared()->function_literal_id(), kTopLevelId);
1111+
CHECK_EQ(f->shared()->function_literal_id(), kFId);
1112+
CHECK_EQ(g->shared()->function_literal_id(), kGId);
1113+
CHECK_EQ(h->shared()->function_literal_id(), kHId);
1114+
1115+
CHECK_EQ(top_level->shared()->script(), *old_script);
1116+
CHECK_EQ(f->shared()->script(), *old_script);
1117+
CHECK_EQ(g->shared()->script(), *old_script);
1118+
CHECK_EQ(h->shared()->script(), *old_script);
1119+
1120+
CHECK_EQ(MakeWeak(top_level->shared()),
1121+
old_script->infos()->get(kTopLevelId));
1122+
CHECK_EQ(MakeWeak(f->shared()), old_script->infos()->get(kFId));
1123+
CHECK_EQ(MakeWeak(g->shared()), old_script->infos()->get(kGId));
1124+
CHECK_EQ(MakeWeak(h->shared()), old_script->infos()->get(kHId));
1125+
1126+
// The old top-level died, so we have a new one.
1127+
CHECK_NE(MakeWeak(top_level->shared()),
1128+
unmutated_old_script_list->get(kTopLevelId));
1129+
// The old f was still alive, so it's the same.
1130+
CHECK_EQ(MakeWeak(f->shared()), unmutated_old_script_list->get(kFId));
1131+
// The old g was still alive, so it's the same.
1132+
CHECK_EQ(MakeWeak(g->shared()), unmutated_old_script_list->get(kGId));
1133+
// The old h died, so it's different.
1134+
CHECK_NE(MakeWeak(h->shared()), unmutated_old_script_list->get(kHId));
1135+
}
1136+
}
1137+
9311138
} // namespace internal
9321139
} // namespace v8

0 commit comments

Comments
 (0)