8138651: -XX:DisableIntrinsic matches intrinsics overly eagerly

Improve parsing of DisableIntrinsic flag.

Reviewed-by: kvn, shade, neliasso

hotspot/src/share/vm/compiler/compilerDirectives.cpp

@@ -175,12 +175,38 @@ DirectiveSet* CompilerDirectives::get_for(AbstractCompiler *comp) {
   return NULL;
 }
 
+// In the list of disabled intrinsics, the ID of the disabled intrinsics can separated:
+// - by ',' (if -XX:DisableIntrinsic is used once when invoking the VM) or
+// - by '\n' (if -XX:DisableIntrinsic is used multiple times when invoking the VM) or
+// - by ' ' (if DisableIntrinsic is used on a per-method level, e.g., with CompileCommand).
+//
+// To simplify the processing of the list, the canonicalize_disableintrinsic() method
+// returns a new copy of the list in which '\n' and ' ' is replaced with ','.
+ccstrlist DirectiveSet::canonicalize_disableintrinsic(ccstrlist option_value) {
+  char* canonicalized_list = NEW_C_HEAP_ARRAY(char, strlen(option_value) + 1, mtCompiler);
+  int i = 0;
+  char current;
+  while ((current = option_value[i]) != '\0') {
+    if (current == '\n' || current == ' ') {
+      canonicalized_list[i] = ',';
+    } else {
+      canonicalized_list[i] = current;
+    }
+    i++;
+  }
+  canonicalized_list[i] = '\0';
+  return canonicalized_list;
+}
+
 DirectiveSet::DirectiveSet(CompilerDirectives* d) :_inlinematchers(NULL), _directive(d) {
 #define init_defaults_definition(name, type, dvalue, compiler) this->name##Option = dvalue;
   compilerdirectives_common_flags(init_defaults_definition)
   compilerdirectives_c2_flags(init_defaults_definition)
   compilerdirectives_c1_flags(init_defaults_definition)
   memset(_modified, 0, sizeof _modified);
+
+  // Canonicalize DisableIntrinsic to contain only ',' as a separator.
+  this->DisableIntrinsicOption = canonicalize_disableintrinsic(DisableIntrinsic);
 }
 
 DirectiveSet::~DirectiveSet() {
@@ -192,12 +218,11 @@ DirectiveSet::~DirectiveSet() {
     tmp = next;
   }
 
-  // Free if modified, otherwise it just points to the global vm flag value
-  // or to the Compile command option
-  if (_modified[DisableIntrinsicIndex]) {
-    assert(this->DisableIntrinsicOption != NULL, "");
-    FREE_C_HEAP_ARRAY(char, (void *)this->DisableIntrinsicOption);
-  }
+  // When constructed, DirectiveSet canonicalizes the DisableIntrinsic flag
+  // into a new string. Therefore, that string is deallocated when
+  // the DirectiveSet is destroyed.
+  assert(this->DisableIntrinsicOption != NULL, "");
+  FREE_C_HEAP_ARRAY(char, (void *)this->DisableIntrinsicOption);
 }
 
 // Backward compatibility for CompileCommands
