Constant descriptor resolved too soon during constant resolution?

[Vicente Romero]

On 08/10/2018 04:25 PM, Brian Goetz wrote:
> Here's my version, similar but with a different error handling strategy:
>
> @Override public Optional<?extends ConstantDesc<ConstantDesc<T>>> 
> describeConstable() {
>     ConstantDesc<?>[] args =new ConstantDesc<?>[bootstrapArgs.length +5];
>     args[0] =bootstrapMethod.owner().descriptorString();
>     args[1] =bootstrapMethod.methodName();
>     args[2] =bootstrapMethod.methodType().descriptorString();
>     args[3] =constantName;
>     args[4] =constantType.descriptorString();
>     for (int i=0; i<bootstrapArgs.length; i++)
>         args[i+5] = (ConstantDesc<?>) 
> ((Constable)bootstrapArgs[i]).describeConstable().orElseThrow();
>     return Optional.of(DynamicConstantDesc.of(BSM_DYNAMICCONSTANTDESC, 
> ConstantDescs.DEFAULT_NAME,
> CR_DynamicConstantDesc, args));
> }
>
>
> All that said, I'm not sure that we land in the right place here; it 
> is probably more work to LDC this DCD from the constant pool than to 
> construct a fresh one.  Its possible we want some more control knobs 
> in Constable to feed back hints into the compiler as to whether or not 
> constantizing is likely to be profitable, or perhaps we need to 
> revisit the constantBoostrap() protocol for DCD.
>
> So, excellent bug report -- four issues in one!

yep great bug report and analysis too! getting into this after a time out

>
>  - Bug -- compiler incorrectly optimizes away static initializer
>  - Bug -- DynamicConstantDesc.describeConstable() needs to quote its 
> static args
>  - API sharp edge -- varargs version of DCD.of(...)
>  - Possible API insufficiency -- constant-izing DCD may be an 
> anti-optimization

I have pushed fixes to the condy-folding branch for the bugs listed above

Thanks,
Vicente

>
>
> On 8/4/2018 5:36 AM, Jorn Vernee wrote:
>>> Brian told me in an earlier email that `ConstantDesc`s should also be
>>> able to describe themselves, so I think the only missing puzzle piece
>>> here is to add a method to `ConstantDesc` for that:
>>
>> Or I guess a much simpler solution is to cast the descriptors to 
>> `Constable`, since any `ConstantDesc` _should_ implement it:
>>
>> ```
>> diff -r 8ba8e64cc9c2 
>> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> --- 
>> a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java 
>>   Thu Jul 26 22:07:46 2018 +0200
>> +++ 
>> b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java 
>>   Sat Aug 04 11:29:34 2018 +0200
>> @@ -364,7 +364,21 @@
>>          args[2] = bootstrapMethod.methodType().descriptorString();
>>          args[3] = constantName;
>>          args[4] = constantType.descriptorString();
>> -        System.arraycopy(bootstrapArgs, 0, args, 5, 
>> bootstrapArgs.length);
>> +
>> +        int i = 5;
>> +        for(ConstantDesc<?> bsArg : bootstrapArgs) {
>> +            if(bsArg instanceof Constable) { // should always be true
>> +                Constable<?> constable = (Constable<?>) bsArg;
>> +                Optional<? extends ConstantDesc<?>> descOp = 
>> constable.describeConstable();
>> +                if(descOp.isPresent()) { // should always have a value
>> +                    args[i] = descOp.get();
>> +                    i++;
>> +                    continue;
>> +                }
>> +            }
>> +            return Optional.empty(); // fallback just in case
>> +        }
>> +
>>          return 
>> Optional.of(DynamicConstantDesc.of(BSM_DYNAMICCONSTANTDESC, 
>> ConstantDescs.DEFAULT_NAME,
>> CR_DynamicConstantDesc, args));
>>      }
>> ```
>>
>> That patch fixes the bug. It generates this BSMT entry for the 
>> `DynamicConstantDesc`:
>>
>> ```
>>   0: #26 REF_invokeStatic 
>> java/lang/constant/DynamicConstantDesc.constantBootstrap:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;[Ljava/lang/constant/ConstantDesc;)Ljava/lang/constant/DynamicConstantDesc;
>>     Method arguments:
>>       #27 Ljava/lang/invoke/ConstantBootstraps;
>>       #28 invoke
>>       #29 
>> (Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;Ljava/lang/invoke/MethodHandle;[Ljava/lang/Object;)Ljava/lang/Object;
>>       #30 _
>>       #31 Ljava/lang/Object;
>>       #32 #2:_:Ljava/lang/constant/DirectMethodHandleDesc;
>>       #33 Hello,
>>       #34 world!
>> ```
>>
>> Which looks right to me. It has a dynamic constant in there to 
>> reconstitute the method handle descriptor.
>>
>> Sorry about my earlier email, I was a little too eager to find a 
>> solution and was just kind of thinking out loud in the email and 
>> probably being a little incoherent.
>>
>> Jorn
>