RFR 8187742 Minimal set of bootstrap methods for dynamic constants

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8187742 Minimal set of bootstrap methods for dynamic constants

Paul Sandoz
Hi,

Please review the patch that adds a minimal set of bootstrap methods can be be used for producing dynamic constants:

  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/>

The aim is to provide a small but common set of BSMs that are likely useful with ldc or as BSM arguments, filling in the gaps for constants that cannot be currently represented, such as null, primitive classes and VarHandles. It’s possible to get more sophisticated regarding say factories but that is something to consider later on.

This patch is based off the minimal dynamic constant support (still in review):

  http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html <http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html>

Paul.

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

Remi Forax
Hi Paul,

You have an import static requireNonNull but it is only used once in nullConstant, all other methods use Objects.requireNonNull.

The test that checks that lookup, name and type are null or not is different in each method,
so by example in primitiveClass, if type equals null, you get an IAE.

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. If it's not used by an indy, why do we need to test that ? Also, why it's not called in invoke ?

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.

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.

In nullConstant, you do not need a requireNonNull here, getting you call isPrimitive() just after.

In primitiveClass, the second test is equivalent to name.length() != 1, to remove the @SuppressWarnings, type should be declared as a Class<Class<?>>. Why not using getstatic to retrieve the field Type of the wrapper instead ?

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.

getstatic should be renamed to getConstant because this is what it does.
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.

in arrayVarHandle, the doc said that lookup is not used but lookup is used.

in validateClassAccess, the return can be moved at the end of the method.

and as a minor issue, all the ifs should be followed by curly braces per the coding convention.

cheers,
Rémi

----- Mail original -----
> De: "Paul Sandoz" <[hidden email]>
> À: "core-libs-dev" <[hidden email]>, "hotspot-dev" <[hidden email]>
> Envoyé: Mardi 7 Novembre 2017 19:38:57
> Objet: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> Hi,
>
> Please review the patch that adds a minimal set of bootstrap methods can be be
> used for producing dynamic constants:
>
>  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/
>  <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/>
>
> The aim is to provide a small but common set of BSMs that are likely useful with
> ldc or as BSM arguments, filling in the gaps for constants that cannot be
> currently represented, such as null, primitive classes and VarHandles. It’s
> possible to get more sophisticated regarding say factories but that is
> something to consider later on.
>
> This patch is based off the minimal dynamic constant support (still in review):
>
>  http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html
>  <http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html>
>
> Paul.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

Remi Forax


----- Mail original -----
> De: "John Rose" <[hidden email]>
> À: "Rémi Forax" <[hidden email]>
> Cc: "Paul Sandoz" <[hidden email]>, "hotspot-dev" <[hidden email]>, "core-libs-dev"
> <[hidden email]>
> Envoyé: Mardi 7 Novembre 2017 21:59:41
> Objet: Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> Good comments!  I can handle a couple of them…
>
> On Nov 7, 2017, at 11:33 AM, Remi Forax <[hidden email]> 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.
>
> I could go either way on this one.

As you said, javac use getstatic (the bytecode), why not reuse getstatic (the BSM) in that case, being a field of an enum doesn't seem important and at least you are sure that you have the semantics of getstatic (you miss the cache in java.lang.Class but i'm not sure it worth it).

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

no, the return type adjustment is ok for me, it avoid unboxing + boxing if the original handle returns a primitive.

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

so getstatic and invoke should have generics in their signature too.

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

yes, right X.class and x.getClass() are the two rules in the JLS that can introduce raw types.
I think i prefer 'type' to be typed as a Class<?> in that case, it's not a raw type and you can assign Class<Class>> to it.

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

Ok, you have to send the wrapper type if you use getstatic, so you are saving a reference to the wrapper type.

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

perhaps getFinalStatic, because it's restricted to final field.

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

it's not the catch line 155 that bother me, it's the other line 165 (and 162).

>
> Again, I could go either way on this one.
>
> Thanks,
> — John

Rémi
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

Remi Forax
In reply to this post by Remi Forax


