Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8273914: Indy string concat changes order of operations #5844

Closed
wants to merge 6 commits into from
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -142,25 +142,6 @@ protected StringConcat(Context context) {
return res.append(tree);
}

/**
* If the type is not accessible from current context, try to figure out the
* sharpest accessible supertype.
*
* @param originalType type to sharpen
* @return sharped type
*/
Type sharpestAccessible(Type originalType) {
if (originalType.hasTag(ARRAY)) {
return types.makeArrayType(sharpestAccessible(types.elemtype(originalType)));
}

Type type = originalType;
while (!rs.isAccessible(gen.getAttrEnv(), type.asElement())) {
type = types.supertype(type);
}
return type;
}

/**
* "Legacy" bytecode flavor: emit the StringBuilder.append chains for string
* concatenation.
@@ -305,6 +286,14 @@ public Item makeConcat(JCTree.JCBinary tree) {

return splits.toList();
}

/**
* Returns true if the argument should be converted to a string eagerly, to preserve
* possible side-effects.
*/
protected boolean shouldConvertToStringEagerly(Type argType) {

This comment has been minimized.

@forax

forax Oct 6, 2021
Member

For me the implementation should be neither a primitive type nor a final class of java.lang. I think that at least the wrappers should not be converted eagerly

This comment has been minimized.

@cushon

cushon Oct 6, 2021
Author Contributor

@forax can you expand on the suggestion here?

The current implementation is not eagerly converting boxes for primitives types, which wrappers should not be converted eagerly?

Also note that one of the motivating examples was StringBuilder, which is a final class in java.lang. It's not just about toString() not having side-effects, it's also about insulting the operands from each others's side effects (e.g. myStringBuilder.append("foo") + myStringBuilder.append("bar"))

This comment has been minimized.

@forax

forax Oct 7, 2021
Member

The current implementation is not eagerly converting boxes for primitives types, which wrappers should not be converted eagerly?

I was just thinking that not calling Boolean or Double.toString() explicitly was Ok

myStringBuilder.append("foo") + myStringBuilder.append("bar")

Aaaah, so only primitives + their boxes should not be evaluated eagerly.

return !types.unboxedTypeOrType(argType).isPrimitive() && argType.tsym != syms.stringType.tsym;
}
}

/**
@@ -333,14 +322,18 @@ protected void emit(JCDiagnostic.DiagnosticPosition pos, List<JCTree> args, bool
for (JCTree arg : t) {
Object constVal = arg.type.constValue();
if ("".equals(constVal)) continue;
if (arg.type == syms.botType) {
dynamicArgs.add(types.boxedClass(syms.voidType).type);
} else {
dynamicArgs.add(sharpestAccessible(arg.type));
Type argType = arg.type;
if (argType == syms.botType) {
argType = types.boxedClass(syms.voidType).type;
}
if (!first || generateFirstArg) {
gen.genExpr(arg, arg.type).load();
}
if (shouldConvertToStringEagerly(argType)) {
gen.callMethod(pos, syms.stringType, names.valueOf, List.of(syms.objectType), true);
argType = syms.stringType;
}
dynamicArgs.add(argType);
first = false;
}
doCall(type, pos, dynamicArgs.toList());
@@ -439,10 +432,15 @@ protected void emit(JCDiagnostic.DiagnosticPosition pos, List<JCTree> args, bool
} else {
// Ordinary arguments come through the dynamic arguments.
recipe.append(TAG_ARG);
dynamicArgs.add(sharpestAccessible(arg.type));
Type argType = arg.type;
if (!first || generateFirstArg) {
gen.genExpr(arg, arg.type).load();
}
if (shouldConvertToStringEagerly(argType)) {
gen.callMethod(pos, syms.stringType, names.valueOf, List.of(syms.objectType), true);
argType = syms.stringType;
}
dynamicArgs.add(argType);
first = false;
}
}
@@ -29,8 +29,8 @@
* after the module read edge is added.
* @compile ModuleLibrary.java
* p2/c2.java
* p5/c5.java
* p7/c7.java
* p5/c5.jasm
* p7/c7.jasm
* @run main/othervm MethodAccessReadTwice
*/

