Address review comments.

Author: peter-hofer

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/ChunkedImageHeapLayouter.java

@@ -118,6 +118,9 @@ public void assignObjectToPartition(ImageHeapObject info, boolean immutable, boo
     }
 
     private ChunkedImageHeapPartition choosePartition(ImageHeapObject info, boolean immutable, boolean hasRelocatables, boolean patched) {
+        if (patched && hasRelocatables) {
+            throw VMError.shouldNotReachHere("Object cannot contain both relocatables and patched constants: " + info.getObject());
+        }
         if (patched) {
             return getWritablePatched();
         } else if (immutable) {

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/linux/LinuxImageHeapProvider.java

@@ -74,6 +74,7 @@
 import com.oracle.svm.core.heap.Heap;
 import com.oracle.svm.core.imagelayer.ImageLayerBuildingSupport;
 import com.oracle.svm.core.imagelayer.ImageLayerSection;
+import com.oracle.svm.core.jdk.UninterruptibleUtils;
 import com.oracle.svm.core.os.AbstractImageHeapProvider;
 import com.oracle.svm.core.os.VirtualMemoryProvider;
 import com.oracle.svm.core.os.VirtualMemoryProvider.Access;
@@ -84,10 +85,11 @@
 import com.oracle.svm.core.util.PointerUtils;
 import com.oracle.svm.core.util.UnsignedUtils;
 import com.oracle.svm.core.util.VMError;
+import com.oracle.svm.hosted.imagelayer.ImageLayerSectionFeature;
+import com.oracle.svm.hosted.imagelayer.LayeredDispatchTableFeature;
 
 import jdk.graal.compiler.nodes.NamedLocationIdentity;
 import jdk.graal.compiler.nodes.PauseNode;
-import jdk.graal.compiler.replacements.nodes.CountTrailingZerosNode;
 import jdk.graal.compiler.word.Word;
 
 /**
@@ -133,7 +135,7 @@ private static final class ImageHeapPatchingState {
         static final Word SUCCESSFUL = Word.unsigned(2);
     }
 
-    private static final CGlobalData<Word> IMAGE_HEAP_PATCHING_STATE = CGlobalDataFactory.createWord();
+    private static final CGlobalData<Word> IMAGE_HEAP_PATCHING_STATE = CGlobalDataFactory.createWord(ImageHeapPatchingState.UNINITIALIZED);
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     private static UnsignedWord getLayeredImageHeapAddressSpaceSize() {
@@ -228,17 +230,22 @@ protected int initializeLayeredImage(Pointer firstHeapStart, Pointer selfReserve
         return result;
     }
 
+    /**
+     * Apply patches to the image heap as specified by each layer. See {@link ImageLayerSection} and
+     * {@link ImageLayerSectionFeature} for the layout of the section that contains the patches and
+     * {@link LayeredDispatchTableFeature} where code patches are gathered.
+     */
     @Uninterruptible(reason = "Thread state not yet set up.")
     public static void patchLayeredImageHeap() {
         Word heapPatchStateAddr = IMAGE_HEAP_PATCHING_STATE.get();
         boolean firstIsolate = heapPatchStateAddr.logicCompareAndSwapWord(0, ImageHeapPatchingState.UNINITIALIZED, ImageHeapPatchingState.IN_PROGRESS, NamedLocationIdentity.OFF_HEAP_LOCATION);
 
         if (!firstIsolate) {
             // spin-wait for first isolate
-            Word state = heapPatchStateAddr.readWordVolatile(0, LocationIdentity.ANY_LOCATION);
+            Word state = heapPatchStateAddr.readWordVolatile(0, NamedLocationIdentity.OFF_HEAP_LOCATION);
             while (state.equal(ImageHeapPatchingState.IN_PROGRESS)) {
                 PauseNode.pause();
-                state = heapPatchStateAddr.readWordVolatile(0, LocationIdentity.ANY_LOCATION);
+                state = heapPatchStateAddr.readWordVolatile(0, NamedLocationIdentity.OFF_HEAP_LOCATION);
             }
 
             /* Patching has already been successfully completed, nothing needs to be done. */
@@ -254,19 +261,24 @@ public static void patchLayeredImageHeap() {
             Pointer data = layerSection.add(ImageLayerSection.getEntryOffset(VARIABLY_SIZED_DATA));
             int offset = 0;
 
-            // Skip singletons table
-            long singletonTableEntryCount = data.readLong(offset);
-            UnsignedWord singletonTableAlignedSize = roundUp(unsigned(singletonTableEntryCount * referenceSize), unsigned(Long.BYTES));
-            offset += Long.BYTES + (int) singletonTableAlignedSize.rawValue();
+            offset = skipSingletonsTable(data, offset, referenceSize);
 
+            /* Patch code offsets to become relative to the code base. */
             Pointer layerHeapRelocs = layerSection.readWord(ImageLayerSection.getEntryOffset(HEAP_RELOCATABLE_BEGIN));
             Pointer layerCode = layerSection.readWord(ImageLayerSection.getEntryOffset(CODE_START));
+            /*
+             * Note that the code base can be above the layer's code section, in which case the
+             * subtraction underflows and the additions of code address computations overflow,
+             * giving the correct result.
+             */
             Word layerCodeOffsetToBase = (Word) layerCode.subtract(codeBase);
             offset = applyLayerCodePointerPatches(data, offset, layerHeapRelocs, layerCodeOffsetToBase);
 
+            /* Patch absolute addresses to become relative to the code base. */
             Word negativeCodeBase = Word.<Word> zero().subtract(codeBase);
             offset = applyLayerCodePointerPatches(data, offset, layerHeapRelocs, negativeCodeBase);
 
+            /* Patch references in the image heap. */
             applyLayerImageHeapRefPatches(data.add(offset), initialLayerImageHeap);
 
             layerSection = layerSection.readWord(ImageLayerSection.getEntryOffset(NEXT_SECTION));
@@ -275,14 +287,23 @@ public static void patchLayeredImageHeap() {
         heapPatchStateAddr.writeWordVolatile(0, ImageHeapPatchingState.SUCCESSFUL);
     }
 
+    @Uninterruptible(reason = "Thread state not yet set up.")
+    private static int skipSingletonsTable(Pointer data, int offset, int referenceSize) {
+        long singletonTableEntryCount = data.readLong(offset);
+        UnsignedWord singletonTableAlignedSize = roundUp(unsigned(singletonTableEntryCount * referenceSize), unsigned(Long.BYTES));
+        return offset + Long.BYTES + UnsignedUtils.safeToInt(singletonTableAlignedSize);
+    }
+
     @Uninterruptible(reason = "Thread state not yet set up.")
     private static int applyLayerCodePointerPatches(Pointer data, int startOffset, Pointer layerHeapRelocs, Word addend) {
         int wordSize = ConfigurationValues.getTarget().wordSize;
 
         int offset = startOffset;
-        int bitmapWordCount = (int) data.readLong(offset);
+        long bitmapWordCountAsLong = data.readLong(offset);
+        int bitmapWordCount = UninterruptibleUtils.NumUtil.safeToInt(bitmapWordCountAsLong);
         offset += Long.BYTES;
         if (addend.equal(0)) {
+            /* Nothing to do. */
             offset += bitmapWordCount * Long.BYTES;
             return offset;
         }
@@ -292,7 +313,7 @@ private static int applyLayerCodePointerPatches(Pointer data, int startOffset, P
             offset += Long.BYTES;
             int j = 0; // index of a 1-bit
             while (bits != 0) {
-                int ntz = CountTrailingZerosNode.countLongTrailingZeros(bits);
+                int ntz = UninterruptibleUtils.Long.countTrailingZeros(bits);
                 j += ntz;
 
                 int at = (i * 64 + j) * wordSize;
@@ -301,11 +322,8 @@ private static int applyLayerCodePointerPatches(Pointer data, int startOffset, P
                 layerHeapRelocs.writeWord(at, w);
 
                 /*
-                 * Note that we must not shift by ntz+1 here because it can be 64, and according to
-                 * the Java Language Specification, 15.19. Shift Operators: If the promoted type of
-                 * the left-hand operand is long, then only the six lowest-order bits of the
-                 * right-hand operand are used as the shift distance. [..] The shift distance
-                 * actually used is therefore always in the range 0 to 63, inclusive.
+                 * Note that we must not shift by ntz+1 here because it can be 64, which would be a
+                 * no-op according to the Java Language Specification, 15.19. Shift Operators.
                  */
                 bits = (bits >>> ntz) >>> 1;
                 j++;
@@ -317,7 +335,8 @@ private static int applyLayerCodePointerPatches(Pointer data, int startOffset, P
     @Uninterruptible(reason = "Thread state not yet set up.")
     private static void applyLayerImageHeapRefPatches(Pointer patches, Pointer layerImageHeap) {
         int referenceSize = ConfigurationValues.getObjectLayout().getReferenceSize();
-        int count = (int) patches.readLong(0);
+        long countAsLong = patches.readLong(0);
+        int count = UninterruptibleUtils.NumUtil.safeToInt(countAsLong);
         int offset = Long.BYTES;
         int endOffset = offset + count * Integer.BYTES;
         while (offset < endOffset) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/imagelayer/ImageLayerSection.java

@@ -33,6 +33,11 @@
 
 import jdk.graal.compiler.api.replacements.Fold;
 
+/**
+ * With layered images, this is a section that each layer has and that is present at runtime. It
+ * contains the addresses of various important locations and information about values to patch at
+ * runtime. See {@code ImageLayerSectionFeature} for details.
+ */
 public abstract class ImageLayerSection implements LayeredImageSingleton {
 
     protected final CGlobalData<Pointer> initialSectionStart;

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/UninterruptibleUtils.java

@@ -36,6 +36,7 @@
 import com.oracle.svm.core.util.VMError;
 
 import jdk.graal.compiler.core.common.SuppressFBWarnings;
+import jdk.graal.compiler.replacements.nodes.CountTrailingZerosNode;
 import jdk.graal.compiler.word.Word;
 import jdk.internal.misc.Unsafe;
 
@@ -501,26 +502,10 @@ public static int toUnsignedInt(byte x) {
     }
 
     public static class Long {
-        /** Uninterruptible version of {@link java.lang.Long#numberOfLeadingZeros(long)}. */
-        @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-        // Checkstyle: stop
-        public static int numberOfLeadingZeros(long i) {
-        // @formatter:off
-            // HD, Figure 5-6
-            if (i == 0)
-               return 64;
-           int n = 1;
-           int x = (int)(i >>> 32);
-           if (x == 0) { n += 32; x = (int)i; }
-           if (x >>> 16 == 0) { n += 16; x <<= 16; }
-           if (x >>> 24 == 0) { n +=  8; x <<=  8; }
-           if (x >>> 28 == 0) { n +=  4; x <<=  4; }
-           if (x >>> 30 == 0) { n +=  2; x <<=  2; }
-           n -= x >>> 31;
-           return n;
-           // @formatter:on
-        }
-        // Checkstyle: resume
+        @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+        public static int countTrailingZeros(long i) {
+            return CountTrailingZerosNode.countLongTrailingZeros(i);
+        }
 
         @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
         public static int hashCode(long value) {
@@ -529,41 +514,6 @@ public static int hashCode(long value) {
     }
 
     public static class Integer {
-        // Checkstyle: stop
-        /** Uninterruptible version of {@link java.lang.Integer#numberOfLeadingZeros(int)}. */
-        @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-        @SuppressWarnings("all")
-        public static int numberOfLeadingZeros(int i) {
-        // @formatter:off
-            // HD, Figure 5-6
-            if (i == 0)
-                return 32;
-            int n = 1;
-            if (i >>> 16 == 0) { n += 16; i <<= 16; }
-            if (i >>> 24 == 0) { n +=  8; i <<=  8; }
-            if (i >>> 28 == 0) { n +=  4; i <<=  4; }
-            if (i >>> 30 == 0) { n +=  2; i <<=  2; }
-            n -= i >>> 31;
-            return n;
-            // @formatter:on
-        }
-
-        /** Uninterruptible version of {@link java.lang.Integer#highestOneBit(int)}. */
-        @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-        @SuppressWarnings("all")
-        public static int highestOneBit(int i) {
-        // @formatter:off
-            // HD, Figure 3-1
-            i |= (i >>  1);
-            i |= (i >>  2);
-            i |= (i >>  4);
-            i |= (i >>  8);
-            i |= (i >> 16);
-            return i - (i >>> 1);
-            // @formatter:on
-        }
-        // Checkstyle: resume
-
         /** Uninterruptible version of {@link java.lang.Integer#compare(int, int)}. */
         @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
         public static int compare(int x, int y) {

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImage.java

@@ -77,6 +77,7 @@
 import com.oracle.svm.core.BuildArtifacts.ArtifactType;
 import com.oracle.svm.core.BuildPhaseProvider;
 import com.oracle.svm.core.FrameAccess;
+import com.oracle.svm.core.FunctionPointerHolder;
 import com.oracle.svm.core.InvalidMethodPointerHandler;
 import com.oracle.svm.core.Isolates;
 import com.oracle.svm.core.OS;
@@ -93,13 +94,17 @@
 import com.oracle.svm.core.graal.code.CGlobalDataReference;
 import com.oracle.svm.core.graal.nodes.TLABObjectHeaderConstant;
 import com.oracle.svm.core.heap.Heap;
+import com.oracle.svm.core.hub.DynamicHub;
 import com.oracle.svm.core.image.ImageHeapLayoutInfo;
 import com.oracle.svm.core.image.ImageHeapPartition;
 import com.oracle.svm.core.imagelayer.DynamicImageLayerInfo;
 import com.oracle.svm.core.imagelayer.ImageLayerBuildingSupport;
+import com.oracle.svm.core.jni.access.JNIAccessibleMethod;
 import com.oracle.svm.core.meta.MethodOffset;
 import com.oracle.svm.core.meta.MethodPointer;
 import com.oracle.svm.core.option.SubstrateOptionsParser;
+import com.oracle.svm.core.os.ImageHeapProvider;
+import com.oracle.svm.core.reflect.SubstrateAccessor;
 import com.oracle.svm.core.util.UserError;
 import com.oracle.svm.core.util.VMError;
 import com.oracle.svm.hosted.DeadlockWatchdog;
@@ -557,7 +562,11 @@ public void build(String imageName, DebugContext debug) {
                 LayeredDispatchTableFeature.singleton().defineDispatchTableSlotSymbols(objectFile, textSection, codeCache, metaAccess);
             }
 
-            // Mark the sections with the relocations from the maps.
+            /*
+             * Mark locations that depend on the memory address of code (text), data, or the heap at
+             * runtime. These typically generate relocation entries which are processed by the
+             * dynamic linker. Additional such locations might be marked somewhere else.
+             */
             markRelocationSitesFromBuffer(textBuffer, textImpl);
             markRelocationSitesFromBuffer(roDataBuffer, roDataImpl);
             markRelocationSitesFromBuffer(rwDataBuffer, rwDataImpl);
@@ -586,7 +595,7 @@ public void markRelocationSitesFromBuffer(RelocatableBuffer buffer, ProgbitsSect
 
             Object target = info.getTargetObject();
             if (target instanceof CFunctionPointer || target instanceof MethodOffset) {
-                markCodeRelocationSite(sectionImpl, offset, info);
+                markSiteOfRelocationToCode(sectionImpl, offset, info);
             } else {
                 if (sectionImpl.getElement() == textSection) {
                     markDataRelocationSiteFromText(buffer, sectionImpl, offset, info);
@@ -631,8 +640,30 @@ private static boolean checkCodeRelocationKind(Info info) {
         return (relocationSize == wordSize && RelocationKind.isDirect(relocationKind)) || (relocationSize == 4 && RelocationKind.isPCRelative(relocationKind));
     }
 
-    /** @see NativeImageHeapWriter#writeConstant */
-    private void markCodeRelocationSite(final ProgbitsSectionImpl sectionImpl, final int offset, final RelocatableBuffer.Info info) {
+    /**
+     * Mark a location that needs to be patched by the dynamic linker at runtime to reflect the
+     * address where code has been loaded.
+     *
+     * {@linkplain DynamicHub Virtual dispatch tables} typically make up the vast majority of such
+     * locations. Frequent other locations to patch are in {@linkplain SubstrateAccessor reflection
+     * accessors}, {@linkplain JNIAccessibleMethod JNI accessors} and in
+     * {@link FunctionPointerHolder}.
+     *
+     * With {@link SubstrateOptions#RelativeCodePointers}, virtual dispatch tables contain offsets
+     * relative to a code base address and so do not need to be patched at runtime, which also
+     * avoids the cost of private copies of memory pages with the patched values.
+     *
+     * With code offsets and layered images, however, the code base refers only to the initial
+     * layer's code section, so we patch offsets to code from other layers to become relative to
+     * that code base ourselves at runtime. We do so in our own code without using the dynamic
+     * linker. See {@link LayeredDispatchTableFeature} which gathers these locations and
+     * {@link ImageLayerSectionFeature} which provides them for patching in the
+     * {@link ImageHeapProvider} at runtime.
+     *
+     * {@link NativeImageHeap#isRelocatableValue} and {@link NativeImageHeapWriter#writeConstant}
+     * determine (for the image heap) whether a code reference requires a linker relocation here.
+     */
+    private void markSiteOfRelocationToCode(final ProgbitsSectionImpl sectionImpl, final int offset, final RelocatableBuffer.Info info) {
         Object targetObject = info.getTargetObject();
         assert targetObject instanceof MethodPointer || targetObject instanceof MethodOffset : "Wrong type for code relocation: " + targetObject.toString();
 
@@ -659,16 +690,19 @@ private void markCodeRelocationSite(final ProgbitsSectionImpl sectionImpl, final
         }
     }
 
+    /**
+     * Whether a method has not been compiled in the current image build, and with layered images,
+     * not in a prior layer, but might be compiled in a future layer.
+     */
     static boolean isInjectedNotCompiled(HostedMethod target) {
         return !target.isCompiled() && !target.isCompiledInPriorLayer();
     }
 
-    static HostedMethod getMethodRefTargetMethod(HostedMetaAccess metaAccess, ResolvedJavaMethod method) {
-        HostedMethod target = (method instanceof HostedMethod) ? (HostedMethod) method : metaAccess.getUniverse().lookup(method);
-        if (isInjectedNotCompiled(target)) {
-            target = metaAccess.lookupJavaMethod(InvalidMethodPointerHandler.METHOD_POINTER_NOT_COMPILED_HANDLER_METHOD);
+    static HostedMethod getMethodRefTargetMethod(HostedMetaAccess metaAccess, HostedMethod method) {
+        if (isInjectedNotCompiled(method)) {
+            return metaAccess.lookupJavaMethod(InvalidMethodPointerHandler.METHOD_POINTER_NOT_COMPILED_HANDLER_METHOD);
         }
-        return target;
+        return method;
     }
 
     private static boolean isAddendAligned(Architecture arch, long addend, RelocationKind kind) {

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java

@@ -258,22 +258,20 @@ private RelocatedPointer prepareRelocatable(ObjectInfo info, JavaConstant value)
 
     /**
      * @see NativeImageHeap#isRelocatableValue
-     * @see NativeImage#markCodeRelocationSite
+     * @see NativeImage#markSiteOfRelocationToCode
      */
     private void writeConstant(RelocatableBuffer buffer, int index, JavaKind kind, Object constantValue, ObjectInfo info) {
         Object value = constantValue;
         if (value instanceof MethodOffset methodOffset) {
             HostedMetaAccess metaAccess = heap.hMetaAccess;
             ResolvedJavaMethod method = methodOffset.getMethod();
-            if (!(method instanceof HostedMethod)) {
-                method = metaAccess.getUniverse().lookup(method);
-            }
-            if (imageLayer && NativeImage.isInjectedNotCompiled((HostedMethod) method)) {
+            HostedMethod hMethod = (method instanceof HostedMethod hm) ? hm : metaAccess.getUniverse().lookup(method);
+            if (imageLayer && NativeImage.isInjectedNotCompiled(hMethod)) {
                 // Will be patched in a future layer (even if it ends up not being compiled at all)
                 addWordConstantRelocation(buffer, index, methodOffset);
                 return;
             }
-            HostedMethod target = NativeImage.getMethodRefTargetMethod(metaAccess, method);
+            HostedMethod target = NativeImage.getMethodRefTargetMethod(metaAccess, hMethod);
             value = target.getCodeAddressOffset();
         } else if (value instanceof RelocatedPointer relocatedPointer) {
             addWordConstantRelocation(buffer, index, relocatedPointer);