----- Mail original -----
> De: "Paul Sandoz" <[hidden email]>
> À: "Remi Forax" <[hidden email]>
> Cc: "core-libs-dev" <[hidden email]>, "hotspot-dev" <[hidden email]>
> Envoyé: Mardi 7 Novembre 2017 21:54:06
> Objet: Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> Hi Remi,
>
> You are asking a lot of the same questions we went through a number of times
> before landing where we are :-)
>
>
>> On 7 Nov 2017, at 11:33, Remi Forax <[hidden email]> wrote:
>>
>> Hi Paul,
>>
>> You have an import static requireNonNull but it is only used once in
>> nullConstant, all other methods use Objects.requireNonNull.
>>
>
> Fixed.
>
>
>> The test that checks that lookup, name and type are null or not is different in
>> each method,
>> so by example in primitiveClass, if type equals null, you get an IAE.
>>
>
> Fixed.
>
>
>> 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. If it's not used by an indy, why do we need to test
>> that ?
>
> We are being deliberately conservative ensuring the lookup is consistent with
> the type in case nefarious byte code spinners punch a hole (not proven a hole
> can be punched, just being conservative).
>
>
>> Also, why it's not called in invoke ?
>>
>
> What “it” are you referring to?

validateClassAccess.

>
>
>> 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.
>>
>
> Yes, but we prefer that the BSM reject thereby better signalling the source of
> the error.

the method returns a Class in its signature, so it can not be something else if not call by a condy and if the type is wrong in the bytecode, the VM will throw a CCE with the same info than your IAE.
i still think it's unnecessary.

>
>
>> 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.
>>
>
> I updated the nullConstant method and removed the type variable.
>
> In general we try to be consistent with the explicit erasure unless it causes
> some contortion in the implementation as is the case for Enum.
>
>
>> In nullConstant, you do not need a requireNonNull here, getting you call
>> isPrimitive() just after.
>>
>
> We decided to be explicit rather than implicit for all null checks.
>
>
>> In primitiveClass, the second test is equivalent to name.length() != 1, to
>> remove the @SuppressWarnings, type should be declared as a Class<Class<?>>. Why
>> not using getstatic to retrieve the field Type of the wrapper instead ?
>>
>
> Try passing Class.class to it. To be honest this is somewhat motivated by
> testing when called explicitly.

see my answer to John.

>
>
>> If you have invoke(), you do not need enumConstant because you can cook a
>> constant method handle Enum.valueOf and send it to invoke.
>
> It’s also possible to support via getStatic as well, as is the case for
> primitive class.
>
> We went back and forth on the generic and specific axis. For cases where we
> considered constants are “honorary" members of the constant pool we provide
> explicit BSMs.
>
>
>> The methods that returns VarHandles are trickier because you need a Lookup
>> object.
>>
>> getstatic should be renamed to getConstant because this is what it does.
>
> No, we want to stick closely with the notion of what the BSM does, there is a
> close connection also with MethodHandle getStatic, it’s performing a static
> field access. “getConstant” is too vague, notionally all the BSMs return
> constants.
>
>
>> 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.
>>
>
> We don’t want to declare Throwable for the API in this case. This try/catch is
> just ceremony since a getstatic MH in the implementation can only throw a VM
> error e.g. related to out of memory or stack overflow. I can add some comments
> in the code.

but you do that in invoke().

>
> Note that reflective exceptions are mapped to their equivalent errors. Although
> the BSM has been linked it is being asked to perform what can be considered
> further linkage.
>
>
>> in arrayVarHandle, the doc said that lookup is not used but lookup is used.
>>
>
> Fixed.
>
>
>> in validateClassAccess, the return can be moved at the end of the method.
>>
>> and as a minor issue, all the ifs should be followed by curly braces per the
>> coding convention.
>>
>
> I expanded to curly braces.
>
> Paul.

Rémi
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

Paul Sandoz

> On 7 Nov 2017, at 14:18, [hidden email] wrote:
>>
>> Try passing Class.class to it. To be honest this is somewhat motivated by
>> testing when called explicitly.
>
> see my answer to John.
>

Fair point, i’ll go with Class<?>. It’s the less smelly option.

Updated along with other changes:

  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/>


