RFR (S) 8158851: MH.publicLookup() init circularity, triggered by custom SecurityManager with String concat and -limitmods java.base

[Claes Redestad]

Looks OK to me.

I didn't mean to derail from the core issue solved by this change. 
Dealing with the eager load of ASM (if needed) in a follow-up seems 
appropriate.

/Claes

On 2016-06-07 19:25, Aleksey Shipilev wrote:
> All right, I will fix the issue that *actually* bites me in the bottom
> all the time: MH.publicLookup() init circularity. Here's a webrev
> without an inline blob:
>    http://cr.openjdk.java.net/~shade/8158851/webrev.03/
>
> If you want to improve startup time along the way, file a separate issue.
>
> (I still don't get where the *profit* is? There are 10 small lines of
> inline bytecode, together with the generator used to get it. This is a
> fallback code anyway. Standalone .class is the same byte blob, but
> resting somewhere else. It is as opaque as the inline byte blob. Trying
> to hook class generation to the build sequence takes another 100+ lines
> of assorted source code. I don't get why should one bother, if there is
> little profit for a lot of hassle)
>
> Thanks,
> -Aleksey
>
> On 06/07/2016 07:01 PM, Jim Laskey (Oracle) wrote:
>> If it’s in the jimage, I/O should not be an issue (defineClass from a direct memory mapped buffer) vs assembling and assigning a byte array.  Measure first?
>>
>>
>>> On Jun 7, 2016, at 12:44 PM, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:
>>>
>>> On 06/07/2016 06:09 PM, Jim Laskey (Oracle) wrote:
>>>> Generally I disagree with this kind of hack. It will bite you in the
>>>> @$$ at some point. Why not just have a class file containing the bytes?
>>>
>>> What's the difference between a small binary blob in a
>>> filesystem/resource vs a small binary blob inlined? Apart from the
>>> additional hassle of packing things into resources (which probably
>>> requires dealing with build?), hitting the I/O for loading it (which
>>> probably defeats the purpose of constructing the class on the fly), etc.
>>>
>>> I wonder if the whole LookupHelper business is here to stay: Alan said
>>> that part of mechanics is up to a reimplementation?
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>>>> On Jun 7, 2016, at 11:56 AM, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:
>>>>>
>>>>> On 06/07/2016 04:51 PM, Alan Bateman wrote:
>>>>>> On 07/06/2016 14:38, Aleksey Shipilev wrote:
>>>>>>> Please review a fix for a MH.publicLookup() circularity, which is
>>>>>>> triggered if you run existing String concat tests with -limitmods
>>>>>>> java.base:
>>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8158851
>>>>>>>
>>>>>>> Webrev:
>>>>>>>   http://cr.openjdk.java.net/~shade/8158851/webrev.01/
>>>>>> As a short term fix then this looks okay. It may be (once there is
>>>>>> another round of work on MethodHandles w/modules) that PL moves back to
>>>>>> java.lang.Object but that has many discussion points for another thread.
>>>>>>
>>>>>> If you want then the doPriv in LookupHelper can do away as it is not
>>>>>> required when you can guarantee that it is initialized before a security
>>>>>> manager is set.
>>>>>
>>>>> Done so.
>>>>>
>>>>>> One downside to initializing PL early is that is LookupHelper will
>>>>>> trigger some ASM to be loaded. This could avoid by generating it at link
>>>>>> time and loading the class bytes as resource. Hard to know if it's worth
>>>>>> it as the first lambda usage is going to trigger these classes to load
>>>>>> anyway.
>>>>>
>>>>> Yes. I agree with Claes here, can't we inline the bytecode to avoid
>>>>> messing with ASM?
>>>>>
>>>>> See:
>>>>> http://cr.openjdk.java.net/~shade/8158851/webrev.02/
>>>>>
>>>>> Still passes all java/lang tests.
>>>>>
>>>>> Thanks,
>>>>> -Aleksey
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>