[Nestmates] RFR (S): Dynamic nestmate update

[Harold David Seigel]

Hi David,

In klassFactory.cpp, does it matter that the assert passes NULL, instead 
of dynamic_nest_host, to create_instance_klass() ?

228 InstanceKlass* result = parser.create_instance_klass(old_stream != 
stream, dynamic_nest_host, CHECK_NULL);
229 assert(result == parser.create_instance_klass(old_stream != stream, 
NULL, THREAD), "invariant"); Thanks, Harold


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
>
> 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
>>>