[PATCH][asmtools] Fix incorrect handling of some CPX2 constant pool entries by JDIS

[Leonid Kuskov]

Thank you for your contribution to asmtools.  The patch (*) has been 
tested and applied successfully.

Best regards,
Leonid

*) http://hg.openjdk.java.net/code-tools/asmtools/rev/29ac1d38dde1

On 3/16/18 20:48, Maxim Degtyarev wrote:
> I think in such case we should at least get rid of `switch (cns.tag)`
> in `ConstantPool::ConstantStrValue()` by replacing it with `if (cns
> instanceof CPX2)`
> Updated patch attached.
>
> BTW, I saw my original message was forwarded to the asmtools-dev, but
> that mailing list looks dead. Which mailing list should be used for
> asmtools-related issues?
Please use the code-tools-dev alias for communication.
>
> 2018-03-14 22:04 GMT+03:00 Leonid Kuskov<Leonid.Kuskov at oracle.com>:
>> Hi Maxim,
>>
>> Thank you for the patch. Indeed it does fix a bug - the fix is correct but
>> it seems to be overloaded with extra code.
>> CX2 class contains enough information to split structures into 2 groups:
>> referencing to CLASS_info
>> (Fieldref, Methodref and InterfaceMethodref) and all others, hence the
>> marker class CPX2_ClassMember is not necessary.
>> There you can just add one method to CX2 as follows:
>> boolean refersClassMember() {
>>      return tag == CONSTANT_FIELD || tag == CONSTANT_METHOD || tag ==
>> CONSTANT_INTERFACEMETHOD;
>> }
>> and change only 1 line:
>> -if (cns2.value1 == cd.this_cpx) {
>>
>> +if ( cns2.value1 == cd.this_cpx && cns2.refersClassMember() ) {
>>
>> What do you think if I use this shorter fix as your contribution?
>>
>> Thanks,
>> Leonid
>>
>>
>> On 3/12/18 16:52, Maxim Degtyarev wrote:
>>> The proposed patch (see attached file jdis-cpx2-bug.diff.txt) address
>>> incorrect handling of some constant pool entries represented with CPX2
>>> class by ASMTOOLS JDIS.
>>>
>>> JDIS incorrectly assume that field CPX2::value1 is always used to hold
>>> index of constant pool entry of type CONSTANT_CLASS.
>>> This assumption is valid only for entries of type CONSTANT_FIELD,
>>> CONSTANT_METHOD and CONSTANT_INTERFACEMETHOD.
>>> For CONSTANT_NAMEANDTYPE, CONSTANT_METHODHANDLE, CONSTANT_DYNAMIC and
>>> CONSTANT_INVOKEDYNAMIC this assumption is wrong.
>>> As a result asmtools jdis may produce malformed output for valid input.
>>>
>>> Example:
>>>
>>> public class Test {
>>>
>>>       // Force `class Test` entry appear in the constant pool before
>>> InvokeDynamic entries.
>>>       public Class<?> CLASS = Test.class;
>>>
>>>       private static Runnable[] workers = {
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test"),
>>>           () -> System.out.println("Test")
>>>       };
>>>
>>>       public static void main(String... args) {
>>>
>>>           for(Runnable worker : workers) {
>>>               worker.run();
>>>           }
>>>
>>>       }
>>>
>>> }
>>>
>>> One of invokedynamic instructions may be rendered by jdis as
>>>
>>>           invokedynamic    InvokeDynamic run:"()Ljava/lang/Runnable;";
>>>
>>> instead of
>>>
>>>           invokedynamic    InvokeDynamic
>>>
>>> REF_invokeStatic:java/lang/invoke/LambdaMetafactory.metafactory:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;":run:"()Ljava/lang/Runnable;"
>>> MethodType "()V", MethodHandle
>>> REF_invokeStatic:Test.lambda$static$1:"()V", MethodType "()V";