[GR-48393] Fix conditional configuration algorithm to avoid losing configuration data

PullRequest: graal/19770

Author: DSouzaM

substratevm/ci/ci.jsonnet

@@ -122,7 +122,7 @@
       // but since we support JDK 21 anyways, there is not good reason to do so.
       "linux:amd64:jdk21": gate + t("30:00"),
     }),
-    "basics": mxgate("build,helloworld,native_unittests,truffle_unittests,debuginfotest,hellomodule,java_agent") + maven + jsonschema + platform_spec(no_jobs) + platform_spec({
+    "basics": mxgate("build,helloworld,native_unittests,truffle_unittests,debuginfotest,hellomodule,java_agent,condconfig") + maven + jsonschema + platform_spec(no_jobs) + platform_spec({
       "linux:amd64:jdk-latest": gate + gdb("14.2") + t("55:00"),
       "windows:amd64:jdk-latest": gate + t("1:30:00"),
     }) + variants({

substratevm/mx.substratevm/mx_substratevm.py

@@ -566,26 +566,28 @@ def conditional_config_task(native_image):
     agent_path = build_native_image_agent(native_image)
     conditional_config_filter_path = join(svmbuild_dir(), 'conditional-config-filter.json')
     with open(conditional_config_filter_path, 'w') as conditional_config_filter:
-        conditional_config_filter.write(
-'''
+        conditional_config_filter.write('''
 {
    "rules": [
         {"includeClasses": "com.oracle.svm.configure.test.conditionalconfig.**"}
    ]
 }
-'''
-        )
-
+''')
     run_agent_conditional_config_test(agent_path, conditional_config_filter_path)
-
     run_nic_conditional_config_test(agent_path, conditional_config_filter_path)
 
 
 def run_nic_conditional_config_test(agent_path, conditional_config_filter_path):
+    """
+    Invoke ConfigurationGenerator test methods across multiple runs to produce multiple partial traces,
+    use native-image-configure to compute the conditional configuration, then compare against the expected
+    configuration.
+    """
     test_cases = [
         "createConfigPartOne",
         "createConfigPartTwo",
         "createConfigPartThree",
+        "createConfigPartFour",
     ]
     config_directories = []
     nic_test_dir = join(svmbuild_dir(), 'nic-cond-config-test')
@@ -602,10 +604,11 @@ def run_nic_conditional_config_test(agent_path, conditional_config_filter_path):
                       'com.oracle.svm.configure.test.conditionalconfig.PartialConfigurationGenerator#' + test_case])
     config_output_dir = join(nic_test_dir, 'config-output')
     nic_exe = mx.cmd_suffix(join(mx.JDKConfig(home=mx_sdk_vm_impl.graalvm_output()).home, 'bin', 'native-image-configure'))
-    nic_command = [nic_exe, 'create-conditional'] \
-                  + ['--user-code-filter=' + conditional_config_filter_path] \
-                  + ['--input-dir=' + config_dir for config_dir in config_directories] \
-                  + ['--output-dir=' + config_output_dir]
+    nic_command = [nic_exe, 'generate-conditional',
+                   '--user-code-filter=' + conditional_config_filter_path,
+                   '--class-name-filter=' + conditional_config_filter_path,
+                   '--output-dir=' + config_output_dir] \
+                  + ['--input-dir=' + config_dir for config_dir in config_directories]
     mx.run(nic_command)
     jvm_unittest(
         ['-Dcom.oracle.svm.configure.test.conditionalconfig.ConfigurationVerifier.configpath=' + config_output_dir,
@@ -619,7 +622,8 @@ def run_agent_conditional_config_test(agent_path, conditional_config_filter_path
         mx.rmtree(config_dir)
 
     agent_opts = ['config-output-dir=' + config_dir,
-                  'experimental-conditional-config-filter-file=' + conditional_config_filter_path]
+                  'experimental-conditional-config-filter-file=' + conditional_config_filter_path,
+                  'conditional-config-class-filter-file=' + conditional_config_filter_path]
     # This run generates the configuration from different test cases
     jvm_unittest(['-agentpath:' + agent_path + '=' + ','.join(agent_opts),
                   '-Dcom.oracle.svm.configure.test.conditionalconfig.ConfigurationGenerator.enabled=true',

substratevm/src/com.oracle.svm.configure.test/src/com/oracle/svm/configure/test/conditionalconfig/ConfigurationGenerator.java

@@ -24,21 +24,27 @@
  */
 package com.oracle.svm.configure.test.conditionalconfig;
 
+import static org.junit.Assume.assumeTrue;
+
 import java.lang.reflect.Proxy;
 
 import org.junit.Assert;
 import org.junit.Test;
 
+/**
+ * Test that generates configuration metadata when traced by the agent. This output is tested
+ * against the expected output in the config-dir directory by {@link ConfigurationVerifier}. This
+ * test is invoked manually from mx.
+ */
 public class ConfigurationGenerator {
 
     @Test
     public void createTestConfig() {
-        if (!Boolean.getBoolean(ConfigurationGenerator.class.getName() + ".enabled")) {
-            return;
-        }
+        assumeTrue("Test must be explicitly enabled because it is not designed for regular execution", Boolean.getBoolean(ConfigurationGenerator.class.getName() + ".enabled"));
         NoPropagationNecessary.runTest();
         PropagateToParent.runTest();
         PropagateButLeaveCommonConfiguration.runTest();
+        PropagateThroughRecursiveCall.runTest();
     }
 
 }
@@ -174,14 +180,65 @@ interface IC {
     }
 
     private static final class Util {
-        static void doWork(String clazz, String resource, Class<?>... intefaceList) {
+        static void doWork(String clazz, String resource, Class<?>... interfaceList) {
             ClassUtil.forName(clazz);
             ClassUtil.forName("PropagateButLeaveCommonConfiguration$C");
             ClassUtil.getResource(resource);
             ClassUtil.getResource("Common.txt");
-            ClassUtil.createProxy(intefaceList);
+            ClassUtil.createProxy(interfaceList);
             ClassUtil.createProxy(IC.class);
         }
     }
 
 }
+
+/**
+ * This is a regression test. It ensures that configuration propagated from a recursive call does
+ * not get lost (which can happen unless the configuration is propagated past any recursive
+ * callers).
+ */
+@SuppressWarnings("unused")
+class PropagateThroughRecursiveCall {
+
+    public static void runTest() {
+        ParentA.doWork();
+        ParentB.doWork();
+    }
+
+    private static final class ParentA {
+        static void doWork() {
+            Recursive.recur(true, "PropagateThroughRecursiveCall$A");
+        }
+    }
+
+    private static final class ParentB {
+        static void doWork() {
+            Recursive.recur(true, "PropagateThroughRecursiveCall$B");
+        }
+    }
+
+    private static final class A1 {
+    }
+
+    private static final class A2 {
+    }
+
+    private static final class B1 {
+    }
+
+    private static final class B2 {
+    }
+
+    private static final class Recursive {
+        static void recur(boolean recurse, String clazz) {
+            ClassUtil.forName("PropagateThroughRecursiveCall$Recursive");
+            if (recurse) {
+                ClassUtil.forName(clazz + "1");
+                recur(false, clazz);
+            } else {
+                ClassUtil.forName(clazz + "2");
+            }
+
+        }
+    }
+}

substratevm/src/com.oracle.svm.configure.test/src/com/oracle/svm/configure/test/conditionalconfig/ConfigurationVerifier.java

@@ -24,6 +24,8 @@
  */
 package com.oracle.svm.configure.test.conditionalconfig;
 
+import static org.junit.Assume.assumeTrue;
+
 import java.io.IOException;
 import java.io.StringWriter;
 import java.net.URL;
@@ -35,21 +37,24 @@
 import com.oracle.svm.configure.ConfigurationBase;
 import com.oracle.svm.configure.config.ConfigurationFileCollection;
 import com.oracle.svm.configure.config.ConfigurationSet;
+import com.oracle.svm.configure.test.AddExports;
 import com.oracle.svm.core.configure.ConfigurationFile;
 import com.oracle.svm.core.util.VMError;
 
 import jdk.graal.compiler.util.json.JsonWriter;
 
+/**
+ * Test that validates the configuration metadata produced by the agent. This test is invoked
+ * manually from mx.
+ */
+@AddExports({"org.graalvm.nativeimage/org.graalvm.nativeimage.impl", "jdk.graal.compiler/jdk.graal.compiler.util"})
 public class ConfigurationVerifier {
 
     public static final String CONFIG_PATH_PROPERTY = ConfigurationVerifier.class.getName() + ".configpath";
 
     @Test
     public void testConfig() throws Exception {
-        String enabledProperty = System.getProperty(ConfigurationVerifier.class.getName() + ".enabled");
-        if (!Boolean.parseBoolean(enabledProperty)) {
-            return;
-        }
+        assumeTrue("Test must be explicitly enabled because it is not designed for regular execution", Boolean.getBoolean(ConfigurationVerifier.class.getName() + ".enabled"));
         ConfigurationSet actualConfig = loadActualConfig();
         ConfigurationSet expectedConfig = loadExpectedConfig();
 

substratevm/src/com.oracle.svm.configure.test/src/com/oracle/svm/configure/test/conditionalconfig/PartialConfigurationGenerator.java

@@ -24,14 +24,20 @@
  */
 package com.oracle.svm.configure.test.conditionalconfig;
 
+import static org.junit.Assume.assumeTrue;
+
 import org.junit.Test;
 
+/**
+ * Like {@link ConfigurationGenerator}, but performs the work across multiple test methods. The
+ * agent is run separately for each and configured to emit "partial" configurations, and then the
+ * results are combined using the {@code native-image-configure generate-conditional} command. This
+ * test is invoked manually from mx.
+ */
 public class PartialConfigurationGenerator {
 
     private static void runIfEnabled(Runnable runnable) {
-        if (!Boolean.getBoolean(PartialConfigurationGenerator.class.getName() + ".enabled")) {
-            return;
-        }
+        assumeTrue("Test must be explicitly enabled because it is not designed for regular execution", Boolean.getBoolean(PartialConfigurationGenerator.class.getName() + ".enabled"));
         runnable.run();
     }
 
@@ -49,4 +55,9 @@ public void createConfigPartTwo() {
     public void createConfigPartThree() {
         runIfEnabled(PropagateButLeaveCommonConfiguration::runTest);
     }
+
+    @Test
+    public void createConfigPartFour() {
+        runIfEnabled(PropagateThroughRecursiveCall::runTest);
+    }
 }