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

[Aleksey Shipilev]

Hey Maurizio,

I have tried to slim down TestInvokeDynamic to make a regression test.
It fails on current javac, and passes on a patched one:
  http://cr.openjdk.java.net/~shade/8129547/webrev.02/

Thanks,
-Aleksey

On 08/18/2015 12:23 AM, Maurizio Cimadamore 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
>>>>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20150821/2c6f10b2/signature.asc>