Re: RFR: JDK-8213480: update internal ASM version to 7.0

[David Holmes]

On 14/11/2018 12:40 am, Brian Goetz wrote:
But don’t they?  We intend to start generating condy in classfiles from 
javac soon; at that point, anyone not using ASM7 will fail when reading 
classfiles generated by javac.


See later email. They need this for nestmates now as well.

Cheers,
David

On Nov 8, 2018, at 4:03 AM, David Holmes > wrote:


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features? If 
so then you could split out the actual update of ASM from the updates 
to the users of ASM (some of which may be quite fine with ASM5).

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Igor,

Thanks for your comment. I have already applied in my local copy.

Vicente

On 11/13/18 8:30 PM, Igor Ignatyev wrote:

Hi Vicente,

you need to replace "@ignore 8194951: some mlvm tests fail w/ ASMv7" w/ "@ignore 
8194951" in all the occurrences, as we have monitoring tools which expect @ignore to be 
followed by a space-separated list of bug ids. the rest looks good to me.

Thanks,
-- Igor


On Nov 13, 2018, at 5:11 PM, Vicente Romero  wrote:

Hi Alan,

On 11/13/18 9:18 AM, Alan Bateman wrote:

On 13/11/2018 14:00, Vicente Romero wrote:

any other comment after the last iteration? we are in a bit of a hurry to push 
this before the JDK 12 train departs :(

The original patch updated all the use sites (and tests) to specify ASM7 for 
the API version. I just checked the webrev again now and it seems to be just 
the ASM refresh now. Assuming all the tests are passing and you've sorted out 
the mvlm test issues with Igor then I suggest go ahead with this push and we 
can update the sites, as needed, at a later time.

in the last update I sent links to two patches [1] is the ASM7 only changes and 
[2] is the changes to the use sites. My plan is to push both together, but I 
split them to ease the review process. But still I will get your go and push it 
as good ;)


-Alan

Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Igor Ignatyev]

Hi Vicente,

you need to replace "@ignore 8194951: some mlvm tests fail w/ ASMv7" w/ 
"@ignore 8194951" in all the occurrences, as we have monitoring tools which 
expect @ignore to be followed by a space-separated list of bug ids. the rest 
looks good to me.

Thanks,
-- Igor

> On Nov 13, 2018, at 5:11 PM, Vicente Romero  wrote:
> 
> Hi Alan,
> 
> On 11/13/18 9:18 AM, Alan Bateman wrote:
>> On 13/11/2018 14:00, Vicente Romero wrote:
>>> any other comment after the last iteration? we are in a bit of a hurry to 
>>> push this before the JDK 12 train departs :(
>> The original patch updated all the use sites (and tests) to specify ASM7 for 
>> the API version. I just checked the webrev again now and it seems to be just 
>> the ASM refresh now. Assuming all the tests are passing and you've sorted 
>> out the mvlm test issues with Igor then I suggest go ahead with this push 
>> and we can update the sites, as needed, at a later time.
> 
> in the last update I sent links to two patches [1] is the ASM7 only changes 
> and [2] is the changes to the use sites. My plan is to push both together, 
> but I split them to ease the review process. But still I will get your go and 
> push it as good ;)
> 
>> 
>> -Alan
> 
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
> [2] 
> http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/
>  

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Alan,

On 11/13/18 9:18 AM, Alan Bateman wrote:

On 13/11/2018 14:00, Vicente Romero wrote:
any other comment after the last iteration? we are in a bit of a 
hurry to push this before the JDK 12 train departs :(
The original patch updated all the use sites (and tests) to specify 
ASM7 for the API version. I just checked the webrev again now and it 
seems to be just the ASM refresh now. Assuming all the tests are 
passing and you've sorted out the mvlm test issues with Igor then I 
suggest go ahead with this push and we can update the sites, as 
needed, at a later time.


in the last update I sent links to two patches [1] is the ASM7 only 
changes and [2] is the changes to the use sites. My plan is to push both 
together, but I split them to ease the review process. But still I will 
get your go and push it as good ;)




