Skip to content

Commit

Permalink
8271459: C2: Missing NegativeArraySizeException when creating StringB…
Browse files Browse the repository at this point in the history
…uilder with negative capacity

Reviewed-by: roland, thartmann, neliasso
  • Loading branch information
chhagedorn committed Oct 5, 2021
1 parent 53d7e95 commit 3953e07
Show file tree
Hide file tree
Showing 4 changed files with 276 additions and 7 deletions.
53 changes: 51 additions & 2 deletions src/hotspot/share/opto/stringopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ class StringConcat : public ResourceObj {
StringMode,
IntMode,
CharMode,
StringNullCheckMode
StringNullCheckMode,
NegativeIntCheckMode
};

StringConcat(PhaseStringOpts* stringopts, CallStaticJavaNode* end):
Expand Down Expand Up @@ -122,12 +123,19 @@ class StringConcat : public ResourceObj {
void push_string(Node* value) {
push(value, StringMode);
}

void push_string_null_check(Node* value) {
push(value, StringNullCheckMode);
}

void push_negative_int_check(Node* value) {
push(value, NegativeIntCheckMode);
}

void push_int(Node* value) {
push(value, IntMode);
}

void push_char(Node* value) {
push(value, CharMode);
}
Expand Down Expand Up @@ -502,13 +510,35 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) {
#ifndef PRODUCT
if (PrintOptimizeStringConcat) {
tty->print("giving up because StringBuilder(null) throws exception");
alloc->jvms()->dump_spec(tty); tty->cr();
alloc->jvms()->dump_spec(tty);
tty->cr();
}
#endif
return NULL;
}
// StringBuilder(str) argument needs null check.
sc->push_string_null_check(use->in(TypeFunc::Parms + 1));
} else if (sig == ciSymbols::int_void_signature()) {
// StringBuilder(int) case.
Node* parm = use->in(TypeFunc::Parms + 1);
assert(parm != NULL, "must exist");
const TypeInt* type = _gvn->type(parm)->is_int();
if (type->_hi < 0) {
// Initial capacity argument is always negative in which case StringBuilder(int) throws
// a NegativeArraySizeException. Bail out from string opts.
#ifndef PRODUCT
if (PrintOptimizeStringConcat) {
tty->print("giving up because a negative argument is passed to StringBuilder(int) which "
"throws a NegativeArraySizeException");
alloc->jvms()->dump_spec(tty);
tty->cr();
}
#endif
return NULL;
} else if (type->_lo < 0) {
// Argument could be negative: We need a runtime check to throw NegativeArraySizeException in that case.
sc->push_negative_int_check(parm);
}
}
// The int variant takes an initial size for the backing
// array so just treat it like the void version.
Expand Down Expand Up @@ -1794,6 +1824,23 @@ void PhaseStringOpts::replace_string_concat(StringConcat* sc) {
for (int argi = 0; argi < sc->num_arguments(); argi++) {
Node* arg = sc->argument(argi);
switch (sc->mode(argi)) {
case StringConcat::NegativeIntCheckMode: {
// Initial capacity argument might be negative in which case StringBuilder(int) throws
// a NegativeArraySizeException. Insert a runtime check with an uncommon trap.
const TypeInt* type = kit.gvn().type(arg)->is_int();
assert(type->_hi >= 0 && type->_lo < 0, "no runtime int check needed");
Node* p = __ Bool(__ CmpI(arg, kit.intcon(0)), BoolTest::ge);
IfNode* iff = kit.create_and_map_if(kit.control(), p, PROB_MIN, COUNT_UNKNOWN);
{
// Negative int -> uncommon trap.
PreserveJVMState pjvms(&kit);
kit.set_control(__ IfFalse(iff));
kit.uncommon_trap(Deoptimization::Reason_intrinsic,
Deoptimization::Action_maybe_recompile);
}
kit.set_control(__ IfTrue(iff));
break;
}
case StringConcat::IntMode: {
Node* string_size = int_stringSize(kit, arg);

Expand Down Expand Up @@ -1963,6 +2010,8 @@ void PhaseStringOpts::replace_string_concat(StringConcat* sc) {
for (int argi = 0; argi < sc->num_arguments(); argi++) {
Node* arg = sc->argument(argi);
switch (sc->mode(argi)) {
case StringConcat::NegativeIntCheckMode:
break; // Nothing to do, was only needed to add a runtime check earlier.
case StringConcat::IntMode: {
start = int_getChars(kit, arg, dst_array, coder, start, string_sizes->in(argi));
break;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
/*
* 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.
*
* 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 8271459
* @requires vm.compiler2.enabled
* @summary C2 applies string opts to StringBuilder object created with a negative size and misses the NegativeArraySizeException.
* @library /test/lib /
* @run driver compiler.c2.irTests.stringopts.TestNegativeArraySize
*/

package compiler.c2.irTests.stringopts;

import compiler.lib.ir_framework.*;
import jdk.test.lib.Asserts;

public class TestNegativeArraySize {

static int iFld;

public static void main(String[] args) {
// Dont inline any StringBuilder methods for this IR test to check if string opts are applied or not.
TestFramework.runWithFlags("-XX:CompileCommand=dontinline,java.lang.StringBuilder::*");
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString", IRNode.INTRINSIC_TRAP})
static String positiveConst() {
// C2 knows that argument is 5 and applies string opts without runtime check.
StringBuilder sb = new StringBuilder(5); // StringBuilder object optimized away by string opts.
return sb.toString(); // Call optimized away by string opts.
}

@Test
@IR(counts = {IRNode.ALLOC_OF, "StringBuilder", "1", IRNode.CALL_OF_METHOD, "toString", "1"})
@IR(failOn = IRNode.INTRINSIC_TRAP) // No runtime check, we bail out of string opts
static String negativeConst() {
StringBuilder sb = new StringBuilder(-5); // C2 knows that we always have a negative int -> bail out of string opts
return sb.toString(); // Call stays due to bailout.
}

@Run(test = "negativeConst")
static void runNegativeConst() {
try {
negativeConst();
Asserts.fail("should have thrown exception");
} catch (NegativeArraySizeException e) {
// Expected;
}
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"})
@IR(counts = {IRNode.INTRINSIC_TRAP, "1"}) // Uncommon trap of runtime check
static String positiveFld() {
// C2 does not know if iFld is positive or negative. It applies string opts but inserts a runtime check to
// bail out to interpreter. This path, however, is never taken because iFld is always positive.
StringBuilder sb = new StringBuilder(iFld);
return sb.toString();
}

@Run(test = "positiveFld")
static void runPositiveFld() {
iFld = 4;
positiveFld();
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"})
@IR(counts = {IRNode.INTRINSIC_TRAP, "1"}) // Uncommon trap of runtime check
static String negativeFld() {
// C2 does not know if iFld is positive or negative. It applies string opts but inserts a runtime check to
// bail out to interpreter. This path is always taken because iFld is always negative.
StringBuilder sb = new StringBuilder(iFld);
return sb.toString();
}

@Run(test = "negativeFld")
static void runNegativeFld() {
iFld = -4;
try {
negativeFld();
Asserts.fail("should have thrown exception");
} catch (NegativeArraySizeException e) {
// Expected;
}
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"})
@IR(counts = {IRNode.INTRINSIC_TRAP, "1"}) // Uncommon trap of runtime check
static String maybeNegativeConst(boolean flag) {
// C2 knows that cap is between -5 and 5. It applies string opts but inserts a runtime check to
// bail out to interpreter. This path is sometimes taken and sometimes not.
int cap = flag ? 5 : -5;
StringBuilder sb = new StringBuilder(cap);
return sb.toString();
}

@Run(test = "maybeNegativeConst")
static void runMaybeNegativeConst() {
boolean flag = TestFramework.toggleBoolean();
try {
maybeNegativeConst(flag);
Asserts.assertTrue(flag);
} catch (NegativeArraySizeException e) {
Asserts.assertFalse(flag);
}
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString", IRNode.INTRINSIC_TRAP})
static String alwaysPositiveConst(boolean flag) {
// C2 knows that cap is between 1 and 100 and applies string opts without runtime check.
int cap = flag ? 1 : 100;
StringBuilder sb = new StringBuilder(cap); // Object optimized away.
return sb.toString(); // Optimized away.
}

@Run(test = "alwaysPositiveConst")
static void runAlwaysPositiveConst() {
alwaysPositiveConst(TestFramework.toggleBoolean());
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"})
@IR(counts = {IRNode.INTRINSIC_TRAP, "1"}) // Uncommon trap of runtime check
static String negativeArg(int cap) {
// C2 does not know if cap is positive or negative. It applies string opts but inserts a runtime check to
// bail out to interpreter. This path is always taken because cap is always negative.
StringBuilder sb = new StringBuilder(cap);
return sb.toString();
}

@Run(test = "negativeArg")
static void runNegativeArg() {
try {
negativeArg(-5);
Asserts.fail("should have thrown exception");
} catch (NegativeArraySizeException e) {
// Expected
}
}

@Test
@IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"})
@IR(counts = {IRNode.INTRINSIC_TRAP, "1"}) // Uncommon trap of runtime check
static String positiveArg(int cap) {
// C2 does not know if cap is positive or negative. It applies string opts but inserts a runtime check to
// bail out to interpreter. This path, however, is never taken because cap is always positive.
StringBuilder sb = new StringBuilder(cap);
return sb.toString();
}

@Run(test = "positiveArg")
static void runPositiveArg() {
positiveArg(5);
}
}
8 changes: 5 additions & 3 deletions test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,10 @@ public class IRNode {
public static final String COUNTEDLOOP = START + "CountedLoop\\b" + MID + END;
public static final String COUNTEDLOOP_MAIN = START + "CountedLoop\\b" + MID + "main" + END;

public static final String CALL = START + "CallStaticJava" + MID + END;
public static final String DYNAMIC_CALL_OF_METHOD = COMPOSITE_PREFIX + START + "CallDynamicJava" + MID + IS_REPLACED + END;
public static final String STATIC_CALL_OF_METHOD = COMPOSITE_PREFIX + START + "CallStaticJava" + MID + IS_REPLACED + END;
public static final String CALL = START + "Call.*Java" + MID + END;
public static final String CALL_OF_METHOD = COMPOSITE_PREFIX + START + "Call.*Java" + MID + IS_REPLACED + " " + END;
public static final String DYNAMIC_CALL_OF_METHOD = COMPOSITE_PREFIX + START + "CallDynamicJava" + MID + IS_REPLACED + " " + END;
public static final String STATIC_CALL_OF_METHOD = COMPOSITE_PREFIX + START + "CallStaticJava" + MID + IS_REPLACED + " " + END;
public static final String TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*reason" + END;
public static final String PREDICATE_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*predicate" + END;
public static final String UNSTABLE_IF_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*unstable_if" + END;
Expand All @@ -125,6 +126,7 @@ public class IRNode {
public static final String NULL_ASSERT_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*null_assert" + END;
public static final String RANGE_CHECK_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*range_check" + END;
public static final String UNHANDLED_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*unhandled" + END;
public static final String INTRINSIC_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*intrinsic" + END;
// Does not work for VM builds without JVMCI like x86_32 (a rule containing this regex will be skipped without having JVMCI built).
public static final String INTRINSIC_OR_TYPE_CHECKED_INLINING_TRAP = START + "CallStaticJava" + MID + "uncommon_trap.*intrinsic_or_type_checked_inlining" + END;

Expand Down
Loading

1 comment on commit 3953e07

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.