[GR-24679] Diagnose should imply DumpOnError.

PullRequest: graal/10100

common.hocon

@@ -43,9 +43,7 @@ labsjdk-ce-17Debug : { downloads : { JAVA_HOME : ${jdks.labsjdk-ce-17Debug} }}
 labsjdk-ee-17Debug : { downloads : { JAVA_HOME : ${jdks.labsjdk-ee-17Debug} }}
 
 common : ${mx} ${deps.common} {
-  catch_files : [
-    "Graal diagnostic output saved in (?P<filename>.+\.zip)"
-  ]
+  catch_files : ${catch_files}
 }
 
 linux : ${common} ${deps.linux}

common.json

@@ -18,13 +18,19 @@
     "labsjdk-ee-17Debug": {"name": "labsjdk",   "version": "ee-17.0.1+9-jvmci-21.3-b03-debug", "platformspecific": true }
   },
 
-  "COMMENT" : "The devkits versions reflect those used to build the JVMCI JDKs (e.g., see devkit_platform_revisions in <jdk>/make/conf/jib-profiles.js)",
+  "COMMENT.devkits" : "The devkits versions reflect those used to build the JVMCI JDKs (e.g., see devkit_platform_revisions in <jdk>/make/conf/jib-profiles.js)",
   "devkits": {
       "windows-oraclejdk8": { "packages" : { "devkit:VS2017-15.9.16+1" : "==0" }},
       "windows-openjdk8":   { "packages" : { "devkit:VS2017-15.9.16+1" : "==0" }},
       "windows-jdk11":      { "packages" : { "devkit:VS2017-15.9.24+1" : "==0" }},
       "windows-jdk17":      { "packages" : { "devkit:VS2019-16.9.3+1"  : "==0" }}
   },
