Do not mark AnalysisType reachable when creating AnalysisField

Author: Christian Wimmer

substratevm/src/com.oracle.graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/StandaloneHost.java

@@ -29,18 +29,18 @@
 import java.util.Comparator;
 import java.util.concurrent.ConcurrentHashMap;
 
-import jdk.graal.compiler.java.GraphBuilderPhase;
-import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
-import jdk.graal.compiler.nodes.graphbuilderconf.IntrinsicContext;
-import jdk.graal.compiler.options.OptionValues;
-import jdk.graal.compiler.phases.OptimisticOptimizations;
-
+import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.api.HostVM;
 import com.oracle.graal.pointsto.meta.AnalysisType;
 import com.oracle.graal.pointsto.meta.HostedProviders;
 import com.oracle.graal.pointsto.standalone.plugins.StandaloneGraphBuilderPhase;
 import com.oracle.graal.pointsto.util.AnalysisError;
 
+import jdk.graal.compiler.java.GraphBuilderPhase;
+import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
+import jdk.graal.compiler.nodes.graphbuilderconf.IntrinsicContext;
+import jdk.graal.compiler.options.OptionValues;
+import jdk.graal.compiler.phases.OptimisticOptimizations;
 import jdk.vm.ci.meta.ResolvedJavaType;
 
 public class StandaloneHost extends HostVM {
@@ -72,7 +72,7 @@ public boolean isInitialized(AnalysisType type) {
     }
 
     @Override
-    public void onTypeReachable(AnalysisType type) {
+    public void onTypeReachable(BigBang bb, AnalysisType type) {
         if (!type.isReachable()) {
             AnalysisError.shouldNotReachHere("Registering and initializing a type that was not yet marked as reachable: " + type);
         }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/api/HostVM.java

@@ -41,11 +41,9 @@
 import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.PointsToAnalysis;
 import com.oracle.graal.pointsto.flow.InvokeTypeFlow;
-import com.oracle.graal.pointsto.meta.AnalysisField;
 import com.oracle.graal.pointsto.meta.AnalysisMethod;
 import com.oracle.graal.pointsto.meta.AnalysisType;
 import com.oracle.graal.pointsto.meta.AnalysisUniverse;
-import com.oracle.graal.pointsto.meta.FieldValueComputer;
 import com.oracle.graal.pointsto.meta.HostedProviders;
 import com.oracle.graal.pointsto.phases.InlineBeforeAnalysisGraphDecoder;
 import com.oracle.graal.pointsto.phases.InlineBeforeAnalysisPolicy;
@@ -155,7 +153,7 @@ public void checkType(ResolvedJavaType type, AnalysisUniverse universe) {
      * 
      * @param newValue the type to initialize
      */
-    public abstract void onTypeReachable(AnalysisType newValue);
+    public abstract void onTypeReachable(BigBang bb, AnalysisType newValue);
 
     /**
      * Check if an {@link AnalysisType} is initialized.
@@ -420,8 +418,4 @@ public boolean allowConstantFolding(AnalysisMethod method) {
          */
         return method.isOriginalMethod();
     }
-
-    public FieldValueComputer createFieldValueComputer(@SuppressWarnings("unused") AnalysisField field) {
-        return null;
-    }
 }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/ImageHeapScanner.java

@@ -545,7 +545,7 @@ private void updateInstanceField(AnalysisField field, ImageHeapInstance imageHea
     }
 
     public boolean isValueAvailable(AnalysisField field) {
-        return field.isValueAvailable();
+        return true;
     }
 
     protected String formatReason(String message, ScanReason reason) {

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/value/LazyValueSupplier.java

@@ -24,6 +24,7 @@
  */
 package com.oracle.graal.pointsto.heap.value;
 
+import java.util.Objects;
 import java.util.function.BooleanSupplier;
 import java.util.function.Supplier;
 
@@ -47,6 +48,6 @@ public boolean isAvailable() {
     @Override
     public V get() {
         AnalysisError.guarantee(isAvailable(), "Value is not yet available.");
-        return valueSupplier.get();
+        return Objects.requireNonNull(valueSupplier.get());
     }
 }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/value/ValueSupplier.java

@@ -24,6 +24,7 @@
  */
 package com.oracle.graal.pointsto.heap.value;
 
+import java.util.Objects;
 import java.util.function.BooleanSupplier;
 import java.util.function.Supplier;
 
@@ -42,7 +43,7 @@
 public interface ValueSupplier<V> {
 
     static <V> ValueSupplier<V> eagerValue(V value) {
-        return new EagerValueSupplier<>(value);
+        return new EagerValueSupplier<>(Objects.requireNonNull(value));
     }
 
     static <V> ValueSupplier<V> lazyValue(Supplier<V> valueSupplier, BooleanSupplier isAvailable) {

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/infrastructure/OriginalClassProvider.java

@@ -28,16 +28,21 @@
 
 import com.oracle.graal.pointsto.util.GraalAccess;
 
+import jdk.vm.ci.meta.JavaType;
 import jdk.vm.ci.meta.ResolvedJavaType;
 
 public interface OriginalClassProvider {
 
-    static Class<?> getJavaClass(ResolvedJavaType javaType) {
+    static Class<?> getJavaClass(JavaType javaType) {
         Class<?> result;
         if (javaType instanceof OriginalClassProvider) {
             result = ((OriginalClassProvider) javaType).getJavaClass();
         } else {
-            result = GraalAccess.getOriginalSnippetReflection().originalClass(javaType);
+            /*
+             * The static analysis and the image generator never use unresolved types. The JavaType
+             * in the method signature is just to avoid casts in the callers.
+             */
+            result = GraalAccess.getOriginalSnippetReflection().originalClass((ResolvedJavaType) javaType);
         }
 
         /*

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/infrastructure/OriginalFieldProvider.java

@@ -41,21 +41,28 @@
  */
 public interface OriginalFieldProvider {
 
+    static ResolvedJavaField getOriginalField(ResolvedJavaField field) {
+        ResolvedJavaField cur = field;
+        while (cur instanceof OriginalFieldProvider originalFieldProvider) {
+            cur = originalFieldProvider.unwrapTowardsOriginalField();
+        }
+        return cur;
+    }
+
     static Field getJavaField(ResolvedJavaField field) {
-        if (field instanceof OriginalFieldProvider) {
-            return ((OriginalFieldProvider) field).getJavaField();
-        } else {
+        ResolvedJavaField originalField = getOriginalField(field);
+        if (originalField != null) {
             try {
-                return GraalAccess.getOriginalSnippetReflection().originalField(field);
+                return GraalAccess.getOriginalSnippetReflection().originalField(originalField);
             } catch (LinkageError ignored) {
                 /*
                  * Ignore any linking problems and incompatible class change errors. Looking up a
                  * reflective representation of a JVMCI field is always a best effort operation.
                  */
-                return null;
             }
         }
+        return null;
     }
 
-    Field getJavaField();
+    ResolvedJavaField unwrapTowardsOriginalField();
 }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisField.java

@@ -24,16 +24,14 @@
  */
 package com.oracle.graal.pointsto.meta;
 
-import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 
-import jdk.graal.compiler.debug.GraalError;
-
 import com.oracle.graal.pointsto.api.DefaultUnsafePartition;
 import com.oracle.graal.pointsto.api.HostVM;
 import com.oracle.graal.pointsto.api.PointstoOptions;
@@ -42,10 +40,12 @@
 import com.oracle.graal.pointsto.infrastructure.OriginalFieldProvider;
 import com.oracle.graal.pointsto.infrastructure.WrappedJavaField;
 import com.oracle.graal.pointsto.typestate.TypeState;
+import com.oracle.graal.pointsto.util.AnalysisFuture;
 import com.oracle.graal.pointsto.util.AtomicUtils;
 import com.oracle.graal.pointsto.util.ConcurrentLightHashSet;
 import com.oracle.svm.util.UnsafePartitionKind;
 
+import jdk.graal.compiler.debug.GraalError;
 import jdk.vm.ci.code.BytecodePosition;
 import jdk.vm.ci.meta.JavaConstant;
 import jdk.vm.ci.meta.JavaKind;
@@ -123,9 +123,10 @@ public abstract class AnalysisField extends AnalysisElement implements WrappedJa
 
     /**
      * Marks a field whose value is computed during image building, in general derived from other
-     * values, and it cannot be constant-folded or otherwise optimized.
+     * values. The actual meaning of the field value is out of scope of the static analysis, but the
+     * value is stored here to allow fast access.
      */
-    protected final FieldValueComputer fieldValueComputer;
+    protected Object fieldValueInterceptor;
 
     @SuppressWarnings("this-escape")
     public AnalysisField(AnalysisUniverse universe, ResolvedJavaField wrappedField) {
@@ -152,8 +153,6 @@ public AnalysisField(AnalysisUniverse universe, ResolvedJavaField wrappedField)
             this.instanceFieldFlow = new ContextInsensitiveFieldTypeFlow(this, getType());
             this.initialInstanceFieldFlow = new FieldTypeFlow(this, getType());
         }
-
-        fieldValueComputer = universe.hostVM().createFieldValueComputer(this);
     }
 
     @Override
@@ -260,6 +259,8 @@ public void cleanupAfterAnalysis() {
     }
 
     public boolean registerAsAccessed(Object reason) {
+        getDeclaringClass().registerAsReachable(this);
+
         assert isValidReason(reason) : "Registering a field as accessed needs to provide a valid reason.";
         boolean firstAttempt = AtomicUtils.atomicSet(this, reason, isAccessedUpdater);
         notifyUpdateAccessInfo();
@@ -275,6 +276,8 @@ public boolean registerAsAccessed(Object reason) {
      * @param reason the reason why this field is read, non-null
      */
     public boolean registerAsRead(Object reason) {
+        getDeclaringClass().registerAsReachable(this);
+
         assert isValidReason(reason) : "Registering a field as read needs to provide a valid reason.";
         boolean firstAttempt = AtomicUtils.atomicSet(this, reason, isReadUpdater);
         notifyUpdateAccessInfo();
@@ -295,6 +298,8 @@ public boolean registerAsRead(Object reason) {
      * @param reason the reason why this field is written, non-null
      */
     public boolean registerAsWritten(Object reason) {
+        getDeclaringClass().registerAsReachable(this);
+
         assert isValidReason(reason) : "Registering a field as written needs to provide a valid reason.";
         boolean firstAttempt = AtomicUtils.atomicSet(this, reason, isWrittenUpdater);
         notifyUpdateAccessInfo();
@@ -311,6 +316,8 @@ public boolean registerAsWritten(Object reason) {
     }
 
     public void registerAsFolded(Object reason) {
+        getDeclaringClass().registerAsReachable(this);
+
         assert isValidReason(reason) : "Registering a field as folded needs to provide a valid reason.";
         if (AtomicUtils.atomicSet(this, reason, isFoldedUpdater)) {
             assert getDeclaringClass().isReachable() : this;
@@ -442,23 +449,12 @@ public void onReachable() {
         notifyReachabilityCallbacks(declaringClass.getUniverse(), new ArrayList<>());
     }
 
-    public boolean isValueAvailable() {
-        if (fieldValueComputer != null) {
-            return fieldValueComputer.isAvailable();
-        }
-        return true;
-    }
-
-    public boolean isComputedValue() {
-        return fieldValueComputer != null;
-    }
-
-    public Class<?>[] computedValueTypes() {
-        return fieldValueComputer.types();
+    public Object getFieldValueInterceptor() {
+        return fieldValueInterceptor;
     }
 
-    public boolean computedValueCanBeNull() {
-        return fieldValueComputer.canBeNull();
+    public void setFieldValueInterceptor(Object fieldValueInterceptor) {
+        this.fieldValueInterceptor = fieldValueInterceptor;
     }
 
     public void setCanBeNull(boolean canBeNull) {
@@ -531,15 +527,34 @@ public String toString() {
     }
 
     @Override
-    public Field getJavaField() {
-        return OriginalFieldProvider.getJavaField(wrapped);
+    public ResolvedJavaField unwrapTowardsOriginalField() {
+        return wrapped;
     }
 
     @Override
     public JavaConstant getConstantValue() {
         return getUniverse().lookup(getWrapped().getConstantValue());
     }
 
+    /**
+     * Ensure that all reachability handler that were present at the time the declaring type was
+     * marked as reachable are executed before accessing field values. This allows field value
+     * transformer to be installed reliably in reachability handler.
+     */
+    public void beforeFieldValueAccess() {
+        declaringClass.registerAsReachable(this);
+        declaringClass.ensureOnTypeReachableTaskDone();
+
+        List<AnalysisFuture<Void>> notifications = declaringClass.scheduledTypeReachableNotifications;
+        if (notifications != null) {
+            for (var notification : notifications) {
+                notification.ensureDone();
+            }
+            /* Now we know all the handlers have been executed, no checks are necessary anymore. */
+            declaringClass.scheduledTypeReachableNotifications = null;
+        }
+    }
+
     public void addAnalysisFieldObserver(AnalysisFieldObserver observer) {
         ConcurrentLightHashSet.addElement(this, OBSERVERS_UPDATER, observer);
     }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisType.java

@@ -681,13 +681,18 @@ private static void forAllSuperTypes(AnalysisType elementType, int arrayDimensio
         if (elementType == null) {
             return;
         }
-        if (processType) {
-            superTypeConsumer.accept(elementType.getArrayClass(arrayDimension));
-        }
         for (AnalysisType interf : elementType.getInterfaces()) {
             forAllSuperTypes(interf, arrayDimension, true, superTypeConsumer);
         }
         forAllSuperTypes(elementType.getSuperclass(), arrayDimension, true, superTypeConsumer);
+        /*
+         * Process the type itself only after visiting all supertypes. This ensures that, e.g., a
+         * type is never seen as reachable by another thread before all of its supertypes are
+         * already marked as reachable too.
+         */
+        if (processType) {
+            superTypeConsumer.accept(elementType.getArrayClass(arrayDimension));
+        }
     }
 
     protected synchronized void addAssignableType(BigBang bb, TypeState typeState) {