From 09e0ad9a7438129f50dbafeef8f4084bd1c784ee Mon Sep 17 00:00:00 2001 From: Jakob Gruber Date: Thu, 15 Apr 2021 10:13:42 +0200 Subject: [PATCH] [compiler] Fix more concurrency issues exposed by tsan - FLAG_turbo_inline_js_wasm_calls data race - Map::instance_descriptors non-atomic concurrent loads - Skip one more cctest incompatible with stress_concurrent_inlining Bug: v8:7790,v8:11648,v8:11651 Change-Id: Ie4833373a1da34497f4cfe129254071d8a5772dd Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2827891 Reviewed-by: Santiago Aboy Solanes Reviewed-by: Georg Neis Commit-Queue: Jakob Gruber Cr-Commit-Position: refs/heads/master@{#73970} --- src/codegen/optimized-compilation-info.cc | 1 + src/codegen/optimized-compilation-info.h | 3 ++- src/compiler/access-info.cc | 4 ++-- src/compiler/js-call-reducer.cc | 5 ++++- src/compiler/js-heap-broker.cc | 4 ++-- src/compiler/pipeline.cc | 4 ++-- test/cctest/cctest.status | 1 + 7 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/codegen/optimized-compilation-info.cc b/src/codegen/optimized-compilation-info.cc index b94dc80959..0103de6ec2 100644 --- a/src/codegen/optimized-compilation-info.cc +++ b/src/codegen/optimized-compilation-info.cc @@ -84,6 +84,7 @@ bool OptimizedCompilationInfo::FlagGetIsValid(Flag flag) const { void OptimizedCompilationInfo::ConfigureFlags() { if (FLAG_untrusted_code_mitigations) set_untrusted_code_mitigations(); + if (FLAG_turbo_inline_js_wasm_calls) set_inline_js_wasm_calls(); switch (code_kind_) { case CodeKind::TURBOFAN: diff --git a/src/codegen/optimized-compilation-info.h b/src/codegen/optimized-compilation-info.h index d77650ab51..e4d62ee44a 100644 --- a/src/codegen/optimized-compilation-info.h +++ b/src/codegen/optimized-compilation-info.h @@ -71,7 +71,8 @@ class V8_EXPORT_PRIVATE OptimizedCompilationInfo final { V(TraceHeapBroker, trace_heap_broker, 17) \ V(WasmRuntimeExceptionSupport, wasm_runtime_exception_support, 18) \ V(ConcurrentInlining, concurrent_inlining, 19) \ - V(DiscardResultForTesting, discard_result_for_testing, 20) + V(DiscardResultForTesting, discard_result_for_testing, 20) \ + V(InlineJSWasmCalls, inline_js_wasm_calls, 21) enum Flag { #define DEF_ENUM(Camel, Lower, Bit) k##Camel = 1 << Bit, diff --git a/src/compiler/access-info.cc b/src/compiler/access-info.cc index 0c853ad298..5f1fd321c1 100644 --- a/src/compiler/access-info.cc +++ b/src/compiler/access-info.cc @@ -427,8 +427,8 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo( Handle receiver_map, Handle map, MaybeHandle holder, InternalIndex descriptor, AccessMode access_mode) const { DCHECK(descriptor.is_found()); - Handle descriptors(map->instance_descriptors(isolate()), - isolate()); + Handle descriptors = broker()->CanonicalPersistentHandle( + map->instance_descriptors(kAcquireLoad)); PropertyDetails const details = descriptors->GetDetails(descriptor); int index = descriptors->GetFieldIndex(descriptor); Representation details_representation = details.representation(); diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index a45f0ac3cd..1a56f79867 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -3442,8 +3442,8 @@ Reduction JSCallReducer::ReduceArraySome(Node* node, #if V8_ENABLE_WEBASSEMBLY namespace { + bool CanInlineJSToWasmCall(const wasm::FunctionSig* wasm_signature) { - DCHECK(FLAG_turbo_inline_js_wasm_calls); if (wasm_signature->return_count() > 1) { return false; } @@ -3460,10 +3460,13 @@ bool CanInlineJSToWasmCall(const wasm::FunctionSig* wasm_signature) { return true; } + } // namespace Reduction JSCallReducer::ReduceCallWasmFunction( Node* node, const SharedFunctionInfoRef& shared) { + DCHECK(flags() & kInlineJSToWasmCalls); + JSCallNode n(node); const CallParameters& p = n.Parameters(); diff --git a/src/compiler/js-heap-broker.cc b/src/compiler/js-heap-broker.cc index eb52b39cfc..cc2f09a832 100644 --- a/src/compiler/js-heap-broker.cc +++ b/src/compiler/js-heap-broker.cc @@ -2308,8 +2308,8 @@ void MapData::SerializeOwnDescriptor(JSHeapBroker* broker, // owner map if it is different than the current map. This is because // {instance_descriptors_} gets set on SerializeOwnDescriptor and otherwise // we risk the field owner having a null {instance_descriptors_}. - Handle descriptors(map->instance_descriptors(isolate), - isolate); + Handle descriptors = broker->CanonicalPersistentHandle( + map->instance_descriptors(kAcquireLoad)); if (descriptors->GetDetails(descriptor_index).location() == kField) { Handle owner(map->FindFieldOwner(isolate, descriptor_index), isolate); diff --git a/src/compiler/pipeline.cc b/src/compiler/pipeline.cc index 7f21e03709..7c9a3db1ca 100644 --- a/src/compiler/pipeline.cc +++ b/src/compiler/pipeline.cc @@ -1385,7 +1385,7 @@ struct InliningPhase { if (data->info()->bailout_on_uninitialized()) { call_reducer_flags |= JSCallReducer::kBailoutOnUninitialized; } - if (FLAG_turbo_inline_js_wasm_calls && data->info()->inlining()) { + if (data->info()->inline_js_wasm_calls() && data->info()->inlining()) { call_reducer_flags |= JSCallReducer::kInlineJSToWasmCalls; } JSCallReducer call_reducer(&graph_reducer, data->jsgraph(), data->broker(), @@ -2746,7 +2746,7 @@ bool PipelineImpl::OptimizeGraph(Linkage* linkage) { #if V8_ENABLE_WEBASSEMBLY if (data->has_js_wasm_calls()) { - DCHECK(FLAG_turbo_inline_js_wasm_calls); + DCHECK(data->info()->inline_js_wasm_calls()); Run(); RunPrintAndVerify(WasmInliningPhase::phase_name(), true); } diff --git a/test/cctest/cctest.status b/test/cctest/cctest.status index 35512fe01d..f838b6e397 100644 --- a/test/cctest/cctest.status +++ b/test/cctest/cctest.status @@ -710,6 +710,7 @@ # crbug.com/v8/11513: Flakily failing due to the additional compile task. 'test-heap/EnsureAllocationSiteDependentCodesProcessed': [PASS, FAIL], 'test-heap/LeakNativeContextViaMapProto': [PASS, FAIL], + 'test-heap/NewSpaceObjectsInOptimizedCode': [PASS, FAIL], 'test-heap/ObjectsInEagerlyDeoptimizedCodeAreWeak': [PASS, FAIL], }], # variant == stress_concurrent_inlining