[GR-65625] [GR-66764] Refine layer compatibility checking for build options.

PullRequest: graal/21257

Author: olpaw

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java

@@ -633,7 +633,6 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean o
     @Option(help = "Enable detection and runtime container configuration support.")//
     public static final HostedOptionKey<Boolean> UseContainerSupport = new HostedOptionKey<>(true);
 
-    @LayerVerifiedOption(kind = Kind.Changed, severity = Severity.Error)//
     @Option(help = "The size of each thread stack at run-time, in bytes.", type = OptionType.User)//
     public static final RuntimeOptionKey<Long> StackSize = new RuntimeOptionKey<>(0L);
 
@@ -876,8 +875,6 @@ private static void validateZapNativeMemory(HostedOptionKey<Boolean> optionKey)
     /*
      * Isolate tear down options.
      */
-
-    @LayerVerifiedOption(kind = Kind.Changed, severity = Severity.Error)//
     @Option(help = "The number of seconds before and between which tearing down an isolate gives a warning message. 0 implies no warning.")//
     public static final RuntimeOptionKey<Long> TearDownWarningSeconds = new RuntimeOptionKey<>(0L, RelevantForCompilationIsolates);
 
@@ -914,7 +911,7 @@ public static long getTearDownFailureNanos() {
     @Option(help = "Perform trivial method inlining in the AOT compiled native image")//
     public static final HostedOptionKey<Boolean> AOTTrivialInline = new HostedOptionKey<>(true);
 
-    @LayerVerifiedOption(kind = Kind.Removed, severity = Severity.Error, positional = false)//
+    @LayerVerifiedOption(kind = Kind.Removed, severity = Severity.Warn, positional = false)//
     @Option(help = "file:doc-files/NeverInlineHelp.txt", type = OptionType.Debug)//
     public static final HostedOptionKey<AccumulatingLocatableMultiOptionValue.Strings> NeverInline = new HostedOptionKey<>(AccumulatingLocatableMultiOptionValue.Strings.build());
 
@@ -1074,8 +1071,13 @@ public static int codeAlignment(OptionValues options) {
     @Option(help = "Determines if debugging-specific helper methods are embedded into the image. Those methods can be called directly from the debugger to obtain or print additional information.", type = OptionType.Debug) //
     public static final HostedOptionKey<Boolean> IncludeDebugHelperMethods = new HostedOptionKey<>(false);
 
+    private static final String ENABLE_DEBUGINFO_OPTION = "-g";
+    // Only raise error if -g is used in current layer build but missing in the previous layer build
+    @LayerVerifiedOption(apiOption = ENABLE_DEBUGINFO_OPTION, kind = Kind.Added, severity = Severity.Error, message = "If you want to use " + ENABLE_DEBUGINFO_OPTION +
+                    " in this layer, use a base layer that also got built with " + ENABLE_DEBUGINFO_OPTION + ".")//
+    // ... but use stricter check for raw (non-API) use of GenerateDebugInfo
     @LayerVerifiedOption(kind = Kind.Changed, severity = Severity.Error)//
-    @APIOption(name = "-g", fixedValue = "2", customHelp = "generate debugging information")//
+    @APIOption(name = ENABLE_DEBUGINFO_OPTION, fixedValue = "2", customHelp = "generate debugging information")//
     @Option(help = "Insert debug info into the generated native image or library")//
     public static final HostedOptionKey<Integer> GenerateDebugInfo = new HostedOptionKey<>(0, SubstrateOptions::validateGenerateDebugInfo) {
         @Override
@@ -1226,7 +1228,6 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Integer o
         @Option(help = "The largest page size of machines that can run the image. The default of 0 automatically selects a typically suitable value.")//
         protected static final HostedOptionKey<Integer> PageSize = new HostedOptionKey<>(0);
 
-        @LayerVerifiedOption(kind = Kind.Changed, severity = Severity.Error)//
         @Option(help = "Physical memory size (in bytes). By default, the value is queried from the OS/container during VM startup.", type = OptionType.Expert)//
         public static final RuntimeOptionKey<Long> MaxRAM = new RuntimeOptionKey<>(0L, RegisterForIsolateArgumentParser);
 
@@ -1259,7 +1260,6 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Integer o
     @Option(help = "For internal purposes only. Disables type id result verification even when running with assertions enabled.", stability = OptionStability.EXPERIMENTAL, type = OptionType.Debug)//
     public static final HostedOptionKey<Boolean> DisableTypeIdResultVerification = new HostedOptionKey<>(true);
 
-    @LayerVerifiedOption(kind = Kind.Changed, severity = Severity.Error)//
     @Option(help = "Enables signal handling", stability = OptionStability.EXPERIMENTAL, type = Expert)//
     public static final RuntimeOptionKey<Boolean> EnableSignalHandling = new RuntimeOptionKey<>(null, Immutable) {
         @Override

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/LayerVerifiedOption.java

@@ -97,4 +97,14 @@ enum Kind {
      * specify it somewhere in its sequence of options.
      */
     boolean positional() default true;
+
+    /**
+     * If the {@code HostedOption} field (this annotation is used with) also has {@link APIOption}
+     * annotations, this annotation element can be used to bind this annotation to a specific
+     * {@link APIOption} annotation instead of being valid for all kinds of {@code HostedOption}
+     * use. Note that one can also have additional {@code @LayerVerifiedOption} annotations that do
+     * not make use of {@code apiOption} on the same {@code HostedOption} field to specify
+     * compatibility checking that should apply for raw (non-API) use of the option.
+     */
+    String apiOption() default "";
 }

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/HostedImageLayerBuildingSupport.java

@@ -28,6 +28,7 @@
 import java.nio.MappedByteBuffer;
 import java.nio.channels.FileChannel;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -43,10 +44,10 @@
 import com.oracle.svm.core.option.LayerVerifiedOption;
 import com.oracle.svm.core.option.LocatableMultiOptionValue.ValueWithOrigin;
 import com.oracle.svm.core.option.OptionUtils;
+import com.oracle.svm.core.option.RuntimeOptionKey;
 import com.oracle.svm.core.option.SubstrateOptionsParser;
 import com.oracle.svm.core.util.ArchiveSupport;
 import com.oracle.svm.core.util.UserError;
-import com.oracle.svm.core.util.VMError;
 import com.oracle.svm.hosted.ImageClassLoader;
 import com.oracle.svm.hosted.NativeImageClassLoaderSupport;
 import com.oracle.svm.hosted.c.NativeLibraries;
@@ -312,9 +313,10 @@ public static HostedImageLayerBuildingSupport initialize(HostedOptionValues valu
         return imageLayerBuildingSupport;
     }
 
-    record OptionLayerVerificationRequests(OptionDescriptor option, EconomicMap<LayerVerifiedOption.Kind, LayerVerifiedOption> requests) {
+    record OptionLayerVerificationRequests(OptionDescriptor option, List<LayerVerifiedOption> requests) {
         OptionLayerVerificationRequests(OptionDescriptor option) {
-            this(option, EconomicMap.create());
+            this(option, new ArrayList<>());
+            assert !(option.getOptionKey() instanceof RuntimeOptionKey) : "LayerVerifiedOption annotation on NI runtime-option";
         }
     }
 
@@ -326,23 +328,17 @@ public static Map<String, OptionLayerVerificationRequests> collectLayerVerificat
         Map<String, OptionLayerVerificationRequests> result = new HashMap<>();
         for (OptionDescriptor optionDescriptor : hostedOptions.getValues()) {
             for (LayerVerifiedOption layerVerification : OptionUtils.getAnnotationsByType(optionDescriptor, LayerVerifiedOption.class)) {
-                result.computeIfAbsent(optionDescriptor.getName(), key -> new OptionLayerVerificationRequests(optionDescriptor)).requests.put(layerVerification.kind(), layerVerification);
+                result.computeIfAbsent(optionDescriptor.getName(), key -> new OptionLayerVerificationRequests(optionDescriptor)).requests.add(layerVerification);
             }
         }
         return result;
     }
 
     @SuppressFBWarnings(value = "NP", justification = "FB reports null pointer dereferencing because it doesn't see through UserError.guarantee.")
     public static void setupSharedLayerLibrary(NativeLibraries nativeLibs) {
-        Path sharedLibPath = HostedImageLayerBuildingSupport.singleton().getLoadLayerArchiveSupport().getSharedLibraryPath();
-        Path parent = sharedLibPath.getParent();
-        VMError.guarantee(parent != null, "Shared layer library path doesn't have a parent.");
-        nativeLibs.getLibraryPaths().add(parent.toString());
-        Path fileName = sharedLibPath.getFileName();
-        VMError.guarantee(fileName != null, "Cannot determine shared layer library file name.");
-        String fullLibName = fileName.toString();
-        VMError.guarantee(fullLibName.startsWith("lib") && fullLibName.endsWith(".so"), "Expecting that shared layer library file starts with lib and ends with .so. Found: %s", fullLibName);
-        String libName = fullLibName.substring("lib".length(), fullLibName.length() - ".so".length());
+        LoadLayerArchiveSupport archiveSupport = HostedImageLayerBuildingSupport.singleton().getLoadLayerArchiveSupport();
+        nativeLibs.getLibraryPaths().add(archiveSupport.getSharedLibraryPath().toString());
+        String libName = archiveSupport.getSharedLibraryBaseName();
         HostedDynamicLayerInfo.singleton().registerLibName(libName);
         nativeLibs.addDynamicNonJniLibrary(libName);
     }

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/LayerArchiveSupport.java

@@ -53,6 +53,7 @@ public class LayerArchiveSupport {
     private static final String SNAPSHOT_GRAPHS_FILE_NAME = "layer-snapshot-graphs.big";
     private static final String LAYER_INFO_MESSAGE_PREFIX = "Native Image Layers";
     protected static final String LAYER_TEMP_DIR_PREFIX = "layerRoot_";
+    protected static final String SHARED_LIB_NAME_PREFIX = "lib";
 
     public static final String LAYER_FILE_EXTENSION = ".nil";
 
@@ -103,7 +104,11 @@ public Path getSnapshotGraphsPath() {
     }
 
     public Path getSharedLibraryPath() {
-        return layerDir.resolve(layerProperties.layerName() + ".so");
+        return layerDir;
+    }
+
+    public String getSharedLibraryBaseName() {
+        return layerProperties.layerName().substring(SHARED_LIB_NAME_PREFIX.length());
     }
 
     private static final Path layerPropertiesFileName = Path.of("META-INF/nilayer.properties");

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/LoadLayerArchiveSupport.java

@@ -27,11 +27,13 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import com.oracle.svm.core.option.LayerVerifiedOption;
@@ -105,59 +107,84 @@ private static boolean verifyCompatibility(List<String> previousArgs, List<Strin
         List<DiffResult<String>> diffResults = DiffTool.diffResults(filteredLeft, filteredRight);
         Map<DiffResult<String>, Severity> violations = new HashMap<>();
         for (var diffResult : diffResults) {
-            Set<Kind> verificationKinds = switch (diffResult.kind()) {
+            DiffResult.Kind diffResultKind = diffResult.kind();
+            Set<Kind> verificationKinds = switch (diffResultKind) {
+                case Equal -> Set.of();
                 case Removed -> Set.of(Kind.Removed, Kind.Changed);
                 case Added -> Set.of(Kind.Added, Kind.Changed);
-                default -> Set.of();
             };
-            for (Kind verificationKind : verificationKinds) {
-                ArgumentOrigin argumentOrigin = splitArgumentOrigin(diffResult.getEntry(left, right));
-                NameValue argumentNameAndValue = argumentOrigin.nameValue();
-                var perOptionVerifications = allRequests.get(argumentNameAndValue.name);
-                if (perOptionVerifications == null) {
-                    continue;
-                }
-                LayerVerifiedOption request = perOptionVerifications.requests().get(verificationKind);
-                if (request == null || request.positional() != positional) {
-                    continue;
-                }
+            if (verificationKinds.isEmpty()) {
+                continue;
+            }
 
-                OptionOrigin origin = OptionOrigin.from(argumentOrigin.origin);
-                String argument = SubstrateOptionsParser.commandArgument(perOptionVerifications.option().getOptionKey(), argumentNameAndValue.value);
-                String message = switch (diffResult.kind()) {
-                    case Removed -> "Previous layer was";
-                    case Added -> "Current layer gets";
-                    case Equal -> throw VMError.shouldNotReachHere("diff for equal");
-                } + " built with option argument '" + argument + "' from " + origin + ".";
-                String suffix;
-                if (!request.message().isEmpty()) {
-                    suffix = request.message();
-                } else {
-                    /* fallback to generic verification message */
-                    suffix = "This is also required to be specified for the " + switch (diffResult.kind()) {
-                        case Removed -> "current layered image build";
-                        case Added -> "previous layer build";
-                        case Equal -> throw VMError.shouldNotReachHere("diff for equal");
-                    };
+            ArgumentOrigin argumentOrigin = splitArgumentOrigin(diffResult.getEntry(left, right));
+            NameValue argumentNameAndValue = argumentOrigin.nameValue();
+            var perOptionVerifications = allRequests.get(argumentNameAndValue.name);
+            if (perOptionVerifications == null) {
+                continue;
+            }
+
+            List<LayerVerifiedOption> requests = perOptionVerifications.requests().stream()
+                            .filter(request -> request.positional() == positional)
+                            .collect(Collectors.toList());
+            String argument = SubstrateOptionsParser.commandArgument(perOptionVerifications.option().getOptionKey(), argumentNameAndValue.value);
+            List<LayerVerifiedOption> matchingAPIRequest = new ArrayList<>();
+            requests.removeIf(request -> {
+                if (request.apiOption().isEmpty()) {
+                    // Keep all non-API requests
+                    return false;
                 }
-                message += " " + suffix + (positional ? " at the same position." : ".");
-                Severity severity = request.severity();
-                violations.put(diffResult, severity);
-                if (verbose) {
-                    LogUtils.info("Error: ", message);
-                } else {
-                    switch (severity) {
-                        case Warn -> LogUtils.warning(message);
-                        case Error -> {
-                            if (strict) {
-                                UserError.abort(message);
-                            } else {
-                                LogUtils.warning(message);
-                            }
-                        }
-                    }
+                // Do record matching API requests ...
+                if (request.apiOption().equals(argument)) {
+                    matchingAPIRequest.add(request);
                 }
+                // ... but remove all API request entries
+                return true;
+            });
+            if (!matchingAPIRequest.isEmpty()) {
+                /*
+                 * If we have a @LayerVerifiedOption annotation with a matching apiOption set, we
+                 * ignore other @LayerVerifiedOption annotations that do not have apiOption set.
+                 */
+                requests = matchingAPIRequest;
             }
+
+            requests.stream()
+                            .filter(request -> verificationKinds.contains(request.kind()))
+                            .forEach(request -> {
+
+                                String message = switch (diffResultKind) {
+                                    case Removed -> "Previous layer was";
+                                    case Added -> "Current layer gets";
+                                    case Equal -> throw VMError.shouldNotReachHere("diff for equal");
+                                } + " built with option argument '" + argument + "' from " + OptionOrigin.from(argumentOrigin.origin) + ".";
+                                if (!request.message().isEmpty()) {
+                                    message += " " + request.message();
+                                } else {
+                                    /* fallback to generic verification message */
+                                    message += " This is also required to be specified for the " + switch (diffResultKind) {
+                                        case Removed -> "current layered image build";
+                                        case Added -> "previous layer build";
+                                        case Equal -> throw VMError.shouldNotReachHere("diff for equal");
+                                    } + (positional ? " at the same position." : ".");
+                                }
+                                Severity severity = request.severity();
+                                violations.put(diffResult, severity);
+                                if (verbose) {
+                                    LogUtils.info("Error: ", message);
+                                } else {
+                                    switch (severity) {
+                                        case Warn -> LogUtils.warning(message);
+                                        case Error -> {
+                                            if (strict) {
+                                                UserError.abort(message);
+                                            } else {
+                                                LogUtils.warning(message);
+                                            }
+                                        }
+                                    }
+                                }
+                            });
         }
 
         boolean violationsFound = !violations.isEmpty();

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/WriteLayerArchiveSupport.java

@@ -31,6 +31,8 @@
 
 import com.oracle.svm.core.BuildArtifacts;
 import com.oracle.svm.core.BuildArtifacts.ArtifactType;
+import com.oracle.svm.core.SubstrateOptions;
+import com.oracle.svm.core.option.SubstrateOptionsParser;
 import com.oracle.svm.core.util.ArchiveSupport;
 import com.oracle.svm.core.util.UserError;
 import com.oracle.svm.hosted.NativeImageClassLoaderSupport;
@@ -41,6 +43,11 @@ public class WriteLayerArchiveSupport extends LayerArchiveSupport {
 
     public WriteLayerArchiveSupport(String layerName, NativeImageClassLoaderSupport classLoaderSupport, Path tempDir, ArchiveSupport archiveSupport) {
         super(layerName, classLoaderSupport.getLayerFile(), tempDir.resolve(LAYER_TEMP_DIR_PREFIX + "write"), archiveSupport);
+        if (!layerName.startsWith(SHARED_LIB_NAME_PREFIX)) {
+            throw UserError.abort("Shared layer library image name given with '" +
+                            SubstrateOptionsParser.commandArgument(SubstrateOptions.Name, layerName) +
+                            "' needs to start with '" + SHARED_LIB_NAME_PREFIX + "'");
+        }
         builderArguments.addAll(classLoaderSupport.getHostedOptionParser().getArguments());
     }