8226389 [asm] : jlink tests fail; asm has wrong understanding of new bytecodes

[Roger Riggs]

Hi Mandy,

The issue was with asm's treatment of the new bytecodes.

I assume later asm will be updated to support the new bytecodes and they 
will developer tests
so the lack of testing is transient.

If the test was jlink focused, I would want to ask what aspect of an 
inline class file is being tested?
New flags, new constant pool entries, etc.

We will get some more coverage when we start using inline classes in the 
JDK itself
as a side effect of the current tests.

I am also concerned about in the long run, how we ensure that every API
that touches a class is passed an inline class (with variations of 
inline classes).
We'll have the same issue with records.

I wondered if the profiling mechanism could be used to accumulate data
on every public API and record whether the instances that are passed are 
ever inline classes.

This was a point f

Thanks, Roger


On 6/21/19 11:00 AM, Mandy Chung wrote:
> I am not aware we imported any ASM test.  At this time I concerned the 
> lworld
> development.  If jlink switched to use a different bytecode library, we
> would have a test reproducing the issue.  It's okay to follow up the test
> separately.
>
> Mandy
>
> On 6/21/19 7:40 AM, Roger Riggs wrote:
>> Hi Mandy,
>>
>> An ASM test would be more appropriate;  jlink itself doesn't much 
>> care about the classes.
>> The particular test that was failing was one of the Strip debug tests.
>>
>> Where are the ASM tests?
>>
>> Thanks, Roger
>>
>>
>> On 6/20/19 7:24 PM, Mandy Chung wrote:
>>> I assume you run into the jlink issues with JDK using the inline class.
>>> It's good to add a new test that creates a module with inline class
>>> and invoke jlink to produce an image.
>>>
>>> Mandy
>>>
>>> On 6/20/19 11:34 AM, Roger Riggs wrote:
>>>> Hi,
>>>>
>>>> I discovered that the visiting of the new opcodes was not correct. 
>>>> It skipped over them correctly
>>>> but did not visit each opcode.
>>>> The 'default' opcode argument is a type name and so should call the 
>>>> visitTypeInsn method.
>>>> The "withfield' opcode argument is a field ref (class and name) so 
>>>> should call visitFieldInsn.
>>>> And the INLINE prefixes are dropped.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~rriggs/webrev-asm-8226389-4/index.html
>>>>
>>>> Thanks, Roger
>>>>
>>>>
>>>> On 6/20/19 1:38 PM, Mandy Chung wrote:
>>>>> Looks okay.  The INLINE_* prefix is not necessary to me and matching
>>>>> the JVMS opcode names makes sense.
>>>>>
>>>>> Mandy
>>>>>
>>>>>
>>>>> On 6/19/19 10:57 AM, Roger Riggs wrote:
>>>>>> Hi Mandy,
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-asm-8226389-2/
>>>>>>
>>>>>> A more robust version that computes the necessary offsets of the 
>>>>>> ASM_ opcodes
>>>>>> to relocate them to the unused indexes above the new WithField 
>>>>>> opcode.
>>>>>>
>>>>>> Thanks, Roger
>>>>>>
>>>>>>
>>>>>> On 6/19/19 12:22 PM, Mandy Chung wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/19/19 7:47 AM, Roger Riggs wrote:
>>>>>>>> Please review a patch to asm to correctly recognize the 
>>>>>>>> Valhalla defined bytecodes for defaultvalue and withfield.
>>>>>>>> (The jlink tests that read and write classfiles were failing).
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-asm-8226389/
>>>>>>>
>>>>>>> Label::resolve method depends on the assumption that IFEQ ... 
>>>>>>> JSR can be changed to ASM_IFEQ to ASM_JSR. It seems that the 
>>>>>>> xxx_DELTA  needs to be adjusted.
>>>>>>>
>>>>>>> Mandy
>>>>>>
>>>>>
>>>>
>>>
>>
>