[Nestmates] RFR (S): Dynamic nestmate update

[David Holmes]

Thanks Lois.

David

On 1/11/2018 6:30 AM, Lois Foltan wrote:
> On 10/31/2018 2:56 AM, David Holmes wrote:
> 
>> 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 for making that change!  Looks good.
> Lois
> 
>>
>> 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
>>>>
>