constantInstruction() & DynamicConstantDesc

[Brian Goetz]

Thanks for the experience report!  

You raised three issues:

 - ConstantDescs.NULL
 - Preservation of signed zero
 - round-tripping of DCD

The signed zero definitely looks like a bug and we should fix it as per your patch.  

Handling ConstantDescs.NULL is an interesting question.  Since this is a “convenience” method (the user isn’t asking for a specific opcode, they’re saying “here’s a constant, do something”), either treatment of NULL is probably valid.  If we do nothing, you’ll get a condy that evaluates to null, so you get the same runtime behavior, but then again we could LDC an integer constant of zero rather than iconst_0.  So this is probably reasonable behavior to special-case NULL here.  

I’ll look into the third one.  

Thanks,
-Brian


> On Jul 18, 2022, at 5:36 AM, Michael van Acken <michael.van.acken at gmail.com> wrote:
> 
> I just started to move my most recent Clojure compiler iteration to
> the Classfile API.  It is a pleasure to use.  I appreciate the huge
> amount of work that is going into this.
> 
> While going through my constant handling tests I found three issues.
> 
> First, I expected ConstantDescs.NULL to be mapped to ACONST_NULL.  I
> can just as well special case this in the compiler, of course.
> 
> Second, to get a test case for the literal constant -0.0 to round-trip
> I had to patch CodeBuilder.java to go via LDC:
> 
>    cause: expected not equivalent to actual value
> expected: [(LDC2_W   -0.0) (DRETURN)]
>   actual: [(DCONST_0     ) (DRETURN)]
> 
> Third, a round-trip involving the describeConstable() of a Character
> instance (via ConstantDescs.BSM_EXPLICIT_CAST) was read back as a DCD
> with a nameAndType of "_":"LC;" instead of the expected "_":"C".
> Changing the ofCanonical invocation in ConcreteEntry.java fixed this
> particular case for me, but I am only fishing around here.
> 
> The patch below has more details.
> 
> -- mva
> 
> 
> diff --git a/src/java.base/share/classes/jdk/classfile/CodeBuilder.java b/src/java.base/share/classes/jdk/classfile/CodeBuilder.java
> index 154050ae8b4..330980af725 100755
> --- a/src/java.base/share/classes/jdk/classfile/CodeBuilder.java
> +++ b/src/java.base/share/classes/jdk/classfile/CodeBuilder.java
> @@ -27,6 +27,7 @@ package jdk.classfile;
>  
>  import java.lang.constant.ClassDesc;
>  import java.lang.constant.ConstantDesc;
> +import java.lang.constant.ConstantDescs;
>  import java.lang.constant.DirectMethodHandleDesc;
>  import java.lang.constant.DynamicCallSiteDesc;
>  import java.lang.constant.MethodTypeDesc;
> @@ -424,7 +425,7 @@ public sealed interface CodeBuilder
>  
>      default CodeBuilder constantInstruction(ConstantDesc value) {
>          // This method must ensure any call to constant(Opcode, ConstantDesc) has a non-null Opcode.
> -        if (value == null) {
> +        if (value == null || ConstantDescs.NULL.equals(value)) {
>              return constantInstruction(Opcode.ACONST_NULL, null);
>          }
>          else if (value instanceof Integer iVal) {
> @@ -437,16 +438,16 @@ public sealed interface CodeBuilder
>              else
>                  return constantInstruction(Opcode.LDC2_W, lVal);
>          } else if (value instanceof Float fVal) {
> -            if (fVal == 0.0)
> +            if (fVal.compareTo(0.0f) == 0) // 0.0f but not -0.0f
>                  return with(ConstantInstruction.ofIntrinsic(Opcode.FCONST_0));
> -            else if (fVal == 1.0)
> +            else if (fVal == 1.0f)
>                  return with(ConstantInstruction.ofIntrinsic(Opcode.FCONST_1));
> -            else if (fVal == 2.0)
> +            else if (fVal == 2.0f)
>                  return with(ConstantInstruction.ofIntrinsic(Opcode.FCONST_2));
>              else
>                  return constantInstruction(Opcode.LDC, fVal);
>          } else if (value instanceof Double dVal) {
> -            if (dVal == 0.0d)
> +            if (dVal.compareTo(0.0d) == 0) // 0.0d but not -0.0d
>                  return with(ConstantInstruction.ofIntrinsic(Opcode.DCONST_0));
>              else if (dVal == 1.0d)
>                  return with(ConstantInstruction.ofIntrinsic(Opcode.DCONST_1));
> diff --git a/src/java.base/share/classes/jdk/classfile/impl/ConcreteEntry.java b/src/java.base/share/classes/jdk/classfile/impl/ConcreteEntry.java
> index bead58d7618..9dcc8eeb7e1 100755
> --- a/src/java.base/share/classes/jdk/classfile/impl/ConcreteEntry.java
> +++ b/src/java.base/share/classes/jdk/classfile/impl/ConcreteEntry.java
> @@ -797,7 +797,7 @@ public abstract sealed class ConcreteEntry {
>                  staticArgs[i] = args.get(i).constantValue();
>  
>              return DynamicConstantDesc.ofCanonical(bootstrap().bootstrapMethod().asSymbol(),
> -                                                   nameAndType().name().stringValue(), ClassDesc.of(Util.toClassString(Util.descriptorToClass(nameAndType().type().stringValue()))), staticArgs);
> +                                                   nameAndType().name().stringValue(), ClassDesc.ofDescriptor(nameAndType().type().stringValue()), staticArgs);
>          }
>      }
>