[Nestmates] RFR Lookup.defineClass nest-host update
mandy.chung at oracle.com
Mon Nov 5 21:34:28 UTC 2018
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:
>>> 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
>> 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.
More information about the valhalla-dev