From 70bb42e910ff431b1a78e2bbf84e23344a51b05c Mon Sep 17 00:00:00 2001 From: Tzvetan Mikov Date: Wed, 11 Feb 2026 20:18:27 -0800 Subject: [PATCH] Fix class variable caching bugs when class is in finally block Summary: Finally blocks cause code to be emitted twice: once for normal flow and once for exception handling. This exposed a class of bugs where internal Variables created during class compilation were not cached. On recompilation, new Variables were created, but cached functions (via findCompiledEntity) still referenced the original Variables, causing crashes or incorrect behavior. Fixed issues in genLegacyClassLike: - classVar, clsPrototypeVar: new Variables created each compilation, but cached initializer functions referenced old ones. Caused assertion failure: "PutOwn requires object operand". - instElemInitFuncVar: same issue for instance element initializers. - Computed field key variables: same issue for computed properties. Fixed issues in emitPrivateNameDeclarations: - staticBrand, instanceBrand: Variables for private method brands were local variables, not cached. Additionally, brand stores were only emitted when creating the Variable (inside getPrivateBrand), not on recompilation. This caused "undefined is not a function" errors when calling private methods in classes defined in finally blocks. The fix introduces LegacyClassInternalVars to cache all internal Variables created during legacy class compilation, keyed by the class AST node. When the same class is compiled multiple times, existing Variables are reused. For private brands, the stores are now emitted at the end of emitPrivateNameDeclarations unconditionally, ensuring they exist in all code paths. Co-Authored-By: Claude Opus 4.5 Reviewed By: fbmal7 Differential Revision: D90690547 fbshipit-source-id: abe039ecb6913fe8534faf4a7544a84745991182 (cherry picked from commit 1e94fbe0ebb46d676ff656287e17c58839765e73) --- lib/IRGen/ESTreeIRGen-legacy-class.cpp | 158 ++++++++++++++++--------- lib/IRGen/ESTreeIRGen.h | 29 +++++ test/IRGen/class-static-in-finally.js | 95 +++++++++++++++ test/IRGen/private-properties.js | 4 +- test/IRGen/regress-class-recompiled.js | 8 +- test/hermes/class-in-finally.js | 112 ++++++++++++++++++ 6 files changed, 342 insertions(+), 64 deletions(-) create mode 100644 test/IRGen/class-static-in-finally.js create mode 100644 test/hermes/class-in-finally.js diff --git a/lib/IRGen/ESTreeIRGen-legacy-class.cpp b/lib/IRGen/ESTreeIRGen-legacy-class.cpp index 3b8dc0ca5e5..f480a768a81 100644 --- a/lib/IRGen/ESTreeIRGen-legacy-class.cpp +++ b/lib/IRGen/ESTreeIRGen-legacy-class.cpp @@ -62,43 +62,35 @@ namespace irgen { /// inspecting the kind of a `Decl`, it's possible to know which kind of element /// in the side table the customData field of the `Decl` points to. void ESTreeIRGen::emitPrivateNameDeclarations( + ESTree::ClassLikeNode *classNode, sema::LexicalScope *scope, Identifier className) { - Variable *staticBrand = nullptr; - Variable *instanceBrand = nullptr; + // Get the cached variables for this class. Brand Variables must be cached + // for recompilation (e.g., finally blocks) to ensure consistency. + auto &cachedVars = legacyClassVars_[classNode]; + /// Lazily create and return the correct private brand for the given static - /// level. - auto getPrivateBrand = - [this, &className, &staticBrand, &instanceBrand](bool isStatic) { - if (isStatic) { - if (!staticBrand) { - staticBrand = Builder.createVariable( - curFunction()->curScope()->getVariableScope(), - Twine("?static_brand_") + className.str(), - Type::createPrivateName(), - /* hidden */ true); - Builder.createStoreFrameInst( - curFunction()->curScope(), - Builder.createCreatePrivateNameInst( - Builder.getLiteralString(className)), - staticBrand); - } - return staticBrand; - } - if (!instanceBrand) { - instanceBrand = Builder.createVariable( - curFunction()->curScope()->getVariableScope(), - Twine("?instance_brand_") + className.str(), - Type::createPrivateName(), - /* hidden */ true); - Builder.createStoreFrameInst( - curFunction()->curScope(), - Builder.createCreatePrivateNameInst( - Builder.getLiteralString(className)), - instanceBrand); - } - return instanceBrand; - }; + /// level. Brands are cached to ensure consistency across recompilations. + auto getPrivateBrand = [this, &className, &cachedVars](bool isStatic) { + if (isStatic) { + if (!cachedVars.staticBrand) { + cachedVars.staticBrand = Builder.createVariable( + curFunction()->curScope()->getVariableScope(), + Twine("?static_brand_") + className.str(), + Type::createPrivateName(), + /* hidden */ true); + } + return cachedVars.staticBrand; + } + if (!cachedVars.instanceBrand) { + cachedVars.instanceBrand = Builder.createVariable( + curFunction()->curScope()->getVariableScope(), + Twine("?instance_brand_") + className.str(), + Type::createPrivateName(), + /* hidden */ true); + } + return cachedVars.instanceBrand; + }; for (sema::Decl *decl : scope->decls) { if (!sema::Decl::isKindPrivateName(decl->kind)) continue; @@ -207,6 +199,25 @@ void ESTreeIRGen::emitPrivateNameDeclarations( continue; } } + + // Emit brand stores. These must be emitted on every compilation (including + // recompiles) because the store instruction must exist in all code paths. + // We check the cached brands rather than the flags because on recompile + // getPrivateBrand may not be called (decl->customData is already set). + if (cachedVars.staticBrand) { + Builder.createStoreFrameInst( + curFunction()->curScope(), + Builder.createCreatePrivateNameInst( + Builder.getLiteralString(className)), + cachedVars.staticBrand); + } + if (cachedVars.instanceBrand) { + Builder.createStoreFrameInst( + curFunction()->curScope(), + Builder.createCreatePrivateNameInst( + Builder.getLiteralString(className)), + cachedVars.instanceBrand); + } } void ESTreeIRGen::emitPrivateBrandCheck(Value *from, Value *brandVal) { @@ -359,21 +370,37 @@ CreateClassInst *ESTreeIRGen::genLegacyClassLike( Value *superCls = superClassNode ? genExpression(superClassNode) : Builder.getEmptySentinel(); - emitPrivateNameDeclarations(classNode->getScope(), className); + emitPrivateNameDeclarations(classNode, classNode->getScope(), className); auto *curScope = curFunction()->curScope(); auto *curVarScope = curScope->getVariableScope(); - // Holds the .prototype of the class. - Variable *clsPrototypeVar = Builder.createVariable( - curScope->getVariableScope(), - Builder.createIdentifier("?" + className.str() + ".prototype"), - Type::createObject(), - true); - // Holds the class - Variable *classVar = Builder.createVariable( - curVarScope, - Builder.createIdentifier("?" + className.str()), - Type::createObject(), - true); + + // Check if we already created internal variables for this class (e.g., when + // recompiling a finally block). If so, reuse them to ensure consistency. + // Note: emitPrivateNameDeclarations may have already created an entry for + // caching brand variables, so we check classVar specifically rather than + // whether the entry exists. + Variable *clsPrototypeVar; + Variable *classVar; + auto &cachedVars = legacyClassVars_[classNode]; + if (cachedVars.classVar) { + clsPrototypeVar = cachedVars.prototypeVar; + classVar = cachedVars.classVar; + } else { + // Holds the .prototype of the class. + clsPrototypeVar = Builder.createVariable( + curScope->getVariableScope(), + Builder.createIdentifier("?" + className.str() + ".prototype"), + Type::createObject(), + true); + // Holds the class + classVar = Builder.createVariable( + curVarScope, + Builder.createIdentifier("?" + className.str()), + Type::createObject(), + true); + cachedVars.classVar = classVar; + cachedVars.prototypeVar = clsPrototypeVar; + } std::shared_ptr savedClsCtx = curFunction()->legacyClassContext; @@ -391,11 +418,18 @@ CreateClassInst *ESTreeIRGen::genLegacyClassLike( CreateFunctionInst *createInstElemInitFunc = Builder.createCreateFunctionInst( curFunction()->curScope(), instElemInitFunc); - Variable *instElemInitFuncVar = Builder.createVariable( - curFunction()->curScope()->getVariableScope(), - (llvh::Twine(""), - Type::createObject(), - /* hidden */ true); + // Reuse cached variable or create a new one. + Variable *instElemInitFuncVar; + if (cachedVars.instElemInitFuncVar) { + instElemInitFuncVar = cachedVars.instElemInitFuncVar; + } else { + instElemInitFuncVar = Builder.createVariable( + curFunction()->curScope()->getVariableScope(), + (llvh::Twine(""), + Type::createObject(), + /* hidden */ true); + cachedVars.instElemInitFuncVar = instElemInitFuncVar; + } Builder.createStoreFrameInst( curFunction()->curScope(), createInstElemInitFunc, instElemInitFuncVar); curFunction()->legacyClassContext->instElemInitFuncVar = @@ -555,12 +589,20 @@ CreateClassInst *ESTreeIRGen::genLegacyClassLike( } else if ( auto *prop = llvh::dyn_cast(&classElement)) { if (prop->_computed) { - Variable *fieldKeyVar = Builder.createVariable( - curVarScope, - Builder.createIdentifier( - className.str() + "_computed_key_" + Twine(computedKeyIdx++)), - Type::createAnyType(), - /* hidden */ true); + // Reuse cached variable or create a new one. + auto cachedKeyIt = cachedVars.computedFieldKeys.find(prop); + Variable *fieldKeyVar; + if (cachedKeyIt != cachedVars.computedFieldKeys.end()) { + fieldKeyVar = cachedKeyIt->second; + } else { + fieldKeyVar = Builder.createVariable( + curVarScope, + Builder.createIdentifier( + className.str() + "_computed_key_" + Twine(computedKeyIdx++)), + Type::createAnyType(), + /* hidden */ true); + cachedVars.computedFieldKeys[prop] = fieldKeyVar; + } curFunction()->legacyClassContext->classComputedFieldKeys[prop] = fieldKeyVar; Builder.createStoreFrameInst( diff --git a/lib/IRGen/ESTreeIRGen.h b/lib/IRGen/ESTreeIRGen.h index 77ab28d21cf..2f1b25d7ff3 100644 --- a/lib/IRGen/ESTreeIRGen.h +++ b/lib/IRGen/ESTreeIRGen.h @@ -612,6 +612,33 @@ class ESTreeIRGen { /// guaranteed to be, if any. llvh::DenseMap finalMethods_{}; + /// Internal variables created during legacy class compilation. + /// These need to be cached so they can be reused when the same class + /// is compiled multiple times (e.g., in a finally block). + struct LegacyClassVars { + /// Variable holding the class constructor object. + Variable *classVar{nullptr}; + /// Variable holding the class prototype object. + Variable *prototypeVar{nullptr}; + /// Variable holding the instance elements initializer closure. + /// May be nullptr if there are no instance elements. + Variable *instElemInitFuncVar{nullptr}; + /// Variable holding the static private brand (for static private methods). + Variable *staticBrand{nullptr}; + /// Variable holding the instance private brand (for instance private + /// methods). + Variable *instanceBrand{nullptr}; + /// Map from computed property nodes to their key variables. + llvh::DenseMap computedFieldKeys{}; + + LegacyClassVars() = default; + LegacyClassVars(Variable *cls, Variable *proto) + : classVar(cls), prototypeVar(proto) {} + }; + + /// Map from a class AST node to its internal variables. + llvh::DenseMap legacyClassVars_{}; + /// A queue of "entities" that have been forward declared and mapped in /// \c compiledEntities_, but need to be actually compiled. This makes the /// compiler non-recursive. @@ -1327,8 +1354,10 @@ class ESTreeIRGen { /// Declare all private names in the class' scope. This will setup the /// customData for all private name decls to point to the required state /// needed for IRGen to handle usages involving that private name. + /// \param classNode The class AST node, used for caching internal variables. /// \param scope The lexical scope, can't be null. void emitPrivateNameDeclarations( + ESTree::ClassLikeNode *classNode, sema::LexicalScope *scope, Identifier className); diff --git a/test/IRGen/class-static-in-finally.js b/test/IRGen/class-static-in-finally.js new file mode 100644 index 00000000000..127d5005ed9 --- /dev/null +++ b/test/IRGen/class-static-in-finally.js @@ -0,0 +1,95 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// RUN: %hermes -O0 %s -dump-ir | %FileCheckOrRegen --match-full-lines %s + +// Regression test: class with static property in finally block. +// The finally block causes code to be emitted twice. Previously, +// internal class variables were created fresh on each compilation, +// but initializer functions were cached. This caused the static +// initializer to load from an uninitialized variable, crashing at +// runtime with "PutOwn requires object operand". + +function main() { + try { + throw 1; + } finally { + class C5 { + static g = "-10238"; + } + } +} +main(); + +// Auto-generated content below. Please do not modify manually. + +// CHECK:scope %VS0 [] + +// CHECK:function global(): any +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = CreateScopeInst (:environment) %VS0: any, empty: any +// CHECK-NEXT: DeclareGlobalVarInst "main": string +// CHECK-NEXT: %2 = CreateFunctionInst (:object) %0: environment, %VS0: any, %main(): functionCode +// CHECK-NEXT: StorePropertyLooseInst %2: object, globalObject: object, "main": string +// CHECK-NEXT: %4 = AllocStackInst (:any) $?anon_0_ret: any +// CHECK-NEXT: StoreStackInst undefined: undefined, %4: any +// CHECK-NEXT: %6 = LoadPropertyInst (:any) globalObject: object, "main": string +// CHECK-NEXT: %7 = CallInst (:any) %6: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined +// CHECK-NEXT: StoreStackInst %7: any, %4: any +// CHECK-NEXT: %9 = LoadStackInst (:any) %4: any +// CHECK-NEXT: ReturnInst %9: any +// CHECK-NEXT:function_end + +// CHECK:scope %VS1 [C5: any, C5#1: any, ?C5.prototype: object, ?C5: object] + +// CHECK:function main(): any +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = GetParentScopeInst (:environment) %VS0: any, %parentScope: environment +// CHECK-NEXT: %1 = CreateScopeInst (:environment) %VS1: any, %0: environment +// CHECK-NEXT: TryStartInst %BB1, %BB2 +// CHECK-NEXT:%BB1: +// CHECK-NEXT: %3 = CatchInst (:any) +// CHECK-NEXT: StoreFrameInst %1: environment, undefined: undefined, [%VS1.C5]: any +// CHECK-NEXT: StoreFrameInst %1: environment, undefined: undefined, [%VS1.C5#1]: any +// CHECK-NEXT: %6 = AllocStackInst (:object) $?anon_1_clsPrototype: any +// CHECK-NEXT: %7 = CreateClassInst (:object) %1: environment, %VS1: any, %C5(): functionCode, empty: any, %6: object +// CHECK-NEXT: %8 = LoadStackInst (:object) %6: object +// CHECK-NEXT: StoreFrameInst %1: environment, %7: object, [%VS1.C5#1]: any +// CHECK-NEXT: StoreFrameInst %1: environment, %7: object, [%VS1.?C5]: object +// CHECK-NEXT: StoreFrameInst %1: environment, %8: object, [%VS1.?C5.prototype]: object +// CHECK-NEXT: %12 = CreateFunctionInst (:object) %1: environment, %VS1: any, %(): functionCode +// CHECK-NEXT: %13 = CallInst (:any) %12: object, %(): functionCode, true: boolean, %1: environment, undefined: undefined, %7: object +// CHECK-NEXT: StoreFrameInst %1: environment, %7: object, [%VS1.C5]: any +// CHECK-NEXT: ThrowInst %3: any +// CHECK-NEXT:%BB2: +// CHECK-NEXT: ThrowInst 1: number, %BB1 +// CHECK-NEXT:function_end + +// CHECK:scope %VS2 [this: object] + +// CHECK:base constructor C5(): object +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = GetParentScopeInst (:environment) %VS1: any, %parentScope: environment +// CHECK-NEXT: %1 = CreateScopeInst (:environment) %VS2: any, %0: environment +// CHECK-NEXT: %2 = GetNewTargetInst (:object) %new.target: object +// CHECK-NEXT: %3 = LoadPropertyInst (:any) %2: object, "prototype": string +// CHECK-NEXT: %4 = AllocObjectLiteralInst (:object) %3: any +// CHECK-NEXT: StoreFrameInst %1: environment, %4: object, [%VS2.this]: object +// CHECK-NEXT: %6 = LoadFrameInst (:object) %1: environment, [%VS2.this]: object +// CHECK-NEXT: ReturnInst %6: object +// CHECK-NEXT:function_end + +// CHECK:scope %VS3 [] + +// CHECK:function (): undefined +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = GetParentScopeInst (:environment) %VS1: any, %parentScope: environment +// CHECK-NEXT: %1 = CreateScopeInst (:environment) %VS3: any, %0: environment +// CHECK-NEXT: %2 = LoadFrameInst (:object) %0: environment, [%VS1.?C5]: object +// CHECK-NEXT: DefineOwnPropertyInst "-10238": string, %2: object, "g": string, true: boolean +// CHECK-NEXT: ReturnInst undefined: undefined +// CHECK-NEXT:function_end diff --git a/test/IRGen/private-properties.js b/test/IRGen/private-properties.js index cbdbfb2aaa2..d79440d7ab1 100644 --- a/test/IRGen/private-properties.js +++ b/test/IRGen/private-properties.js @@ -91,9 +91,9 @@ function simpleMethods() { // CHECK-NEXT: StoreFrameInst %1: environment, undefined: undefined, [%VS2.B]: any // CHECK-NEXT: StoreFrameInst %1: environment, undefined: undefined, [%VS2.B#1]: any // CHECK-NEXT: %4 = CreatePrivateNameInst (:privateName) "B": string -// CHECK-NEXT: StoreFrameInst %1: environment, %4: privateName, [%VS2.?instance_brand_B]: privateName +// CHECK-NEXT: StoreFrameInst %1: environment, %4: privateName, [%VS2.?static_brand_B]: privateName // CHECK-NEXT: %6 = CreatePrivateNameInst (:privateName) "B": string -// CHECK-NEXT: StoreFrameInst %1: environment, %6: privateName, [%VS2.?static_brand_B]: privateName +// CHECK-NEXT: StoreFrameInst %1: environment, %6: privateName, [%VS2.?instance_brand_B]: privateName // CHECK-NEXT: %8 = CreateFunctionInst (:object) %1: environment, %VS2: any, %(): functionCode // CHECK-NEXT: StoreFrameInst %1: environment, %8: object, [%VS2.]: object // CHECK-NEXT: %10 = AllocStackInst (:object) $?anon_0_clsPrototype: any diff --git a/test/IRGen/regress-class-recompiled.js b/test/IRGen/regress-class-recompiled.js index 5b055dfca91..2a9287eee73 100644 --- a/test/IRGen/regress-class-recompiled.js +++ b/test/IRGen/regress-class-recompiled.js @@ -34,7 +34,7 @@ // CHECK-NEXT: ReturnInst %6: any // CHECK-NEXT:function_end -// CHECK:scope %VS1 [e: any, C1: any, C1#1: any, ?C1.prototype: object, ?C1: object, : object, ?C1.prototype#1: object, ?C1#1: object, #1: object] +// CHECK:scope %VS1 [e: any, C1: any, C1#1: any, ?C1.prototype: object, ?C1: object, : object] // CHECK:function ""(): any // CHECK-NEXT:%BB0: @@ -46,13 +46,13 @@ // CHECK-NEXT: StoreFrameInst %1: environment, undefined: undefined, [%VS1.C1]: any // CHECK-NEXT: StoreFrameInst %1: environment, undefined: undefined, [%VS1.C1#1]: any // CHECK-NEXT: %6 = CreateFunctionInst (:object) %1: environment, %VS1: any, %(): functionCode -// CHECK-NEXT: StoreFrameInst %1: environment, %6: object, [%VS1.#1]: object +// CHECK-NEXT: StoreFrameInst %1: environment, %6: object, [%VS1.]: object // CHECK-NEXT: %8 = AllocStackInst (:object) $?anon_1_clsPrototype: any // CHECK-NEXT: %9 = CreateClassInst (:object) %1: environment, %VS1: any, %C1(): functionCode, empty: any, %8: object // CHECK-NEXT: %10 = LoadStackInst (:object) %8: object // CHECK-NEXT: StoreFrameInst %1: environment, %9: object, [%VS1.C1#1]: any -// CHECK-NEXT: StoreFrameInst %1: environment, %9: object, [%VS1.?C1#1]: object -// CHECK-NEXT: StoreFrameInst %1: environment, %10: object, [%VS1.?C1.prototype#1]: object +// CHECK-NEXT: StoreFrameInst %1: environment, %9: object, [%VS1.?C1]: object +// CHECK-NEXT: StoreFrameInst %1: environment, %10: object, [%VS1.?C1.prototype]: object // CHECK-NEXT: %14 = CreateFunctionInst (:object) %1: environment, %VS1: any, %(): functionCode // CHECK-NEXT: %15 = CallInst (:any) %14: object, %(): functionCode, true: boolean, %1: environment, undefined: undefined, %9: object // CHECK-NEXT: StoreFrameInst %1: environment, %9: object, [%VS1.C1]: any diff --git a/test/hermes/class-in-finally.js b/test/hermes/class-in-finally.js new file mode 100644 index 00000000000..5d6edda5162 --- /dev/null +++ b/test/hermes/class-in-finally.js @@ -0,0 +1,112 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// RUN: %hermes -O0 %s | %FileCheck --match-full-lines %s +// RUN: %hermes -O %s | %FileCheck --match-full-lines %s + +// Test that classes work correctly when defined inside finally blocks. +// Finally blocks emit code twice (normal flow + exception handler), +// which requires proper caching of internal variables. + +function testStaticProperty() { + try { + throw 1; + } finally { + class C { + static g = 42; + } + print(C.g); +// CHECK: 42 + } +} +try { testStaticProperty(); } catch (e) {} +print("testStaticProperty passed"); +// CHECK-NEXT: testStaticProperty passed + +function testInstanceProperty() { + try { + throw 1; + } finally { + class C { + f = 52; + } + print(new C().f); +// CHECK-NEXT: 52 + } +} +try { testInstanceProperty(); } catch (e) {} +print("testInstanceProperty passed"); +// CHECK-NEXT: testInstanceProperty passed + +function testComputedProperty() { + var key = "x"; + try { + throw 1; + } finally { + class C { + static [key] = 42; + [key] = 52; + } + print(C[key]); +// CHECK-NEXT: 42 + print(new C()[key]); +// CHECK-NEXT: 52 + } +} +try { testComputedProperty(); } catch (e) {} +print("testComputedProperty passed"); +// CHECK-NEXT: testComputedProperty passed + +function testPrivateMethod() { + try { + throw 1; + } finally { + class C { + #m() { return 62; } + call() { return this.#m(); } + } + print(new C().call()); +// CHECK-NEXT: 62 + } +} +try { testPrivateMethod(); } catch (e) {} +print("testPrivateMethod passed"); +// CHECK-NEXT: testPrivateMethod passed + +function testStaticPrivateMethod() { + try { + throw 1; + } finally { + class C { + static #m() { return 72; } + static call() { return C.#m(); } + } + print(C.call()); +// CHECK-NEXT: 72 + } +} +try { testStaticPrivateMethod(); } catch (e) {} +print("testStaticPrivateMethod passed"); +// CHECK-NEXT: testStaticPrivateMethod passed + +function testPrivateAccessor() { + try { + throw 1; + } finally { + class C { + #val = 82; + get #x() { return this.#val; } + set #x(v) { this.#val = v; } + call() { this.#x = this.#x + 10; return this.#x; } + } + print(new C().call()); +// CHECK-NEXT: 92 + } +} +try { testPrivateAccessor(); } catch (e) {} +print("testPrivateAccessor passed"); +// CHECK-NEXT: testPrivateAccessor passed