> On 7 Nov 2017, at 12:59, John Rose <john.r.r...@oracle.com> wrote: > > Good comments! I can handle a couple of them… > > On Nov 7, 2017, at 11:33 AM, Remi Forax <fo...@univ-mlv.fr> wrote: >> >> I fail to see the point of using validateClassAccess(), if the BSM is used >> through an indy, the type is created only if the caller class can create it, >> so the test seems redundant. > > That's true, but… > >> If it's not used by an indy, why do we need to test that ? Also, why it's >> not called in invoke ? > > …Enum.valueOf doesn't do a security check; that is its choice. > This means that if you pass it an enum type that is not public > or not in a package exported to you, you can still peek at its > enum values. Meanwhile, when javac emits a reference to > an enum, it does so with getstatic. The getstatic bytecode > *does* perform access checks. The call to validateClassAccess > performs those checks, for alignment with the semantics > of getstatic. The internal use of Enum.valueOf is just a detail > of the emulation of getstatic in the case of an enum. > > (Note to self: Never use enums to implement a shared > secrets pattern.) > > For bootstrap methods I prefer to use the most restrictive > set of applicable access rules, handshaking with the lookup. > > In the case of enums it doesn't matter much, as you say, > because Enum.valueOf leaves the door open. >
Yes, Brian and I noticed that so we punted on the access control. > I could go either way on this one. > For consistency with our conservative position on other BSMs we should probably perform the checks. I’ll update. >> There is also no reason to validate that the parameter type is the right one >> because the VM will validate the return type of the BSM is asTypable to the >> type sent as argument. > > For condy, having the BSM validate types is a code smell > for the reason you mention. If the BSM should only accept specific types it seems within its remit to reject thereby better associating the error with the dynamic constant entry. We are dealing here with known fixed types (Class and VarHandle). > Also, when primitives (and value > types) are in the mix, people usually code the validation > incorrectly, since Class.isInstance is the wrong tool, and > the right tool is non-obvious (MHs.identity.asType). > > Are you commenting on the return type adjustment in invoke? > That's just a minor optimization to avoid (un)boxing, which > should have no semantic effect. > >> Some methods use generics, so other don't. Given it's not useful to use >> generics if the methods is used as a BSM, i think the generics signature >> should be removed, otherwise, getstatic and invoke should use generics. > > The generics are present to make normal calls from source > code type check. Use cases: Combinators, test code. > >> to remove the @SuppressWarnings, type should be declared as a >> Class<Class<?>>. > > Paul explained this one to me when I made the same objection. Turns > out that the class literal "Class.class" (which is required to call that > BSM from source code) can only have a raw type. Yuck. So we > declare the formal with a matching amount of raw-ness. > >> Why not using getstatic to retrieve the field Type of the wrapper instead ? > > Because the descriptor is a more compact notation for a primitive type. > Primitive type references will be common and we want them to be small. > Yes, i forgot to mention the size aspect. >> If you have invoke(), you do not need enumConstant because you can cook a >> constant method handle Enum.valueOf and send it to invoke. The methods that >> returns VarHandles are trickier because you need a Lookup object. > > The set is minimal, but not that minimal. We *could* express *everything* > using invoke but that would be grotesquely verbose and opaque. > The "extra" API points are thought to be helpful in reducing class file size, > and also clarifying the intentions of the affected constants. > Yes. >> getstatic should be renamed to getConstant because this is what it does. > > (I could go either way; since the JVM has ConstantValue attrs, "Constant" > is a Thing. If it's getstatic I prefer getStatic, for alignment with > pre-existing > names in jli.) > +1 on getStatic alignment with jli. getConstant is too general for this case IMO. The naming convention we chose was to append Constant if the prefix was a keyword such as null or enum. Paul. >> wrapping the Throwable in a LinkageError seems wrong to me given that a >> calls to a BSM already does that, so getstatic can just declare "throws >> Throawble" like invoke and you have the same semantics. > > The point is that we are emulating the semantics of a getstatic instruction, > which only throws LEs. We don't want other stuff to leak out of the > implementation. Remember that "bytecode behavior" is a normative > specification, to use when applicable. It's applicable here. > > Again, I could go either way on this one. > > Thanks, > — John