>>
>>
>>> If you have invoke(), you do not need enumConstant because you can cook a
>>> constant method handle Enum.valueOf and send it to invoke.
>>
>> It’s also possible to support via getStatic as well, as is the case for
>> primitive class.
>>
>> We went back and forth on the generic and specific axis. For cases where we
>> considered constants are “honorary" members of the constant pool we provide
>> explicit BSMs.
>>
>>
>>> The methods that returns VarHandles are trickier because you need a Lookup
>>> object.
>>>
>>> getstatic should be renamed to getConstant because this is what it does.
>>
>> No, we want to stick closely with the notion of what the BSM does, there is a
>> close connection also with MethodHandle getStatic, it’s performing a static
>> field access. “getConstant” is too vague, notionally all the BSMs return
>> constants.
>>
>>
>>> 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.
>>>
>>
>> We don’t want to declare Throwable for the API in this case. This try/catch is
>> just ceremony since a getstatic MH in the implementation can only throw a VM
>> error e.g. related to out of memory or stack overflow. I can add some comments
>> in the code.
>
> but you do that in invoke().
>

There is no choice when Invoking a method handle that is passed to us.

In the case of getStatic the use of a MH can be considered an implementation detail (we could use reflection) and we know what the j.l.invoke implementation does. In this case i believe only VM errors can be produced.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

John Rose-3
In reply to this post by Remi Forax
On Nov 7, 2017, at 1:35 PM, [hidden email] wrote:
>
> perhaps getFinalStatic, because it's restricted to final field.

The restriction to final field is because we are expecting the
method to be used from condy, where it seems to be a syntax
error to sample a constant from a non-constant variable.

That said, I too would prefer to make it explicit (even in the name)
that the BSM applies only to static final fields.

So +1 for getStaticFinal instead of getStatic.  We also discussed
this internally back and forth.   Your comment pushed me over
the edge.

("getFinalStatic" is wrong BTW because "Final" is an extra
constraint after "getStatic", and because "static final" is the
usual written order for the corresponding modifiers, not
"final static", at least in the relevant source bases.)

— John
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

John Rose-3
In reply to this post by Remi Forax
On Nov 7, 2017, at 12:59 PM, John Rose <[hidden email]> wrote:
>
> For condy, having the BSM validate types is a code smell
> for the reason you mention.  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).

For the record, here is a method that performs the correct conversions:

https://bugs.openjdk.java.net/browse/JDK-8190982

The bug suggests that it be called Class::convert.

The code itself is simple enough to cut-n-paste into your own BSMs.

— John
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

Volker Simonis
In reply to this post by Paul Sandoz
On Thu, Nov 9, 2017 at 8:06 PM, Paul Sandoz <[hidden email]> wrote:

> Hi Volker,
>
>> On 9 Nov 2017, at 01:01, Volker Simonis <[hidden email]> wrote:
>>
>> Hi Paul,
>>
>> just some quick process-related questions.
>>
>> Is this intended to be targeted for jdk 10?
>>
>
> Yes, that’s what we are aiming for. I am front loading reviews ahead of a propose to target (which should hopefully happen soon) to make better use of time given the shorter cycles, thus increasing the quality and giving heads up to other developers like yourself who are responsible for other platforms.
>
>
>> Is the current implementation already available in a separate
>> repository and/or branch? I've read in the RFR for 8186046 that some
>> parts are currently being refined in the amber repository. Or is the
>> complete implementation already available there?
>>
>
> A super set of functionality is present in the amber repo under the condy branch. We pealed a sub-set of that off and created patches for the idk/hs repo tracked in:
>
>   Minimal ConstantDynamic support
>   https://bugs.openjdk.java.net/browse/JDK-8186046
>   Tool support for ConstantDynamic
>   https://bugs.openjdk.java.net/browse/JDK-8186209
>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>
>   Minimal set of bootstrap methods for dynamic constants
>   https://bugs.openjdk.java.net/browse/JDK-8187742
>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/
>
> While these are under review they might diverge a little from the condy branch in the amber repo but i will sync back up in batches.
>
>
>> If I want to port this to ppc64, what's the best way to start and what
>> do I need? Is it just the two webrevs for 8187742 and 8186046 on top
>> of the jdk hs repo? Or is it a branch of the amber repo?
>>
>
> Yes, and focus on the following applied to the jdk/hs repo:
>
>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>
> There are tests in that patch that are currently targeted to run only on x86 that would otherwise tickle failures on other platforms.
>
>
>> You may know that there's currently a discussion going on about what
>> is a JEP and what's the right way to implement and integrate a JEP
>> into the mainline. The general idea is that only 'finished' JEPs will
>> be targeted and integrated into the always feature complete main line.
>> So is this and the review for 8186046 about the integration into the
>> jdk repo (which shouldn't happen before the JEP is not targeted for
>> jdk10) or is it just the review before the JEP can actually be
>> targeted? This is important because there's not much time before the
>> jdk10 repos will enter Rampdown phase 1 and we would still have to
>> port this to ppc (and also ARM/SPARC) if it will be targeted for jdk
>> 10.
>>
>> I don't want to question the merits and quality of the current
>> implementation which I'm sure are great. For me as an external
>> observer its just hard to oversee the current status of the
>> implementation.
>>
>
> I hear you, thanks for your patience. Is it a little clearer now?
>