-Alan


Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Brian Goetz]

But don’t they?  We intend to start generating condy in classfiles from javac 
soon; at that point, anyone not using ASM7 will fail when reading classfiles 
generated by javac.  

> On Nov 8, 2018, at 4:03 AM, David Holmes  wrote:
> 
> Is it that case that the code the uses the ASM library, like the JFR code and 
> jlink code, and the tests, doesn't actually _have to_ change to specifying 
> Opcodes.ASM7 unless they plan on using ASM7 features? If so then you could 
> split out the actual update of ASM from the updates to the users of ASM (some 
> of which may be quite fine with ASM5).

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Alan Bateman]

On 13/11/2018 14:00, Vicente Romero wrote:
any other comment after the last iteration? we are in a bit of a hurry 
to push this before the JDK 12 train departs :(
The original patch updated all the use sites (and tests) to specify ASM7 
for the API version. I just checked the webrev again now and it seems to 
be just the ASM refresh now. Assuming all the tests are passing and 
you've sorted out the mvlm test issues with Igor then I suggest go ahead 
with this push and we can update the sites, as needed, at a later time.


-Alan

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

any other comment after the last iteration? we are in a bit of a hurry 
to push this before the JDK 12 train departs :(


Thanks,
Vicente

On 11/8/18 11:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only 
the changes to the internal asm and [2] contains the changes to the 
clients plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/


On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal 
version in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[forax]

It's not the simplest solution but you can use a classloader or an agent to 
patch the bytecode of ASM with ASM at runtime :)

Rémi

- Mail original -
> De: "Igor Ignatyev" 
> À: "Remi Forax" 
> Cc: "Vicente Romero" , "Alan Bateman" 
> , "hotspot-dev"
> , "core-libs-dev" 
> 
> Envoyé: Vendredi 9 Novembre 2018 22:12:48
> Objet: Re: RFR: JDK-8213480: update internal ASM version to 7.0

> Hi Remi,
> 
> 
> w/ SymbolTable::get being private method _and_ SymbolTable being final class, 
> it
> doesn't sound like the easiest way ;) w/ that being said, I haven't looked at
> the latest version of ASM, so there might be ways to get that we need.
> 
> Thanks,
> -- Igor
> 
>> On Nov 9, 2018, at 11:33 AM, Remi Forax  wrote:
>> 
>> Hi Igor,
>> sorry to have not answered to your mail :(
>> 
>> I believe that the easiest way to disable the caching of the entries of the
>> constant pool in ASM is to change SymbolTable.get() [1] to always return 
>> null.
>> 
>> regards,
>> Rémi
>> 
>> [1]
>> https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/SymbolTable.java#L393
>> 
>> - Mail original -----
>>> De: "Igor Ignatyev" 
>>> À: "Vicente Romero" , "Alan Bateman"
>>> 
>>> Cc: "hotspot-dev" , "core-libs-dev"
>>> 
>>> Envoyé: Vendredi 9 Novembre 2018 17:25:49
>>> Objet: Re: RFR: JDK-8213480: update internal ASM version to 7.0
>> 
>>> Vicente, Alan,
>>> 
>>> back when we 1st bumped into this problem w/ ClassWriterExt (about 1y ago), 
>>> it
>>> was my AI to discuss it w/ Remi. I sent him an email, didn't get a replay, 
>>> and
>>> as it usually goes, had to switch to something else, completely forgot about
>>> that and didn't follow up.
>>> 
>>> Thanks,
>>> -- Igor
>>> 
>>>> On Nov 8, 2018, at 8:40 AM, Vicente Romero  
>>>> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 11/8/18 8:14 AM, Alan Bateman wrote:
>>>>> On 07/11/2018 19:33, Igor Ignatyev wrote:
>>>>>> Hi Vicente,
>>>>>> 
>>>>>> I recall an (internal?) discussion about updating ASM somewhen in JDK 
>>>>>> 11TF, and
>>>>>> AFAIR it was decided not to update ASM b/c nothing in JDK needs that, 
>>>>>> has it
>>>>>> been changed? put somewhat differently, why are we doing this?
>>>>>> 
>>>>>> in any case, I don't like the changes in mlvm tests. I understand that
>>>>>> ClassWriter has been significantly changed in ASM 7.0, so ClassWriterExt 
>>>>>> can't
>>>>>> disable CP entries caching (at least not in the way it used to), but 
>>>>>> removing
>>>>>> setCache* calls from the tests changed them and in some cases made them 
>>>>>> invalid
>>>>>> as they don't test that they supposed to. therefore I'd prefer to leave 
>>>>>> all
>>>>>> calls setCache* as-is, change setCache* implementation to throw an 
>>>>>> exception
>>>>>> (similarly to the fix in JDK-8194826
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8194826>) and mark all tests 
>>>>>> which
>>>>>> throw this exception w/ '@ignore 8194951' jtreg tag.
>>>>>> 
>>>>>> 
>>>>> ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
>>>>> there been
>>>>> any discussion Remi or others on ASM to make it easier for the JDK to 
>>>>> upgrade?
>>>> 
>>>> I'm not aware of any such discussions.
>>>> 
>>>>> 
>>>>> -Alan
>>>> 
> >>> Vicente

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Igor Ignatyev]

Hi Remi,


w/ SymbolTable::get being private method _and_ SymbolTable being final class, 
it doesn't sound like the easiest way ;) w/ that being said, I haven't looked 
at the latest version of ASM, so there might be ways to get that we need.

Thanks,
-- Igor

> On Nov 9, 2018, at 11:33 AM, Remi Forax  wrote:
> 
> Hi Igor,
> sorry to have not answered to your mail :(
> 
> I believe that the easiest way to disable the caching of the entries of the 
> constant pool in ASM is to change SymbolTable.get() [1] to always return null.
> 
> regards,
> Rémi
> 
> [1] 
> https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/SymbolTable.java#L393
> 
> - Mail original -
>> De: "Igor Ignatyev" 
>> À: "Vicente Romero" , "Alan Bateman" 
>> 
>> Cc: "hotspot-dev" , "core-libs-dev" 
>> 
>> Envoyé: Vendredi 9 Novembre 2018 17:25:49
>> Objet: Re: RFR: JDK-8213480: update internal ASM version to 7.0
> 
>> Vicente, Alan,
>> 
>> back when we 1st bumped into this problem w/ ClassWriterExt (about 1y ago), 
>> it
>> was my AI to discuss it w/ Remi. I sent him an email, didn't get a replay, 
>> and
>> as it usually goes, had to switch to something else, completely forgot about
>> that and didn't follow up.
>> 
>> Thanks,
>> -- Igor
>> 
>>> On Nov 8, 2018, at 8:40 AM, Vicente Romero  
>>> wrote:
>>> 
>>> 
>>> 
>>> On 11/8/18 8:14 AM, Alan Bateman wrote:
>>>> On 07/11/2018 19:33, Igor Ignatyev wrote:
>>>>> Hi Vicente,
>>>>> 
>>>>> I recall an (internal?) discussion about updating ASM somewhen in JDK 
>>>>> 11TF, and
>>>>> AFAIR it was decided not to update ASM b/c nothing in JDK needs that, has 
>>>>> it
>>>>> been changed? put somewhat differently, why are we doing this?
>>>>> 
>>>>> in any case, I don't like the changes in mlvm tests. I understand that
>>>>> ClassWriter has been significantly changed in ASM 7.0, so ClassWriterExt 
>>>>> can't
>>>>> disable CP entries caching (at least not in the way it used to), but 
>>>>> removing
>>>>> setCache* calls from the tests changed them and in some cases made them 
>>>>> invalid
>>>>> as they don't test that they supposed to. therefore I'd prefer to leave 
>>>>> all
>>>>> calls setCache* as-is, change setCache* implementation to throw an 
>>>>> exception
>>>>> (similarly to the fix in JDK-8194826
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8194826>) and mark all tests 
>>>>> which
>>>>> throw this exception w/ '@ignore 8194951' jtreg tag.
>>>>> 
>>>>> 
>>>> ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
>>>> there been
>>>> any discussion Remi or others on ASM to make it easier for the JDK to 
>>>> upgrade?
>>> 
>>> I'm not aware of any such discussions.
>>> 
>>>> 
>>>> -Alan
>>> 
>>> Vicente

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi David,

On 11/8/18 11:00 PM, David Holmes wrote:

Hi Vicente,

On 9/11/2018 2:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible 
to see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ 
change to specifying Opcodes.ASM7 unless they plan on using ASM7 
features?


I changed only the tests for which the new ASM was complaining about 
a particular API available only for ASM7


I could not understand how this could be if the tests and other code 
were unchanged, so I applied the ASM-only patch and investigated the 
first failure running nasgen. And of course the problem is the 
NestHost/NestMembers attributes! We modified our internal version of 
ASM to add nestmate support, but of course this update removes that 
and replaces it with the official support. But the official support is 
only enabled for ASMv7 so we must update all users of ASM to request 
version 7.


thanks for double checking this


Thanks,
David
-


Vicente



If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with 
ASM5).


I have made two webrevs to make the review easier [1], contain only 
the changes to the internal asm and [2] contains the changes to the 
clients plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 



On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal 
version in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Remi Forax]

