From 6edb7aec7c06bc15253031deb32a4e35e4f8face Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Thu, 26 Aug 2021 19:03:21 +0000 Subject: [PATCH] Bug 1724031 - Part 13: Fix race writing recover info r=tcampbell This fixes an intermittent TSAN failure due to accessing JSFunction off main thread. Differential Revision: https://phabricator.services.mozilla.com/D123729 --- js/src/jit/CodeGenerator.cpp | 2 +- js/src/jit/CompileInfo.h | 23 +++++++++++++++++------ js/src/jit/Recover.cpp | 8 +++++--- js/src/jit/WarpBuilder.cpp | 8 ++++---- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/js/src/jit/CodeGenerator.cpp b/js/src/jit/CodeGenerator.cpp index 8721a4faa805..c7aac7917ac0 100644 --- a/js/src/jit/CodeGenerator.cpp +++ b/js/src/jit/CodeGenerator.cpp @@ -7169,7 +7169,7 @@ void CodeGenerator::visitNewNamedLambdaObject(LNewNamedLambdaObject* lir) { using Fn = js::NamedLambdaObject* (*)(JSContext*, HandleFunction, gc::InitialHeap); OutOfLineCode* ool = oolCallVM( - lir, ArgList(ImmGCPtr(info.funMaybeLazy()), Imm32(gc::DefaultHeap)), + lir, ArgList(info.funMaybeLazy(), Imm32(gc::DefaultHeap)), StoreRegisterTo(objReg)); TemplateObject templateObject(lir->mir()->templateObj()); diff --git a/js/src/jit/CompileInfo.h b/js/src/jit/CompileInfo.h index 1f214e54b66b..3a58d0052b8f 100644 --- a/js/src/jit/CompileInfo.h +++ b/js/src/jit/CompileInfo.h @@ -15,6 +15,7 @@ #include "jit/CompileWrappers.h" // CompileRuntime #include "jit/JitFrames.h" // MinJITStackSize +#include "jit/shared/Assembler-shared.h" #include "js/TypeDecls.h" // jsbytecode #include "vm/BindingKind.h" // BindingLocation #include "vm/BytecodeUtil.h" // JSOp @@ -55,6 +56,13 @@ inline unsigned CountArgSlots(JSScript* script, JSFunction* fun) { return StartArgSlot(script) + (fun ? fun->nargs() + 1 : 0); } +inline unsigned CountArgSlots(JSScript* script, bool hasFun, + uint32_t funArgCount) { + // Same as the previous function, for use when the JSFunction is not + // available. + return StartArgSlot(script) + (hasFun ? funArgCount + 1 : 0); +} + // Contains information about the compilation source for IR being generated. class CompileInfo { public: @@ -150,11 +158,14 @@ class CompileInfo { JSScript* script() const { return script_; } bool compilingWasm() const { return script() == nullptr; } - JSFunction* funMaybeLazy() const { return fun_; } ModuleObject* module() const { return script_->module(); } jsbytecode* osrPc() const { return osrPc_; } InlineScriptTree* inlineScriptTree() const { return inlineScriptTree_; } + // It's not safe to access the JSFunction off main thread. + bool hasFunMaybeLazy() const { return fun_; } + ImmGCPtr funMaybeLazy() const { return ImmGCPtr(fun_); } + const char* filename() const { return script_->filename(); } unsigned lineno() const { return script_->lineno(); } @@ -185,7 +196,7 @@ class CompileInfo { return 2; } uint32_t thisSlot() const { - MOZ_ASSERT(funMaybeLazy()); + MOZ_ASSERT(hasFunMaybeLazy()); MOZ_ASSERT(nimplicit_ > 0); return nimplicit_ - 1; } @@ -209,7 +220,7 @@ class CompileInfo { uint32_t stackSlot(uint32_t i) const { return firstStackSlot() + i; } uint32_t totalSlots() const { - MOZ_ASSERT(script() && funMaybeLazy()); + MOZ_ASSERT(script() && hasFunMaybeLazy()); return nimplicit() + nargs() + nlocals(); } @@ -253,7 +264,7 @@ class CompileInfo { // Formal argument slots. if (slot >= firstArgSlot()) { - MOZ_ASSERT(funMaybeLazy()); + MOZ_ASSERT(hasFunMaybeLazy()); MOZ_ASSERT(slot - firstArgSlot() < nargs()); // Preserve formal arguments if they might be read when creating a rest or @@ -266,7 +277,7 @@ class CompileInfo { } // |this| slot is observable but it can be recovered. - if (funMaybeLazy() && slot == thisSlot()) { + if (hasFunMaybeLazy() && slot == thisSlot()) { return SlotObservableKind::ObservableRecoverable; } @@ -289,7 +300,7 @@ class CompileInfo { // The arguments object is observable. If it does not escape, it can // be recovered. if (needsArgsObj() && slot == argsObjSlot()) { - MOZ_ASSERT(funMaybeLazy()); + MOZ_ASSERT(hasFunMaybeLazy()); return SlotObservableKind::ObservableRecoverable; } diff --git a/js/src/jit/Recover.cpp b/js/src/jit/Recover.cpp index 8925f653fe31..ef0c4330cb93 100644 --- a/js/src/jit/Recover.cpp +++ b/js/src/jit/Recover.cpp @@ -60,7 +60,8 @@ bool MResumePoint::writeRecoverData(CompactBufferWriter& writer) const { writer.writeUnsigned(uint32_t(RInstruction::Recover_ResumePoint)); MBasicBlock* bb = block(); - JSFunction* fun = bb->info().funMaybeLazy(); + bool hasFun = bb->info().hasFunMaybeLazy(); + uint32_t nargs = bb->info().nargs(); JSScript* script = bb->info().script(); uint32_t exprStack = stackDepth() - bb->info().ninvoke(); @@ -101,16 +102,17 @@ bool MResumePoint::writeRecoverData(CompactBufferWriter& writer) const { } #endif + uint32_t formalArgs = CountArgSlots(script, hasFun, nargs); + // Test if we honor the maximum of arguments at all times. This is a sanity // check and not an algorithm limit. So check might be a bit too loose. +4 // to account for scope chain, return value, this value and maybe // arguments_object. - MOZ_ASSERT(CountArgSlots(script, fun) < SNAPSHOT_MAX_NARGS + 4); + MOZ_ASSERT(formalArgs < SNAPSHOT_MAX_NARGS + 4); #ifdef JS_JITSPEW uint32_t implicit = StartArgSlot(script); #endif - uint32_t formalArgs = CountArgSlots(script, fun); uint32_t nallocs = formalArgs + script->nfixed() + exprStack; JitSpew(JitSpew_IonSnapshots, diff --git a/js/src/jit/WarpBuilder.cpp b/js/src/jit/WarpBuilder.cpp index 1e3da1361196..b1bdc46757e8 100644 --- a/js/src/jit/WarpBuilder.cpp +++ b/js/src/jit/WarpBuilder.cpp @@ -185,7 +185,7 @@ bool WarpBuilder::startNewOsrPreHeaderBlock(BytecodeLocation loopHead) { osrBlock->initSlot(info().argsObjSlot(), argsObj); } - if (info().funMaybeLazy()) { + if (info().hasFunMaybeLazy()) { // Initialize |this| parameter. MParameter* thisv = MParameter::New(alloc(), MParameter::THIS_SLOT); osrBlock->add(thisv); @@ -438,7 +438,7 @@ bool WarpBuilder::buildPrologue() { return false; } - if (info().funMaybeLazy()) { + if (info().hasFunMaybeLazy()) { // Initialize |this|. MParameter* param = MParameter::New(alloc(), MParameter::THIS_SLOT); current->add(param); @@ -1873,7 +1873,7 @@ bool WarpBuilder::build_SuperCall(BytecodeLocation loc) { } bool WarpBuilder::build_FunctionThis(BytecodeLocation loc) { - MOZ_ASSERT(info().funMaybeLazy()); + MOZ_ASSERT(info().hasFunMaybeLazy()); if (script_->strict()) { // No need to wrap primitive |this| in strict mode. @@ -2631,7 +2631,7 @@ bool WarpBuilder::build_Instanceof(BytecodeLocation loc) { bool WarpBuilder::build_NewTarget(BytecodeLocation loc) { MOZ_ASSERT(script_->isFunction()); - MOZ_ASSERT(info().funMaybeLazy()); + MOZ_ASSERT(info().hasFunMaybeLazy()); if (scriptSnapshot()->isArrowFunction()) { MDefinition* callee = getCallee();