Draft JVMS changes for Nestmates
david.holmes at oracle.com
Thu Apr 20 04:12:24 UTC 2017
Limited response as issues have been covered elsewhere, and Dan has
revisited suggested spec wording. But for completeness ...
On 20/04/2017 10:32 AM, John Rose wrote:
> On Apr 18, 2017, at 11:40 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>> 4.7.26 The MemberOfNest Attribute
>> "the class is implicitly a member of its own nest"
>> I suggest changing "a" to "the sole". Otherwise "a" implies there may
>> be other members. As these are static properties being defined I don't
>> think we need to be accounting for some future dynamic expansion of a
> I think you're missing the point of the spec. logic here. If there is a
> non-empty NestMembers attribute, then in fact there *are* other members.
> Therefore "a" is correct, and "the sole" would be incorrect.
Yes another gaseous brain expulsion on my part. I was thinking only of
the case where neither attributes are present.
> Separately, we do need to leave wiggle room for dynamic injection.
> Dan, I think that would be done by allowing a dynamic injection
> operation to implicitly extend the NestMembers list to make a
> "ticket" for the injected class, and implicitly add (if necessary)
> a MemberOfNest to the injected class.
>> 4.10.1 Verification by Type Checking
>> Does the order in the rules mandate the order in which the VM must
>> actually check things? classHasValidNest seems much more complex than
>> the actual runtime process of validation (though it may just be
>> exposing distinct steps that are lumped together at runtime).
> Agree. I think this logic can be simplified. (Will take that to a
> separate sub-thread.)
>> 220.127.116.11 Type Checking for Restricted Member References
>> I'm not very good at reading or understanding these rules, but I'm
>> surprised that the invokespecial rules seem to make no mention of
>> nestmates at all:
>> "A method reference is allowed by an invokespecial instruction if it
>> is not a restricted invokespecial reference, ..."
>> This seems to open things up to all private method references??
> Well, the whole point of nestmates is to extend, in a regular way,
> all access rules that pertain to self-access and privacy.
> If the extension is truly regular, it is not surprising that the spec.
> changes are subtle like this. We're not trying to do an ad hoc
> patch on some set of particular behaviors, but rather extend
> pre-existing notions so they apply in more conditions, including
> invokespecial restrictions.
I was expecting to see the restrictions have a dependency on nestmates
actually being involved - which is how I've currently implemented it. I
can throw away some of the verifier changes if I only need to check for
private access. But I'm unclear exactly where (in the spec) we enforce
that only nestmates get this right to use invokespecial on private
methods? Is it the access check applied during method resolution?
>> 5.4.4 Access Control
>> I don't think you need to restate:
>> "A class with a MemberOfNest attribute belongs to the nest hosted by
>> the referenced host class.
>> A class without a MemberOfNest attribute implicitly belongs to a nest
>> hosted by the class itself. (If the class also lacks a NestMembers
>> attribute, then the nest has only one member.)"
>> The rule is simply stated as is: "belonging to the same nest as D" -
>> belonging to the same nest is, or should be, already defined elsewhere.
> Actually, there is no "elsewhere"; this is where the condition is defined.
> We don't need a special section for "nests"; we don't have a special
> section for "packages" either. So that part reads fine to me.
>> I'm avoiding commenting on the protected access changes but do want to
>> raise a concern that there are no changes to Method resolution
>> (18.104.22.168) yet resolution relies on the definition of access control to
>> prune/discard inaccessible methods. It appears now that we're allowing
>> more potential successful resolutions, then using the additional rules
>> on the invoke* bytecodes to try and discard any undesirable results.
>> 6.5 getfield (and others)
>> You note "These rules are redundant: verification already guarantees
>> them " but this brings me back to an area I keep raising concerns
>> about: the split between static verification based checks and dynamic
>> runtime checks. Yes the verifier precludes certain things but if we
>> run without verification the rules expressed for the bytecodes are
>> considered to be still required at runtime. If you delete them because
>> they overlap with verifier rules we have no way to tell which rules
>> must be enforced regardless of verification status.
>> 6.5 invokespecial
>> You note regarding the IllegalAccessError:
>> "This replaces a VerifyError previously specified by 4.9.2. The check
>> must be delayed until after resolution in order to determine whether
>> the referenced method is private."
>> We know the access flags for the referenced method at verification
>> time - shouldn't that be the sole basis for the verifier check?
>> Afterall it is verifying the static properties of the classfile and
>> bytecode. If the actual resolved method has different access to the
>> referenced method then that may lead to ICCE (depending on the exact
>> nature of the change - a private method made public may not be an
>> issue for example).
>> 22.214.171.124 Method resolution
>> You did not make any changes here, but as per my comment in the bug
>> report we do require, IMHO, further tightening here to ensure nestmate
>> invokespecial invocations do not resolve to method implementations
>> that they should not. The example here is a hierarchy of nestmates (C
>> extends B extends A) where A and C both declare a private method "void
>> m()" and we have an invocation c.m() where c is an instance of class
>> C. We then "separately compile" C to remove the definition of m(). At
>> runtime method resolution will locate A.m, even though the method
>> reference was for C.m. Normally the accessibility rules would exclude
>> A.m from being a viable candidate but here the classes are nestmates
>> so A.m is accessible. But it would be wrong to invoke A.m. This case
>> should throw ICCE or NSME.
More information about the valhalla-spec-observers