Hi Igor,
sorry to have not answered to your mail :(

I believe that the easiest way to disable the caching of the entries of the 
constant pool in ASM is to change SymbolTable.get() [1] to always return null.

regards,
Rémi

[1] 
https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/SymbolTable.java#L393

- Mail original -
> De: "Igor Ignatyev" 
> À: "Vicente Romero" , "Alan Bateman" 
> 
> Cc: "hotspot-dev" , "core-libs-dev" 
> 
> Envoyé: Vendredi 9 Novembre 2018 17:25:49
> Objet: Re: RFR: JDK-8213480: update internal ASM version to 7.0

> Vicente, Alan,
> 
> back when we 1st bumped into this problem w/ ClassWriterExt (about 1y ago), it
> was my AI to discuss it w/ Remi. I sent him an email, didn't get a replay, and
> as it usually goes, had to switch to something else, completely forgot about
> that and didn't follow up.
> 
> Thanks,
> -- Igor
> 
>> On Nov 8, 2018, at 8:40 AM, Vicente Romero  wrote:
>> 
>> 
>> 
>> On 11/8/18 8:14 AM, Alan Bateman wrote:
>>> On 07/11/2018 19:33, Igor Ignatyev wrote:
>>>> Hi Vicente,
>>>> 
>>>> I recall an (internal?) discussion about updating ASM somewhen in JDK 
>>>> 11TF, and
>>>> AFAIR it was decided not to update ASM b/c nothing in JDK needs that, has 
>>>> it
>>>> been changed? put somewhat differently, why are we doing this?
>>>> 
>>>> in any case, I don't like the changes in mlvm tests. I understand that
>>>> ClassWriter has been significantly changed in ASM 7.0, so ClassWriterExt 
>>>> can't
>>>> disable CP entries caching (at least not in the way it used to), but 
>>>> removing
>>>> setCache* calls from the tests changed them and in some cases made them 
>>>> invalid
>>>> as they don't test that they supposed to. therefore I'd prefer to leave all
>>>> calls setCache* as-is, change setCache* implementation to throw an 
>>>> exception
>>>> (similarly to the fix in JDK-8194826
>>>> <https://bugs.openjdk.java.net/browse/JDK-8194826>) and mark all tests 
>>>> which
>>>> throw this exception w/ '@ignore 8194951' jtreg tag.
>>>> 
>>>> 
>>> ClassWriterExt the MLVM tests have come in previous upgrades too. Has there 
>>> been
>>> any discussion Remi or others on ASM to make it easier for the JDK to 
>>> upgrade?
>> 
>> I'm not aware of any such discussions.
>> 
>>> 
>>> -Alan
>> 
> > Vicente

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Igor Ignatyev]

Vicente, Alan,

back when we 1st bumped into this problem w/ ClassWriterExt (about 1y ago), it 
was my AI to discuss it w/ Remi. I sent him an email, didn't get a replay, and 
as it usually goes, had to switch to something else, completely forgot about 
that and didn't follow up.

Thanks,
-- Igor

> On Nov 8, 2018, at 8:40 AM, Vicente Romero  wrote:
> 
> 
> 
> On 11/8/18 8:14 AM, Alan Bateman wrote:
>> On 07/11/2018 19:33, Igor Ignatyev wrote:
>>> Hi Vicente,
>>> 
>>> I recall an (internal?) discussion about updating ASM somewhen in JDK 11TF, 
>>> and AFAIR it was decided not to update ASM b/c nothing in JDK needs that, 
>>> has it been changed? put somewhat differently, why are we doing this?
>>> 
>>> in any case, I don't like the changes in mlvm tests. I understand that 
>>> ClassWriter has been significantly changed in ASM 7.0, so ClassWriterExt 
>>> can't disable CP entries caching (at least not in the way it used to), but 
>>> removing setCache* calls from the tests changed them and in some cases made 
>>> them invalid as they don't test that they supposed to. therefore I'd prefer 
>>> to leave all calls setCache* as-is, change setCache* implementation to 
>>> throw an exception (similarly to the fix in JDK-8194826 
>>> ) and mark all tests 
>>> which throw this exception w/ '@ignore 8194951' jtreg tag.
>>> 
>>> 
>> ClassWriterExt the MLVM tests have come in previous upgrades too. Has there 
>> been any discussion Remi or others on ASM to make it easier for the JDK to 
>> upgrade?
> 
> I'm not aware of any such discussions.
> 
>> 
>> -Alan
> 
> Vicente

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[David Holmes]

Hi Vicente,

On 9/11/2018 2:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


I could not understand how this could be if the tests and other code 
were unchanged, so I applied the ASM-only patch and investigated the 
first failure running nasgen. And of course the problem is the 
NestHost/NestMembers attributes! We modified our internal version of ASM 
to add nestmate support, but of course this update removes that and 
replaces it with the official support. But the official support is only 
enabled for ASMv7 so we must update all users of ASM to request version 7.


Thanks,
David
-

If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only the 
changes to the internal asm and [2] contains the changes to the clients 
plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 



On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only the 
changes to the internal asm and [2] contains the changes to the clients 
plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/


On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

On 11/8/18 8:14 AM, Alan Bateman wrote:

On 07/11/2018 19:33, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 
11TF, and AFAIR it was decided not to update ASM b/c nothing in JDK 
needs that, has it been changed? put somewhat differently, why are we 
doing this?


in any case, I don't like the changes in mlvm tests. I understand 
that ClassWriter has been significantly changed in ASM 7.0, so 
ClassWriterExt can't disable CP entries caching (at least not in the 
way it used to), but removing setCache* calls from the tests changed 
them and in some cases made them invalid as they don't test that they 
supposed to. therefore I'd prefer to leave all calls setCache* as-is, 
change setCache* implementation to throw an exception (similarly to 
the fix in JDK-8194826 
) and mark all 
tests which throw this exception w/ '@ignore 8194951' jtreg tag.



ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
there been any discussion Remi or others on ASM to make it easier for 
the JDK to upgrade?


