Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[serguei.spit...@oracle.com]

On 4/17/20 16:52, Mandy Chung wrote:



On 4/17/20 3:51 PM, Chris Plummer wrote:

Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type 
signature" in one place, but left it as "JNI signature" everywhere 
else. Should they all be changed?




JDWP signature is changed because there is no JNI signature 
representing a hidden class.    I leave other references to JNI 
signature as is since those only apply for normal classes as I wanted 
to keep this change specifically due to hidden classes.  I think it's 
good to file a JBS issue to follow up on JDWP and JDI to determine if 
the spec should be upgraded to reference the new TypeDescriptor API.




In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an 
initiating loader of the returned classes." -> "That is, all 
classes for which this class loader has been recorded as an 
initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it 
possible that all this detail could be put elsewhere and referenced? 


Any suggestion?   We attempted to place those description in JVM TI 
Class section or ClassLoad event.   However, that's not ideal place 
since that's needed by JDWP, JDI and Instrumentation.   I found 
inlining this description is not ideal but it provides adequate 
clarification.


The JDI (transitively via JDWP), JDWP and Instrumentation 
implementations are based on the JVMTI.

I've tried to suggest once to link these API's to the JVMTI.
The problem is there was no such practice in the specs of these API's 
before but we can make a step to introduce it now.
Placing this description either in JVM TI Class section or ClassLoad 
event would be good enough.


An alternative approach is to make JVMTI/JDI/JDWP/Instrument to refer to 
the java.lang.Class spec for
general information about class loading and classes defined and 
initiated by class loaders.


Thanks,
Serguei

Aren't there other places in other specs where a similar 
clarification of "initiating ClassLoader" is needed (I see now that 
ClassLoaderClasses in the JVMTI spec, 
ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but 
not all in the exact same way).




I took the conservative side and make sure the clarification is in 
place for all APIs.  I'm open to any suggestion for example having 
JDWP and JDI to link to JVM TI spec if you think appropriate.




In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses 
in the JDWP spec, although not as badly. A simple concept ends up 
with a complex description, and it seems that description should 
really be in a more centralized place.  I would also suggest a bit of 
cleanup of these lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating loader 
of the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you 
just finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, is 
"JNI type signature" formally defined somewhere? This relates to my 
JDWP spec comment above.




It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.


" where N is the binary name encoded in internal form indicated by 
the class file". Is "binary name encoded in internal form" explained 
somewhere? 


JVMS 4.2.1

https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1 




Also, can you add an example of a returned hidden class signature?


OK



In the JVMTI spec ClassLoad section:

"representation using [a] class loader" (add "a")

"By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 
invoking Lookup::defineHiddenClass to create ..."


"certain Java SE Platform API" -> Should be "APIs"


In JDI ClassLoaderReference.definedClasses()

"loaded at least to the point of preparation and types ..." -> 
"loaded at least to the point of preparation, and types ..." (Note, 
this not a new issue with your edits)



In Instrumentation.getAllLoadedClasses:

The reference to `class` did not

Re: RFR(T) : 8244052 : remove copying of s.h.WB$WhiteBoxPermission in test/jdk

[serguei.spit...@oracle.com]

LGTM++

Thanks,
Serguei


On 4/28/20 23:28, David Holmes wrote:

Looks good!

Thanks,
David

On 29/04/2020 1:20 pm, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/

34 lines changed: 0 ins; 11 del; 23 mod;


Hi all,

could you please review this trivial patch?
from JBS:
JDK-8199290 made it unnecessary to explicitly pass 
sun.hotspot.WhiteBox$WhiteBoxPermission as an argument to 
ClassFileInstaller b/c the former now copies it every time it copies 
sun.hotspot.WhiteBox.


besides removing WhiteBoxPermission from the list of 
ClassFileInstaller's arguments, the patch also makes sure 
ClassFileInstaller is run in a driver mode in all the touched tests.


testing: the changed tests
webrev: http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8244052

Thanks,
-- Igor

Re: RFR: JDK-8225056 VM support for sealed classes

[serguei.spit...@oracle.com]

Hi Harold,

The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp 
refactoring if you are okay with it.


Thanks,
Serguei


On 5/18/20 22:26, David Holmes wrote:

Hi Harold,

Adding serviceability-dev for the serviceability related changes.

Nit: "VM support for sealed classes"

This RFR covers the VM, core-libs, serviceability and even some 
langtools tests. AFAICS only the javac changes are not included here 
so when and where will they be reviewed and under what bug id? Ideally 
there will be a single JBS issue for "Implementation of JEP 360: 
Sealed types". It's okay to break up the RFRs across different areas, 
but it should be done under one bug id.


Overall this looks good. I've looked at all files and mainly have some 
style nits in various places. But there are some more significant 
items below.


On 14/05/2020 7:09 am, Harold Seigel wrote:

Hi,

Please review this patch for JVM and Core-libs support for the JEP 
360 Sealed Classes preview feature.  Code changes include the following:


  * Processing of the new PermittedSubclasses attribute to enforce that
    a class or interface, whose super is a sealed class or interface,
    must be listed in the super's PermittedSubclasses attribute.
  * Disallow redefinition of a sealed class or interface if its
    redefinition would change its PermittedSubclasses attribute.
  * Support API's to determine if a class or interface is sealed and, if
    it's sealed, return a list of its permitted subclasses.
  * asm support for the PermittedSubclasses attribute


I assume Remi is providing the upstream support in ASM? :) But also 
see below ...




Open Webrev: 
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html


make/data/jdwp/jdwp.spec

There needs to be a sub-task and associated CSR request for this JDWP 
spec update. I couldn't see this covered anywhere.


---

src/hotspot/share/classfile/classFileParser.cpp

3215 u2 
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,

