RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Claes Redestad]

Extending the existing StringConcatFactoryInvariants test we can easily 
get most basic
combinations covered, along with some basic verification.

This uncovered what appears to be a similar issue in the BC_SB SCF 
strategy, which can be
dealt with by coercing the possibly non-String constant to String at 
generation time:

http://cr.openjdk.java.net/~redestad/8186500/jdk.01/

/Claes

On 08/21/2017 08:35 PM, Aleksey Shipilev wrote:
> On 08/21/2017 08:17 PM, Aleksey Shipilev wrote:
>> On 08/21/2017 08:06 PM, Paul Sandoz wrote:
>>>> On 21 Aug 2017, at 07:48, Claes Redestad <claes.redestad at oracle.com> wrote:
>>>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants fails
>>>> when providing an Integer as a constant, which appears to be due to failure to
>>>> coerce boxed types to the corresponding primitive types when looking up various
>>>> methods in StringConcatHelper:
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500
>>>>
>>>> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
>>>> counterpart, and is a (semantical) no-op for other types, e.g., String.
>>> Looks good. Perhaps a token test would be useful (maybe hard to test all code paths here without some combinator test).
>> Please add tests. We have to at least assert that passing "null" works fine there -- I am not sure
>> it does right now. Since we are talking non-String constants, passing java/lang/Object is a good
>> test too.
> I remembered why we haven't covered this before:
>
>       *   <li><em>\2 (Unicode point 0002):</em> a constant. This input passed
>       *   through static bootstrap argument. This constant can be any value
>       *   representable in constant pool. If necessary, the factory would call
>       *   {@code toString} to perform a one-time String conversion.</li>
>
> Neither Integer, nor Object are representable in constant pool, and so this bug cannot surface via
> JDK 9 bytecode, but only via direct SCF call. So, from some point of view, SCF may exhibit undefined
> behavior, including throwing up on non-CP-representable value.
>
> Using "should be the value representable in constant pool" instead of "can..." would make it
> stronger, but alas. Once other constant flavors are added (/me looks at Paul, Brian and John), it
> would become a possibility. This is why I think we need to add Integer and Object constant tests for
> future proofing SCF, while this is still a gray area.
>
> ...and maybe add {Class, MethodHandle, MethodType} as the tested arguments too, as per:
> https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4
>
> Thanks for taking care of this!
>
> -Aleksey
>
>