I'm not aware of any such discussions.



-Alan


Vicente

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Alan Bateman]

On 07/11/2018 19:33, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 11TF, and 
AFAIR it was decided not to update ASM b/c nothing in JDK needs that, has it 
been changed? put somewhat differently, why are we doing this?

in any case, I don't like the changes in mlvm tests. I understand that ClassWriter 
has been significantly changed in ASM 7.0, so ClassWriterExt can't disable CP entries 
caching (at least not in the way it used to), but removing setCache* calls from the 
tests changed them and in some cases made them invalid as they don't test that they 
supposed to. therefore I'd prefer to leave all calls setCache* as-is, change 
setCache* implementation to throw an exception (similarly to the fix in JDK-8194826 
) and mark all tests which 
throw this exception w/ '@ignore 8194951' jtreg tag.


ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
there been any discussion Remi or others on ASM to make it easier for 
the JDK to upgrade?


-Alan

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[David Holmes]

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change to 
specifying Opcodes.ASM7 unless they plan on using ASM7 features? If so 
then you could split out the actual update of ASM from the updates to 
the users of ASM (some of which may be quite fine with ASM5).


Thanks,
David

On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, yay!, 
and we want to include it in JDK 12. Please review [1] which includes:
- the new version perse substituting the preview ASM internal version in 
the JDK
- changes to additional files in particular some tests, mostly hotspot 
tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Igor,

