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

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 >

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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) {  

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

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

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

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

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,

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

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:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent

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 <matthias.baes...@sap.com>; Alan Bateman <alan.bate...@oracle.com>; core-libs-dev@open

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

Re: RFR: JDK-8179538: Update jdk.jdi to be HTML-5 friendly

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

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

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

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

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

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

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

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

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:

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

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

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

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

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

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

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

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

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

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

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

://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

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

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

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

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

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

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

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

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.

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

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

Re: RFR - Changes to address CCC 8014135: Support for statically linked agents

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

Re: RFR: 8009681: TEST_BUG: MethodExitReturnValuesTest.java fails with when there are unexpected background threads

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

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

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

Re: RFR(S): 8006637 Failure to filter out native frame events on Solaris

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 staffan.lar...@oracle.com

Re: RFR(S): 8006637 Failure to filter out native frame events on Solaris

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 staffan.lar...@oracle.com wrote: Please review the following test fix. webrev:

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

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

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

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

Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket

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

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

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

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

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

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

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

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

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.