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

2020-04-17 Thread 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 the

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

2020-04-28 Thread 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

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

2020-05-19 Thread 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 rel

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

2020-05-19 Thread 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

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

2020-05-19 Thread 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 ||

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

2020-06-17 Thread 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 >

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

2019-06-21 Thread 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

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

2019-09-11 Thread 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

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

2019-10-29 Thread 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.ne

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

2019-11-19 Thread 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, S

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

2019-11-20 Thread serguei.spit...@oracle.com
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: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

2020-03-29 Thread 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 redefi

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

2020-03-30 Thread 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) {  3

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

2020-03-30 Thread 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

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

2020-04-06 Thread 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

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

2012-01-03 Thread 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. T

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

2012-08-23 Thread 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

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

2012-08-23 Thread 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 expos

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

2012-08-23 Thread 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

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

2015-11-20 Thread 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 (MemberNameInS

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

2015-11-20 Thread 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/hotspo

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

2015-11-20 Thread 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/sr

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

2015-11-24 Thread 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/StackStreamFacto

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

2014-11-19 Thread 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 scri

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

2014-12-02 Thread 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

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

2014-12-04 Thread 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

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

2015-06-03 Thread 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 fold

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

2015-06-03 Thread 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 th

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

2015-06-03 Thread serguei.spit...@oracle.com
ad: 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

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

2015-06-04 Thread 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 chan

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

2019-03-21 Thread 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 ergonom

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

2016-05-19 Thread 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

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

2017-12-07 Thread 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/

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

2017-12-07 Thread 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

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

2017-12-20 Thread 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 required

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

2018-09-07 Thread 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 revie

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

2017-03-15 Thread 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

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

2017-03-15 Thread serguei.spit...@oracle.com
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: JDK 9 RFR of JDK-8177638: com/sun/jarsigner, jdk/internal/loader (and more) are missed in TEST.group

2017-03-31 Thread 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

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

2017-05-02 Thread 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 s

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

2017-05-18 Thread 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

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

2017-05-22 Thread serguei.spit...@oracle.com
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

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

2013-03-05 Thread 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

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

2013-03-06 Thread serguei.spit...@oracle.com
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

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

2013-03-07 Thread 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: htt

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

2013-03-07 Thread serguei.spit...@oracle.com
i 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 w

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

2013-03-18 Thread serguei.spit...@oracle.com
t 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:ser

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

2013-03-18 Thread 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 a

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

2013-04-17 Thread 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.

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

2013-07-04 Thread 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

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

2013-10-03 Thread 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