Skip to content

Commit b2b082c

Browse files
LeszekSwirskiV8 LUCI CQ
LeszekSwirski
authored and
V8 LUCI CQ
committed
[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 Change-Id: I3589bc5169c8a17b32347a1b372773c9f54c6e12 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5786964 Commit-Queue: Leszek Swirski <[email protected]> Auto-Submit: Leszek Swirski <[email protected]> Reviewed-by: Patrick Thier <[email protected]> Cr-Commit-Position: refs/heads/main@{#95631}
1 parent a40596e commit b2b082c

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
@@ -2257,6 +2257,12 @@ void BackgroundMergeTask::SetUpOnMainThread(
22572257
cached_script_ = persistent_handles_->NewHandle(*cached_script);
22582258
}
22592259

2260+
static bool force_gc_during_next_merge_for_testing_ = false;
2261+
2262+
void BackgroundMergeTask::ForceGCDuringNextMergeForTesting() {
2263+
force_gc_during_next_merge_for_testing_ = true;
2264+
}
2265+
22602266
void BackgroundMergeTask::BeginMergeInBackground(
22612267
LocalIsolate* isolate, DirectHandle<Script> new_script) {
22622268
DCHECK_EQ(state_, kPendingBackgroundWork);
@@ -2298,20 +2304,22 @@ void BackgroundMergeTask::BeginMergeInBackground(
22982304
// this function literal.
22992305
Tagged<SharedFunctionInfo> old_sfi =
23002306
Cast<SharedFunctionInfo>(maybe_old_info.GetHeapObjectAssumeWeak());
2307+
// Make sure to allocate a persistent handle to the old sfi whether or
2308+
// not it or the new sfi have bytecode -- this is necessary to keep the
2309+
// old sfi reference in the old script list alive, so that pointers to
2310+
// the new sfi are redirected to the old sfi.
2311+
Handle<SharedFunctionInfo> old_sfi_handle =
2312+
local_heap->NewPersistentHandle(old_sfi);
23012313
if (old_sfi->HasBytecodeArray()) {
23022314
// Reset the old SFI's bytecode age so that it won't likely get
23032315
// flushed right away. This operation might be racing against
23042316
// concurrent modification by another thread, but such a race is not
23052317
// catastrophic.
23062318
old_sfi->set_age(0);
2307-
// Make sure we'll keep the old sfi alive so it'll be installed in the
2308-
// new bytecode by the forwarder.
2309-
local_heap->NewPersistentHandle(old_sfi);
23102319
} else if (new_sfi->HasBytecodeArray()) {
23112320
// Also push the old_sfi to make sure it stays alive / isn't replaced.
23122321
new_compiled_data_for_cached_sfis_.push_back(
2313-
{local_heap->NewPersistentHandle(old_sfi),
2314-
local_heap->NewPersistentHandle(new_sfi)});
2322+
{old_sfi_handle, local_heap->NewPersistentHandle(new_sfi)});
23152323
// Pick up existing scope infos from the old sfi. The new sfi will be
23162324
// copied over the old sfi later. This will ensure that we'll keep
23172325
// using the old sfis. This will also allow us check later whether new
@@ -2348,6 +2356,17 @@ void BackgroundMergeTask::BeginMergeInBackground(
23482356
}
23492357
}
23502358

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

‎src/codegen/compiler.h‎

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

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

‎src/heap/factory.cc‎

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

2610+
Handle<WeakFixedArray> Factory::CopyWeakFixedArray(
2611+
DirectHandle<WeakFixedArray> src) {
2612+
DCHECK(!IsTransitionArray(*src)); // Compacted by GC, this code doesn't work
2613+
return CopyArrayWithMap(src, weak_fixed_array_map(), AllocationType::kOld);
2614+
}
2615+
26102616
Handle<WeakFixedArray> Factory::CopyWeakFixedArrayAndGrow(
26112617
DirectHandle<WeakFixedArray> src, int grow_by) {
26122618
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
@@ -572,6 +572,8 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
572572
Handle<WeakArrayList> NewWeakArrayList(
573573
int capacity, AllocationType allocation = AllocationType::kYoung);
574574

575+
Handle<WeakFixedArray> CopyWeakFixedArray(DirectHandle<WeakFixedArray> array);
576+
575577
Handle<WeakFixedArray> CopyWeakFixedArrayAndGrow(
576578
DirectHandle<WeakFixedArray> array, int grow_by);
577579

‎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

@@ -935,5 +936,211 @@ TEST_F(CompilerTest, ProfilerEnabledDuringBackgroundCompile) {
935936
cpu_profiler->StopProfiling(profile);
936937
}
937938

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

0 commit comments

Comments
 (0)