[Nestmates] RFR (S): Dynamic nestmate update

[David Holmes]

Hi Lois,

I've pushed set_nest_host down into fill_instance_class, where it is set 
as early as possible - which is after apply_parsed_class_metadata as 
that sets the static nest attributes in the IK.

I also had to remove an assertion that's no longer valid at the time we 
try to assert it.

Full webrev: 
http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.v3/

Incremental: 
http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.v3-incr/

This seems early enough to address all your concerns re super access, 
initializers and JFR, even if we don't currently need to see the nest_host.

Thanks,
David

On 31/10/2018 8:25 AM, David Holmes wrote:
> Hi Lois,
> 
> On 31/10/2018 3:43 AM, Lois Foltan wrote:
>> On 10/30/2018 3:28 AM, David Holmes wrote:
>>
>>> Hi Lois,
>>>
>>> Here's an update:
>>>
>>> http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.v2
>>>
>>> Now we push the dynamic_nest_host through to the 
>>> KlassFactory::create_from_stream level, where we set the nest-host 
>>> directly after ClassFileParser.create_instance_klass has been called. 
>>> That should ensure the class is not findable until after the 
>>> nest-host has successfully been set, and doesn't require any further 
>>> cleanup (as far as I can see) if the setting of the nest-host fails.
>>
>> Thank you!  I think the parameter "nest_host" should be even pushed 
>> further into ClassFileParser::create_instance_klass() & then into 
>> ClassFileParser::fill_instance_klass().  The call to set_nest_host 
>> should occur in ClassFileParser::fill_instance_klass for a couple of 
>> reasons.
>>      - 1. check_super_class_access and check_super_interface_access 
>> could be affected by the setting of nest host?
> 
> Not today - nest-based access checks are only for members. If we get to 
> sealed classes and allow dynamic nest members to relate to such sealed 
> classes then we are already in a position where the nest host has to be 
> known much earlier - and there are problems with that. This could be a 
> preemptive move towards supporting that.
> 
>>      - 2. when the mirror is created and static fields are initialized 
>> one of those field's initializers could access a nest member?  Would 
>> this be an issue if nest_host is not set?
> 
> This is default (and compile-time constant?) initialization of fields. 
> Any non-default initialization happens at <clinit> time which is much 
> later - well after the nest-host has been set.
> 
>>      - 3. JFR might want to know this information by the time 
>> JFR_ONLY(INIT_ID(ik);) is invoked at the end of 
>> ClassFileParser::fill_instance_klass()
> 
> Okay - that may be true in the future.
> 
>> And this brings me to my nest code review comment for both of us. I'm 
>> passing 3 more parameters (nonfindable, is_weak and vm_annotations) 
>> from jvm_lookup_define_class() all the way down into ClassFileParser 
>> construction.  We may need to consider creating a new data structure 
>> much like ClassInfo or LinkInfo, something like ClassParseInfo that 
>> contains information for not only our new parameters but existing 
>> unsafe anonymous class information that is passed into 
>> SystemDictionary::parse_stream as well.  Let me know what you think?
> 
> Yes - there is too much tramp data. Unclear whether to simply define a 
> new holder class and pass that around everywhere, or whether to try and 
> incorporate into existing strcutures e.g. perhaps attach it to the 
> ClassFileParser? Depends on where the different bits of data are getting 
> used.
> 
> I'll simply pass the nest-host further down directly for now to test out 
> how this works.
> 
>>>
>>> No changes to InstanceKlass::set_nest_host from previous version 
>>> (sorry I'm not setup to do incremental webrevs ... need to fix that).
>> A couple of minor comments for InstanceKlass::set_nest_host()
>> - line #325 - comment "are a" should be "are"
> 
> Fixed.
> 
>> - If host is NULL, would you consider setting _nest_host to the 
>> current instance class?  Aren't you your own nest host?
> 
> In a static sense you are your own nest host if nothing else is 
> specified. But for this dynamic API a NULL host is a programming error 
> to me.
> 
> Thanks,
> David
> 
>>>
>>> I've also removed Mandy's temporary nestmate access check code in 
>>> reflection.cpp as it is not needed. So InstanceKlass:raw_nest_host() 
>>> is also removed.
>> Good, thank you, that had been something on my list to look at!
>>
>>>
>>> I also reworked jvm_lookup_define_class a little - the use of "const" 
>>> caused flow-on problems that were simpler to avoid by not using "const".
>> Looks good.
>>
>> Thanks,
>> Lois
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 30/10/2018 9:35 AM, David Holmes wrote:
>>>> Hi Lois,
>>>>
>>>> On 30/10/2018 6:01 AM, Lois Foltan wrote:
>>>>> On 10/29/2018 2:37 AM, David Holmes wrote:
>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev/
>>>>>>
>>>>>> This fleshes out Mandy's initial implementation with additional 
>>>>>> error checking on the VM side to match the defineClass API, and 
>>>>>> with an additional VM test.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>> Hi David,
>>>>>
>>>>> I have a couple of concerns about this approach longer term for 
>>>>> setting the nest host post return from either 
>>>>> SystemDictionary::resolve_from_stream or 
>>>>> SystemDictionary::parse_stream within jvm_lookup_define_class().
>>>>>
>>>>> - for a findable class as soon as that class is added to the 
>>>>> SystemDictionary, is there any concern that another thread could 
>>>>> request that class and access it before its nest host is 
>>>>> legitimately set? Would it be better to pass nest host down into 
>>>>> ClassFileParser so it is known at the point the InstanceKlass is 
>>>>> created?
>>>>
>>>> Good question. I hadn't looked at exactly how and when the 
>>>> defineClass logic set the nest host, I was only only updating the 
>>>> actual set_nest_host internal logic.
>>>>
>>>> There are a couple of concerns here:
>>>>
>>>> 1. Finding the class before the nest-host has been set
>>>> 2. Cleaning up in the case that setting the nest-host failed
>>>>
>>>> I don't think either are currently handled correctly - and my change 
>>>> triggers #2. We need a way to make the complete logical operation 
>>>> atomic. That most likely requires pushing the setting of the 
>>>> nest-host deeper into the class loading logic - possibly immediately 
>>>> after IK creation as you suggest.
>>>>
>>>>> - At the time class file nest members attributes are parsed and the 
>>>>> nest host setting within the byte stream matches the nest host used 
>>>>> when jvm_lookup_define_class() is invoked, than in my opinion an 
>>>>> error should not result.
>>>>
>>>> I disagree. You're either using dynamic nest membership or you're 
>>>> using static nest membership - using both, even if the end result 
>>>> would be the same, seems an error in the programming model to me. 
>>>> Afterall if the defined class is already a valid static nest member 
>>>> then you don't need to use the NESTMATE property to inject it. And 
>>>> it is definitely an error to claim a static nest-host when the host 
>>>> does not statically list you as a member!
>>>>
>>>>> - Finally, should the setting of nest_host be known at the point 
>>>>> class file load hook is processed?
>>>>
>>>> I'm not sure what you mean. At what point in the current code would 
>>>> the byte[] passed to defineClass be replaced by one from the CLFH? 
>>>> As long as the byte[] is final before we actually set_nest_host, I 
>>>> don't think it matters.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Lois
>>