Thanks Paul!

Things are much clearer now. I'll start looking at the changes
required for ppc/s390.

Regards,
Volker

> From my own perspective this is the first flight over the new release cycle territory, the ride might be a little bumpy while we learn to pilot this better :-)
>
> Paul.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

stanislav lukyanov
In reply to this post by Paul Sandoz
On 15.11.2017 6:19, Paul Sandoz wrote:
>> On 10 Nov 2017, at 01:36, stanislav lukyanov <[hidden email]> wrote:
>>
>> On 10.11.2017 0:13, Paul Sandoz wrote:
>>> <...>
>>> I would prefer to follow up later on with more non-normative explanatory text. It will take some careful crafting and i don’t want that to side-track the review for the moment (including guidance on what forms of computed constants are acceptable).
>> Fair. Does it make sense to add a sub-task for this to JDK-8185992 to keep it on the radar?
> You would mind adding a sub-task with a tbd minor target?
TBD_Minor would mean an update release, right?
I think it can't be a TBD_Minor then since it is technically a
specification change and should happen in a Java SE release (maybe 11?),
but maybe I'm wrong.
Anyway, as long as that's logged and not completely forgotten, I'm happy :)

Thanks,
Stas

>
>
>>> <...>
>>>
>>> And adjusted the invoke method:
>>>
>>> * @param args the arguments to pass to the method handle, as if with
>>> * {@link MethodHandle#invokeWithArguments}.  Each argument may be
>>> * {@code null}.
>>> ...
>>> * @throws NullPointerException if {@code args} is {@code null}
>>> * (each argument of {@code args} may be {@code null}).
>> I think the `@throws NPE` is not needed, at least not from the normative perspective.
>> The class-level text about NPE doesn't assume anything about the contents of objects passed to the methods,
>> and the second part of the `@param args` already clarifies the behavior for this particular case.
>> Arguably having this `@throws` might improve clarity, but I would rather remove it.
> Ok, if you are fine with that i will remove it.
>
>
>>> We have been fleshing out the NPE behaviour because we know you will log bugs against us later on if we don’t :-)
>> Yep, that's sounds familiar :)
>>
>> <...>
>>
>> Thanks for the updates!
>>
>> Also, javac isn't going to use `::primitiveClass` instead of `getstatic Integer.TYPE` to load int.class, right?
>> If it is, int.class probably needs to be changed to Integer.TYPE in sun.invoke.util.Wrapper to avoid cycles.
>>
> Yes, i noticed that too :-)
>
> There are no plans in this patch to update javac code generation for primitive classes but if we do we may need to modify the wrapper implementation as you point out. There are might other areas within the base module (specifically in j.l.invoke) where referring to say int.class may cause class initialization issues early in the startup. That’s awkward (just like we cannot use lambda expressions in certain cases), we might need to retain the old translation strategy just for code within the base module or support a compilation error.
>
> Paul.