Skip to content

Commit 82438b3

Browse files
woesswoess
committed
[GR-44414] Fix Nashorn test failing in shared-engine mode.
1 parent 7b377eb commit 82438b3

File tree

3 files changed

+80
-40
lines changed

3 files changed

+80
-40
lines changed

‎graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/PropertyCacheNode.java‎

Lines changed: 78 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* The Universal Permissive License (UPL), Version 1.0
@@ -82,6 +82,7 @@
8282
import com.oracle.truffle.js.runtime.objects.JSObject;
8383
import com.oracle.truffle.js.runtime.objects.JSProperty;
8484
import com.oracle.truffle.js.runtime.objects.JSShape;
85+
import com.oracle.truffle.js.runtime.objects.Null;
8586
import com.oracle.truffle.js.runtime.objects.Undefined;
8687
import com.oracle.truffle.js.runtime.util.DebugCounter;
8788

@@ -519,6 +520,8 @@ public TraversePrototypeChainShapeCheckNode(Shape shape, JSDynamicObject thisObj
519520
protoShapes[i] = depthShape;
520521
getPrototypeNodes[i] = GetPrototypeNode.create();
521522
}
523+
524+
traversePrototypeChainShapeCheckCount.inc();
522525
}
523526

524527
@ExplodeLoop
@@ -592,6 +595,8 @@ public TraversePrototypeShapeCheckNode(Shape shape, JSDynamicObject thisObj) {
592595
super(shape);
593596
this.protoShape = JSObject.getPrototype(thisObj).getShape();
594597
this.getPrototypeNode = GetPrototypeNode.create();
598+
599+
traversePrototypeShapeCheckCount.inc();
595600
}
596601