@@ -0,0 +1,76 @@
/*
* Copyright (c) 2021, Google LLC. 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 input for the fix for JDK-8174954, which checks for an expected
* IllegalAccessError when the parameter type of an invokedynamic is
* inaccessible.
*
* The test assumes that given the string concatenation expression "" + param,
* javac generates an invokedynamic that uses the specific type of param. The
* fix for JDK-8273914 make javac eagerly convert param to a String before
* passing it to the invokedynamic call, which avoids the accessibility issue
* the test is trying to exercise.
*
* This jasm file contains the bytecode javac generated before the fix for
* JDK-8273914, to continue to exercise the invokedynamic behaviour that
* JDK-8174954 is testing.
*/

package p5;

super public class c5
version 61:0
{
public Method "<init>":"()V"
stack 1 locals 1
{
aload_0;
invokespecial Method java/lang/Object."<init>":"()V";
return;
}
public Method method5:"(Lp2/c2;)V"
stack 2 locals 2
{
getstatic Field java/lang/System.out:"Ljava/io/PrintStream;";
aload_1;
invokedynamic InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Lp2/c2;)Ljava/lang/String;" {
String "In c5\'s method5 with param = "
};
invokevirtual Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
return;
}
public Method methodAddReadEdge:"(Ljava/lang/Module;)V"
stack 2 locals 2
{
ldc class c5;
invokevirtual Method java/lang/Class.getModule:"()Ljava/lang/Module;";
aload_1;
invokevirtual Method java/lang/Module.addReads:"(Ljava/lang/Module;)Ljava/lang/Module;";
pop;
return;
}

public static final InnerClass Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles;

} // end Class c5
@@ -0,0 +1,113 @@
/*
* Copyright (c) 2021, Google LLC. 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 input for the fix for JDK-8174954, which checks for an expected
* IllegalAccessError when the parameter type of an invokedynamic is
* inaccessible.
*
* The test assumes that given the string concatenation expression "" + param,
* javac generates an invokedynamic that uses the specific type of param. The
* fix for JDK-8273914 make javac eagerly convert param to a String before
* passing it to the invokedynamic call, which avoids the accessibility issue
* the test is trying to exercise.
*
* This jasm file contains the bytecode javac generated before the fix for
* JDK-8273914, to continue to exercise the invokedynamic behaviour that
* JDK-8174954 is testing.
*/

package p7;

super public class c7
version 61:0
{
public Method "<init>":"()V"
stack 1 locals 1
{
aload_0;
invokespecial Method java/lang/Object."<init>":"()V";
return;
}
public Method method7:"(Lp2/c2;Ljava/lang/Module;)V"
stack 3 locals 4
{
try t0;
getstatic Field java/lang/System.out:"Ljava/io/PrintStream;";
aload_1;
invokedynamic InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Lp2/c2;)Ljava/lang/String;" {
String "In c7\'s method7 with param = "
};
invokevirtual Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
new class java/lang/RuntimeException;
dup;
ldc String "c7 failed to throw expected IllegalAccessError";
invokespecial Method java/lang/RuntimeException."<init>":"(Ljava/lang/String;)V";
athrow;
endtry t0;
catch t0 java/lang/IllegalAccessError;
stack_frame_type stack1;
stack_map class java/lang/IllegalAccessError;
astore_3;
aload_0;
aload_2;
invokevirtual Method methodAddReadEdge:"(Ljava/lang/Module;)V";
try t1;
getstatic Field java/lang/System.out:"Ljava/io/PrintStream;";
aload_1;
invokedynamic InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Lp2/c2;)Ljava/lang/String;" {
String "In c7\'s method7 with param = "
};
invokevirtual Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
endtry t1;
goto L61;
catch t1 java/lang/IllegalAccessError;
stack_frame_type stack1;
stack_map class java/lang/IllegalAccessError;
astore_3;
new class java/lang/RuntimeException;
dup;
aload_3;
invokevirtual Method java/lang/IllegalAccessError.getMessage:"()Ljava/lang/String;";
invokedynamic InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Ljava/lang/String;)Ljava/lang/String;" {
String "Unexpected IllegalAccessError: "
};
invokespecial Method java/lang/RuntimeException."<init>":"(Ljava/lang/String;)V";
athrow;
L61: stack_frame_type same;
return;
}
public Method methodAddReadEdge:"(Ljava/lang/Module;)V"
stack 2 locals 2
{
ldc class c7;
invokevirtual Method java/lang/Class.getModule:"()Ljava/lang/Module;";
aload_1;
invokevirtual Method java/lang/Module.addReads:"(Ljava/lang/Module;)Ljava/lang/Module;";
pop;
return;
}

