RFR 8129547 (S): Excess entries in BootstrapMethods with the same (bsm, bsmKind, bsmStaticArgs), but different dynamicArgs

[John Rose]

I think one reason this issue comes up is that the elements of the BootstrapMethods array seems to be different from the other entries in the constant pool.

Logically, it would be reasonable to define a CONSTANT_BootstrapMethod which would consist (uniquely) of a CONSTANT_MethodHandle plus a counted array of (0 to N, N ~~ 252) additional constant pool references.

The JSR 292 EG decided to put these guys into a separate constant pool attribute, not because they are not logically part of the constant pool, but because they markedly different in structure from all other constant pool entries.  Note that all constant pool entries are either leaf nodes (utf8, integer, etc.) or else are a tagged structure of one or two constant pool references.  A bootstrap method specifier is a structure of up to 1+N constant pool references.

The Pack200 format deals with BootstrapMethods like this:

> <p>Three more constant pools hold constants defined as part of the
> Java 7 classfile format (major version 51 or later). One more
> constant pool holds constants to be assembled into the
> <tt>BootstrapMethods</tt> attribute, which may be viewed as an
> appendix to the classfile constant pool.</p>

http://docs.oracle.com/javase/8/docs/technotes/guides/pack200/pack-spec.html

(The constant pool is a single heterogeneous array, except for the BSMs attribute, which is a homogeneous array that adds to the constant pool.  If I were king of all classfile formats, and it was time to revolutionize my kingdom, I would reorganize the constant pool into a cadre of homogeneous arrays, as in the Pack200 format.)

— John

On Aug 17, 2015, at 2:23 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> Whoops - that's what I meant to write, sorry; I'm glad you like the overall scheme. I will cook up a test tomorrow - we do have one test which attaches fake dynamic symbols to AST nodes, (using an annotation processor) then invokes code generation and checks the resulting bytecode [1]. I think one such test might be useful here.
> 
> [1] - http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/tip/test/tools/javac/lambda/TestInvokeDynamic.java
> 
> Maurizio
> 
> On 17/08/15 20:24, Aleksey Shipilev wrote:
>> Hi Maurizio,
>> 
>> Thanks for an idea! It almost works, but not quite: super.equals() from
>> Method checks the type, so we need to guard super.*, as you did in
>> hashCode(). We also need a proper subclass check for the keys alone,
>> which means we might move equals() there, and make key a proper subclass
>> to gain access to "other".
>> 
>> In short, this version works:
>>  http://cr.openjdk.java.net/~shade/8129547/webrev.01/
>> 
>> Thanks,
>> -Aleksey
>> 
>> On 17.08.2015 17:15, Maurizio Cimadamore wrote:
>>> Ok, I think I know what you meant now; DynamicMethod is used for two
>>> different purposes:
>>> 
>>> * for the NameAndType CP entry associated with an invokedynamic
>>> instruction (pointed by the CONSTANT_InvokeDynamic entry); this bit
>>> _requires_ dynamic info to be taken into account - as different invoked
>>> types need to map to different entries - even if their underlying BSM
>>> entry is the same.
>>> 
>>> * as keys in the BootstrapMethod attribute; in this case you want the
>>> dynamic info to be gone, as all it's going to be stored in the table is
>>> a bsmKind + static args list.
>>> 
>>> How about something like this:
>>> 
>>> http://cr.openjdk.java.net/~mcimadamore/8129547/
>>> 
>>> Maurizio
>>> 
>>> 
>>> 
>>> On 17/08/15 14:48, Maurizio Cimadamore wrote:
>>>> Hi Alex,
>>>> the only dfference I see between your patch and the current code is
>>>> that the current code is comparing the invokedType, while yours is not
>>>> - am I reading your patch correctly?
>>>> 
>>>> If that's the case, wouldn't dropping the call to super.equals from
>>>> Pool.DynamicMethod also resolve the issue?
>>>> 
>>>> I'm, saying this because, from a design perspective, javac symbols
>>>> usually don't care about uniqueness etc. - that's an extra value added
>>>> when storing them into a constant pool.
>>>> 
>>>> Maurizio
>>>> 
>>>> On 17/08/15 13:33, Aleksey Shipilev wrote:
>>>>> Hi,
>>>>> 
>>>>> This issue gets into way with my current work:
>>>>>    https://bugs.openjdk.java.net/browse/JDK-8129547
>>>>> 
>>>>> There is a proof-of-concept patch:
>>>>>    http://cr.openjdk.java.net/~shade/8129547/webrev.00/
>>>>> 
>>>>> The crux of an issue seems to be the Method.equals() call in
>>>>> DynamicMethod.equals(), that compares the type. The type of
>>>>> DynamicMethod includes the DynamicMethodSymbol.type, that includes
>>>>> dynamic args. It seems DynamicMethodSymbol is a better fit for
>>>>> BootstrapMethod table key, since it includes BSM symbol and static
>>>>> arguments only.
>>>>> 
>>>>> I would be grateful if somebody from compiler team can help me out with
>>>>> this. Is this the fix above valid? Can you do it better?
>>>>> 
>>>>> How would one write a regression test for it? My cursory grep through
>>>>> langtools tests does not yield a clear way to emit a special-shaped indy
>>>>> for such a test.
>>>>> 
>>>>> Thanks,
>>>>> -Aleksey
>>>>> 
>> 
>