+
+  "catch_files" : [
+    "Graal diagnostic output saved in '(?P<filename>[^']+)'",
+    "Dumping debug output to '(?P<filename>[^']+)'"
+  ],
+
   "deps": {
     "COMMENT.common": [
       "pip:isort is a dependency of pip:pylint. The explicit dependency on the pip package works around",

common.jsonnet

@@ -105,9 +105,7 @@
     # enforce self.arch (useful for generating job names)
     arch:: error "self.arch not set",
     capabilities +: [],
-    catch_files +: [
-      "Graal diagnostic output saved in (?P<filename>.+\\.zip)"
-    ]
+    catch_files +: common_json.catch_files
   },
 
   linux:: deps.linux + self.common + {

compiler/mx.compiler/mx_compiler.py

@@ -43,7 +43,7 @@
 
 import mx
 import mx_gate
-from mx_gate import Task
+from mx_gate import Task, Tags
 from mx import SafeDirectoryUpdater
 
 import mx_unittest
@@ -158,6 +158,23 @@ def source_supplier():
 mx_gate.add_jacoco_excludes(['com.oracle.truffle'])
 mx_gate.add_jacoco_excluded_annotations(['@Snippet', '@ClassSubstitution'])
 
+def _get_graal_option(vmargs, name, default=None, prefix='-Dgraal.'):
+    """
+    Gets the value of the `name` Graal option in `vmargs`.
+
+    :param list vmargs: VM arguments to inspect
+    :param str name: the name of the option
+    :param default: the default value of the option if it's not present in `vmargs`
+    :param str prefix: the prefix used for Graal options in `vmargs`
+    :return: the value of the option as specified in `vmargs` or `default`
+    """
+    if vmargs:
+        for arg in reversed(vmargs):
+            selector = prefix + name + '='
+            if arg.startswith(selector):
+                return arg[len(selector):]
+    return default
+
 def _get_XX_option_value(vmargs, name, default):
     """
     Gets the value of an ``-XX:`` style HotSpot VM option.
@@ -371,13 +388,44 @@ def _is_batik_supported(jdk):
         mx.warn('Batik uses Sun internal class com.sun.image.codec.jpeg.TruncatedFileException which is not present in ' + jdk.home)
         return False
 
+def _compiler_error_options(default_compilation_failure_action='ExitVM', vmargs=None, prefix='-Dgraal.'):
+    """
+    Gets options to be prefixed to the VM command line related to Graal compilation errors to improve
+    the chance of graph dumps being emitted and preserved in CI build logs.
+
+    :param str default_compilation_failure_action: value for CompilationFailureAction if it is added
+    :param list vmargs: arguments to search for existing instances of the options added by this method
+    :param str prefix: the prefix used for Graal options in `vmargs` and to use when adding options
+    """
+    action = _get_graal_option(vmargs, 'CompilationFailureAction')
+    res = []
+
+    # Add CompilationFailureAction if absent from vmargs
+    if action is None:
+        action = default_compilation_failure_action
+        res.append(prefix + 'CompilationFailureAction=' + action)
+
+    # Add DumpOnError=true if absent from vmargs and CompilationFailureAction is Diagnose or ExitVM.
+    dump_on_error = _get_graal_option(vmargs, 'DumpOnError', prefix=prefix)
+    if action in ('Diagnose', 'ExitVM'):
+        if dump_on_error is None:
+            res.append(prefix + 'DumpOnError=true')
+            dump_on_error = 'true'
+
+    # Add ShowDumpFiles=true if absent from vmargs and DumpOnError=true.
+    if dump_on_error == 'true':
+        show_dump_files = _get_graal_option(vmargs, 'ShowDumpFiles', prefix=prefix)
+        if show_dump_files is None:
+            res.append(prefix + 'ShowDumpFiles=true')
+    return res
+
 def _gate_dacapo(name, iterations, extraVMarguments=None, force_serial_gc=True, set_start_heap_size=True, threads=None):
     if iterations == -1:
         return
     vmargs = ['-XX:+UseSerialGC'] if force_serial_gc else []
     if set_start_heap_size:
         vmargs += ['-Xms2g']
-    vmargs += ['-XX:-UseCompressedOops', '-Djava.net.preferIPv4Stack=true', '-Dgraal.CompilationFailureAction=ExitVM'] + _remove_empty_entries(extraVMarguments)
+    vmargs += ['-XX:-UseCompressedOops', '-Djava.net.preferIPv4Stack=true'] + _compiler_error_options() + _remove_empty_entries(extraVMarguments)
     args = ['-n', str(iterations), '--preserve']
     if threads is not None:
         args += ['-t', str(threads)]
@@ -390,12 +438,59 @@ def jdk_includes_corba(jdk):
 def _gate_scala_dacapo(name, iterations, extraVMarguments=None):
     if iterations == -1:
         return
-    vmargs = ['-Xms2g', '-XX:+UseSerialGC', '-XX:-UseCompressedOops', '-Dgraal.CompilationFailureAction=ExitVM'] + _remove_empty_entries(extraVMarguments)
+    vmargs = ['-Xms2g', '-XX:+UseSerialGC', '-XX:-UseCompressedOops'] + _compiler_error_options() + _remove_empty_entries(extraVMarguments)
 
     args = ['-n', str(iterations), '--preserve']
     return mx_benchmark.gate_mx_benchmark(["scala-dacapo:{}".format(name), "--tracker=none", "--"] + vmargs + ["--"] + args)
 
+def _check_catch_files():
+    """
+    Verifies that there is a "catch_files" array in common.json at the root of
+    the repository containing this suite and that the array contains elements
+    matching DebugContext.DUMP_FILE_MESSAGE_REGEXP and
+    StandardPathUtilitiesProvider.DIAGNOSTIC_OUTPUT_DIRECTORY_MESSAGE_REGEXP.
+    """
+    catch_files_fields = (
+        ('DebugContext', 'DUMP_FILE_MESSAGE_REGEXP'),
+        ('StandardPathUtilitiesProvider', 'DIAGNOSTIC_OUTPUT_DIRECTORY_MESSAGE_REGEXP')
+    )
+
+    def get_regexp(class_name, field_name):
+        source_path = join(_suite.dir, 'src', 'org.graalvm.compiler.debug', 'src', 'org', 'graalvm', 'compiler', 'debug', class_name + '.java')
+        regexp = None
+        with open(source_path) as fp:
+            for line in fp.readlines():
+                decl = field_name + ' = "'
+                index = line.find(decl)
+                if index != -1:
+                    start_index = index + len(decl)
+                    end_index = line.find('"', start_index)
+                    regexp = line[start_index:end_index]
+
+                    # Convert from Java style regexp to Python style
+                    return regexp.replace('(?<', '(?P<')
+
+        if not regexp:
+            mx.abort('Could not find value of ' + field_name + ' in ' + source_path)
+        return regexp
+
+    common_path = join(dirname(_suite.dir), 'common.json')
+    if not exists(common_path):
+        mx.abort('Required file does not exist: {}'.format(common_path))
+    with open(common_path) as common_file:
+        common_cfg = json.load(common_file)
+    catch_files = common_cfg.get('catch_files')
+    if catch_files is None:
+        mx.abort('Could not find catch_files attribute in {}'.format(common_path))
+    for class_name, field_name in catch_files_fields:
+        regexp = get_regexp(class_name, field_name)
+        if regexp not in catch_files:
+            mx.abort('Could not find catch_files entry in {} matching "{}"'.format(common_path, regexp))
+
 def compiler_gate_runner(suites, unit_test_runs, bootstrap_tests, tasks, extraVMarguments=None, extraUnitTestArguments=None):
+    with Task('CheckCatchFiles', tasks, tags=[Tags.style]) as t:
+        if t: _check_catch_files()
+
     if jdk.javaCompliance >= '9':
         with Task('JDK_java_base_test', tasks, tags=['javabasetest']) as t:
             if t: java_base_unittest(_remove_empty_entries(extraVMarguments) + [])
@@ -426,7 +521,8 @@ def compiler_gate_runner(suites, unit_test_runs, bootstrap_tests, tasks, extraVM
     with Task('CTW:hosted', tasks, tags=GraalTags.ctw) as t:
         if t:
             ctw([
-                    '-DCompileTheWorld.Config=Inline=false CompilationFailureAction=ExitVM CompilationBailoutAsFailure=false', '-esa', '-XX:-UseJVMCICompiler', '-XX:+EnableJVMCI',
+                    '-DCompileTheWorld.Config=Inline=false CompilationBailoutAsFailure=false ' + ' '.join(_compiler_error_options(prefix='')),
+                    '-esa', '-XX:-UseJVMCICompiler', '-XX:+EnableJVMCI',
                     '-DCompileTheWorld.MultiThreaded=true', '-Dgraal.InlineDuringParsing=false', '-Dgraal.TrackNodeSourcePosition=true',
                     '-DCompileTheWorld.Verbose=false', '-XX:ReservedCodeCacheSize=300m',
                 ], _remove_empty_entries(extraVMarguments))
@@ -540,7 +636,7 @@ def compiler_gate_benchmark_runner(tasks, extraVMarguments=None, prefix=''):
 
 _defaultFlags = ['-Dgraal.CompilationWatchDogStartDelay=60.0D']
 _assertionFlags = ['-esa', '-Dgraal.DetailedAsserts=true']
-_graalErrorFlags = ['-Dgraal.CompilationFailureAction=ExitVM']
+_graalErrorFlags = _compiler_error_options()
 _graalEconomyFlags = ['-Dgraal.CompilerConfiguration=economy']
 _verificationFlags = ['-Dgraal.VerifyGraalGraphs=true', '-Dgraal.VerifyGraalGraphEdges=true', '-Dgraal.VerifyGraalPhasesSize=true', '-Dgraal.VerifyPhases=true']
 _coopFlags = ['-XX:-UseCompressedOops']
@@ -716,8 +812,7 @@ def _parseVmArgs(args, addDefaultArgs=True):
     # The default for CompilationFailureAction in the code is Silent as this is
     # what we want for GraalVM. When using Graal via mx (e.g. in the CI gates)
     # Diagnose is a more useful "default" value.
-    if not any(a.startswith('-Dgraal.CompilationFailureAction=') for a in args):
-        argsPrefix.append('-Dgraal.CompilationFailureAction=Diagnose')
+    argsPrefix.extend(_compiler_error_options('Diagnose', args))
 
     # It is safe to assume that Network dumping is the desired default when using mx.
     # Mx is never used in production environments.

compiler/src/org.graalvm.compiler.core.test/src/org/graalvm/compiler/core/test/DumpPathTest.java

@@ -32,6 +32,7 @@
 
 import org.graalvm.collections.EconomicMap;
 import org.graalvm.compiler.debug.DebugOptions;
+import org.graalvm.compiler.debug.TTY;
 import org.graalvm.compiler.debug.DebugOptions.PrintGraphTarget;
 import org.graalvm.compiler.options.OptionKey;
 import org.graalvm.compiler.options.OptionValues;
@@ -47,21 +48,25 @@ public static Object snippet() {
         return new String("snippet");
     }
 
+    @SuppressWarnings("try")
     @Test
-    public void testDump() throws IOException {
+    public void testDump() throws Exception {
         assumeManagementLibraryIsLoadable();
         try (TemporaryDirectory temp = new TemporaryDirectory(Paths.get("."), "DumpPathTest")) {
             String[] extensions = new String[]{".cfg", ".bgv", ".graph-strings"};
             EconomicMap<OptionKey<?>, Object> overrides = OptionValues.newOptionMap();
             overrides.put(DebugOptions.DumpPath, temp.toString());
+            overrides.put(DebugOptions.ShowDumpFiles, false);
             overrides.put(DebugOptions.PrintCFG, true);
             overrides.put(DebugOptions.PrintGraph, PrintGraphTarget.File);
             overrides.put(DebugOptions.PrintCanonicalGraphStrings, true);
             overrides.put(DebugOptions.Dump, "*");
             overrides.put(DebugOptions.MethodFilter, null);
 
-            // Generate dump files.
-            test(new OptionValues(getInitialOptions(), overrides), "snippet");
+            try (AutoCloseable c = new TTY.Filter()) {
+                // Generate dump files.
+                test(new OptionValues(getInitialOptions(), overrides), "snippet");
+            }
             // Check that IGV files got created, in the right place.
             checkForFiles(temp.path, extensions);
         }

compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugContext.java

@@ -84,6 +84,21 @@
  */
 public final class DebugContext implements AutoCloseable {
 
+    /**
+     * The format of the message printed on the console by {@link #getDumpPath} when
+     * {@link DebugOptions#ShowDumpFiles} is true. The {@code %s} placeholder is replaced with the
+     * absolute path of the dump file (i.e. the value returned by the method).
+     */
+    public static final String DUMP_FILE_MESSAGE_FORMAT = "Dumping debug output to '%s'";
+
+    /**
+     * The regular expression for matching the message derived from
+     * {@link #DUMP_FILE_MESSAGE_FORMAT}.
+     *
+     * Keep in sync with the {@code catch_files} array in {@code common.json}.
+     */
+    public static final String DUMP_FILE_MESSAGE_REGEXP = "Dumping debug output to '(?<filename>[^']+)'";
+
     public static final Description NO_DESCRIPTION = new Description(null, "NO_DESCRIPTION");
     public static final GlobalMetrics NO_GLOBAL_METRIC_VALUES = null;
     public static final Iterable<DebugHandlersFactory> NO_CONFIG_CUSTOMIZERS = Collections.emptyList();
@@ -613,7 +628,7 @@ public String getDumpPath(String extension, boolean createMissingDirectory) {
             String label = description == null ? null : description.getLabel();
             String result = PathUtilities.createUnique(immutable.options, DumpPath, id, label, extension, createMissingDirectory);
             if (ShowDumpFiles.getValue(immutable.options)) {
-                TTY.println("Dumping debug output to %s", result);
+                TTY.println(DUMP_FILE_MESSAGE_FORMAT, result);
             }
             return result;
         } catch (IOException ex) {

compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugOptions.java

@@ -124,7 +124,7 @@ public enum PrintGraphTarget {
     public static final OptionKey<Boolean> DebugStubsAndSnippets = new OptionKey<>(false);
     @Option(help = "Send compiler IR to dump handlers on error.", type = OptionType.Debug)
     public static final OptionKey<Boolean> DumpOnError = new OptionKey<>(false);
-    @Option(help = "Specify the DumpLevel if CompilationFailureAction#Diagnose is used." +
+    @Option(help = "Specify the dump level if CompilationFailureAction#Diagnose is used." +
                     "See CompilationFailureAction for details. file:doc-files/CompilationFailureActionHelp.txt", type = OptionType.Debug)
     public static final OptionKey<Integer> DiagnoseDumpLevel = new OptionKey<>(DebugContext.VERBOSE_LEVEL);
     @Option(help = "Disable intercepting exceptions in debug scopes.", type = OptionType.Debug)
@@ -196,6 +196,13 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean o
 
     // @formatter:on
 
+    /**
+     * The format of the message printed on the console by {@link #getDumpDirectory} when
+     * {@link DebugOptions#ShowDumpFiles} is true. The {@code %s} placeholder is replaced with the
+     * value returned by {@link #getDumpDirectory}.
+     */
+    private static final String DUMP_DIRECTORY_MESSAGE_FORMAT = "Dumping debug output in '%s'";
+
     /**
      * Gets the directory in which {@link DebugDumpHandler}s can generate output. This will be the
      * directory specified by {@link #DumpPath} if it has been set otherwise it will be derived from
@@ -214,7 +221,7 @@ public static String getDumpDirectory(OptionValues options) throws IOException {
                 if (!exists(dumpDir)) {
                     createDirectories(dumpDir);
                     if (ShowDumpFiles.getValue(options)) {
-                        TTY.println("Dumping debug output in %s", dumpDir.toString());
+                        TTY.println(DUMP_DIRECTORY_MESSAGE_FORMAT, dumpDir);
                     }
                 }
             }

compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/StandardPathUtilitiesProvider.java

@@ -52,6 +52,22 @@
  * A {@link PathUtilitiesProvider} implemented in terms of {@link File} and {@link Path} APIs.
  */
 public class StandardPathUtilitiesProvider implements PathUtilitiesProvider {
+
+    /**
+     * The format of the message printed on the console by {@link #archiveAndDelete} showing the
+     * absolute path of the zip file created. The {@code %s} placeholder is replaced with the
+     * absolute path of the zip file.
+     */
+    public static final String DIAGNOSTIC_OUTPUT_DIRECTORY_MESSAGE_FORMAT = "Graal diagnostic output saved in '%s'";
+
+    /**
+     * The regular expression for matching the message derived from
+     * {@link #DIAGNOSTIC_OUTPUT_DIRECTORY_MESSAGE_FORMAT}.
+     *
+     * Keep in sync with the {@code catch_files} array in {@code common.json}.
+     */
+    public static final String DIAGNOSTIC_OUTPUT_DIRECTORY_MESSAGE_REGEXP = "Graal diagnostic output saved in '(?<filename>[^']+)'";
+
     @Override
     public String createDirectories(String path) throws IOException {
         Files.createDirectories(Paths.get(path));
@@ -175,7 +191,7 @@ public FileVisitResult postVisitDirectory(Path d, IOException exc) throws IOExce
                     }
                 });
                 // Keep this in sync with the catch_files in common.hocon
-                TTY.println("Graal diagnostic output saved in %s", zipFile);
+                TTY.println(DIAGNOSTIC_OUTPUT_DIRECTORY_MESSAGE_FORMAT, zipFile);
                 return zipFile.getAbsolutePath();
             } catch (IOException e) {
                 toDelete.clear();

compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Graph.java

@@ -582,7 +582,11 @@ final void event(NodeEvent e, Node node) {
                     inputChanged(node);
                     break;
                 case ZERO_USAGES:
+                    GraalError.guarantee(node.isAlive(), "must be alive");
                     usagesDroppedToZero(node);
+                    if (!node.isAlive()) {
+                        throw new GraalError("%s must not kill %s", this, node);
+                    }
                     break;
                 case NODE_ADDED:
                     nodeAdded(node);