[Nestmates] RFR Lookup.defineClass nest-host update

[David Holmes]

Hi Mandy,

On 6/11/2018 7:34 AM, Mandy Chung wrote:
> 
> 
> On 11/5/18 1:16 PM, David Holmes wrote:
>> Hi Mandy,
>>
>> On 6/11/2018 4:52 AM, Mandy Chung wrote:
>>> Hi David,
>>>
>>> On 11/4/18 11:03 PM, David Holmes wrote:
>>>> webrev: 
>>>> http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.nest_host_update/ 
>>>>
>>>>
>>>> Testing showed that the internal use of Class.getNestHost inside 
>>>> Lookup.defineClass was not suitable as invalidly defined 
>>>> nest-hosts/members resulted in an inappropriate nest-host being 
>>>> passed into the the VM to set_nest_host (a class becomes it's own 
>>>> nest-host if there are any validation errors). set_nest_host needs a 
>>>> valid nest-host else we'll hit assertion failures.
>>>>
>>>> The solution was to pass the lookupClass through to the VM and have 
>>>> jvm_lookup_define_class do a direct InstanceKlass::nest_host() 
>>>> lookup that will throw exceptions if there are validation issues.
>>>>
>>>
>>> The fix looks fine to me.  The adjusted logging statement reads 
>>> better, thanks.  Should we add a test for it?
>>
>> What kind of test, it is just an informational output AFAICS.
>>
> 
> I referred to ICCE.  I was thinking when the LoggerFinder tests that 
> uncover this bug are updated and whether this issue is caught by 
> existing tests.

Oops - sorry misunderstood. :)

Yes LoggerFinder throws the ICCE with this fix (and use of dynamic 
nestmates). This doesn't violate the Lookup.defineClass API as this 
exception pertains to the lookupClass not to the class being defined. We 
should probably keep the current "broken" form of the LoggerFinder as a 
regression test for this part. I'll extract it into a new testcase.

>>> Alternatively  we can keep JVM_LookupDefineClass to expect the lookup 
>>> class parameter must be the nest host (that's the intent - do you see 
>>> a check in the VM side to enforce that?).  The library will do the 
>>> validation and pass a valid nest host to JVM entry point to keep it 
>>> simple.   In that case perhaps JVM_GetNestHost can take an additional 
>>> argument with exception or not.   Lois may have an opinion.
>>
>> I wanted to avoid having to expose a second API to call into the VM to 
>> get the nest-host but allow for exceptions. This also reduces the 
>> number of calls into the VM.
>>
>> Further, all the way through the library code and into the VM the 
>> class parameter was already called "lookup", but in fact was only 
>> non-NULL when it was actually the nest-host - which need not be the 
>> same as the lookup class. At least now the parameter is always 
>> non-null and always the lookup.
> 
> I'm fine with your current patch.  I agree to avoid a second API to get 
> the nest host with the exception.

Thanks,
David

> Mandy