[Nestmates] RFR Lookup.defineClass nest-host update

[Mandy Chung]

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.

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

Mandy