@@ -253,6 +278,14 @@ DirectiveSet* DirectiveSet::compilecommand_compatibility_init(methodHandle metho
     compilerdirectives_c2_flags(init_default_cc)
     compilerdirectives_c1_flags(init_default_cc)
 
+    // Canonicalize DisableIntrinsic to contain only ',' as a separator.
+    ccstrlist option_value;
+    if (!_modified[DisableIntrinsicIndex] &&
+        CompilerOracle::has_option_value(method, "DisableIntrinsic", option_value)) {
+      set->DisableIntrinsicOption = canonicalize_disableintrinsic(option_value);
+    }
+
+
     if (!changed) {
       // We didn't actually update anything, discard.
       delete set;
@@ -352,8 +385,26 @@ bool DirectiveSet::is_intrinsic_disabled(methodHandle method) {
   vmIntrinsics::ID id = method->intrinsic_id();
   assert(id != vmIntrinsics::_none, "must be a VM intrinsic");
 
-  ccstr disable_intr =  DisableIntrinsicOption;
-  return ((disable_intr != '\0') && strstr(disable_intr, vmIntrinsics::name_at(id)) != NULL);
+  ResourceMark rm;
+
+  // Create a copy of the string that contains the list of disabled
+  // intrinsics. The copy is created because the string
+  // will be modified by strtok(). Then, the list is tokenized with
+  // ',' as a separator.
+  size_t length = strlen(DisableIntrinsicOption);
+  char* local_list = NEW_RESOURCE_ARRAY(char, length + 1);
+  strncpy(local_list, DisableIntrinsicOption, length + 1);
+
+  char* token = strtok(local_list, ",");
+  while (token != NULL) {
+    if (strcmp(token, vmIntrinsics::name_at(id)) == 0) {
+      return true;
+    } else {
+      token = strtok(NULL, ",");
+    }
+  }
+
+  return false;
 }
 
 DirectiveSet* DirectiveSet::clone(DirectiveSet const* src) {
@@ -371,15 +422,13 @@ DirectiveSet* DirectiveSet::clone(DirectiveSet const* src) {
     compilerdirectives_c2_flags(copy_members_definition)
     compilerdirectives_c1_flags(copy_members_definition)
 
-  // Must duplicate ccstr option if it was modified, otherwise it is global.
-  if (src->_modified[DisableIntrinsicIndex]) {
-    assert(src->DisableIntrinsicOption != NULL, "");
-    size_t len = strlen(src->DisableIntrinsicOption) + 1;
-    char* s = NEW_C_HEAP_ARRAY(char, len, mtCompiler);
-    strncpy(s, src->DisableIntrinsicOption, len);
-    assert(s[len-1] == '\0', "");
-    set->DisableIntrinsicOption = s;
-  }
+  // Create a local copy of the DisableIntrinsicOption.
+  assert(src->DisableIntrinsicOption != NULL, "");
+  size_t len = strlen(src->DisableIntrinsicOption) + 1;
+  char* s = NEW_C_HEAP_ARRAY(char, len, mtCompiler);
+  strncpy(s, src->DisableIntrinsicOption, len);
+  assert(s[len-1] == '\0', "");
+  set->DisableIntrinsicOption = s;
   return set;
 }
 

hotspot/src/share/vm/compiler/compilerDirectives.hpp

@@ -47,7 +47,7 @@
     cflags(DumpReplay,              bool, false, DumpReplay) \
     cflags(DumpInline,              bool, false, DumpInline) \
     cflags(CompilerDirectivesIgnoreCompileCommands, bool, CompilerDirectivesIgnoreCompileCommands, X) \
-    cflags(DisableIntrinsic,        ccstr, DisableIntrinsic, DisableIntrinsic)
+    cflags(DisableIntrinsic,        ccstrlist, DisableIntrinsic, DisableIntrinsic)
 
 #ifdef COMPILER1
   #define compilerdirectives_c1_flags(cflags)
@@ -100,6 +100,8 @@ private:
   InlineMatcher* _inlinematchers;
   CompilerDirectives* _directive;
 
+  static ccstrlist canonicalize_disableintrinsic(ccstrlist option_value);
+
 public:
   DirectiveSet(CompilerDirectives* directive);
   ~DirectiveSet();
@@ -141,6 +143,7 @@ public:
   void print_bool(outputStream* st, ccstr n, bool v, bool mod) { if (mod) { st->print("%s:%s ", n, v ? "true" : "false"); } }
   void print_double(outputStream* st, ccstr n, double v, bool mod) { if (mod) { st->print("%s:%f ", n, v); } }
   void print_ccstr(outputStream* st, ccstr n, ccstr v, bool mod) { if (mod) { st->print("%s:%s ", n, v); } }
+  void print_ccstrlist(outputStream* st, ccstr n, ccstr v, bool mod) { print_ccstr(st, n, v, mod); }
 
 void print(outputStream* st) {
     print_inline(st);

hotspot/src/share/vm/compiler/directivesParser.cpp

@@ -288,7 +288,7 @@ bool DirectivesParser::set_option_flag(JSON_TYPE t, JSON_VAL* v, const key* opti
       break;
 
     case JSON_STRING:
-      if (option_key->flag_type != ccstrFlag) {
+      if (option_key->flag_type != ccstrFlag && option_key->flag_type != ccstrlistFlag) {
         error(VALUE_ERROR, "Cannot use string value for a %s flag", flag_type_names[option_key->flag_type]);
         return false;
       } else {

hotspot/src/share/vm/compiler/directivesParser.hpp

@@ -33,6 +33,7 @@ enum FlagType {
   intxFlag,
   doubleFlag,
   ccstrFlag,
+  ccstrlistFlag,
   UnknownFlagType
 };
 
@@ -41,6 +42,7 @@ static const char* flag_type_names[] = {
     "int",
     "double",
     "string",
+    "string list",
     "unknown"
 };
 

hotspot/test/compiler/intrinsics/IntrinsicDisabledTest.java

@@ -0,0 +1,211 @@
+/*
+ * Copyright (c) 2015, 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.
+ *
+ * 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.
+ */
+
+/*
+ * @test
+ * @bug 8138651
+ * @library /testlibrary /../../test/lib
+ * @build IntrinsicDisabledTest
+ * @run main ClassFileInstaller sun.hotspot.WhiteBox
+ *                              sun.hotspot.WhiteBox$WhiteBoxPermission
+ * @run main/othervm -Xbootclasspath/a:.
+ *                   -XX:+UnlockDiagnosticVMOptions
+ *                   -XX:+WhiteBoxAPI
+ *                   -XX:DisableIntrinsic=_putCharVolatile,_putInt
+ *                   -XX:DisableIntrinsic=_putIntVolatile
+ *                   -XX:CompileCommand=option,sun.misc.Unsafe::putChar,ccstrlist,DisableIntrinsic,_getCharVolatile,_getInt
+ *                   -XX:CompileCommand=option,sun.misc.Unsafe::putCharVolatile,ccstrlist,DisableIntrinsic,_getIntVolatile
+ *                   IntrinsicDisabledTest
+ */
+
+import java.lang.reflect.Executable;
+import java.util.Objects;
+
+import sun.hotspot.WhiteBox;
+
+import jdk.test.lib.Platform;
+
+public class IntrinsicDisabledTest {
+
+    private static final WhiteBox wb = WhiteBox.getWhiteBox();
+
+    /* Compilation level corresponding to C1. */
+    private static final int COMP_LEVEL_SIMPLE = 1;
+
+    /* Compilation level corresponding to C2. */
+    private static final int COMP_LEVEL_FULL_OPTIMIZATION = 4;
+
+    /* Determine if tiered compilation is enabled. */
+    private static boolean isTieredCompilationEnabled() {
+        return Boolean.valueOf(Objects.toString(wb.getVMFlag("TieredCompilation")));
+    }
+
+    /* This test uses several methods from sun.misc.Unsafe. The method
+     * getMethod() returns a different Executable for each different
+     * combination of its input parameters. There are eight possible
+     * combinations, getMethod can return an Executable representing
+     * the following methods: putChar, putCharVolatile, getChar,
+     * getCharVolatile, putInt, putIntVolatile, getInt,
+     * getIntVolatile. These methods were selected because they can
+     * be intrinsified by both the C1 and the C2 compiler.
+     */
+    static Executable getMethod(boolean isChar, boolean isPut, boolean isVolatile) throws RuntimeException {
+        Executable aMethod;
+        String methodTypeName = isChar ? "Char" : "Int";
+
+        try {
+            Class aClass = Class.forName("sun.misc.Unsafe");
+            if (isPut) {
+                aMethod = aClass.getDeclaredMethod("put" + methodTypeName + (isVolatile ? "Volatile" : ""),
+                                                   Object.class,
+                                                   long.class,
+                                                   isChar ? char.class : int.class);
+            } else {
+                aMethod = aClass.getDeclaredMethod("get" + methodTypeName + (isVolatile ? "Volatile" : ""),
+                                                   Object.class,
+                                                   long.class);
+            }
+        } catch (NoSuchMethodException e) {
+            throw new RuntimeException("Test bug, method is unavailable. " + e);
+        } catch (ClassNotFoundException e) {
+            throw new RuntimeException("Test bug, class is unavailable. " + e);
+        }
+
+        return aMethod;
+    }
+
+    public static void test(int compLevel) {
+
+        Executable putChar = getMethod(true, /*isPut =*/ true, /*isVolatile = */ false);
+        Executable getChar = getMethod(true, /*isPut =*/ false, /*isVolatile = */ false);
+        Executable putCharVolatile = getMethod(true, /*isPut =*/ true, /*isVolatile = */ true);
+        Executable getCharVolatile = getMethod(true, /*isPut =*/ false, /*isVolatile = */ true);
+        Executable putInt = getMethod(false, /*isPut =*/ true, /*isVolatile = */ false);
+        Executable getInt = getMethod(false, /*isPut =*/ false, /*isVolatile = */ false);
+        Executable putIntVolatile = getMethod(false, /*isPut =*/ true, /*isVolatile = */ true);
+        Executable getIntVolatile = getMethod(false, /*isPut =*/ false, /*isVolatile = */ true);
+
+        /* Test if globally disabling intrinsics works. */
+        if (!wb.isIntrinsicAvailable(putChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putChar.toGenericString() +
+                                       "] is not available globally although it should be.");
+        }
+
+        if (wb.isIntrinsicAvailable(putCharVolatile, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putCharVolatile.toGenericString() +
+                                       "] is available globally although it should not be.");
+        }
+
+        if (wb.isIntrinsicAvailable(putInt, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putInt.toGenericString() +
+                                       "] is available globally although it should not be.");
+        }
+
+        if (wb.isIntrinsicAvailable(putIntVolatile, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putIntVolatile.toGenericString() +
+                                       "] is available globally although it should not be.");
+        }
+
+        /* Test if disabling intrinsics on a per-method level
+         * works. The method for which intrinsics are
+         * disabled (the compilation context) is putChar. */
+        if (!wb.isIntrinsicAvailable(getChar, putChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getChar.toGenericString() +
+                                       "] is not available for intrinsification in [" +
+                                       putChar.toGenericString() + "] although it should be.");
+        }
+
+        if (wb.isIntrinsicAvailable(getCharVolatile, putChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getCharVolatile.toGenericString() +
+                                       "] is available for intrinsification in [" +
+                                       putChar.toGenericString() + "] although it should not be.");
+        }
+
+        if (wb.isIntrinsicAvailable(getInt, putChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getInt.toGenericString() +
+                                       "] is available for intrinsification in [" +
+                                       putChar.toGenericString() + "] although it should not be.");
+        }
+
+        if (wb.isIntrinsicAvailable(getIntVolatile, putCharVolatile, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getIntVolatile.toGenericString() +
+                                       "] is available for intrinsification in [" +
+                                       putCharVolatile.toGenericString() + "] although it should not be.");
+        }
+
+        /* Test if disabling intrinsics on a per-method level
+         * leaves those intrinsics enabled globally. */
+        if (!wb.isIntrinsicAvailable(getCharVolatile, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getCharVolatile.toGenericString() +
+                                       "] is not available globally although it should be.");
+        }
+
+        if (!wb.isIntrinsicAvailable(getInt, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getInt.toGenericString() +
+                                       "] is not available globally although it should be.");
+        }
+
+
+        if (!wb.isIntrinsicAvailable(getIntVolatile, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + getIntVolatile.toGenericString() +
+                                       "] is not available globally although it should be.");
+        }
+
+        /* Test if disabling an intrinsic globally disables it on a
+         * per-method level as well. */
+        if (!wb.isIntrinsicAvailable(putChar, getChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putChar.toGenericString() +
+                                       "] is not available for intrinsification in [" +
+                                       getChar.toGenericString() + "] although it should be.");
+        }
+
+        if (wb.isIntrinsicAvailable(putCharVolatile, getChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putCharVolatile.toGenericString() +
+                                       "] is available for intrinsification in [" +
+                                       getChar.toGenericString() + "] although it should not be.");
+        }
+
+        if (wb.isIntrinsicAvailable(putInt, getChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putInt.toGenericString() +
+                                       "] is available for intrinsification in [" +
+                                       getChar.toGenericString() + "] although it should not be.");
+        }
+
+        if (wb.isIntrinsicAvailable(putIntVolatile, getChar, compLevel)) {
+            throw new RuntimeException("Intrinsic for [" + putIntVolatile.toGenericString() +
+                                       "] is available for intrinsification in [" +
+                                       getChar.toGenericString() + "] although it should not be.");
+        }
+    }
+
+    public static void main(String args[]) {
+        if (Platform.isServer()) {
+            if (isTieredCompilationEnabled()) {
+                test(COMP_LEVEL_SIMPLE);
+            }
+            test(COMP_LEVEL_FULL_OPTIMIZATION);
+        } else {
+            test(COMP_LEVEL_SIMPLE);
+        }
+    }
+}