IGV should always convert collections to Strings early

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

@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package org.graalvm.compiler.core.test;
+
+import java.io.IOException;
+import java.nio.file.Paths;
+import java.util.Arrays;
+
+import org.graalvm.collections.EconomicMap;
+import org.graalvm.compiler.debug.DebugOptions;
+import org.graalvm.compiler.debug.DebugOptions.PrintGraphTarget;
+import org.graalvm.compiler.nodes.StructuredGraph;
+import org.graalvm.compiler.options.OptionKey;
+import org.graalvm.compiler.options.OptionValues;
+import org.junit.Test;
+
+import jdk.vm.ci.meta.ResolvedJavaMethod;
+
+/**
+ * Force a dump with a lot of detail to exercise encoding step.
+ */
+public class GraphDumpWithAssertionsTest extends GraalCompilerTest {
+
+    public static Object snippet(Object[] array) {
+        return Arrays.toString(array);
+    }
+
+    @Test
+    public void testDump() throws IOException {
+        try (TemporaryDirectory temp = new TemporaryDirectory(Paths.get("."), "GraphDumpWithAssertionsTest")) {
+            EconomicMap<OptionKey<?>, Object> overrides = OptionValues.newOptionMap();
+            overrides.put(DebugOptions.DumpPath, temp.toString());
+            overrides.put(DebugOptions.Dump, ":3");
+            overrides.put(DebugOptions.PrintGraph, PrintGraphTarget.File);
+            overrides.put(DebugOptions.MethodFilter, null);
+
+            // Generate dump files.
+            ResolvedJavaMethod method = getResolvedJavaMethod("snippet");
+            StructuredGraph graph = parseForCompile(method, new OptionValues(getInitialOptions(), overrides));
+            getCode(method, graph);
+        }
+    }
+}

compiler/src/org.graalvm.graphio/src/org/graalvm/graphio/GraphProtocol.java

@@ -35,6 +35,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Objects;
@@ -880,6 +881,31 @@ private static boolean isFound(Object obj, Object[] found) {
         return true;
     }
 
+    private static HashSet<Class<?>> badToString;
+
+    /**
+     * This is a helper to identify objects that are encoded as POOL_STRING and have a poor
+     * {@link Object#toString()} implementation where two objects that are
+     * {@link Object#equals(Object)} have different String representations. Only the first mismatch
+     * is reported since this is a systematic issue and reporting every failure would be too much
+     * useless output.
+     */
+    private static synchronized void reportBadToString(Object lookupKey, Object value) {
+        if (badToString == null) {
+            badToString = new HashSet<>();
+        }
+        if (badToString.add(lookupKey.getClass())) {
+            System.err.println("GraphProtocol: toString mismatch for " + lookupKey.getClass() + ": " + value + " != " + lookupKey.toString());
+        }
+    }
+
+    private static boolean checkToString(Object lookupKey, Object value) {
+        if (!lookupKey.toString().equals(value)) {
+            reportBadToString(lookupKey, value);
+        }
+        return true;
+    }
+
     /**
      * This class maintains a limited pool of constants for use by the graph protocol. Once the
      * cache fills up the oldest slots are replaced with new values in a cyclic fashion.
@@ -900,17 +926,25 @@ private static final class ConstantPool {
         ConstantPool() {
         }
 
-        Character get(Object key, int type) {
+        private static Object getLookupKey(Object key) {
+            // Collections must be converted to a String early since they can be mutated after
+            // being inserted into the map.
+            return (key instanceof Collection) ? key.toString() : key;
+        }
+
+        Character get(Object initialKey, int type) {
+            Object key = getLookupKey(initialKey);
             Object value = map.get(key);
             if (value instanceof String) {
-                value = map.get(value);
-                Character id = (Character) value;
-                if (id != null && keys[id] == value) {
+                Character id = (Character) map.get(value);
+                if (id != null && keys[id].equals(value)) {
+                    assert checkToString(key, value);
                     return id;
                 }
+                value = null;
             }
             Character id = (Character) value;
-            if (id != null && keys[id] == key) {
+            if (id != null && keys[id].equals(key)) {
                 return id;
             }
             if (type == POOL_STRING && !(key instanceof String)) {
@@ -926,14 +960,15 @@ Character get(Object key, int type) {
             return null;
         }
 
-        char add(Object key, int type) {
+        char add(Object initialKey, int type) {
             char id = nextId++;
             if (nextId == CONSTANT_POOL_MAX_SIZE) {
                 nextId = 0;
             }
             if (keys[id] != null) {
                 map.remove(keys[id]);
             }
+            Object key = getLookupKey(initialKey);
             if (type == POOL_STRING && !(key instanceof String)) {
                 // Insert a forwarding entry from the original object to the string representation
                 // and then directly insert the string with the pool id.