3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {

Indention on L3216/17 needs fixing after the copy'n'edit.

3515   return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516  _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517  Arguments::enable_preview();

Too much indentation on L3516/17

3790 // Check for PermittedSubclasses tag

That comment (copied from my nestmates code :) is in the wrong place. 
It needs to be before


3788 if (tag == vmSymbols::tag_permitted_subclasses()) {


Minor nit: I would suggest checking 
parsed_permitted_subclasses_attribute before checking ACC_FINAL.


3876   if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses = 
parse_classfile_permitted_subclasses_attribute(

3878    cfs,
3879 permitted_subclasses_attribute_start,
3880    CHECK);

Although it looks odd the preceding, similarly shaped, sections all 
indent to the same absolute position. Can you make L3878/78/80 match 
please.


3882   guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884   sizeof(num_of_subclasses) + sizeof(u2) * 
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file 
%s", CHECK);


Nits: please reformat as:

3882   guarantee_property(
3883 permitted_subclasses_attribute_length == 
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file 
%s", CHECK);


It would also look slightly better if you shortened the name of the 
num_of_subclasses variable.


---

src/hotspot/share/classfile/classFileParser.hpp

+   u2 parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,
+ const u1* const 
permitted_subclasses_attribute_start,

+ TRAPS);

Please fix indentation after copy'n'edit.

---

src/hotspot/share/oops/instanceKlass.cpp

 247   if (classloader1 != classloader2) {

I'm not clear what rule this is verifying. The same module check 
follows this one. The rule is that the subclass must be accessible to 
the superclass implying:

1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not 
public; or

3. public subclass

Having the same classloader implies same package, but that alone 
doesn't address 2 or 3. So this doesn't conform to proposed JVMS rules.



 264 if (_constants->tag_at(cp_index).is_klass()) {
 265   Klass* k2 = _constants->klass_at(cp_index, CHECK_false);

You've copied this code from the nestmember checks but your changes 
don't quite make sense to me. If we have already checked is_klass() 
then klass_at() cannot lead to any excep

Re: RFR: JDK-8225056 VM support for sealed classes

[serguei.spit...@oracle.com]

On 5/19/20 09:46, Harold Seigel wrote:

That sounds good!

Thanks, Harold

On 5/19/2020 11:53 AM, serguei.spit...@oracle.com wrote:

Hi Harold,

The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp 
refactoring if you are okay with it.


Filed enhancement and assigned to myself:
  https://bugs.openjdk.java.net/browse/JDK-8245321
    refactor the redefine check that an attribute consisting of a list 
of classes has not changed


Thanks,
Serguei



Thanks,
Serguei


On 5/18/20 22:26, David Holmes wrote:

Hi Harold,

Adding serviceability-dev for the serviceability related changes.

Nit: "VM support for sealed classes"

This RFR covers the VM, core-libs, serviceability and even some 
langtools tests. AFAICS only the javac changes are not included here 
so when and where will they be reviewed and under what bug id? 
Ideally there will be a single JBS issue for "Implementation of JEP 
360: Sealed types". It's okay to break up the RFRs across different 
areas, but it should be done under one bug id.


Overall this looks good. I've looked at all files and mainly have 
some style nits in various places. But there are some more 
significant items below.


On 14/05/2020 7:09 am, Harold Seigel wrote:

Hi,

Please review this patch for JVM and Core-libs support for the JEP 
360 Sealed Classes preview feature.  Code changes include the 
following:


  * Processing of the new PermittedSubclasses attribute to enforce 
that

    a class or interface, whose super is a sealed class or interface,
    must be listed in the super's PermittedSubclasses attribute.
  * Disallow redefinition of a sealed class or interface if its
    redefinition would change its PermittedSubclasses attribute.
  * Support API's to determine if a class or interface is sealed 
and, if

    it's sealed, return a list of its permitted subclasses.
  * asm support for the PermittedSubclasses attribute


I assume Remi is providing the upstream support in ASM? :) But also 
see below ...




Open Webrev: 
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html


make/data/jdwp/jdwp.spec

There needs to be a sub-task and associated CSR request for this 
JDWP spec update. I couldn't see this covered anywhere.


---

src/hotspot/share/classfile/classFileParser.cpp

3215 u2 
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,

3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {

Indention on L3216/17 needs fixing after the copy'n'edit.

3515   return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516  _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517  Arguments::enable_preview();

Too much indentation on L3516/17

3790 // Check for PermittedSubclasses tag

That comment (copied from my nestmates code :) is in the wrong 
place. It needs to be before


3788 if (tag == vmSymbols::tag_permitted_subclasses()) {


Minor nit: I would suggest checking 
parsed_permitted_subclasses_attribute before checking ACC_FINAL.


3876   if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses = 
parse_classfile_permitted_subclasses_attribute(

3878    cfs,
3879 permitted_subclasses_attribute_start,
3880    CHECK);

Although it looks odd the preceding, similarly shaped, sections all 
indent to the same absolute position. Can you make L3878/78/80 match 
please.


3882   guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884   sizeof(num_of_subclasses) + sizeof(u2) * 
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class 
file %s", CHECK);


Nits: please reformat as:

3882   guarantee_property(
3883 permitted_subclasses_attribute_length == 
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class 
file %s", CHECK);


It would also look slightly better if you shortened the name of the 
num_of_subclasses variable.


---

src/hotspot/share/classfile/classFileParser.hpp

+   u2 parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,
+ const u1* const 
permitted_subclasses_attribute_start,

+ TRAPS);

Please fix indentation after copy'n'edit.

---

src/hotspot/share/oops/instanceKlass.cpp

 247   if (classloader1 != classloader2) {

I'm not clear what rule this is verifying. The same module check 
follows this one. The rule is that the subclass must be accessible 
to the superclass implying:

1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not 
public; or

3

Re: RFR: JDK-8225056 VM support for sealed classes

[serguei.spit...@oracle.com]

Hi David,

On 5/19/20 19:31, David Holmes wrote:

Hi Serguei,

On 20/05/2020 1:49 am, serguei.spit...@oracle.com wrote:

Hi Harold and David,

Just one comment.
We could save on the CSR's for:
   make/data/jdwp/jdwp.spec
|| src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
|| 
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java


Just to be clear, you don't need a CSR request per change but each 
change must be covered by some CSR request.


Oh, right.
I was confused by your comments against each change.

As we discussed with David, it could make sense to remove duplication 
in the Serviceability class redefinition and retransformation specs.

I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/

If the direction looks okay then I could file RFE+CSR (and post an RFR).


I like the idea of keeping one list referred to by the other specs!


Thanks, I've started this.
Probably, it is better to wait until this fix is pushed.

Thanks,
Serguei


Thanks,
David
-


Thanks,
Serguei


On 5/18/20 22:26, David Holmes wrote:

Hi Harold,

Adding serviceability-dev for the serviceability related changes.

Nit: "VM support for sealed classes"

This RFR covers the VM, core-libs, serviceability and even some 
langtools tests. AFAICS only the javac changes are not included here 
so when and where will they be reviewed and under what bug id? 
Ideally there will be a single JBS issue for "Implementation of JEP 
360: Sealed types". It's okay to break up the RFRs across different 
areas, but it should be done under one bug id.


Overall this looks good. I've looked at all files and mainly have 
some style nits in various places. But there are some more 
significant items below.


On 14/05/2020 7:09 am, Harold Seigel wrote:

Hi,

Please review this patch for JVM and Core-libs support for the JEP 
360 Sealed Classes preview feature.  Code changes include the 
following:


  * Processing of the new PermittedSubclasses attribute to enforce 
that

    a class or interface, whose super is a sealed class or interface,
    must be listed in the super's PermittedSubclasses attribute.
  * Disallow redefinition of a sealed class or interface if its
    redefinition would change its PermittedSubclasses attribute.
  * Support API's to determine if a class or interface is sealed 
and, if

    it's sealed, return a list of its permitted subclasses.
  * asm support for the PermittedSubclasses attribute


I assume Remi is providing the upstream support in ASM? :) But also 
see below ...




Open Webrev: 
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html


make/data/jdwp/jdwp.spec

There needs to be a sub-task and associated CSR request for this 
JDWP spec update. I couldn't see this covered anywhere.


---

src/hotspot/share/classfile/classFileParser.cpp

3215 u2 
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,

3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {

Indention on L3216/17 needs fixing after the copy'n'edit.

3515   return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516  _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517  Arguments::enable_preview();

Too much indentation on L3516/17

3790 // Check for PermittedSubclasses tag

That comment (copied from my nestmates code :) is in the wrong 
place. It needs to be before


3788 if (tag == vmSymbols::tag_permitted_subclasses()) {


Minor nit: I would suggest checking 
parsed_permitted_subclasses_attribute before checking ACC_FINAL.


3876   if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses = 
parse_classfile_permitted_subclasses_attribute(

3878    cfs,
3879 permitted_subclasses_attribute_start,
3880    CHECK);

Although it looks odd the preceding, similarly shaped, sections all 
indent to the same absolute position. Can you make L3878/78/80 match 
please.


3882   guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884   sizeof(num_of_subclasses) + sizeof(u2) * 
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class 
file %s", CHECK);


Nits: please reformat as:

3882   guarantee_property(
3883 permitted_subclasses_attribute_length == 
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class 
file %s", CHECK);


It would also look slightly better if you shortened the name of the 
num_of_subclasses variable.


---

src/hotspot/share/classfile/classFileParser.hpp

+   u2 parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,
+ c

Re: (15) RFR: JDK-8247444: Trust final fields in records

[serguei.spit...@oracle.com]

Hi Mandy,

This looks good from the Serviceability point of view.

> No change is made in JNI.  JNI could be considered to disallow 
modification of

> final fields in records and hidden classes (static and instance fields).
> But JNI has superpower and the current spec already allows to modify
> the value of a final static field even after it's constant folded
> (via JNI SetField and SetStaticField), then all bets are off.
> This should be re-visited when we consider "final is truly final" for 
all classes.


This can potentially impact the JDWP agent as it uses the JNI to set 
fields values.

Please, see ObjectReference#setValue:
https://docs.oracle.com/en/java/javase/14/docs/api/jdk.jdi/com/sun/jdi/ObjectReference.html#setValue(com.sun.jdi.Field,com.sun.jdi.Value)

Thanks,
Serguei


On 6/15/20 14:58, Mandy Chung wrote:

This patch is joint contribution from Christoph Dreis [1] and me.

Webrev: 
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/

CSR: https://bugs.openjdk.java.net/browse/JDK-8247517

This proposes to make final fields in records notmodifiable via 
reflection.  Field::set throws IAE if a Field is not modifiable. 
Thecurrent specification specifies the following Fields not modifiable:

- static final fields in any class
- final fields in a hidden class

The spec of Field::set is changed to specify that records are not 
modifiable via reflection.
 Noe that no change in Field::setAccessible(true), i.e. it will 
succeed to allow existing frameworks to have read access to final 
fields in records.  Just no write access.


VarHandle does not support write access if it's a static final field 
or an instance final field.


This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and 
`sun.misc.Unsafe::staticField{Offset/Base}` not to support records.


No change is made in JNI.  JNI could be considered to disallow 
modification of final fields in records and hidden classes (static and 
instance fields).  But JNI has superpower and the current spec already 
allows to modify the value of a final static field even after it's 
constant folded (via JNI SetField and SetStaticField), 
then all bets are off.  This should be re-visited when we consider 
"final is truly final" for all classes.


Make final fields in records not modifiable via reflection enables JIT 
optimization as these final fields are effectively truly final.


This change impacts 3rd-party frameworks including 3rd-party 
serialization framework that rely on core reflection `setAccessible` 
or `sun.misc.Unsafe::allocateInstance` and `objectFieldOffset` etc to 
construct records but not using the canonical constructor.
These frameworks would need to be updated to construct records via its 
canonical constructor as done by the Java serialization.


I see this change gives a good opportunity to engage the maintainers 
of the serialization frameworks and work together to support new 
features including records, inline classes and the new serialization 
mechanism and which I think it is worth the investment.


This is a low risk enhancement.  I'd like to request approval for a 
late enhancement in JDK 15.  It extends the pre-existing code path 
with refactoring the hidden classes to prepare for new kinds of 
classes with trusted final fields.  The change is straight-forward.


Can this wait to integrate in JDK 16?

  It's important to get this enhancement in when record is a preview 
feature that we can get feedback and give time to 3rd-party 
serialization frameworks to look into adding the support for records.  
If we delayed this change in 16 and records exit preview, it would be 
bad for frameworks if they verify that they support records in 15 but 
fail in 16.  OTOH the risk of this patch is low.


Performance Impact

I addressed the performance concern I raised earlier myself.  For 
reflection, VM creates the reflective Field objects and fills in 
MemberName when resolving a member.  VM will tag if this 
Field/MemberName is trusted final field.  I think this is a cleaner 
approach rather than in each place to check for final static and final 
fields in hidden or record class to determine if it has write access 
or not.


`sun.misc.Unsafe` does not use Field::isTrustedFinalField because (1) 
Unsafe has been allowing access of static final fields of any classes 
but isTrustedFinalField is not limited to instance fields (2) Unsafe 
disallows access to all fields in a hidden class (not limited to 
trusted final fields).  So it follows the precedence and simply checks 
if the declaring class is a record. `Class::isRecord` calls 
`Class::getSuperclass` to check if it's a subtype of `Record`.  As 
`Class::getSuperclass` is intrinsified, the call on isRecord on a 
normal class is fast. Christoph has contributed the microbenchmarks 
that confirm that no performance regression.


Thanks
Mandy
[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-June/040096.html

Re: RFR XS,docs,13 JDK-8226593 Fix HTML in com/sun/jdi/doc-files/signature.html

[serguei.spit...@oracle.com]

Hi Jon,

It looks good to me.

Thanks,
Serguei


On 6/21/19 11:58, Jonathan Gibbons wrote:

Please review a fix for an accessibility issue reported by Axe in
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html

The issue is a conflict between an explicit `role="main">...``  specified in the file and the automatic 
`` added by javadoc.  The fix is just to remove the `` 
element.


JBS: https://bugs.openjdk.java.net/browse/JDK-8226593

-- Jon


$ hg diff -R open
diff -r 97c75e545302 
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
--- a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html 
Fri Jun 21 11:41:29 2019 -0700
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html 
Fri Jun 21 11:52:35 2019 -0700

@@ -40,7 +40,6 @@
 
 
 
-
 JDI Type Signatures
 
 JDI Type Signatures
@@ -74,6 +73,5 @@
 has the following type signature:
 (ILjava/lang/String;[I)J
 
-
 
 

Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[serguei.spit...@oracle.com]

Hi Brent,

The updated webrev looks good to me.
At least, I do not see any issues.

Thanks,
Serguei


On 9/5/19 17:09, Brent Christian wrote:

Hi, David

On 9/5/19 12:52 AM, David Holmes wrote:


Good to see this all come together :)


:)

So to clarify for others any current caller to 
find_class_from_class_loader that passes true for "init" was already 
implicitly asking for a linked class (as you must be linked before 
you can be initialized). With that in mind I would suggest that in 
find_class_from_class_loader you add an assert:


! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name, 
jboolean init, jboolean link,
   Handle loader, Handle 
protection_domain,

   jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link 
arguments");


just to ensure the callers understand this.


I'm good with adding an assert to check for coherent arguments.

(Another interpretation is that if 'init' is set, then the 'link' 
argument is ignored, since linking is implied).


Aside: in looking at this I've spotted another existing bug. JNI 
FindClass is not specified to perform class initialization, but the 
implementation passes init=true!


OK, thanks.  I've noted this in JDK-8226540.


src/java.base/share/classes/java/lang/invoke/MethodHandles.java

I don't see the need for the new note given it already has

* @throws LinkageError if the linkage fails


(Discussed in the CSR)


src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a 
number of others are possible - in particular IllegalAccessError 
might occur during verification. Other than the fact a test obviously 
triggered this, it's not clear why ACE should be singled out here. ??


Also discussed in the CSR[1].


test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java

45 // public static void foo(Foo f) { }

Unclear why this exists. Also the test refers initially to class Foo 
but then uses Foo2 and Foo3. ??


I'm guessing it's just a leftover from an earlier version of the test. 
I've removed the comment, and updated the test @summary.


Serguei, anything to add?


There is also a CSR[2] in need of review.


I've added a couple of comments and will add myself as a reviewer 
once things are near finalized.


Thanks for taking a look.

Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/

-Brent

1. 
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303




The spec for the 2-arg and 3-arg Class.forName() methods states they 
will "locate, load, and link" the class, however the linking part is 
not ensured to happen.


Although the VM spec allows flexibility WRT when classes are linked, 
this is a corner where the Class.forName() spec is not being upheld. 
While this is not an issue for most usages, 8181144 [3] demonstrates 
how this can be a problem (with the debugging interface, in this case).


This fix ensures that linking happens during the course of 
Class.forName().  Class.forName() already @throws LinkageError, so 
no spec change is needed there. MethodHandles.Lookup.findClass() 
needs a small spec update, due to calling Class.forName with 
init=false,


Of course Errors are not required to be caught.  It is therefore 
possible that the new behavior could introduce previously unseen, 
possibly unhandled LinkageErrors.  A new VM flag 
(-XX:+ClassForNameDeferLinking) is introduced to restore the 
previous behavior (to keep such code running until it can be updated).


This change surfaced a couple new "A JNI error has occurred" 
situations (see 8181033[5]) in the Launcher, by way of

   test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
   test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).

The Launcher is updated to maintain non-confusing error messages :).

The new test included with this fix ensures that 8181144[3] is 
addressed.  Thanks go to Serguei Spitsyn for writing the test.


Automated corelibs and hotspot tests pass cleanly.

Thanks,
-Brent

--
1. https://bugs.openjdk.java.net/browse/JDK-8212117

2. https://bugs.openjdk.java.net/browse/JDK-8222071

3. https://bugs.openjdk.java.net/browse/JDK-8181144

4. 
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 



5. https://bugs.openjdk.java.net/browse/JDK-8181033

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

[serguei.spit...@oracle.com]

Hi David,

The fix looks good to me.
I did not pay much attention to the Graal related changes though.
The test coverage for Serviceability is complete.
Running java/lang/instrument tests is not necessary.

Thanks,
Serguei


On 10/29/19 00:42, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but 
only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently of whether the thread is alive (and to make things 
easier for loom in the near future). It is very straightforward:


- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt 
mechanism. Special thanks to Patricio for tracking down a bug I had 
introduced in that regard!


Special Note (Hi Roger!): on windows we still need to set/clear the 
_interrupt_event used by the Process.waitFor logic. To facilitate 
clearing via Thread.interrupted() I had to introduce a native method 
that is a no-op except on Windows. This seemed the cheapest and least 
intrusive means to achieve this.


Other changes revolve around the fact we used to have an intrinsic for 
Thread.isInterrupted and that is not needed any more. So we strip some 
code out of C1/C2.


The changes in the JVMCI/Graal code are a bit more far reaching as 
entire classes disappear. I've cc'd Doug and Tom at Vladimir's request 
so that they can comment on the JVMCI changes and whether I have gone 
too far or not far enough. There are a bunch of tests for interruption 
in JVMCI that could potentially be deleted if they are only intended 
to test the JVMCI handling of interrupt:


./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java 



Testing:
 - Tiers 1-3 on all Oracle platforms
 - Focused testing on Linux x64:
    - Stress runs of JSR166TestCase
    - Anything that seems to use interrupt():
  - JDK
    - java/lang/Thread
    - java/util/concurrent
    - jdk/internal/loader/InterruptedClassLoad.java
    - javax/management
    - java/nio/file/Files
    - java/nio/channels
    - java/net/Socket/Timeouts.java
    - java/lang/Runtime/shutdown/
    - java/lang/ProcessBuilder/Basic.java
    - com/sun/jdi/
  - Hotspot
    - vmTestbase/nsk/monitoring/
    - vmTestbase/nsk/jdwp
    - vmTestbase/nsk/jdb/
    - vmTestbase/nsk/jdi/
    - vmTestbase/nsk/jvmti/
    - runtime/Thread
    - serviceability/jvmti/
    - serviceability/jdwp
    - serviceability/sa

Thanks,
David
-

Re: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[serguei.spit...@oracle.com]

Hi Christoph,

The fix looks good to me.
I'd recommend to run the jdk_instrument and vmTestbase_nsk_jvmti tests.
Also, it can be safe to run:
  open/test/hotspot/jtreg/serviceability/jvmti
  open/test/hotspot/jtreg/runtime/cds/appcds
  open/test/hotspot/jtreg/runtime/BootClassAppendProp

Thanks,
Serguei

On 11/14/19 07:37, Langer, Christoph wrote:

Hi,

please review this cleanup change regarding function "canonicalize" of libjava.

Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/


The goal is to cleanup how this function is defined and used. One thing is, that there was an unnecessary 
wrapper function "Canonicalize" in jni_util.c. It wrapped the call to "canonicalize". We 
can get rid of this wrapper. Unfortunately, it is not possible to just export "canonicalize" since 
this will conflict with a method signature from the math library, at least on modern Linuxes. So I decided to 
call the method JDK_Canonicalize and will correctly define it in jdk_util.h which can be included everywhere.



Hotspot's classloader.cpp will dynamically resolve this method, so I add a 
local declaration of the function pointer in there.



This change shall be predecessor of JDK-8223261, where a review was already 
started here: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-November/063398.html

Thanks
Christoph

Re: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[serguei.spit...@oracle.com]

Thanks, Christoph!
I forgot to tell that my recommendation is for the serviceability 
(j.l.instrument) coverage only.


Thanks,
Serguei

On 11/19/19 23:47, Langer, Christoph wrote:

Hi Serguei,

thanks for the review.

The patch successfully ran through our nightly test system which covers all 
these tests on several platforms.

Best regards
Christoph


-Original Message-
From: serguei.spit...@oracle.com 
Sent: Mittwoch, 20. November 2019 03:21
To: Langer, Christoph ; core-libs-
d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; gerard
ziemski 
Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
libjava, hotspot and libinstrument

Hi Christoph,

The fix looks good to me.
I'd recommend to run the jdk_instrument and vmTestbase_nsk_jvmti tests.
Also, it can be safe to run:
    open/test/hotspot/jtreg/serviceability/jvmti
    open/test/hotspot/jtreg/runtime/cds/appcds
    open/test/hotspot/jtreg/runtime/BootClassAppendProp

Thanks,
Serguei

On 11/14/19 07:37, Langer, Christoph wrote:

Hi,

please review this cleanup change regarding function "canonicalize" of

libjava.

Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/


The goal is to cleanup how this function is defined and used. One thing is,

that there was an unnecessary wrapper function "Canonicalize" in jni_util.c.
It wrapped the call to "canonicalize". We can get rid of this wrapper.
Unfortunately, it is not possible to just export "canonicalize" since this will
conflict with a method signature from the math library, at least on modern
Linuxes. So I decided to call the method JDK_Canonicalize and will correctly
define it in jdk_util.h which can be included everywhere.



Hotspot's classloader.cpp will dynamically resolve this method, so I add a

local declaration of the function pointer in there.



This change shall be predecessor of JDK-8223261, where a review was

already started here: https://mail.openjdk.java.net/pipermail/core-libs-
dev/2019-November/063398.html

Thanks
Christoph

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[serguei.spit...@oracle.com]

Hi Mandy and Chris,


On 3/29/20 19:17, Mandy Chung wrote:



On 3/27/20 8:51 PM, Chris Plummer wrote:

Hi Mandy,

A couple of very minor nits in the jvmtiRedefineClasses.cpp comments:

 153 // classes for primitives, arrays, hidden and vm unsafe 
anonymous classes
 154 // cannot be redefined.  Check here so following code can 
assume these classes

 155 // are InstanceKlass.
 156 if (!is_modifiable_class(mirror)) {
 157   _res = JVMTI_ERROR_UNMODIFIABLE_CLASS;
 158   return false;
 159 }

I think this code and comment predate anonymous classes. Probably 
before anonymous classes the check was not for !is_modifiable_class() 
but instead was just a check for primitive or array class types since 
they are not an InstanceKlass, and would cause issues when cast to 
one in the code that lies below this section. When anonymous classes 
were added, the code got changed to use !is_modifiable_class() and 
the comment was not correctly updated (anonymous classes are an 
InstanceKlass). Then with this webrev the mention of hidden classes 
was added, also incorrectly implying they are not an InstanceKlass. I 
think you should just leave off the last sentence of the comment.




I agree with you that this comment needs update.   Perhaps it should 
say "primitive, array types and hidden classes are non-modifiable. A 
modifiable class must be an InstanceKlass."


I leave it to Serguei who may have other opinion.


We already had a chat with Chris about this.
This suggestion looks right.


There's some ambiguity in the application of adjectives in the 
following:


 297   // Cannot redefine or retransform a hidden or an unsafe 
anonymous class.


I'd suggest:

 297   // Cannot redefine or retransform a hidden class or an unsafe 
anonymous class.




+1


+1

There are some places in libjdwp that need to be fixed. I spoke to 
Serguei about those this afternoon. Basically the 
convertSignatureToClassname() function needs to be fixed to handle 
hidden classes. Without the fix classname filtering will have 
problems if the filter contains a pattern with a '/' to filter on 
hidden classes. Also CLASS_UNLOAD events will not properly convert 
hidden class names. We also need tests for these cases. I think these 
are all things that can be addressed later.




Good catch.  I have created a subtask under JDK-8230502:
   https://bugs.openjdk.java.net/browse/JDK-8230502


Yes, it is good catch. Thank you for filing the subtask.
We discussed this with Chris.
This was expected to be found with new test coverage and fixed in the 
JDI chunk of work which we have decided to separate from JEP 371.



Thanks,
Serguei


I still need to look over the JVMTI tests.



Thanks
Mandy

thanks,

Chris

On 3/26/20 4:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The 
main changes are in core-libs and hotspot runtime area. Small 
changes are made in javac, VM compiler (intrinsification of 
Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been reviewed 
and is in the finalized state (see specdiff and javadoc below for 
reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's 
point

of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not 
registered in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection. setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden 
class.
4. Field::setXXX method will throw IAE on a final field of a hidden 
class

   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining 
loader.  There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control
   check no longer throws LinkageError but instead it will throw 

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[serguei.spit...@oracle.com]

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html

 356   void add_classes(LoadedClassInfo* first_class, int num_classes, 
bool has_class_mirror_holder) {

 357 LoadedClassInfo** p_list_to_add_to;
 358 bool is_hidden = first_class->_klass->is_hidden();
 359 if (has_class_mirror_holder) {
 360   p_list_to_add_to = is_hidden ? &_hidden_weak_classes : 
&_anon_classes;

 361 } else {
 362   p_list_to_add_to = &_classes;
 363 }
 364 // Search tail.
 365 while ((*p_list_to_add_to) != NULL) {
 366   p_list_to_add_to = &(*p_list_to_add_to)->_next;
 367 }
 368 *p_list_to_add_to = first_class;
 369 if (has_class_mirror_holder) {
 370   if (is_hidden) {
 371 _num_hidden_weak_classes += num_classes;
 372   } else {
 373 _num_anon_classes += num_classes;
 374   }
 375 } else {
 376   _num_classes += num_classes;
 377 }
 378   }

 Q1: I'm just curious, what happens if a cld has arrays of hidden classes?
 Is the bottom_klass always expected to be the first?


Thanks,
Serguei


On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main 
changes are in core-libs and hotspot runtime area.  Small changes are 
made in javac, VM compiler (intrinsification of Class::isHiddenClass), 
JFR, JDI, and jcmd.  CSR [1]has been reviewed and is in the finalized 
state (see specdiff and javadoc below for reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered 
in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection.  setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining loader.  
There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control

   check no longer throws LinkageError but instead it will throw IAE with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError and 
intends
to have the newly created class linked.  However, the implementation 
in 14

does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/ 



JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf 



CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[serguei.spit...@oracle.com]

On 3/30/20 02:30, serguei.spit...@oracle.com wrote:

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html 



 356   void add_classes(LoadedClassInfo* first_class, int num_classes, 
bool has_class_mirror_holder) {

 357 LoadedClassInfo** p_list_to_add_to;
 358 bool is_hidden = first_class->_klass->is_hidden();
 359 if (has_class_mirror_holder) {
 360   p_list_to_add_to = is_hidden ? &_hidden_weak_classes : 
&_anon_classes;

 361 } else {
 362   p_list_to_add_to = &_classes;
 363 }
 364 // Search tail.
 365 while ((*p_list_to_add_to) != NULL) {
 366   p_list_to_add_to = &(*p_list_to_add_to)->_next;
 367 }
 368 *p_list_to_add_to = first_class;
 369 if (has_class_mirror_holder) {
 370   if (is_hidden) {
 371 _num_hidden_weak_classes += num_classes;
 372   } else {
 373 _num_anon_classes += num_classes;
 374   }
 375 } else {
 376   _num_classes += num_classes;
 377 }
 378   }

 Q1: I'm just curious, what happens if a cld has arrays of hidden 
classes?

 Is the bottom_klass always expected to be the first?


Please, skip it. I've got the answer.
The array classes were not included into the LoadedClassInfo* by the 
classes_do.


Thanks,
Serguei



Thanks,
Serguei


On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main 
changes are in core-libs and hotspot runtime area.  Small changes are 
made in javac, VM compiler (intrinsification of 
Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been reviewed 
and is in the finalized state (see specdiff and javadoc below for 
reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's 
point

of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered 
in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection.  setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden 
class

   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining 
loader.  There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control
   check no longer throws LinkageError but instead it will throw IAE 
with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError 
and intends
to have the newly created class linked.  However, the implementation 
in 14

does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[serguei.spit...@oracle.com]

On 4/6/20 11:54, Mandy Chung wrote:

On 4/6/20 9:56 AM, serguei.spit...@oracle.com wrote:


The suggested fix is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-regression-8242166.1/



This patch looks okay. I'll include in my local patch.

On 4/6/20 11:00 AM, Chris Plummer wrote:


I think that's fine but I don't think it should be done in the 
context of this Vahalla webrev since it has nothing to do with 
Vahalla. I'd suggest filing an RFE and pushing it to jdk/jdk. Easier 
to track that way if there are issues down the road.




I am okay to follow up as a separate RFE.


Filed RFE:
  https://bugs.openjdk.java.net/browse/JDK-8242241
    add assert to ClassUnloadEventImpl::className

Thanks,
Serguei



thanks
Mandy

Re: Request for Review (XXL): 7104647: Adding a diagnostic command framework

[serguei.spit...@oracle.com]

Hi Frederik,

Your fix is already in a good shape!

src/share/vm/services/management.cpp:

  It is better to have different diagnostic messages at lines 2181 and 2186
  so that it is easy to find out what of the two lines caused an exception.
  The messages would be better to be more specific.
  The same applies to the lines 2221 and 2226.
  Indent must be aligned with the first argument above.

2178   oop cmd = JNIHandles::resolve_external_guard(command);
2179   if (cmd == NULL) {
2180 THROW_MSG(vmSymbols::java_lang_NullPointerException(),
2181"Command line cannot be null.");
2182   }
2183   char* cmd_name = java_lang_String::as_utf8_string(cmd);
2184   if (cmd_name == NULL) {
2185 THROW_MSG(vmSymbols::java_lang_NullPointerException(),
2186"Command line cannot be null.");
2187   }
...
2219   if (cmd == NULL) {
2220 THROW_MSG_NULL(vmSymbols::java_lang_NullPointerException(),
2221"Command line cannot be null.");
   }
2223   char* cmdline = java_lang_String::as_utf8_string(cmd);
2224   if (cmdline == NULL) {

2225 THROW_MSG_NULL(vmSymbols::java_lang_NullPointerException(),
2226"Command line cannot be null.");
2227   }

The lines 2189 and 2194 look redundant:

src/share/vm/services/diagnosticFramework.cpp:

2188   DCmd* dcmd = NULL;
2189   {
2190 DCmdFactory*factory = DCmdFactory::factory(cmd_name, strlen(cmd_name));
2191 if (factory != NULL) {
2192   dcmd = factory->create_resource_instance(NULL);
2193 }
2194   }

Indent is not correct at the lines 2205-2211:

2204   for (int i = 0; i<  array->length(); i++) {
2205   infoArray[i].name = array->at(i)->name();
2206   infoArray[i].description = array->at(i)->description();
2207   infoArray[i].type = array->at(i)->type();
2208   infoArray[i].default_string = array->at(i)->default_string();
2209   infoArray[i].mandatory = array->at(i)->is_mandatory();
2210   infoArray[i].option = array->at(i)->is_option();
2211   infoArray[i].position = array->at(i)->position();
2212   }


   Space is missed after 'while':

   320   while(arg != NULL) {
   326   while(arg != NULL) {


Thanks,
Serguei



On 12/12/11 7:56 AM, Frederic Parain wrote:

Minor updates:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.04/

Fred

On 12/12/11 03:29 PM, Frederic Parain wrote:

Hi Paul,

Thank you for the review.
I've applied all your recommendations except the refactoring
in diagnosticCommandFramework.cpp (too few lines can be really
factored out without passing many arguments).

New webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.03/

Regards,

Fred

On 12/ 8/11 07:26 PM, Paul Hohensee wrote:

For the hotspot part at
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.00/

Most of my comments are style-related. Nice job on the implementation
architecture.

In attachListener.cpp:

You might want to delete the first "return JNI_OK;" line, since the 
code

under
HAS_PENDING_EXCEPTION just falls through.

In jmm.h:

Be nice to indent "(JNIEnv" on lines 318, 319 and 325 the same as the
existing declarations. Add a newline just before it on line 322.


In diagnosticFramework.hpp:

Fix indenting for lines 63-66, insert blank before "size_t" on line 48.

Hotspot convention is that getter method names don't include a "get_"
prefix.
So, e.g., DCmdArgIter::get_key_addr() s/b DCmdArgIter::key_addr().
Similarly, getters such as is_enabled() should retrieve a field named
"_is_enabled"
rather than one named "enabled". You follow the "_is_enabled" 
convention

in other places such as GenDCmdArgument. Or you could use enabled() to
get the value of the _enabled field.

Also generally, I'd use accessor methods in the implementation of more
complex member methods rather than access the underlying fields 
directly.

E.g. in GenDCmdArgument::read_value, I'd use is_set() and
set_is_set(true),
(set_is_set() is not actually defined, but should be) rather than
directly
accessing _is_set. Though sometimes doing this is too much of a pain
with fields whose type is a template argument, as in the
DCmdArggument::parse_value() method in diagnosticArgument.cpp.

For easy readability, it'd be nice to line up field names (the ones
with an
_ prefix) at the same column.

On line 200, "instanciated" -> "instantiated"

On line 218, I'd use "heap_allocated" rather than "heap" for the formal
arg name.

On line 248, you could indent the text to start underneath
"outputStream".
I generally find that indenting arguments lines (formal or actual) so
they line
up with the first argument position make the code more readable, but 
I'm

not
religious about it.

On line 265, "instanciated" -> "instantiated"

DCmdFactorys are members of a singly-linked list, right? If so, it'd be
good to
have a comment to that effect on the declaration of _next.

On line 322, insert a blank before "true". You might fix this in other
places
where there's no blank between a comma in an arg

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[serguei.spit...@oracle.com]

Mandy,

It looks good.
Just a question below.

|| *src/share/classes/java/lang/management/ManagementFactory.java*

596 String intfName = mxbeanInterface.getName();
599 " is not an instance of " + mxbeanInterface);

Did you want this?:
596 String intfName = cls.getName();
599 " is not an instance of " + cls);


Thanks,
Serguei


On 8/23/12 10:43 AM, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

Summary:
The VM bootstrap class loader (null) has been the defining class loader
for all system classes (i.e. rt.jar and any classes on the 
bootclasspath).


In modular world, the system classes are going to be modularized into
multiple modulesand and many of which will be loaded by its own
(non-null) module loader.

The JDK implementation has the assumption that the defining class loader
of system classes is null and it'll no longer be true when running as
modules. Typical patterns in the JDK code include:

Class.forName(classname, false, null) should be changed to:
   Class.forName(classname, false,)

if (loader == null) condition check should be modified to check if the 
loader

is responsible for loading system classes.

This is the first set of change for this problem we identified. Similar
change in other components will be made in the future.

Testing: run all jdk core test targets on all platforms.

Thanks
Mandy
P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in
the java.lang.management.

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[serguei.spit...@oracle.com]

Looks good.

Thanks,
Serguei


On 8/23/12 12:33 PM, Mandy Chung wrote:

On 8/23/2012 11:58 AM, Alan Bateman wrote:

On 23/08/2012 18:43, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/


This looks good to me and it's good to have these changes in jdk8.

One suggestion for ReflectUtil is to add a private static boolean 
isAncestor method as I think that would make the checks in 
needsPackageAccessCheck easier to read. Also in ClassLoader you could 
use just use needsClassLoaderPermissionCheck(from,to).




Done.  This is a good cleanup I considered to do too but missed in the 
previous webrev.


http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/

Thanks for the review.
Mandy

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[serguei.spit...@oracle.com]

I was thinking about the same.
But a CCC request is needed for such a change in public API.
It can be done separately if any other API changes are necessary.

Thanks,
Serguei


On 8/23/12 6:27 PM, David Holmes wrote:

Hi Mandy,

While I understand what you are doing and that it "works" it is far 
from obvious upon code inspection that where you replace null with 
Foo.class.getClassLoader(), that the result would always be null.


Another way to simplify this would be to add a new overload of 
Class.forName():


public static Class forName(String name, boolean initialize)

That way the loader argument could be dropped completely from a number 
of the use-cases.


David

On 24/08/2012 3:43 AM, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8. This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

Summary:
The VM bootstrap class loader (null) has been the defining class loader
for all system classes (i.e. rt.jar and any classes on the 
bootclasspath).


In modular world, the system classes are going to be modularized into
multiple modulesand and many of which will be loaded by its own
(non-null) module loader.

The JDK implementation has the assumption that the defining class loader
of system classes is null and it'll no longer be true when running as
modules. Typical patterns in the JDK code include:

Class.forName(classname, false, null) should be changed to:
Class.forName(classname, false,)

if (loader == null) condition check should be modified to check if the
loader
is responsible for loading system classes.

This is the first set of change for this problem we identified. Similar
change in other components will be made in the future.

Testing: run all jdk core test targets on all platforms.

Thanks
Mandy
P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in
the java.lang.management.

Re: Code Review for JEP 259: Stack-Walking API

[serguei.spit...@oracle.com]

Hi Mandy,


It looks pretty good to me.
At least, I do not see any obvious issues.


Just some minor comments below.


webrev.03/hotspot/src/share/vm/classfile/javaClasses.cpp

2128 Symbol* java_lang_StackFrameInfo::get_file_name(Handle stackFrame, 
InstanceKlass* holder) {

2129 if (MemberNameInStackFrame) {
2130 return holder->source_file_name();
2131 } else {
2132 int bci = stackFrame->int_field(_bci_offset);
2133 short version = stackFrame->short_field(_version_offset);
2134
2135 return Backtrace::get_source_file_name(holder, version);
2136 }
2137 }

 The 'int bci' is not used above.

2139 void java_lang_StackFrameInfo::set_method_and_bci(Handle 
stackFrame, const methodHandle& method, int bci) {

2140 if (MemberNameInStackFrame) {
2141 oop mname = stackFrame->obj_field(_memberName_offset);
2142 InstanceKlass* ik = method->method_holder();
2143 CallInfo info(method(), ik);
2144 MethodHandles::init_method_MemberName(mname, info);
2145 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2146 // method may be redefined; store the version
2147 int version = method->constants()->version();
2148 assert((jushort)version == version, "version should be short");
2149 java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
2150 } else {
2151 int mid = method->orig_method_idnum();
2152 int version = method->constants()->version();
2153 int cpref = method->name_index();
2154 assert((jushort)mid == mid, "mid should be short");
2155 assert((jushort)version == version, "version should be short");
2156 assert((jushort)cpref == cpref, "cpref should be short");
2157
2158 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
2159 java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
2160 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
2161 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2162 }
2163 } Minor: The calls to set_version() and set_bci() are the same for 
both alternatives and can be done just once before the if-else statement 
as below. With such refactoring it is clear what the common part is. 
void java_lang_StackFrameInfo::set_method_and_bci(Handle stackFrame, 
const methodHandle& method, int bci) { 
java_lang_StackFrameInfo::set_bci(stackFrame(), bci); // method may be 
redefined; store the version int version = 
method->constants()->version(); assert((jushort)version == version, 
"version should be short"); 
java_lang_StackFrameInfo::set_version(stackFrame(), (short)version); if 
(MemberNameInStackFrame) { oop mname = 
stackFrame->obj_field(_memberName_offset); InstanceKlass* ik = 
method->method_holder(); CallInfo info(method(), ik); 
MethodHandles::init_method_MemberName(mname, info); } else { int mid = 
method->orig_method_idnum(); int cpref = method->name_index(); 
assert((jushort)mid == mid, "mid should be short"); 
assert((jushort)cpref == cpref, "cpref should be short"); 
java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid); 
java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref); } }



webrev.03/hotspot/src/share/vm/prims/jvm.cpp

572 int limit = start_index+frame_count; 597 int limit = 
start_index+frame_count;


Minor: Need spaces around the '+'. 
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp


  62 // Parameters:
  63 //  thread.Current Java thread.
  64 //  magic. Magic value used for each stack walking
  65 //  classes_array  User-supplied buffers.  The 0th element is reserved
  . . .

  86 // Parameters:
  87 //   mode.  Restrict which frames to be decoded.
  88 //   decode_limit.  Maximum of frames to be decoded.
  89 //   start_index.   Start index to the user-supplied buffers.
  90 //   classes_array. Buffer to store classes in, starting at start_index.
  91 //   frames_array.  Buffer to store StackFrame in, starting at start_index.
  92 //  NULL if not used.
  93 //   end_index. End index to the user-supplied buffers with unpacked 
frames.
  94 //
  . . .
 274 // Parameters:
 275 //   stackStream.   StackStream object
 276 //   mode.  Stack walking mode.
 277 //   skip_frames.   Number of frames to be skipped.
 278 //   frame_count.   Number of frames to be traversed.
 279 //   start_index.   Start index to the user-supplied buffers.
 280 //   classes_array. Buffer to store classes in, starting at start_index.
 281 //   frames_array.  Buffer to store StackFrame in, starting at start_index.
 . . .
 414 // Parameters:
 415 //   stackStream.   StackStream object
 416 //   mode.  Stack walking mode.
 417 //   magic. Must be valid value to continue the stack walk
 418 //   frame_count.   Number of frames to be decoded.
 419 //   start_index.   Start index to the user-supplied buffers.
 420 //   classes_array. Buffer to store classes in, starting at start_index.
 421 //   frames_array.  Buffer to store StackFrame in, starting at start_index.

  Minor: Dots after the parameter names looks strange.  
Probably better to remove them or re

Re: Code Review for JEP 259: Stack-Walking API

[serguei.spit...@oracle.com]

Somehow some of the formatting in my reply is gone.
I'm trying to fix it below...

Thanks,
Serguei


On 11/20/15 01:59, serguei.spit...@oracle.com wrote:

Hi Mandy,


It looks pretty good to me.
At least, I do not see any obvious issues.


Just some minor comments below.


webrev.03/hotspot/src/share/vm/classfile/javaClasses.cpp

2128 Symbol* java_lang_StackFrameInfo::get_file_name(Handle 
stackFrame, InstanceKlass* holder) {

2129   if (MemberNameInStackFrame) {
2130  return holder->source_file_name();
2131   } else {
2132  int bci = stackFrame->int_field(_bci_offset);
2133  short version = stackFrame->short_field(_version_offset);
2134
2135  return Backtrace::get_source_file_name(holder, version);
2136   }
2137 }

 The 'int bci' is not used above.


2139 void java_lang_StackFrameInfo::set_method_and_bci(Handle 
stackFrame, const methodHandle& method, int bci) {

2140   if (MemberNameInStackFrame) {
2141 oop mname = stackFrame->obj_field(_memberName_offset);
2142 InstanceKlass* ik = method->method_holder();
2143 CallInfo info(method(), ik);
2144 MethodHandles::init_method_MemberName(mname, info);
2145 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2146 // method may be redefined; store the version
2147 int version = method->constants()->version();
2148 assert((jushort)version == version, "version should be short");
2149 java_lang_StackFrameInfo::set_version(stackFrame(), 
(short)version);

2150   } else {
2151 int mid = method->orig_method_idnum();
2152 int version = method->constants()->version();
2153 int cpref = method->name_index();
2154 assert((jushort)mid == mid, "mid should be short");
2155 assert((jushort)version == version, "version should be short");
2156 assert((jushort)cpref == cpref, "cpref should be short");
2157
2158 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
2159 java_lang_StackFrameInfo::set_version(stackFrame(), 
(short)version);

2160 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
2161 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2162   }
2163 }

Minor: The calls to set_version() and set_bci() are the same for both 
alternatives
   and can be done just once before the if-else statement as 
below.

   With such refactoring it is clear what the common part is.

void java_lang_StackFrameInfo::set_method_and_bci(Handle stackFrame, 
const methodHandle& method, int bci) {
   java_lang_StackFrameInfo::set_bci(stackFrame(), bci); // method may 
be redefined; store the version

   int version = method->constants()->version();
   assert((jushort)version == version, "version should be short");
   java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
   if (MemberNameInStackFrame) {
 oop mname = stackFrame->obj_field(_memberName_offset);
 InstanceKlass* ik = method->method_holder();
 CallInfo info(method(), ik);
 MethodHandles::init_method_MemberName(mname, info);
  } else {
 int mid = method->orig_method_idnum();
 int cpref = method->name_index();
 assert((jushort)mid == mid, "mid should be short");
 assert((jushort)cpref == cpref, "cpref should be short");
 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
  }
}


webrev.03/hotspot/src/share/vm/prims/jvm.cpp

572 int limit = start_index+frame_count; 597 int limit = 
start_index+frame_count;


Minor: Need spaces around the '+'. 
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp



  62 // Parameters:
  63 //  thread.Current Java thread.
  64 //  magic. Magic value used for each stack walking
  65 //  classes_array  User-supplied buffers.  The 0th element is 
reserved

  . . .

  86 // Parameters:
  87 //   mode.  Restrict which frames to be decoded.
  88 //   decode_limit.  Maximum of frames to be decoded.
  89 //   start_index.   Start index to the user-supplied buffers.
  90 //   classes_array. Buffer to store classes in, starting at 
start_index.
  91 //   frames_array.  Buffer to store StackFrame in, starting at 
start_index.

  92 //  NULL if not used.
  93 //   end_index. End index to the user-supplied buffers with 
unpacked frames.

  94 //
  . . .
 274 // Parameters:
 275 //   stackStream.   StackStream object
 276 //   mode.  Stack walking mode.
 277 //   skip_frames.   Number of frames to be skipped.
 278 //   frame_count.   Number of frames to be traversed.
 279 //   start_index.   Start index to the user-supplied buffers.
 280 //   classes_array. Buffer to store classes in, starting at 
start_index.
 281 //   frames_array.  Buffer to store StackFrame in, starting at 
start_index.

 . . .
 414 // Parameters:
 415 //   stackStream.   Sta

Re: Code Review for JEP 259: Stack-Walking API

[serguei.spit...@oracle.com]

On 11/20/15 08:39, Mandy Chung wrote:

On Nov 20, 2015, at 1:59 AM, serguei.spit...@oracle.com wrote:

  The 'int bci' is not used above.

This is weird.   Daniel caught that and I took that line out already.
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03-delta/hotspot/src/share/vm/classfile/javaClasses.cpp.sdiff.html

Anyway this webrev is the up-to-date one including fixing the nits you pointed 
out.
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.04


   Minor: The calls to set_version() and set_bci() are the same for both 
alternatives
  and can be done just once before the if-else statement as below.
  With such refactoring it is clear what the common part is.


Moved.


Thanks.





webrev.03/hotspot/src/share/vm/prims/jvm.cpp

Minor: Need spaces around the '+'. 
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp

I am not sure if that’s the convention applied consistently in VM.  I fixed it 
anyway.


webrev.04/hotspot/src/share/vm/prims/jvm.cpp

  One place left with inconsistent formatting:

597 int limit = start_index+frame_count;




   Minor: Indent at the line 115 must be +2.

I don’t see any indentation/formatting issue there.

   I see it fixed in new webrev. :)



  360 for (int n=0; n < skip_frames && !vfst.at_end(); vfst.next(), n++) {


I prefer to keep n=0 and there are other places using that convention.

  Ok.



  451   int count = frame_count+start_index;
   Minor: Need spaces around the '=' and '+’.

Fixed.
Thank you for making the changes! I do not need another webrev. Amazing 
work in general! Thanks, Serguei


Thanks
Mandy

Re: 8143911: java/lang/StackWalker tests fail on Solaris with IllegalStateException

[serguei.spit...@oracle.com]

Looks good.

Thanks,
Serguei


On 11/24/15 14:37, Mandy Chung wrote:

On Nov 24, 2015, at 2:20 PM, Daniel D. Daugherty  
wrote:

You use both 'this.anchor' and 'anchor'. Seems inconsistent.


Oh yeah.  I took out “this.” from it.

diff --git a/src/java.base/share/classes/java/lang/StackStreamFactory.java 
b/src/java.base/share/classes/java/lang/StackStreamFactory.java
--- a/src/java.base/share/classes/java/lang/StackStreamFactory.java
+++ b/src/java.base/share/classes/java/lang/StackStreamFactory.java
@@ -225,17 +225,17 @@
  }
  switch (state) {
  case NEW:
-if (this.anchor != 0) {
+if (anchor != 0) {
  throw new IllegalStateException("This stack stream is 
being reused.");
  }
  break;
  case OPEN:
-if (this.anchor <= 0) {
-throw new IllegalStateException("This stack stream is not 
valid for walking");
+if (anchor == 0 || anchor == -1L) {
+throw new IllegalStateException("This stack stream is not 
valid for walking: " + anchor);
  }
  break;
  case CLOSED:
-if (this.anchor != -1L) {
+if (anchor != -1L) {
  throw new IllegalStateException("This stack stream is not 
closed.");
  }
  }

Mandy

Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault

[serguei.spit...@oracle.com]

Reviewed

Thanks,
Serguei

On 11/19/14 12:49 AM, Chris Plummer wrote:
I've update the webrev to add STACK_SIZE_MINIMUM in place of the 32k 
references, and also moved the test from hotspot/test/runtime to 
jdk/test/tools/launcher as David requested. That required some 
adjustments to the test script, since test_env.sh does not exist in 
jdk/test, so I had to pull in the bits I needed into the script.


http://cr.openjdk.java.net/~cjplummer/6762191/webrev.01/

I still need to rerun through JPRT. I'll do so once there are no more 
suggested changes.


thanks,

Chris

On 11/18/14 2:08 PM, Chris Plummer wrote:
Adding core-libs-dev@openjdk.java.net, since one of the changes is in 
java.c.


Chris

On 11/12/14 6:43 PM, David Holmes wrote:

Hi Chris,

Sorry for the delay.

On 13/11/2014 5:44 AM, Chris Plummer wrote:

Hi,

I'm still looking for reviewers.


As the change is to the launcher it needs to be reviewed by the 
launcher owner - which I think is serviceability (though also cc'd 
Kumar :) ).


Launcher change, and your rationale, seems okay to me. I'd probably 
put the test in to jdk/test/tools/launcher/ though.


Thanks,
David


thanks,

Chris

On 11/7/14 7:53 PM, Chris Plummer wrote:

This is an initial review for 6762191. I'm guessing there will be
recommendations to fix in a different way, but thought this would 
be a

good time to start the discussion.

https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.jdk/
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.hotspot/

The bug is that if the -Xss size is set to something very small (like
16k), on linux there will be a crash due to overwriting the end of 
the

stack. This happens before hotspot can compute its stack needs and
verify that the stack is big enough.

It didn't seem viable to move the hotspot stack size check 
earlier. It
depends on too much other work done before that point, and the 
changes

would have been disruptive. The stack size check is currently done in
os::init_2().

What is needed is a check before the thread is created. That way we
can create a thread with a big enough stack to handle all needs up to
the point of the check in os::init_2(). This initial check does not
need to be the final check. It just needs to confirm that we have
enough stack to get us to the check in os::init_2().

I decided to check in java.c if the -Xss size is too small, and 
set it
to a larger size if it is. I hard coded this size to 32k (I'll 
explain

why 32k later). I suspect this is the part that will result in some
debate. If you have better suggestions let me know. If it does stay
here, then probably the 32k needs to be a #define, and maybe even an
OS porting interface, but I'm not sure where to put it.

The reason I chose 32k is because this is big enough for all 
platforms

to get to the stack size check in os::init_2(). It is also smaller
than the actual minimum stack size allowed on any platform. 32-bit
windows has the smallest requirement at 64k. I add some printfs to
print the minimum stack requirement, and then ran a simple JTReg test
with every JPRT supported platform to get the results.

The TooSmallStackSize.sh will run "java -version" with -Xss16k,
-Xss32k, and -XXss, where  is the size from the
error message produced by the JVM, such as in the following:

$ java -Xss32k -version
The stack size specified is too small, Specify at least 100k
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

I ran this test through JPRT on all platforms, and they all pass.

One thing to point out is that Windows behaves a bit different than
the other platforms. It always rounds the stack size up to a multiple
of 64k , so even if you specify -Xss16k, you get a 64k stack. On
32-bit Windows with C1, 64k is also the minimum requirement, so there
is no error produced in this case. However, on 32-bit Windows with 
C2,

68k is the minimum, so an error is produced since the stack will only
be 64k. There is no bug here. It's just a bit confusing.

thanks,

Chris

Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault

[serguei.spit...@oracle.com]

The fix still looks good to me.

Thanks,
Serguei


On 12/1/14 6:39 PM, Chris Plummer wrote:
Sorry about the long delay in getting back to this. I ran into two 
separate JPRT issues that were preventing me from testing these 
changes, plus I was on vacation last week. Here's an updated webrev. 
I'm not sure where we left things, so I'll just say what's changed 
since the original version:


1. Rewrote the test to be in Java instead of a shell script.
2. Moved the test from hotspot/test/runtime/memory to 
jdk/test/tools/launcher
3. Added STACK_SIZE_MINIMUM to java.c, allowing a makefile to override 
the default 32k minimum value.


https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.02/

thanks,

Chris

On 11/19/14 7:52 AM, Chris Plummer wrote:

On 11/19/14 2:12 AM, David Holmes wrote:

On 19/11/2014 6:49 PM, Chris Plummer wrote:

I've update the webrev to add STACK_SIZE_MINIMUM in place of the 32k
references, and also moved the test from hotspot/test/runtime to
jdk/test/tools/launcher as David requested. That required some
adjustments to the test script, since test_env.sh does not exist in
jdk/test, so I had to pull in the bits I needed into the script.


Is there a reason this needs a shell script instead of using the 
testlibrary tools to launch the VM and check the output?
Not that I'm aware of. I guess I just really didn't look at what it 
would take to make it all in java. I'll have a look at java examples 
and convert it.


Chris


Sorry that should have been mentioned much earlier.

David



http://cr.openjdk.java.net/~cjplummer/6762191/webrev.01/

I still need to rerun through JPRT. I'll do so once there are no more
suggested changes.

thanks,

Chris

On 11/18/14 2:08 PM, Chris Plummer wrote:

Adding core-libs-dev@openjdk.java.net, since one of the changes is in
java.c.

Chris

On 11/12/14 6:43 PM, David Holmes wrote:

Hi Chris,

Sorry for the delay.

On 13/11/2014 5:44 AM, Chris Plummer wrote:

Hi,

I'm still looking for reviewers.


As the change is to the launcher it needs to be reviewed by the
launcher owner - which I think is serviceability (though also cc'd
Kumar :) ).

Launcher change, and your rationale, seems okay to me. I'd probably
put the test in to jdk/test/tools/launcher/ though.

Thanks,
David


thanks,

Chris

On 11/7/14 7:53 PM, Chris Plummer wrote:

This is an initial review for 6762191. I'm guessing there will be
recommendations to fix in a different way, but thought this 
would be a

good time to start the discussion.

https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.jdk/
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.hotspot/

The bug is that if the -Xss size is set to something very small 
(like
16k), on linux there will be a crash due to overwriting the end 
of the

stack. This happens before hotspot can compute its stack needs and
verify that the stack is big enough.

It didn't seem viable to move the hotspot stack size check 
earlier. It
depends on too much other work done before that point, and the 
changes
would have been disruptive. The stack size check is currently 
done in

os::init_2().

What is needed is a check before the thread is created. That 
way we
can create a thread with a big enough stack to handle all needs 
up to
the point of the check in os::init_2(). This initial check does 
not

need to be the final check. It just needs to confirm that we have
enough stack to get us to the check in os::init_2().

I decided to check in java.c if the -Xss size is too small, and 
set it
to a larger size if it is. I hard coded this size to 32k (I'll 
explain
why 32k later). I suspect this is the part that will result in 
some
debate. If you have better suggestions let me know. If it does 
stay
here, then probably the 32k needs to be a #define, and maybe 
even an

OS porting interface, but I'm not sure where to put it.

The reason I chose 32k is because this is big enough for all 
platforms

to get to the stack size check in os::init_2(). It is also smaller
than the actual minimum stack size allowed on any platform. 32-bit
windows has the smallest requirement at 64k. I add some printfs to
print the minimum stack requirement, and then ran a simple 
JTReg test

with every JPRT supported platform to get the results.

The TooSmallStackSize.sh will run "java -version" with -Xss16k,
-Xss32k, and -XXss, where  is the size from the
error message produced by the JVM, such as in the following:

$ java -Xss32k -version
The stack size specified is too small, Specify at least 100k
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

I ran this test through JPRT on all platforms, and they all pass.

One thing to point out is that Windows behaves a bit different 
than
the other platforms. It always rounds the stack size up to a 
multiple

of 64k , so even if you specify -Xss16k, you get a 64k stack. On
32-bit Windows with

Re: [9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault

[serguei.spit...@oracle.com]

It still looks good to me too. :)

Thanks,
Serguei

On 12/4/14 3:46 PM, David Holmes wrote:
Looks good to me too Chris - sorry for the delay getting back to you. 
But at least Kumar spotted all the typos :)


David

On 4/12/2014 6:12 PM, Chris Plummer wrote:

On 12/3/14 4:56 AM, Alan Bateman wrote:

On 02/12/2014 02:39, Chris Plummer wrote:

Sorry about the long delay in getting back to this. I ran into two
separate JPRT issues that were preventing me from testing these
changes, plus I was on vacation last week. Here's an updated webrev.
I'm not sure where we left things, so I'll just say what's changed
since the original version:

1. Rewrote the test to be in Java instead of a shell script.
2. Moved the test from hotspot/test/runtime/memory to
jdk/test/tools/launcher
3. Added STACK_SIZE_MINIMUM to java.c, allowing a makefile to
override the default 32k minimum value.

https://bugs.openjdk.java.net/browse/JDK-6762191
http://cr.openjdk.java.net/~cjplummer/6762191/webrev.02/

This looks to me. A minor comment for java.c is that this code uses
4-space indent (different to hotspot).

The test looks okay too, you might just checking the copyright date as
I assume was not written in 2010. Also I think the import of
java.io.File may be left behind from the previous round.

-Alan

Hi Alan,

While removing the java.io.File import, I also questioned why I had
java.io.IOException being imported. There were a couple of methods that
declared it thrown, and the main method therefore had to catch it, but
it turns out this was just copy/paste from the Settings.java test I used
as a template, and is not actually needed. I removed the import, throws,
and try/catch of IOException.

All the other issues mentioned by others have also been addressed. A new
webrev can be found at:

http://cr.openjdk.java.net/~cjplummer/6762191/webrev.03/

thanks,

Chris

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

[serguei.spit...@oracle.com]

Chris,

It looks good in general.
I'd suggest to rename the folder:
|| test/com/sun/jdi/CDSJDITests
to:
  test/com/sun/jdi/cds

There is no need to spell "JDI" as it is already a sub-folder of the 
com/sun/jdi

and there is no need to spell "Tests" too as it is in the test repo.
Also, all the folders are normally named in the lower case.

Thanks,
Serguei


On 6/2/15 8:25 PM, Chris Plummer wrote:
Ok, I'm going to keep this as one webrev, but I did create JDK-8081771 
for the ProcessTool.java changes:


https://bugs.openjdk.java.net/browse/JDK-8081771

I will commit ProcessTool.java under JDK-8081771, and the rest of the 
changes under JDK-8054386. Both will then be pushed together. I also 
started a new thread for the review of JDK-8081771:


http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014930.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html

thanks,

Chris

On 6/2/15 11:55 AM, Chris Plummer wrote:
I'm going to have to separate out the ProcessTool.java changes into a 
separate changeset (and CR). In the meantime, feel free to review 
what I have below. The code won't be changing at all when I separate 
out the ProcessTool.java changes.


thanks,

Chris

On 6/2/15 12:36 AM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes 
changes to jdk/test library code]


Please review the updated webrev:

Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386

There were concerns about the new hotspot tests referencing jdk 
tests. One concern was that if the jdk tests change, they could 
break the hotspot tests, and this might initially go undetected. The 
other concern is that if the jdk and hotspot tests are placed in 
separate test bundles, then it would not be possible to run the 
hotspot tests.


Because of these concerns, I moved the tests that were in 
hotspot/test/runtime/CDSJDITests to instead be in 
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the 
tests in the process. Also, I had to update the jdk version of 
ProcessTool.java to include the createJavaProcessBuilder() variant 
that is in the hotspot version of ProcessTool.java.


Lastly, in CDSJITTest.java I changed:

OutputAnalyzer output = new OutputAnalyzer(pb.start());

to instead be:

OutputAnalyzer output = ProcessTools.executeProcess(pb);

I had to do this since the jdk version of the OutputAnalyzer 
constructor is not public. The 1st version is what is commonly used 
in hostspot tests, and the 2nd version is what is commonly used in 
jdk tests. I decided to adopt the jdk way rather than make the 
OutputAnalyzer constructors public, although this will probably 
happen eventually when the two versions are unified.


thanks,

Chris


On 5/19/15 7:25 AM, Chris Plummer wrote:

Hi,

Please review the following changes for allowing java debugging 
when CDS is enabled.


Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386

The VM changes are simple. I removed the check that prevents 
debugging with CDS enabled, and added logic that will map the CDS 
archive RW when debugging is enabled.


The tests are a bit more complex. There are a bunch of existing JDI 
tests for testing debugging support. Rather than start from scratch 
or clone them, I instead just wrote wrapper tests that put the 
relevant JDI test classes in the archive, and then invoke the JDI 
test. I did this for 3 of the JDI tests. If you feel there are 
others that would be good candidates, I'd be happy to add them. I'm 
looking for ones that would result in modification of the RO class 
metadata, such as setting a breakpoint (for which I already added 
two tests).


Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT "-testset hotspot" run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and 
then running them.
-Putting a simple test class in the archive and then setting a 
breakpoint on it using jdb


Some of the above testing resulted in the discovery of bugs that 
still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.


I also verified that without the change to map the archive RW, the 
above testing resulted in a SEGV, which is what you would expect 
(and actually want to see to prove that the testing is effective).


thanks,

Chris

Re: [9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument

[serguei.spit...@oracle.com]

It looks good to me.
Reviewed all together.

Thanks,
Serguei

Thanks,
Serguei

On 6/2/15 8:20 PM, Chris Plummer wrote:

Please review the following:

Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8081771

This review only concerns the changes to ProcessTool.java. The 
CDSJDITests and filemapp.cpp changes will be committed under CR 
JDK-8054386, but they rely on this change to ProcessTool.java, so both 
CRs will be pushed together. See the following thread for the 
JDK-8054386 review:


http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014923.html 



thanks,

Chris

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

[serguei.spit...@oracle.com]

Hi Chris,

I've replied with a thumbs up on another thread.

Thanks,
Serguei


On 6/3/15 4:23 PM, Chris Plummer wrote:

Hi Serguei,

I'll take care of the rename. Can I also put you down for the 
ProcessTool.java changes, which are officially being reviewed on 
another thread:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html 



thanks,

Chris

On 6/3/15 1:40 AM, serguei.spit...@oracle.com wrote:

Chris,

It looks good in general.
I'd suggest to rename the folder:
|| test/com/sun/jdi/CDSJDITests
to:
  test/com/sun/jdi/cds

There is no need to spell "JDI" as it is already a sub-folder of the 
com/sun/jdi

and there is no need to spell "Tests" too as it is in the test repo.
Also, all the folders are normally named in the lower case.

Thanks,
Serguei


On 6/2/15 8:25 PM, Chris Plummer wrote:
Ok, I'm going to keep this as one webrev, but I did create 
JDK-8081771 for the ProcessTool.java changes:


https://bugs.openjdk.java.net/browse/JDK-8081771

I will commit ProcessTool.java under JDK-8081771, and the rest of 
the changes under JDK-8054386. Both will then be pushed together. I 
also started a new thread for the review of JDK-8081771:


http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014930.html 

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html 



thanks,

Chris

On 6/2/15 11:55 AM, Chris Plummer wrote:
I'm going to have to separate out the ProcessTool.java changes into 
a separate changeset (and CR). In the meantime, feel free to review 
what I have below. The code won't be changing at all when I 
separate out the ProcessTool.java changes.


thanks,

Chris

On 6/2/15 12:36 AM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes 
changes to jdk/test library code]


Please review the updated webrev:

Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386

There were concerns about the new hotspot tests referencing jdk 
tests. One concern was that if the jdk tests change, they could 
break the hotspot tests, and this might initially go undetected. 
The other concern is that if the jdk and hotspot tests are placed 
in separate test bundles, then it would not be possible to run the 
hotspot tests.


Because of these concerns, I moved the tests that were in 
hotspot/test/runtime/CDSJDITests to instead be in 
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of 
the tests in the process. Also, I had to update the jdk version of 
ProcessTool.java to include the createJavaProcessBuilder() variant 
that is in the hotspot version of ProcessTool.java.


Lastly, in CDSJITTest.java I changed:

OutputAnalyzer output = new OutputAnalyzer(pb.start());

to instead be:

OutputAnalyzer output = ProcessTools.executeProcess(pb);

I had to do this since the jdk version of the OutputAnalyzer 
constructor is not public. The 1st version is what is commonly 
used in hostspot tests, and the 2nd version is what is commonly 
used in jdk tests. I decided to adopt the jdk way rather than make 
the OutputAnalyzer constructors public, although this will 
probably happen eventually when the two versions are unified.


thanks,

Chris


On 5/19/15 7:25 AM, Chris Plummer wrote:

Hi,

Please review the following changes for allowing java debugging 
when CDS is enabled.


Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386

The VM changes are simple. I removed the check that prevents 
debugging with CDS enabled, and added logic that will map the CDS 
archive RW when debugging is enabled.


The tests are a bit more complex. There are a bunch of existing 
JDI tests for testing debugging support. Rather than start from 
scratch or clone them, I instead just wrote wrapper tests that 
put the relevant JDI test classes in the archive, and then invoke 
the JDI test. I did this for 3 of the JDI tests. If you feel 
there are others that would be good candidates, I'd be happy to 
add them. I'm looking for ones that would result in modification 
of the RO class metadata, such as setting a breakpoint (for which 
I already added two tests).


Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT "-testset hotspot" run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive 
and then running them.
-Putting a simple test class in the archive and then setting a 
breakpoint on it using jdb


Some of the above testing resulted in the discovery of bugs that 
still need to be addressed: JDK-8078644, JDK-8078730, and 
JDK-8079181.


I also verified that without the change to map the archive RW, 
the above testing resulted in a SEGV, which is what you would 
expect (and a

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

[serguei.spit...@oracle.com]

I guess, there is no need to re-review.
It looks good anyway.

Thanks,
Serguei


On 6/4/15 4:32 PM, Chris Plummer wrote:

Hi David,

Here's an updated webrev:

http://cr.openjdk.java.net/~cjplummer/8054386/webrev.04/

thanks,

Chris

On 6/3/15 11:29 PM, David Holmes wrote:

Hi Chris,

Hotspot change is good.

Only a couple of style nits with the tests (where are our Java style 
guidelines ???). Taking CDSJDITest.java as an example:


If you are okay with this line length:

 115 public static OutputAnalyzer executeAndLog(ProcessBuilder 
pb, String logName) throws Exception {


then you can remove a number of line breaks in the headers of other 
functions. (I don't follow the 70-80 char line length dogma ;) )


If you do break a header of a function the { still stays on the same 
line as the last header component ie:


 private static void addToClassList(PrintStream ps, String 
classes[])

 throws IOException {

not:

 139 private static void addToClassList(PrintStream ps, String 
classes[])

 140 throws IOException
 141 {

Cheers,
David

On 2/06/2015 5:36 PM, Chris Plummer wrote:

[Adding core-libs-dev@openjdk.java.net since this update includes
changes to jdk/test library code]

Please review the updated webrev:

Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386

There were concerns about the new hotspot tests referencing jdk tests.
One concern was that if the jdk tests change, they could break the
hotspot tests, and this might initially go undetected. The other 
concern

is that if the jdk and hotspot tests are placed in separate test
bundles, then it would not be possible to run the hotspot tests.

Because of these concerns, I moved the tests that were in
hotspot/test/runtime/CDSJDITests to instead be in
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the
tests in the process. Also, I had to update the jdk version of
ProcessTool.java to include the createJavaProcessBuilder() variant that
is in the hotspot version of ProcessTool.java.

Lastly, in CDSJITTest.java I changed:

 OutputAnalyzer output = new OutputAnalyzer(pb.start());

to instead be:

 OutputAnalyzer output = ProcessTools.executeProcess(pb);

I had to do this since the jdk version of the OutputAnalyzer 
constructor

is not public. The 1st version is what is commonly used in hostspot
tests, and the 2nd version is what is commonly used in jdk tests. I
decided to adopt the jdk way rather than make the OutputAnalyzer
constructors public, although this will probably happen eventually when
the two versions are unified.

thanks,

Chris


On 5/19/15 7:25 AM, Chris Plummer wrote:

Hi,

Please review the following changes for allowing java debugging when
CDS is enabled.

Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386

The VM changes are simple. I removed the check that prevents debugging
with CDS enabled, and added logic that will map the CDS archive RW
when debugging is enabled.

The tests are a bit more complex. There are a bunch of existing JDI
tests for testing debugging support. Rather than start from scratch or
clone them, I instead just wrote wrapper tests that put the relevant
JDI test classes in the archive, and then invoke the JDI test. I did
this for 3 of the JDI tests. If you feel there are others that would
be good candidates, I'd be happy to add them. I'm looking for ones
that would result in modification of the RO class metadata, such as
setting a breakpoint (for which I already added two tests).

Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT "-testset hotspot" run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and
then running them.
-Putting a simple test class in the archive and then setting a
breakpoint on it using jdb

Some of the above testing resulted in the discovery of bugs that still
need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.

I also verified that without the change to map the archive RW, the
above testing resulted in a SEGV, which is what you would expect (and
actually want to see to prove that the testing is effective).

thanks,

Chris

Re: RFR: 8220674: [TESTBUG] MetricsMemoryTester failcount test in docker container only works with debug JVMs

[serguei.spit...@oracle.com]

Hi Bob,

It looks good to me.

Thanks,
Serguei


On 3/21/19 10:12, Bob Vandette wrote:

Please review this fix for a container test that fails on some Linux 
distributions.

The test fails to see the Memory Fail count metric value increase.  This is 
caused by
the fact that we are allowing ergonomics to select the amount of Heap size.  
This
size varies depending on the amount of free memory that’s available on different
docker implementations.

The fix is to force the VM to always set the heap size to the size of the 
containers
memory.  This change also stops allocating once it sees the fail count increase.
This is needed to keep the container from getting killed by the out of memory
killer.

The fix has been verified by the submitter along with a local run of the 
container tests.

BUG:

https://bugs.openjdk.java.net/browse/JDK-8220674

WEBREV:

http://cr.openjdk.java.net/~bobv/8220674/webrev/

Bob.

Re: RFR (XS): 8157188: 2 test failures in demo/jvmti due to unexpected class file version 53

[serguei.spit...@oracle.com]

David,

The change looks good but you already have enough reviewers. :)
Just wanted to thank you for taking steps on this issue.

Thanks,
Serguei


On 5/18/16 21:52, David Holmes wrote:
Not sure who really owns this file so cc'ing core-libs and 
serviceability.


bug: https://bugs.openjdk.java.net/browse/JDK-8157188

The file src/java.base/share/native/include/classfile_constants.h 
describes information about classfiles and is used by libverify and 
./demo/share/jvmti/java_crw_demo/java_crw_demo.c


This file has not been updated for classfile version 53 and so asserts 
will fail in java_crw_demo.c when it encounters classes compiled for 
version 53 - as they now are. This version change caused test failures 
in the hotspot forest when it was pulled down earlier this week.


This fix trivially bumps the current version number to 53 to fix the 
failing tests. It is a separate issue as to whether other changes are 
needed in this file to reflect what is new with classfile version 53.


Diff below.

Thanks,
David
--

diff -r 3eea6819cc1f 
src/java.base/share/native/include/classfile_constants.h

--- a/src/java.base/share/native/include/classfile_constants.h
+++ b/src/java.base/share/native/include/classfile_constants.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, 2012, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2004, 2016, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,7 +31,7 @@
 #endif

 /* Classfile version number for this information */
-#define JVM_CLASSFILE_MAJOR_VERSION 52
+#define JVM_CLASSFILE_MAJOR_VERSION 53
 #define JVM_CLASSFILE_MINOR_VERSION 0

 /* Flags */

Re: RFR(XXS): 8182307 - Error during JRMP connection establishment

[serguei.spit...@oracle.com]

Hi Dan,

The fix looks good to me.
Nice, you have caught it.

Do you want this fixed in 10 or 11?
I thought that the jdk/hs is for 11 now.
Is it correct?

Thanks,
Serguei


On 12/7/17 09:09, Daniel D. Daugherty wrote:

Roger,

Thanks for the review!

Dan

P.S.
I'm planning to push this fix to jdk/hs since the only sightings
have been in jdk/hs testing or in projects that are parented to
jdk/hs repos... Hope that's okay...


On 12/7/17 12:07 PM, Roger Riggs wrote:

+1

On 12/7/2017 12:00 PM, Gerald Thornbrugh wrote:

Hi Dan,

Your fix looks good.

Jerry


Adding core-libs-dev@... since this is RMI code. Thanks Alan!

Folks, this review spans three OpenJDK aliases so Thunderbird's
reply-to-list feature won't get it right. This is one of the
few times that reply-to-all is the right thing to do...

Dan

On 12/7/17 11:38 AM, Daniel D. Daugherty wrote:

Greetings,

I have a small fix for a very intermittent ServerSocket related test
failure:

    JDK-8182307: Error during JRMP connection establishment
https://bugs.openjdk.java.net/browse/JDK-8182307

The fix is copied from Jerry's work on the following bugs:

    JDK-8182757 JDWP: Socket Transport handshake hangs on Solaris
https://bugs.openjdk.java.net/browse/JDK-8182757

    JDK-8178676 nsk/jvmti/AttachOnDemand/attach045 fails with 
Exception

https://bugs.openjdk.java.net/browse/JDK-8178676

For the gory details of the reasons for this fix please see
Jerry's bugs.

Webrev URL: 
http://cr.openjdk.java.net/~dcubed/8182307-webrev/jdk10-0/



I observed this test failure one time in my Thread-SMR work and have
been including this fix in months of Thread-SMR stress testing. It 
was
also included in both JPRT and Mach5 tier[1-5] testing of the 
Thread-SMR

changes.

Thanks, in advance, for any comments, suggestions or questions.

Dan

Re: RFR(XXS): 8182307 - Error during JRMP connection establishment

[serguei.spit...@oracle.com]

On 12/7/17 10:08, serguei.spit...@oracle.com wrote:

Hi Dan,

The fix looks good to me.
Nice, you have caught it.

Do you want this fixed in 10 or 11?
I thought that the jdk/hs is for 11 now.
Is it correct?


Never mind.
I've just found a message from Jesper the jdk/hs is used for 10 pushes 
for one more week.


Thanks,
Serguei



Thanks,
Serguei


On 12/7/17 09:09, Daniel D. Daugherty wrote:

Roger,

Thanks for the review!

Dan

P.S.
I'm planning to push this fix to jdk/hs since the only sightings
have been in jdk/hs testing or in projects that are parented to
jdk/hs repos... Hope that's okay...


On 12/7/17 12:07 PM, Roger Riggs wrote:

+1

On 12/7/2017 12:00 PM, Gerald Thornbrugh wrote:

Hi Dan,

Your fix looks good.

Jerry


Adding core-libs-dev@... since this is RMI code. Thanks Alan!

Folks, this review spans three OpenJDK aliases so Thunderbird's
reply-to-list feature won't get it right. This is one of the
few times that reply-to-all is the right thing to do...

Dan

On 12/7/17 11:38 AM, Daniel D. Daugherty wrote:

Greetings,

I have a small fix for a very intermittent ServerSocket related test
failure:

    JDK-8182307: Error during JRMP connection establishment
https://bugs.openjdk.java.net/browse/JDK-8182307

The fix is copied from Jerry's work on the following bugs:

    JDK-8182757 JDWP: Socket Transport handshake hangs on Solaris
https://bugs.openjdk.java.net/browse/JDK-8182757

    JDK-8178676 nsk/jvmti/AttachOnDemand/attach045 fails with 
Exception

https://bugs.openjdk.java.net/browse/JDK-8178676

For the gory details of the reasons for this fix please see
Jerry's bugs.

Webrev URL: 
http://cr.openjdk.java.net/~dcubed/8182307-webrev/jdk10-0/



I observed this test failure one time in my Thread-SMR work and have
been including this fix in months of Thread-SMR stress testing. 
It was
also included in both JPRT and Mach5 tier[1-5] testing of the 
Thread-SMR

changes.

Thanks, in advance, for any comments, suggestions or questions.

Dan

Re: [11] RFR (XS) 8193838: Update jtreg requiredVersion to 4.2 b11 for JDK 11 and 12 support

[serguei.spit...@oracle.com]

David,

It looks good.

Thanks,
Serguei


On 12/20/17 02:22, David Holmes wrote:
Before we can update to JDK version 11, jtreg needs to be updated to 
recognize that release value - which is happening in jtreg 4.2 b11. So 
we then need to ensure that jtreg 4.2 b11 is used by updating the 
requiredVersion in each of the TEST.ROOT files, and in the jib 
configuiration.


bug: https://bugs.openjdk.java.net/browse/JDK-8193838
webrev: http://cr.openjdk.java.net/~dholmes/8193838/webrev/

Testing - TBD once b11 is promoted
 - local build using jib
 - hs/jdk tier1 and tier2

Thanks,
David

Re: RFR(L/M) : 8210112 : remove jdk.testlibrary.ProcessTools

[serguei.spit...@oracle.com]

Hi Igor,

I do not see any issues with this refactoring.

Thanks,
Serguei


On 9/6/18 09:19, Igor Ignatyev wrote:

JC,
thanks for your review!

Core-libs team,
as the majority of changed tests are core-libs tests, I'd really appreciate if 
someone from core-libs (preferably a Reviewer) could review the patch.

for the record, [1] is the latest version of webrev.

[1] http://cr.openjdk.java.net/~iignatyev//8210112/webrev.02/index.html 


2433 lines changed: 334 ins; 1677 del; 422 mod;

Cheers,
-- Igor



On Sep 5, 2018, at 5:03 PM, JC Beyler  wrote:

Hi Igor,

Looks good to me then. I agree most are nits, personal preferences, and just I 
noticed them as you were cleaning them up!

Awesome clean up :-)
Jc

On Wed, Sep 5, 2018 at 3:20 PM Igor Ignatyev mailto:igor.ignat...@oracle.com>> wrote:
Hi JC,



On Sep 5, 2018, at 2:59 PM, JC Beyler mailto:jcbey...@google.com>> wrote:

Hi Igor,

I like this much better! A few more comments:

- 
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/lib/testlibrary/OutputAnalyzerTest.java.udiff.html
 

   -> If the shouldMatch call fails, it throws an exception, why not just let that 
fail test, why are you catching and then rethrowing (like you do for 
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html
 
)

just to follow the style used by this tests, e.g.


   54 try {
   55 output.shouldContain(stdout);
   56 output.stdoutShouldContain(stdout);
   57 output.shouldContain(stderr);
   58 output.stderrShouldContain(stderr);
   59 } catch (RuntimeException e) {
   60 throw new Exception("shouldContain() failed", e);
   61 }
   86 try {
   87 output.shouldNotContain(nonExistingString);
   88 output.stdoutShouldNotContain(nonExistingString);
   89 output.stderrShouldNotContain(nonExistingString);
   90 } catch (RuntimeException e) {
   91 throw new Exception("shouldNotContain() failed", e);
   92 }
   93



- 
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html
 

There is now only a 1-liner for this method and it is called only once, 
should we inline and remove the method?

we can, but I ain't sure we should. from my point of view

   80 output.shouldHaveExitValue(0);
   81 output.shouldContain("sun.tools.jcmd.JCmd");
   82 matchListedProcesses(output);
is a bit easier to understand than
   80 output.shouldHaveExitValue(0);
   81 output.shouldContain("sun.tools.jcmd.JCmd");
   82 output.shouldMatchByLine(JCMD_LIST_REGEX);

however, I don't have strong preference here and if serviceability team wants, 
I can inline matchListedProcesses.


- Same for (we could inline): 
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdSanity.java.udiff.html
 
same
 here
- 
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/lib/jdk/test/lib/process/OutputAnalyzer.java.udiff.html
 

 "There is no lines" -> "There are no lines"

fixed. thanks for spotting.


 - What is the advantage of having the return at all now for the 
shouldMatch methods, if it fails it throws, the test fails; otherwise it 
doesn't return anything, the test can move on, no? I saw no moment when you get 
the return to do something more with it

OutputAnalazyer is supposed to be a fluent interface, and in some cases you 
might find it used that way, so I'd prefer to have possibility to use these 
methods in a method chain, as w/ we already have for the the most of other 
should* method. I've fixed a few more should* methods to return this.



Thanks for the incremental webrev, that made looking at the changes so much 
easier!

here is the next one -- 
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.1-2/index.html 


-- Igor

Jc

On Wed, Sep 5, 2018 at 2:32 PM Igor Ignatyev mailto:igor.ignat...@oracle.com>> wrote:
Hi JC,

thanks for reviewing this! I agree w/ both your comments and have updated the 
code very similarly to your suggestion.

I've also noticed that j.t.l.p.OutputAnalyzer::shouldMatchByLine met

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

[serguei.spit...@oracle.com]

Hi Igor,

This looks good to me.
A couple of questions below.


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html

- * @modules jdk.jdi
  *  @library /test/lib
+ * @modules java.management
+ * jdk.jdiShould the jdk.jdi be removed from this com/sun/jdi test?


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html

  *  @modules java.corba
  *   jdk.jdi

  Should the jdk.jdi be removed from this com/sun/jdi test?


Thanks,
Serguei


On 3/15/17 11:16, Igor Ignatyev wrote:

Shura,

Thank you for your review. I have update 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked 
that there are no similar things in other TEST.properties files.

Still looking for a review from a Reviewer.

[1]

--- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties Mon 
Mar 13 18:54:58 2017 -0700
+++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties Wed 
Mar 15 11:09:06 2017 -0700
@@ -1,3 +1,2 @@
-modules = java.management \
-  java.logging
+modules = java.logging


Thanks,
— Igor


On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline 
 wrote:

Igor,

I have looked through a bunch of tests where @modules is changed - that looks 
good.

One minor thing I noticed is that, in 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are 
explicitly calling out java.management. You do not have to do that because 
“modules” property is concatenated through the hierarchy.  java.management is 
already listed in test/java/lang/management/TEST.properties.

Shura


On Mar 13, 2017, at 9:03 PM, Igor Ignatyev  wrote:

Shura,

Thank you for review, I agree that having separate bugs is more convenient. [1] 
is new webrev w/ changes only in the files w/ incorrect module dependency 
declarations.

[1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html

Thanks,
— Igor


On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline 
 wrote:

Igor,

I have reviewed some number tests which change the @modules - they are fine.

You, however, fix more things with this than missing module dependency 
declaration. There is a redesign of line-number-sensitive tests [1] and other 
multiple improvements such as in [2]. Would it be more convenient to have that 
as separate bugs?

Shura

[1] 
http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
[2] 
http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html


On Mar 7, 2017, at 1:07 PM, Igor Ignatyev  wrote:

http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html

2586 lines changed: 669 ins; 484 del; 1433 mod;

Hi all,

Could you please review this changeset which fix @modules dependency 
declaration in jdk_svc tests?
there are a couple issues w/ modules in jdk_svc tests:
- some tests do not specify modules which they depend on
- modules in TEST.properties is not used in cases there all tests (should) have 
the same @modules directive
- @modules directive isn't placed according to current convention (before the 
1st run directive)

Since this fix has already touched lots of tests, I have decided to use this 
opportunity and reordered some of jtreg tags as well, so there won’t be two 
massive updates in the tests.

Some of our tests are line number sensitive, and then I fixed jtreg 
declaration, they started to fail. It was really hard to find our all line 
number sensitive tests, so I have unified the way we declare that as a part of 
this fix. Please let me know if you prefer to have it done separately.

There are two one-liners which, I hope, can simplify review:
[1] shows only the changes which are not in comments. Besides obvious new added 
TEST.properties, there are changes in the following line number sensitive tests 
(which I mentioned before):
test/com/sun/jdi/ArgumentValuesTest.java
test/com/sun/jdi/BreakpointTest.java
test/com/sun/jdi/FetchLocals.java
test/com/sun/jdi/GetLocalVariables.java
test/com/sun/jdi/GetSetLocalTest.java
test/com/sun/jdi/LambdaBreakpointTest.java
test/com/sun/jdi/LineNumberOnBraceTest.java
test/com/sun/jdi/PopAndStepTest.java

[2] shows changes in jtreg tags, it can help to see that almost all changes in 
jtreg tags are either moving of tags which does not affect execution order or 
@modules changes.

webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
bug: https://bugs.openjdk.java.net/browse/JDK-8176176
Testing:
- jdk_svc on linux, windows, mac
- checked that all tests which could be executed with full jdk before still can 
be executed with full jdk

Thanks,
— Igor

[1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
[2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

[serguei.spit...@oracle.com]

Igor,

It was my guess but it is nice to have it explicitly explained.

Thanks!
Serguei



On 3/15/17 22:07, Igor Ignatyev wrote:

Hi Serguei,

1s of all, thank you for your review.

since these tests have dependencies on more modules than corresponding 
TEST.properties file declares, we have to specify @modules tag in them, and 
because @modules in a test overrides modules from TEST.properties, jdk.jdi 
module should be mentioned in the tests.

— Igor
  

On Mar 15, 2017, at 9:53 PM, serguei.spit...@oracle.com wrote:

Hi Igor,

This looks good to me.
A couple of questions below.


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html

- * @modules jdk.jdi
  *  @library /test/lib
+ * @modules java.management
+ * jdk.jdiShould the jdk.jdi be removed from this com/sun/jdi test?


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html

  *  @modules java.corba
  *   jdk.jdi

  Should the jdk.jdi be removed from this com/sun/jdi test?


Thanks,
Serguei


On 3/15/17 11:16, Igor Ignatyev wrote:

Shura,

Thank you for your review. I have update 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked 
that there are no similar things in other TEST.properties files.

Still looking for a review from a Reviewer.

[1]

--- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties Mon 
Mar 13 18:54:58 2017 -0700
+++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties Wed 
Mar 15 11:09:06 2017 -0700
@@ -1,3 +1,2 @@
-modules = java.management \
-  java.logging
+modules = java.logging

Thanks,
— Igor


On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline 
 wrote:

Igor,

I have looked through a bunch of tests where @modules is changed - that looks 
good.

One minor thing I noticed is that, in 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are 
explicitly calling out java.management. You do not have to do that because 
“modules” property is concatenated through the hierarchy.  java.management is 
already listed in test/java/lang/management/TEST.properties.

Shura


On Mar 13, 2017, at 9:03 PM, Igor Ignatyev  wrote:

Shura,

Thank you for review, I agree that having separate bugs is more convenient. [1] 
is new webrev w/ changes only in the files w/ incorrect module dependency 
declarations.

[1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html

Thanks,
— Igor


On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline 
 wrote:

Igor,

I have reviewed some number tests which change the @modules - they are fine.

You, however, fix more things with this than missing module dependency 
declaration. There is a redesign of line-number-sensitive tests [1] and other 
multiple improvements such as in [2]. Would it be more convenient to have that 
as separate bugs?

Shura

[1] 
http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
[2] 
http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html


On Mar 7, 2017, at 1:07 PM, Igor Ignatyev  wrote:

http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html

2586 lines changed: 669 ins; 484 del; 1433 mod;

Hi all,

Could you please review this changeset which fix @modules dependency 
declaration in jdk_svc tests?
there are a couple issues w/ modules in jdk_svc tests:
- some tests do not specify modules which they depend on
- modules in TEST.properties is not used in cases there all tests (should) have 
the same @modules directive
- @modules directive isn't placed according to current convention (before the 
1st run directive)

Since this fix has already touched lots of tests, I have decided to use this 
opportunity and reordered some of jtreg tags as well, so there won’t be two 
massive updates in the tests.

Some of our tests are line number sensitive, and then I fixed jtreg 
declaration, they started to fail. It was really hard to find our all line 
number sensitive tests, so I have unified the way we declare that as a part of 
this fix. Please let me know if you prefer to have it done separately.

There are two one-liners which, I hope, can simplify review:
[1] shows only the changes which are not in comments. Besides obvious new added 
TEST.properties, there are changes in the following line number sensitive tests 
(which I mentioned before):
test/com/sun/jdi/ArgumentValuesTest.java
test/com/sun/jdi/BreakpointTest.java
test/com/sun/jdi/FetchLocals.java
test/com/sun/jdi/GetLocalVariables.java
test/com/sun/jdi/GetSetLocalTest.java
test/com/sun/jdi/LambdaBreakpointTest.java
test/com/sun/jdi/LineNumberOnBraceTest.java
test/com/sun/jdi/PopAndStepTest.java

[2] shows changes in jtreg tags, it can help to see that almost all changes in 
jtreg tags are either moving of tags which does not affect execution order or 
@modules changes.

webrev: http://cr.openjdk.jav

Re: JDK 9 RFR of JDK-8177638: com/sun/jarsigner, jdk/internal/loader (and more) are missed in TEST.group

[serguei.spit...@oracle.com]

Hi Amy,

It looks good to me.

Thanks,
Serguei


On 3/30/17 22:42, Amy Lu wrote:

I'm still waiting for reviewer's feedback for the TEST.groups update:

jdk/internal/loader (add to jdk_lang)
jdk/internal/util   (add to jdk_util_other)
jdk/internal/agent  (add to jdk_management)

(Security part has been reviewed. Thank you Max!)

Thanks,
Amy

On 3/27/17 11:06 AM, Amy Lu wrote:

jdk/internal/loader
jdk/internal/util
com/sun/jarsigner
jdk/internal/agent

Somehow these are missed in TEST.group.

Please review this patch to update TEST.group

Tested on all platforms, TestVersionedStream.java fails on Windows, 
put it in ProblemList.txt for now.


bug: https://bugs.openjdk.java.net/browse/JDK-8177638
webrev: http://cr.openjdk.java.net/~amlu/8177638/webrev.00/

Thanks,
Amy


--- old/test/ProblemList.txt2017-03-27 10:57:36.0 +0800
+++ new/test/ProblemList.txt2017-03-27 10:57:36.0 +0800
@@ -286,6 +286,7 @@

 java/util/BitSet/BitSetStreamTest.java 8079538 generic-all

+jdk/internal/util/jar/TestVersionedStream.java 8177640 windows-all

  



--- old/test/TEST.groups2017-03-27 10:57:38.0 +0800
+++ new/test/TEST.groups2017-03-27 10:57:38.0 +0800
@@ -70,6 +70,7 @@
 sun/reflect \
 jdk/internal/reflect \
 jdk/lambda \
+jdk/internal/loader \
 jdk/internal/misc \
 jdk/internal/ref \
 jdk/internal/jimage \
@@ -87,6 +88,7 @@
 jdk_util_other = \
 java/util \
 sun/util \
+jdk/internal/util \
 -:jdk_collections \
 -:jdk_concurrent \
 -:jdk_stream
@@ -189,6 +191,7 @@
 lib/security

 jdk_security4 = \
+com/sun/jarsigner \
 com/sun/security/jgss \
 javax/security/auth/kerberos \
 sun/security/krb5 \
@@ -207,7 +210,8 @@
 jdk_management = \
 java/lang/management \
 com/sun/management \
-sun/management
+sun/management \
+jdk/internal/agent

 jdk_instrument = \
 java/lang/instrument

Re: RFR: JDK-8179538: Update jdk.jdi to be HTML-5 friendly

[serguei.spit...@oracle.com]

Hi Kumar,

It looks fine to me too.

Thanks,
Serguei


On 5/2/17 16:23, Mandy Chung wrote:

Looks fine to me.

Mandy


On May 2, 2017, at 12:40 PM, Kumar Srinivasan  
wrote:

Hello,

Please review changes to make jdk.jdi HTML5 friendly,
table cell padding has not been addressed and will be fixed
separately, this takes care of others.

Note: Some of the errors were due to incorrect@throws but
in all cases there is the correct one further down, please use
sdiffs in these cases.

@throws {@link InvalidTypeException} if any

before:jdk.jdi {error=42, warning=1}
after: jdk.jdi {error=1}

http://cr.openjdk.java.net/~ksrini/8179538/webrev.00/index.html
https://bugs.openjdk.java.net/browse/JDK-8179538

Thanks
Kumar

Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent

[serguei.spit...@oracle.com]

Hi Matthias,

The fix looks good to me.
It must be tested at least with the JTreg com/sun/jdi tests.
Do you need a sponsor?

Thanks,
Serguei



On 5/16/17 03:21, Baesken, Matthias wrote:

  Sure, I forward it to  serviceability-dev .

-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Dienstag, 16. Mai 2017 11:51
To: Baesken, Matthias ; core-libs-dev@openjdk.java.net
Cc: Simonis, Volker 
Subject: Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent



On 16/05/2017 10:04, Baesken, Matthias wrote:

Hello, could you please review this small change :

http://cr.openjdk.java.net/~mbaesken/webrevs/8180413/

it fixes a number of places in   jdk.jdwp.agent   where in case of an error it 
is attempted to write to NULL .

Bug :  JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent


https://bugs.openjdk.java.net/browse/JDK-8180413



Can you bring this to serviceability-dev as that is the mailing list
where the JDWP agent is maintained?

-Alan

Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent

[serguei.spit...@oracle.com]

Hello Matthias,

It is great, you are able to push this fix!

Thanks,
Serguei



On 5/22/17 01:49, Baesken, Matthias wrote:

Hello Serguei,  the  com/sun/jdi  tests were executed ,  same number of  passed 
and errors in a patched and unpatched JDK .
We do not need a sponsor because  the change is in JDK not HS .

Best regards, Matthias


-Original Message-
From: serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com]
Sent: Freitag, 19. Mai 2017 02:18
To: Baesken, Matthias ; Alan Bateman 
; core-libs-dev@openjdk.java.net; 
serviceability-...@openjdk.java.net
Cc: Simonis, Volker 
Subject: Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent

Hi Matthias,

The fix looks good to me.
It must be tested at least with the JTreg com/sun/jdi tests.
Do you need a sponsor?

Thanks,
Serguei



On 5/16/17 03:21, Baesken, Matthias wrote:

   Sure, I forward it to  serviceability-dev .

-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Dienstag, 16. Mai 2017 11:51
To: Baesken, Matthias ; core-libs-dev@openjdk.java.net
Cc: Simonis, Volker 
Subject: Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent



On 16/05/2017 10:04, Baesken, Matthias wrote:

Hello, could you please review this small change :

http://cr.openjdk.java.net/~mbaesken/webrevs/8180413/

it fixes a number of places in   jdk.jdwp.agent   where in case of an error it 
is attempted to write to NULL .

Bug :  JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent


https://bugs.openjdk.java.net/browse/JDK-8180413



Can you bring this to serviceability-dev as that is the mailing list
where the JDWP agent is maintained?

-Alan

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

[serguei.spit...@oracle.com]

Hi Staffan,

Thank you for this discovery!
It looks good, but I have a couple of comments.

It seems, there is one more problem in this code:

  68 /* check for NULL path */
  69 if (p == pathname) {
  70 continue;<== Endless loop if we hit this line
  71 }


Do we need to do 'pathname++' before continuing at the line #70?
It is going to be endless loop in cases there is a PATH_SEPARATOR
at the beginning of paths or two PATH_SEPARATOR's in a row.
These would be incorrect path lists but the code above is targeting 
exactly such cases.


Also, the argument name "pathname" in the original code is confusing.
Should we rename it to "pathnames"?

Thanks,
Serguei


On 3/4/13 10:56 AM, Staffan Larsen wrote:

I accidentally stepped on this bug the other day. There is a problem in 
linker_md.c : dll_build_name() where an internal pointer can be moved to point 
outside the string. The code looks like:

   57 static void dll_build_name(char* buffer, size_t buflen,
   58const char* pname, const char* fname) {
   59 // Based on os_solaris.cpp
   60
   61 char *path_sep = PATH_SEPARATOR;
   62 char *pathname = (char *)pname;
   63 while (strlen(pathname) > 0) {
   64 char *p = strchr(pathname, *path_sep);
   65 if (p == NULL) {
   66 p = pathname + strlen(pathname);
   67 }
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;
   71 }
   72 (void)snprintf(buffer, buflen, "%.*s/lib%s." LIB_SUFFIX, (p - 
pathname),
   73pathname, fname);
   74
   75 if (access(buffer, F_OK) == 0) {
   76 break;
   77 }
   78 pathname = p + 1;
   79 *buffer = '\0';
   80 }

If the supplied pname is a buffer with a simple path without any path 
separators in it, p will be set to the terminating nul on line 66. Then on line 
78 it will be moved outside the buffer. Fixing this also necessitates fixes to 
the callers to check for an empty return string (in buffer).

The same code show up in both the solaris code and the windows code as well as 
hprof.

bug: http://bugs.sun.com/view_bug.do?bug_id=8009397
webrev: http://cr.openjdk.java.net/~sla/8009397/webrev.00/

Thanks,
/Staffan

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

[serguei.spit...@oracle.com]

Staffan,

Thank you for the confirmation and taking care about this issue!

Thanks,
Serguei


On 3/6/13 11:36 AM, Staffan Larsen wrote:

Nice catch, Serguei!

Unfortunately I have already pushed my change, but I filed 
"JDK-8009558: linked_md.c::dll_build_name can get stuck in an infinite 
loop" to track this new problem and I am working on a fix.


Thanks,
/Staffan


On 5 mar 2013, at 20:26, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:



Hi Staffan,

Thank you for this discovery!
It looks good, but I have a couple of comments.

It seems, there is one more problem in this code:
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;<== Endless loop if we hit this line
   71 }

Do we need to do 'pathname++' before continuing at the line #70?
It is going to be endless loop in cases there is a PATH_SEPARATOR
at the beginning of paths or two PATH_SEPARATOR's in a row.
These would be incorrect path lists but the code above is targeting 
exactly such cases.


Also, the argument name "pathname" in the original code is confusing.
Should we rename it to "pathnames"?

Thanks,
Serguei


On 3/4/13 10:56 AM, Staffan Larsen wrote:

I accidentally stepped on this bug the other day. There is a problem in 
linker_md.c : dll_build_name() where an internal pointer can be moved to point 
outside the string. The code looks like:

   57 static void dll_build_name(char* buffer, size_t buflen,
   58const char* pname, const char* fname) {
   59 // Based on os_solaris.cpp
   60
   61 char *path_sep = PATH_SEPARATOR;
   62 char *pathname = (char *)pname;
   63 while (strlen(pathname) > 0) {
   64 char *p = strchr(pathname, *path_sep);
   65 if (p == NULL) {
   66 p = pathname + strlen(pathname);
   67 }
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;
   71 }
   72 (void)snprintf(buffer, buflen, "%.*s/lib%s." LIB_SUFFIX, (p - 
pathname),
   73pathname, fname);
   74
   75 if (access(buffer, F_OK) == 0) {
   76 break;
   77 }
   78 pathname = p + 1;
   79 *buffer = '\0';
   80 }

If the supplied pname is a buffer with a simple path without any path 
separators in it, p will be set to the terminating nul on line 66. Then on line 
78 it will be moved outside the buffer. Fixing this also necessitates fixes to 
the callers to check for an empty return string (in buffer).

The same code show up in both the solaris code and the windows code as well as 
hprof.

bug:http://bugs.sun.com/view_bug.do?bug_id=8009397
webrev:http://cr.openjdk.java.net/~sla/8009397/webrev.00/

Thanks,
/Staffan

Re: RFR(S): 8006637 Failure to filter out native frame events on Solaris

[serguei.spit...@oracle.com]

Looks good.

Thanks,
Serguei

On 3/7/13 12:10 AM, Staffan Larsen wrote:

Adding core-libs-dev and re-asking for a review.

Thanks,
/Staffan

On 27 feb 2013, at 15:59, Staffan Larsen  wrote:


Please review the following test fix.

webrev: http://cr.openjdk.java.net/~sla/8006637/webrev.00/
bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006637

The problem is that the test assumes that no other threads call String.intern() 
while the tests is running. This is normally ok, but when running with JFR 
there are other threads calling String.intern() in the background and the test 
fails. The solution is to add a thread filter to the method exit requests so 
that only calls to String.inter() in the main thread are reported.

Thanks,
/Staffan

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

[serguei.spit...@oracle.com]

Nice fix!
This is more clear and robust.
Extra memory allocation is Ok.

Thanks,
Serguei


On 3/7/13 3:54 AM, Staffan Larsen wrote:
Here is a webrev for fixing this problem. I actually removed all of 
our own tokenization code in dll_build_name() and replaced it with 
calls to strtok_r (strtok_s on windows) instead. I think this should 
be more robust, at the cost of an extra memory allocation. I've also 
added the const qualifier to some of the parameters.


http://cr.openjdk.java.net/~sla/8009558/webrev.02/ 
<http://cr.openjdk.java.net/%7Esla/8009558/webrev.02/>


All of the jdi and hprof tests passes with this change.

Thanks,
/Staffan

On 6 mar 2013, at 20:48, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:



Staffan,

Thank you for the confirmation and taking care about this issue!

Thanks,
Serguei


On 3/6/13 11:36 AM, Staffan Larsen wrote:

Nice catch, Serguei!

Unfortunately I have already pushed my change, but I filed 
"JDK-8009558: linked_md.c::dll_build_name can get stuck in an 
infinite loop" to track this new problem and I am working on a fix.


Thanks,
/Staffan


On 5 mar 2013, at 20:26, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:



Hi Staffan,

Thank you for this discovery!
It looks good, but I have a couple of comments.

It seems, there is one more problem in this code:
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;<== Endless loop if we hit this line
   71 }

Do we need to do 'pathname++' before continuing at the line #70?
It is going to be endless loop in cases there is a PATH_SEPARATOR
at the beginning of paths or two PATH_SEPARATOR's in a row.
These would be incorrect path lists but the code above is targeting 
exactly such cases.


Also, the argument name "pathname" in the original code is confusing.
Should we rename it to "pathnames"?

Thanks,
Serguei


On 3/4/13 10:56 AM, Staffan Larsen wrote:

I accidentally stepped on this bug the other day. There is a problem in 
linker_md.c : dll_build_name() where an internal pointer can be moved to point 
outside the string. The code looks like:

   57 static void dll_build_name(char* buffer, size_t buflen,
   58const char* pname, const char* fname) {
   59 // Based on os_solaris.cpp
   60
   61 char *path_sep = PATH_SEPARATOR;
   62 char *pathname = (char *)pname;
   63 while (strlen(pathname) > 0) {
   64 char *p = strchr(pathname, *path_sep);
   65 if (p == NULL) {
   66 p = pathname + strlen(pathname);
   67 }
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;
   71 }
   72 (void)snprintf(buffer, buflen, "%.*s/lib%s." LIB_SUFFIX, (p - 
pathname),
   73pathname, fname);
   74
   75 if (access(buffer, F_OK) == 0) {
   76 break;
   77 }
   78 pathname = p + 1;
   79 *buffer = '\0';
   80 }

If the supplied pname is a buffer with a simple path without any path 
separators in it, p will be set to the terminating nul on line 66. Then on line 
78 it will be moved outside the buffer. Fixing this also necessitates fixes to 
the callers to check for an empty return string (in buffer).

The same code show up in both the solaris code and the windows code as well as 
hprof.

bug:http://bugs.sun.com/view_bug.do?bug_id=8009397
webrev:http://cr.openjdk.java.net/~sla/8009397/webrev.00/

Thanks,
/Staffan

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

[serguei.spit...@oracle.com]

I already reviewed this, and it is a good fix.
Please, see my email attached.

Thanks,
Serguei

On 3/18/13 7:13 AM, Staffan Larsen wrote:

Can I get an official Review of this change?

Thanks,
/Staffan

On 7 mar 2013, at 12:54, Staffan Larsen <mailto:staffan.lar...@oracle.com>> wrote:


Here is a webrev for fixing this problem. I actually removed all of 
our own tokenization code in dll_build_name() and replaced it with 
calls to strtok_r (strtok_s on windows) instead. I think this should 
be more robust, at the cost of an extra memory allocation. I've also 
added the const qualifier to some of the parameters.


http://cr.openjdk.java.net/~sla/8009558/webrev.02/ 
<http://cr.openjdk.java.net/%7Esla/8009558/webrev.02/>


All of the jdi and hprof tests passes with this change.

Thanks,
/Staffan

On 6 mar 2013, at 20:48, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:



Staffan,

Thank you for the confirmation and taking care about this issue!

Thanks,
Serguei


On 3/6/13 11:36 AM, Staffan Larsen wrote:

Nice catch, Serguei!

Unfortunately I have already pushed my change, but I filed 
"JDK-8009558: linked_md.c::dll_build_name can get stuck in an 
infinite loop" to track this new problem and I am working on a fix.


Thanks,
/Staffan


On 5 mar 2013, at 20:26, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:



Hi Staffan,

Thank you for this discovery!
It looks good, but I have a couple of comments.

It seems, there is one more problem in this code:
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;<== Endless loop if we hit this line
   71 }

Do we need to do 'pathname++' before continuing at the line #70?
It is going to be endless loop in cases there is a PATH_SEPARATOR
at the beginning of paths or two PATH_SEPARATOR's in a row.
These would be incorrect path lists but the code above is 
targeting exactly such cases.


Also, the argument name "pathname" in the original code is confusing.
Should we rename it to "pathnames"?

Thanks,
Serguei


On 3/4/13 10:56 AM, Staffan Larsen wrote:

I accidentally stepped on this bug the other day. There is a problem in 
linker_md.c : dll_build_name() where an internal pointer can be moved to point 
outside the string. The code looks like:

   57 static void dll_build_name(char* buffer, size_t buflen,
   58const char* pname, const char* fname) {
   59 // Based on os_solaris.cpp
   60
   61 char *path_sep = PATH_SEPARATOR;
   62 char *pathname = (char *)pname;
   63 while (strlen(pathname) > 0) {
   64 char *p = strchr(pathname, *path_sep);
   65 if (p == NULL) {
   66 p = pathname + strlen(pathname);
   67 }
   68 /* check for NULL path */
   69 if (p == pathname) {
   70 continue;
   71 }
   72 (void)snprintf(buffer, buflen, "%.*s/lib%s." LIB_SUFFIX, (p - 
pathname),
   73pathname, fname);
   74
   75 if (access(buffer, F_OK) == 0) {
   76 break;
   77 }
   78 pathname = p + 1;
   79 *buffer = '\0';
   80 }

If the supplied pname is a buffer with a simple path without any path 
separators in it, p will be set to the terminating nul on line 66. Then on line 
78 it will be moved outside the buffer. Fixing this also necessitates fixes to 
the callers to check for an empty return string (in buffer).

The same code show up in both the solaris code and the windows code as well as 
hprof.

bug:http://bugs.sun.com/view_bug.do?bug_id=8009397
webrev:http://cr.openjdk.java.net/~sla/8009397/webrev.00/

Thanks,
/Staffan












--- Begin Message ---

My vpn was disconnected when I sent this.
Resending to make sure you've got it.

Thanks,
Serguei

On 3/7/13 9:39 AM, serguei.spit...@oracle.com wrote:

Nice fix!
This is more clear and robust.
Extra memory allocation is Ok.

Thanks,
Serguei


On 3/7/13 3:54 AM, Staffan Larsen wrote:
Here is a webrev for fixing this problem. I actually removed all of 
our own tokenization code in dll_build_name() and replaced it with 
calls to strtok_r (strtok_s on windows) instead. I think this should 
be more robust, at the cost of an extra memory allocation. I've also 
added the const qualifier to some of the parameters.


http://cr.openjdk.java.net/~sla/8009558/webrev.02/ 
<http://cr.openjdk.java.net/%7Esla/8009558/webrev.02/>


All of the jdi and hprof tests passes with this change.

Thanks,
/Staffan

On 6 mar 2013, at 20:48, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:



Staffan,

Thank you for the confirmation and taking care about this issue!

Thanks,
Serguei


On 3/6/13 11:36 AM, Staffan Larsen wrote:

Nice catch, Serguei!

Unfortunately I have already pushed my change, but I filed 
"JDK-8009

Re: RFR(S): 8006637 Failure to filter out native frame events on Solaris

[serguei.spit...@oracle.com]

It is just a remainder that I reviewed this as well (the fix is good too).
My email is attached.

Thanks,
Serguei


On 3/18/13 7:14 AM, Staffan Larsen wrote:

I still need an official Review for this change.

Thanks,
Staffan

On 7 mar 2013, at 09:10, Staffan Larsen  wrote:


Adding core-libs-dev and re-asking for a review.

Thanks,
/Staffan

On 27 feb 2013, at 15:59, Staffan Larsen  wrote:


Please review the following test fix.

webrev: http://cr.openjdk.java.net/~sla/8006637/webrev.00/
bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006637

The problem is that the test assumes that no other threads call String.intern() 
while the tests is running. This is normally ok, but when running with JFR 
there are other threads calling String.intern() in the background and the test 
fails. The solution is to add a thread filter to the method exit requests so 
that only calls to String.inter() in the main thread are reported.

Thanks,
/Staffan


--- Begin Message ---

Looks good.

Thanks,
Serguei

On 3/7/13 12:10 AM, Staffan Larsen wrote:

Adding core-libs-dev and re-asking for a review.

Thanks,
/Staffan

On 27 feb 2013, at 15:59, Staffan Larsen  wrote:


Please review the following test fix.

webrev: http://cr.openjdk.java.net/~sla/8006637/webrev.00/
bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006637

The problem is that the test assumes that no other threads call String.intern() 
while the tests is running. This is normally ok, but when running with JFR 
there are other threads calling String.intern() in the background and the test 
fails. The solution is to add a thread filter to the method exit requests so 
that only calls to String.inter() in the main thread are reported.

Thanks,
/Staffan


--- End Message ---

Re: RFR: 8009681: TEST_BUG: MethodExitReturnValuesTest.java fails with when there are unexpected background threads

[serguei.spit...@oracle.com]

Hi Mikael,

It looks good.
Thank you for figuring out how to make it more stable!

BTW, the webrev frames mode does not work.

Thanks,
Serguei

On 4/17/13 6:03 AM, Mikael Auno wrote:
Hi, I'd like some reviews on 
http://cr.openjdk.java.net/~nloodin/8009681/webrev.01/ for JDK-8009681 
(http://bugs.sun.com/view_bug.do?bug_id=8009681).


The issue here is that when MethodExitReturnValuesTest hooks into 
MethodExit events through JDI it uses an exclude list to filter out 
classes from which it is not interested in these events. This is bound 
to break over and over again as new features are added to the JDK. 
I've changed the test to use an include list instead, containing only 
the handful of classes the test is actually interested in.


Thanks,
Mikael

Re: RFR - Changes to address CCC 8014135: Support for statically linked agents

[serguei.spit...@oracle.com]

Hi Bill,

I've already had a chance to read your webrev several weeks ago, but 
need more time.


Thanks,
Serguei

On 7/3/13 2:17 PM, BILL PITTORE wrote:
These changes address bug 8014135 which adds support for statically 
linked agents in the VM. This is a followup to the recent JNI spec 
changes that addressed statically linked JNI libraries( 8005716).

The JEP for this change is the same JEP as the JNI changes:
http://openjdk.java.net/jeps/178

Webrevs are here:

http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/

The changes to jvmti.xml can also be seen in the output file that I 
placed here:

http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html

Thanks,
bill

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[serguei.spit...@oracle.com]

Coleen,

The fix looks good.
It is nice you've come up with this.

Thanks,
Serguei

On 10/3/13 11:02 AM, Coleen Phillimore wrote:
Summary: Redefined class in stack trace may not be found by 
method_idnum so handle null.


This is a simple change.  I had another change to save the method name 
(as u2) in the backtrace, but it's not worth the extra footprint in 
backtraces for this rare case.


The root problem was that we save method_idnum in the backtrace (u2) 
instead of Method* to avoid Method* from being redefined and 
deallocated.  I made a change to InstanceKlass::method_from_idnum() to 
return null rather than the last method in the list, which causes this 
crash.   Dan and I went down the long rabbit-hole of why method_idnum 
is changed for obsolete methods and we think there's some cleanup and 
potential bugs in this area.  But this is not that change.  I'll file 
another bug to continue this investigation for jdk9 (or 8uN).


Staffan created a test - am including core-libs for the review 
request.  Also tested with all of the vm testbase tests, mlvm tests, 
and java/lang/instrument tests.


open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

Thanks,
Coleen