public static final InnerClass Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles;

} // end Class c7
@@ -0,0 +1,66 @@
/*
* Copyright (c) 2021, Google LLC. 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 8273914
* @summary Indy string concat changes order of operations
*

This comment has been minimized.

@shipilev

shipilev Oct 7, 2021
Contributor

Here and later: please add the test block with -XDstringConcat=inline here as well, so that test would verify that every javac strategy produces the same result?

This comment has been minimized.

@cushon

cushon Oct 7, 2021
Author Contributor

Done, thanks

This comment has been minimized.

@cushon

cushon Oct 7, 2021
Author Contributor

(The WellKnownTypes test still only exercises the indy strategies, because it's testing logic that only exists in those strategies.)

This comment has been minimized.

@shipilev

shipilev Oct 12, 2021
Contributor

But still, we probably want to confirm that inline strategy yields the same answer in WellKnownTypes? That's my thinking why to test inline: that is a baseline.

This comment has been minimized.

@cushon

cushon Oct 12, 2021
Author Contributor

I agree in general about using inline as a baseline, but WellKnownTypes doesn't currently check the output of string concatenation, it checks the signature of the invokedynamic it find in the test case.

What do you have in mind for inline here? Should I update the test to assert on the output of string concatenation (and tolerate not being able to find an invokedynamic, which could make the coverage for the indy strategies more fragile)? Or add a different test with similar inputs, and check the output of string concatenation on primitives and boxes?

I think it might make sense to rely on other tests to ensure inline and the indy strategies produce the same results, and use WellKnownTypes as a test just for shouldConvertToStringEagerly, which is only used by the indy strategies.

This comment has been minimized.

@shipilev

shipilev Oct 14, 2021
Contributor

Ah, I missed that WellKnownTypes does not verify concat output. Yes, that resolves my concern, we don't need to handle inline there. Maybe rename WellKnownTypes to WellKnownTypeSignatures or something?

But yes, I think adding another test that verifies the "special" input types produce consistent results across all strategies would be useful.

This comment has been minimized.

@cushon

cushon Oct 14, 2021
Author Contributor

Sure, thanks--I renamed WellKnownTypes to WellKnownTypeSignatures, and added a new WellKnownTypes that tests the actual concat behaviour on primitives for all of the strategies.

This comment has been minimized.

@shipilev

shipilev Oct 15, 2021
Contributor

Thank you, the last change resolves my comments.

* @clean *
* @compile -XDstringConcat=indy StringAppendEvaluatesInOrder.java
* @run main StringAppendEvaluatesInOrder
*
* @clean *
* @compile -XDstringConcat=indyWithConstants StringAppendEvaluatesInOrder.java
* @run main StringAppendEvaluatesInOrder
*
* @clean *
* @compile -XDstringConcat=inline StringAppendEvaluatesInOrder.java
* @run main StringAppendEvaluatesInOrder
*/

public class StringAppendEvaluatesInOrder {
static String test() {
StringBuilder builder = new StringBuilder("foo");
int i = 15;
return "Test: " + i + " " + (++i) + builder + builder.append("bar");
}

static String compoundAssignment() {
StringBuilder builder2 = new StringBuilder("foo");
Object oo = builder2;
oo += "" + builder2.append("bar");
return oo.toString();
}

public static void main(String[] args) throws Exception {
assertEquals(test(), "Test: 15 16foofoobar");
assertEquals(compoundAssignment(), "foofoobar");
}

private static void assertEquals(String actual, String expected) {
if (!actual.equals(expected)) {
throw new AssertionError("expected: " + expected + ", actual: " + actual);
}
}
}