On 11/7/18 2:33 PM, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 
11TF, and AFAIR it was decided not to update ASM b/c nothing in JDK 
needs that, has it been changed? put somewhat differently, why are we 
doing this?


we need the support ASM 7 has of condy, many amber projects depend on this



in any case, I don't like the changes in mlvm tests. I understand 
that ClassWriter has been significantly changed in ASM 7.0, 
so ClassWriterExt can't disable CP entries caching (at least not in 
the way it used to), but removing setCache* calls from the tests 
changed them and in some cases made them invalid as they don't test 
that they supposed to. therefore I'd prefer to leave all calls 
setCache* as-is, change setCache* implementation to throw an exception 
(similarly to the fix in JDK-8194826 
) and mark all tests 
which throw this exception w/ '@ignore 8194951' jtreg tag.


sounds good, I will do this



Thanks,
-- Igor


Thanks,
Vicente



On Nov 7, 2018, at 7:56 AM, Vicente Romero > wrote:


Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Igor Ignatyev]

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 11TF, and 
AFAIR it was decided not to update ASM b/c nothing in JDK needs that, has it 
been changed? put somewhat differently, why are we doing this?

in any case, I don't like the changes in mlvm tests. I understand that 
ClassWriter has been significantly changed in ASM 7.0, so ClassWriterExt can't 
disable CP entries caching (at least not in the way it used to), but removing 
setCache* calls from the tests changed them and in some cases made them invalid 
as they don't test that they supposed to. therefore I'd prefer to leave all 
calls setCache* as-is, change setCache* implementation to throw an exception 
(similarly to the fix in JDK-8194826 
) and mark all tests which 
throw this exception w/ '@ignore 8194951' jtreg tag.

Thanks,
-- Igor

> On Nov 7, 2018, at 7:56 AM, Vicente Romero  wrote:
> 
> Hi,
> 
> Version 7.0 of ASM has been released. This version supports condy, yay!, and 
> we want to include it in JDK 12. Please review [1] which includes:
> - the new version perse substituting the preview ASM internal version in the 
> JDK
> - changes to additional files in particular some tests, mostly hotspot tests.
> 
> Thanks,
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi,

Version 7.0 of ASM has been released. This version supports condy, yay!, 
and we want to include it in JDK 12. Please review [1] which includes:
- the new version perse substituting the preview ASM internal version in 
the JDK
- changes to additional files in particular some tests, mostly hotspot 
tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/