597602
@Override
@@ -977,14 +982,14 @@ protected T createSpecialization(Object thisObj, T currentHead, int cachedCount,
977982
T specialized = null;
978983

979984
JSDynamicObject store = null;
980-
if (JSDynamicObject.isJSDynamicObject(thisObj)) {
985+
if (JSObject.isJSObject(thisObj)) {
981986
if ((!JSAdapter.isJSAdapter(thisObj) && !JSProxy.isJSProxy(thisObj)) || key instanceof HiddenKey) {
982987
store = (JSDynamicObject) thisObj;
983988
}
984989
} else if (JSRuntime.isForeignObject(thisObj)) {
985990
assert !JSDynamicObject.isJSDynamicObject(thisObj);
986991
specialized = createTruffleObjectPropertyNode();
987-
} else {
992+
} else if (!JSRuntime.isNullOrUndefined(thisObj)) {
988993
store = wrapPrimitive(thisObj);
989994
}
990995

@@ -1035,11 +1040,18 @@ protected T createSpecialization(Object thisObj, T currentHead, int cachedCount,
10351040
}
10361041

10371042
store = (JSDynamicObject) JSRuntime.toJavaNull(JSObject.getPrototype(store));
1038-
if (store != null) {
1039-
depth++;
1040-
}
1043+
depth++;
10411044
}
10421045

1046+
// <depth>: number of prototypes to check to validate the property access.
1047+
// If isOwnProperty(), or <thisObj> is a proxy or nullish value, depth is always 0.
1048+
// If the property is present, <depth> points to the object that has the property:
1049+
// e.g. depth == 1 for obj0:{a} -> proto1:{b} -> proto2:{c} -> null, with key "b".
1050+
// If there's a proxy in the prototype chain, we always stop counting there:
1051+
// e.g. depth == 1 for obj0 -> proxy1 -> proto2 -> null.
1052+
// If the property is absent, <depth> includes the final null prototype:
1053+
// e.g. depth == 3 for obj0 -> proto1 -> proto2 -> null.
1054+
10431055
if (specialized == null) {
10441056
specialized = createUndefinedPropertyNode(thisObj, thisObj, depth, value);
10451057
}
@@ -1239,8 +1251,6 @@ protected static final JSDynamicObject wrapPrimitive(Object thisObject) {
12391251
protected final AbstractShapeCheckNode createShapeCheckNode(Shape shape, JSDynamicObject thisObj, int depth, boolean isConstantObjectFinal, boolean isDefine) {
12401252
if (depth == 0) {
12411253
return createShapeCheckNodeDepth0(shape, thisObj, isConstantObjectFinal, isDefine);
1242-
} else if (depth == 1) {
1243-
return createShapeCheckNodeDepth1(shape, thisObj, depth, isConstantObjectFinal);
12441254
} else {
12451255
return createShapeCheckNodeDeeper(shape, thisObj, depth, isConstantObjectFinal);
12461256
}
@@ -1257,41 +1267,71 @@ private AbstractShapeCheckNode createShapeCheckNodeDepth0(Shape shape, JSDynamic
12571267
}
12581268
}
12591269

1260-
private AbstractShapeCheckNode createShapeCheckNodeDepth1(Shape shape, JSDynamicObject thisObj, int depth, boolean isConstantObjectFinal) {
1261-
assert depth == 1;
1262-
if (JSConfig.SkipPrototypeShapeCheck && prototypesInShape(thisObj, depth) && propertyAssumptionsValid(thisObj, depth, isConstantObjectFinal)) {
1263-
return isConstantObjectFinal
1264-
? ConstantObjectPrototypeChainShapeCheckNode.create(shape, thisObj, key, depth, getContext())
1265-
: PrototypeShapeCheckNode.create(shape, thisObj, key, depth, getContext());
1270+
private AbstractShapeCheckNode createShapeCheckNodeDeeper(Shape shape, JSDynamicObject thisObj, int depth, boolean isConstantObjectFinal) {
1271+
assert depth >= 1;
1272+
JSDynamicObject protoAtDepth = getPrototypeAtDepth(thisObj, depth);
1273+
boolean allPrototypesInShape = prototypesInShape(thisObj, 0, depth);
1274+
if (JSConfig.SkipPrototypeShapeCheck && allPrototypesInShape && propertyAssumptionsValid(thisObj, depth, isConstantObjectFinal)) {
1275+
if (isConstantObjectFinal) {
1276+
return ConstantObjectPrototypeChainShapeCheckNode.create(shape, thisObj, key, depth, getContext());
1277+
} else if (depth == 1) {
1278+
return PrototypeShapeCheckNode.create(shape, thisObj, key, depth, getContext());
1279+
} else {
1280+
return PrototypeChainShapeCheckNode.create(shape, thisObj, key, depth, getContext());
1281+
}
12661282
} else {
1267-
traversePrototypeShapeCheckCount.inc();
1268-
return new TraversePrototypeShapeCheckNode(shape, thisObj);
1283+
/*
1284+
* If we're checking that a property is absent, we must check all the way down including
1285+
* the final null prototype, so as to not miss any prototype change.
1286+
*
1287+
* However, the last prototype link to null is typically from either Object.prototype
1288+
* (an immutable prototype exotic object) or another object with a constant-prototype
1289+
* shape, in which case we can skip the final prototype == null check, since setting a
1290+
* different prototype is either not allowed or would result in a shape change, and
1291+
* we're already checking the shape of that last prototype object anyway.
1292+
*/
1293+
int checkedDepth = depth;
1294+
if (protoAtDepth == Null.instance && (allPrototypesInShape || prototypesInShape(thisObj, depth - 1, depth))) {
1295+
checkedDepth--;
1296+
}
1297+
return createTraversePrototypeShapeCheck(shape, thisObj, checkedDepth);
12691298
}
12701299
}
12711300

1272-
private AbstractShapeCheckNode createShapeCheckNodeDeeper(Shape shape, JSDynamicObject thisObj, int depth, boolean isConstantObjectFinal) {
1273-
assert depth > 1;
1274-
if (JSConfig.SkipPrototypeShapeCheck && prototypesInShape(thisObj, depth) && propertyAssumptionsValid(thisObj, depth, isConstantObjectFinal)) {
1275-
return isConstantObjectFinal
1276-
? ConstantObjectPrototypeChainShapeCheckNode.create(shape, thisObj, key, depth, getContext())
1277-
: PrototypeChainShapeCheckNode.create(shape, thisObj, key, depth, getContext());
1278-
} else {
1279-
traversePrototypeChainShapeCheckCount.inc();
1280-
return new TraversePrototypeChainShapeCheckNode(shape, thisObj, depth);
1301+
private static AbstractShapeCheckNode createTraversePrototypeShapeCheck(Shape shape, JSDynamicObject thisObj, int depth) {
1302+
if (depth == 0) {
1303+
return new ShapeCheckNode(shape);
1304+
} else if (depth == 1) {
1305+
return new TraversePrototypeShapeCheckNode(shape, thisObj);
12811306
}
1307+
return new TraversePrototypeChainShapeCheckNode(shape, thisObj, depth);
12821308
}
12831309

1284-
protected static boolean prototypesInShape(JSDynamicObject thisObj, int depth) {
1310+
protected static boolean prototypesInShape(JSDynamicObject thisObj, int start, int end) {
12851311
JSDynamicObject depthObject = thisObj;
1286-
for (int i = 0; i < depth; i++) {
1287-
if (!JSShape.isPrototypeInShape(depthObject.getShape())) {
1312+
for (int i = 0; i < end; i++) {
1313+
assert depthObject != Null.instance && !JSProxy.isJSProxy(depthObject) : depthObject;
1314+
if (i >= start && !JSShape.isPrototypeInShape(depthObject.getShape())) {
12881315
return false;
12891316
}
1317+
if (i + 1 >= end) {
1318+
// no need to get the prototype if we're about to exit the loop
1319+
break;
1320+
}
12901321
depthObject = JSObject.getPrototype(depthObject);
12911322
}
12921323
return true;
12931324
}
12941325

1326+
protected static JSDynamicObject getPrototypeAtDepth(JSDynamicObject thisObj, int depth) {
1327+
JSDynamicObject depthObject = thisObj;
1328+
for (int i = 0; i < depth; i++) {
1329+
assert depthObject != Null.instance && !JSProxy.isJSProxy(depthObject) : depthObject;
1330+
depthObject = JSObject.getPrototype(depthObject);
1331+
}
1332+
return depthObject;
1333+
}
1334+
12951335
protected final boolean propertyAssumptionsValid(JSDynamicObject thisObj, int depth, boolean checkDepth0) {
12961336
if (!getContext().isSingleRealm()) {
12971337
return false;
@@ -1302,12 +1342,13 @@ protected final boolean propertyAssumptionsValid(JSDynamicObject thisObj, int de
13021342
return false;
13031343
}
13041344
for (int i = 0; i < depth; i++) {
1345+
assert depthObject != Null.instance;
13051346
if ((depth != 0 || checkDepth0) && !JSShape.getPrototypeAssumption(depthShape).isValid()) {
13061347
return false;
13071348
}
13081349
depthObject = JSObject.getPrototype(depthObject);
13091350
depthShape = depthObject.getShape();
1310-
if (!JSShape.getPropertyAssumption(depthShape, key, true).isValid()) {
1351+
if (depthObject != Null.instance && !JSShape.getPropertyAssumption(depthShape, key, true).isValid()) {
13111352
return false;
13121353
}
13131354
}
@@ -1321,10 +1362,16 @@ protected final ReceiverCheckNode createPrimitiveReceiverCheck(Object thisObj, i
13211362
} else {
13221363
assert JSRuntime.isJSPrimitive(thisObj) || thisObj instanceof Long;
13231364
JSDynamicObject wrapped = wrapPrimitive(thisObj);
1324-
if (JSConfig.SkipPrototypeShapeCheck && prototypesInShape(wrapped, depth) && propertyAssumptionsValid(wrapped, depth, false)) {
1365+
JSDynamicObject protoAtDepth = getPrototypeAtDepth(wrapped, depth);
1366+
boolean allPrototypesInShape = prototypesInShape(wrapped, 0, depth);
1367+
if (JSConfig.SkipPrototypeShapeCheck && allPrototypesInShape && propertyAssumptionsValid(wrapped, depth, false)) {
13251368
return ValuePrototypeChainCheckNode.create(valueClass, wrapped.getShape(), wrapped, key, depth, context);
13261369
} else {
1327-
return new TraverseValuePrototypeChainCheckNode(valueClass, wrapped.getShape(), wrapped, depth, JSObject.getJSClass(wrapped));
1370+
int checkedDepth = depth;
1371+
if (protoAtDepth == Null.instance && (allPrototypesInShape || prototypesInShape(wrapped, depth - 1, depth))) {
1372+
checkedDepth--;
1373+
}
1374+
return new TraverseValuePrototypeChainCheckNode(valueClass, wrapped.getShape(), wrapped, checkedDepth, JSObject.getJSClass(wrapped));
13281375
}
13291376
}
13301377
}

‎graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/PropertyGetNode.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* The Universal Permissive License (UPL), Version 1.0
@@ -1924,7 +1924,7 @@ private boolean isEligibleForFinalSpecialization(Shape cacheShape, JSDynamicObje
19241924
if (depth == 0) {
19251925
return (JSConfig.SkipFinalShapeCheck && isPropertyAssumptionCheckEnabled() && JSShape.getPropertyAssumption(cacheShape, key).isValid());
19261926
} else {
1927-
return (prototypesInShape(thisObj, depth) && propertyAssumptionsValid(thisObj, depth, isConstantObjectFinal));
1927+
return (prototypesInShape(thisObj, 0, depth) && propertyAssumptionsValid(thisObj, depth, isConstantObjectFinal));
19281928
}
19291929
}
19301930

‎graal-js/test/testNashorn.json‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -481,13 +481,6 @@
481481
}, {
482482
"filePath" : "basic/JDK-8133119.js",
483483
"status" : "FAIL"
484-
}, {
485-
"filePath" : "basic/JDK-8134569.js",
486-
"status" : "PASS",
487-
"statusOverrides" : {
488-
"SHARED_ENGINE" : "FAIL"
489-
},
490-
"comment" : "Currently failing in shared-engine mode (GR-44414)"
491484
}, {
492485
"filePath" : "basic/JDK-8134609.js",
493486
"status" : "FAIL"

0 commit comments

Comments
 (0)