Re: RFR: 8325055: Rename Injector.h [v2]
On Wed, 31 Jan 2024 15:15:16 GMT, Kim Barrett wrote: >> Please review this trivial change that renames the file >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/Injector.h to Injector.hpp. >> >> Testing: mach5 tier1 > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > fix name in README Seems fine. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17656#pullrequestreview-1855696820
Re: RFR: 8324845: management.properties text "interface name" is misleading [v2]
On Wed, 31 Jan 2024 21:29:14 GMT, Kevin Walls wrote: >> We have the text "host-or-interface-name" describing the >> com.sun.management.jmxremote.host property in management.properties, but >> interface names (like "eth0" or "lo", or "ens3"...) are not what it accepts. >> It should just say it needs to be a host name or address. >> >> This change only affects comment text in a properties file. >> >> (We don't currently mention this property in other docs, e.g. in the M >> guide.) > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > text update Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17615#pullrequestreview-1855667712
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Thu, 1 Feb 2024 05:42:07 GMT, David Holmes wrote: >> Hello David, I had actually first put it in that line you noted, but that >> then lead to a compilation error: >> >> >> In file included from >> /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33: >> /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error: >> unknown type name 'FILE' >> void print_message(FILE *fp, const char *prefix, const char *suffix, >>^ >> /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: >> a parameter list without types is only allowed in a function definition >> const char * jvmtiErrorText(jvmtiError); >> ^ >> /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: >> a parameter list without types is only allowed in a function definition >> const char * jdwpErrorText(jdwpError); >>^ >> 3 errors generated. >> >> >> >> I think the reason for those compilation errors is that `error_messages.h` >> and a few other header files are not explicitly listing their necessary >> includes and are relying on transitive includes. So I decided to take the >> easy route out, by adding the `error_messages.h` after the `util.h` to rely >> on the transitive includes to get past that compilation error. >> >> I think to properly fix that compile error, this following change (which I >> tested locally and works) would be needed: >> >> >> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> index 92a9f457e8a..2683c6791a1 100644 >> --- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> @@ -26,6 +26,8 @@ >> #ifndef JDWP_JDWP_H >> #define JDWP_JDWP_H >> >> +#include "jni_md.h" >> +#include "jvmti.h" >> #include "JDWPCommands.h" >> >> /* >> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> index 4126b76f226..810ab384f1a 100644 >> --- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> @@ -26,6 +26,9 @@ >> #ifndef JDWP_ERROR_MESSAGES_H >> #define JDWP_ERROR_MESSAGES_H >> >> +#include >> +#include "JDWP.h" >> + >> /* It is assumed that ALL strings are UTF-8 safe on entry */ >> #define TTY_MESSAGE(args) ( tty_message args ) >> #define ERROR_MESSAGE(args) ( \ >> diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c >> b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c >> index b43d4de80... > > Please file a JBS issue, but no need for you to address it. Thanks. I've created https://bugs.openjdk.org/browse/JDK-8325094 to track this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1473868787
Integrated: 8324668: JDWP process management needs more efficient file descriptor handling
On Fri, 26 Jan 2024 14:52:49 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address > https://bugs.openjdk.org/browse/JDK-8324668? > > This change proposes to fix the issue in jdwp where when launching a child > process (for the `launch=` option), it iterates over an extremely large > number of file descriptors to close each one of those. Details about the > issue and the proposed fixed are added in a subsequent comment in this PR > https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, > tier2 and tier3 testing is currently in progress with this change. This pull request has now been integrated. Changeset: a6632487 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/a6632487863db5ff3136cdcc76b7440c15ce6be9 Stats: 121 lines in 1 file changed: 106 ins; 12 del; 3 mod 8324668: JDWP process management needs more efficient file descriptor handling Reviewed-by: gziemski, dholmes, cjplummer - PR: https://git.openjdk.org/jdk/pull/17588
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 07:52:18 GMT, Jaikiran Pai wrote: >> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 35: >> >>> 33: #include "sys.h" >>> 34: #include "util.h" >>> 35: #include "error_messages.h" >> >> Nit: to maintain include sort order this should have gone where >> `log_messages.h` was removed. > > Hello David, I had actually first put it in that line you noted, but that > then lead to a compilation error: > > > In file included from > /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33: > /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error: > unknown type name 'FILE' > void print_message(FILE *fp, const char *prefix, const char *suffix, >^ > /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: a > parameter list without types is only allowed in a function definition > const char * jvmtiErrorText(jvmtiError); > ^ > /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: a > parameter list without types is only allowed in a function definition > const char * jdwpErrorText(jdwpError); >^ > 3 errors generated. > > > > I think the reason for those compilation errors is that `error_messages.h` > and a few other header files are not explicitly listing their necessary > includes and are relying on transitive includes. So I decided to take the > easy route out, by adding the `error_messages.h` after the `util.h` to rely > on the transitive includes to get past that compilation error. > > I think to properly fix that compile error, this following change (which I > tested locally and works) would be needed: > > > diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > index 92a9f457e8a..2683c6791a1 100644 > --- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > +++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > @@ -26,6 +26,8 @@ > #ifndef JDWP_JDWP_H > #define JDWP_JDWP_H > > +#include "jni_md.h" > +#include "jvmti.h" > #include "JDWPCommands.h" > > /* > diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > index 4126b76f226..810ab384f1a 100644 > --- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > +++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > @@ -26,6 +26,9 @@ > #ifndef JDWP_ERROR_MESSAGES_H > #define JDWP_ERROR_MESSAGES_H > > +#include > +#include "JDWP.h" > + > /* It is assumed that ALL strings are UTF-8 safe on entry */ > #define TTY_MESSAGE(args) ( tty_message args ) > #define ERROR_MESSAGE(args) ( \ > diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c > b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c > index b43d4de80fc..cf0d00db192 100644 > --- a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c > +++ b/src/jdk.jdwp.agent... Please file a JBS issue, but no need for you to address it. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1473821677
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v7]
On Wed, 31 Jan 2024 08:01:15 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Minor - fix alignment Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1855525479
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 31 Jan 2024 18:57:23 GMT, Coleen Phillimore wrote: > This change uses a claim token to allocate multi dimensional arrays rather > than holding MultiArray_lock around metaspace allocation. We can't hold a > mutex around metaspace allocation because it can create an OOM object and it > can also call into JVMTI for a resource exhausted event. Also, we were > creating mirrors and more metadata arrays while holding this lock. See the > bug for more details and other ideas considered and rejected. > > Tested with tier1-7. Can't you just use a Monitor to implement the claim token, rather than this lock-free approach? (Similar to how class initialization is handled.) - PR Review: https://git.openjdk.org/jdk/pull/17660#pullrequestreview-1855472920
Re: RFR: 8313710: jcmd: typo in the documentation of JFR.start and JFR.dump [v3]
On Wed, 31 Jan 2024 06:41:44 GMT, Taizo Kurashige wrote: >> Changes requested by dholmes (Reviewer). > > @dholmes-ora @egahlin > > I'm sorry that my slow response has prolonged this issue. I created a subtask > and modified the source. If possible, please review them. Thanks @kurashige23 , the manpage subtask will be handled by someone from Oracle. I'll leave it to JFR folk to do the review here. - PR Comment: https://git.openjdk.org/jdk/pull/16413#issuecomment-1920524186
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Thu, 1 Feb 2024 04:13:52 GMT, Martin Doerr wrote: >> I didn't follow that. You mean i need to keep a check if it is null and >> print it out ? > > An assertion is only used for debug builds. Such an error should be handled > in product builds as well. I think an attempt to load an invalid library > should simply fail. You may add logging if needed. > @tstuefe: Do you agree or have another proposal to handle such errors? In addition, the nullptr check is important to avoid undefined behavior when passing `pointer_to_dot` to any string function. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1473774551
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Wed, 31 Jan 2024 15:06:54 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1176: >> >>> 1174: strncpy(file_path,filename, buffer_length + 1); >>> 1175: char* const pointer_to_dot = strrchr(file_path, '.'); >>> 1176: assert(pointer_to_dot != nullptr, "Attempting to load a shared >>> object without extension? %s", filename); >> >> This should not only be an assertion. I think the check could be used >> instead of the strcmp below. > > I didn't follow that. You mean i need to keep a check if it is null and print > it out ? An assertion is only used for debug builds. Such an error should be handled in product builds as well. I think an attempt to load an invalid library should simply fail. You may add logging if needed. @tstuefe: Do you agree or have another proposal to handle such errors? - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1473772905
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Thu, 1 Feb 2024 01:38:21 GMT, Alex Menkov wrote: >> The fix adds new test for FollowReferences JVMTI function to verify >> correctness of reported field indexes. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17580#pullrequestreview-1855244184
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Thu, 1 Feb 2024 00:17:57 GMT, Chris Plummer wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 93:
>
>> 91:
>> 92: /*
>> 93: Per jvmtiHeapReferenceInfoField spec:
>
> I think you should also include mention that this is for
> JVMTI_HEAP_REFERENCE_STATIC_FIELD.
>
> Indentation should be fixed to match our C++ comment style, and the "bullets"
> from the spec should be included to make it clearer that these are two lists
> of steps.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 108:
>
>> 106: where n is the count of the fields in all the superinterfaces of I.
>> 107:
>> 108: 'Klass' struct contains all required data to calculate field indeces.
>
> "indices"
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 153:
>
>> 151:
>> 152: /*
>> 153: For each test object 'Object' struct is created and pointer to it set
>> as a tag for jobject.
>
> Suggestion:
>
> For each test object, the 'Object' struct is created and a pointer to it is
> set as the jobject's tag.
Fixed.
Updated comment for 'Klass' class the same way
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 225:
>
>> 223:
>> 224: // Explores all interfaces implemented by 'klass' sorting out duplicates
>> 225: // and store them in the 'arr' starting from 'index'.
>
> Suggestion:
>
> // Explores all interfaces implemented by 'klass', sorts out duplicates,
> // and stores the interfaces in the 'arr' starting from 'index'.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 254:
>
>> 252:
>> 253: // And explore its superinterfaces.
>> 254: count += fill_interfaces(arr, index + count, env,
>> interfaces[i]);
>
> I assume here we are always dealing with test classes that are known not to
> be deeply nested.
Yes, we explore only test classes/interfaces
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 348:
>
>> 346:
>> 347: jclass obj_klass = env->GetObjectClass(obj);
>> 348:
>
> Unnecessary newline.
removed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 350:
>
>> 348:
>> 349: Klass* klass = Klass::explore(env, obj_klass);
>> 350:
>
> Unnecessary newline.
removed
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 492:
>
>> 490: Java_FieldIndicesTest_prepare(JNIEnv *env, jclass cls, jobject testObj)
>> {
>> 491: Object::explore(env, testObj);
>> 492: fflush(0);
>
> stdout?
fflush(0) flashes all opened streams (including stout and stderr).
It's to handle the case when some shared test routines log to stderr.
But flash() expects `FILE *`, so I updated all calls to use `nullptr`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473693579
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473695968
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473696254
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473696356
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473698778
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701344
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701389
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701268
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
> The fix adds new test for FollowReferences JVMTI function to verify > correctness of reported field indexes. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback - Changes: - all: https://git.openjdk.org/jdk/pull/17580/files - new: https://git.openjdk.org/jdk/pull/17580/files/6276448d..c5e7b787 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17580=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17580=00-01 Stats: 27 lines in 1 file changed: 1 ins; 3 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v4]
On Wed, 31 Jan 2024 23:07:16 GMT, Chris Plummer wrote: >> I noticed that "clhsdb jstack" seemed to hang when I attached to process >> with a somewhat large heap. It had taken over 10 minutes when I finally >> decided to have a look at the SA process (using bin/jstack), which came up >> with the following in the stack: >> >> >> at >> sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) >> at >> sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/StackTrace.java:71) >> >> >> This code is meant to print information about j.u.c. locks. It it works by >> searching the entire java heap for instances of AbstractOwnableSynchronizer. >> This is a very expensive operation because it means not only does SA need to >> read in the entire java heap, but it needs to create a Klass mirror for >> every heap object so it can determine its type. >> >> Our testing doesn't seem to run into this slowness problem because "clhsdb >> jstack" only iterates over the heap if AbstractOwnableSynchronizer has been >> loaded, which it won't be if no j.u.c. locks have been created. This seems >> to be the case wherever we use "clhsdb jstack" in testing. We do have some >> tests that likely result in j.u.c locks, but they all use "jhsdb jstack", >> which doesn't have this issue (it requires use of the --locks argument to >> enable printing of j.u.c locks). >> >> This issue was already called out in >> [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am >> proposing that "clhsdb jstack" not include j.u.c lock scanning by default, >> and add the -l argument to enable it. This will make it similar to >> bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has >> a --locks argument (which maps internally to the Jstack.java -l argument). >> >> The same changes are also being made to "clhsdb pstack". >> >> Tested with all tier1, tier2, and tier5 svc tests. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > simplify regex passed to String.split(). Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17479#pullrequestreview-1855155769
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes
On Thu, 25 Jan 2024 23:05:19 GMT, Alex Menkov wrote:
> The fix adds new test for FollowReferences JVMTI function to verify
> correctness of reported field indexes.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 93:
> 91:
> 92: /*
> 93: Per jvmtiHeapReferenceInfoField spec:
I think you should also include mention that this is for
JVMTI_HEAP_REFERENCE_STATIC_FIELD.
Indentation should be fixed to match our C++ comment style, and the "bullets"
from the spec should be included to make it clearer that these are two lists of
steps.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 108:
> 106: where n is the count of the fields in all the superinterfaces of I.
> 107:
> 108: 'Klass' struct contains all required data to calculate field indeces.
"indices"
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 153:
> 151:
> 152: /*
> 153: For each test object 'Object' struct is created and pointer to it set as
> a tag for jobject.
Suggestion:
For each test object, the 'Object' struct is created and a pointer to it is set
as the jobject's tag.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 225:
> 223:
> 224: // Explores all interfaces implemented by 'klass' sorting out duplicates
> 225: // and store them in the 'arr' starting from 'index'.
Suggestion:
// Explores all interfaces implemented by 'klass', sorts out duplicates,
// and stores the interfaces in the 'arr' starting from 'index'.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 254:
> 252:
> 253: // And explore its superinterfaces.
> 254: count += fill_interfaces(arr, index + count, env, interfaces[i]);
I assume here we are always dealing with test classes that are known not to be
deeply nested.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 348:
> 346:
> 347: jclass obj_klass = env->GetObjectClass(obj);
> 348:
Unnecessary newline.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 350:
> 348:
> 349: Klass* klass = Klass::explore(env, obj_klass);
> 350:
Unnecessary newline.
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 492:
> 490: Java_FieldIndicesTest_prepare(JNIEnv *env, jclass cls, jobject testObj) {
> 491: Object::explore(env, testObj);
> 492: fflush(0);
stdout?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473643948
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473644156
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473646542
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473647766
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473649198
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473651074
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473651163
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473650827
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]
On Wed, 31 Jan 2024 04:34:52 GMT, Chris Plummer wrote:
>> test/hotspot/jtreg/serviceability/sa/ClhsdbJstackWithConcurrentLock.java
>> line 65:
>>
>>> 63: for (String line : lines) {
>>> 64: if (line.contains(key)) {
>>> 65: String[] words = line.split(key + "|[, ]");
>>
>> String.split() expect regexp as an argument
>> Not sure how this code works (without escaping '$' and parentheses).
>> I think it would be clearer to use standard regexp pattern/matcher,
>> something like
>>
>>
>> Pattern pattern = Pattern.compile("\\s+- <0x([0-9a-fA-F]+)>, \(a
>> java/util/concurrent/locks/ReentrantLock\$NonfairSync\)");
>> for (String line: lines) {
>> Matcher matcher = pattern.matcher(line);
>> if (matcher.find()) {
>> addressString = matcher.group(1);
>> break;
>> }
>> }
>
> I think `key` doesn't really matter when doing the split. `key` is mainly for
> finding the right line. The split is for tokenizing that line into words, and
> the only parts that matters for that are ' ' and the ',' being used as word
> delimiters. I think `key` should just be removed from the regex. I think the
> source of the bug is code copied from ClhsdbInspect.java:
>
> // Escape the token "Method*=" because the split
> method uses
> // a regex, not just a straight String.
> String escapedKey = key.replace("*","\*");
> String[] words = line.split(escapedKey+"|[ ]");
>
> In this case key is `Method*=`. The code works and the escaping and searching
> for `key` is necessary because we are looking for something like
> `Method*=<0xffc2ed70>`, but I think this could have been simplified
> by including `=` in the split pattern rather than all of `escapedKey`.
Well, I see now.
In this case `"[, ]"` separator should work fine.
I still think that use of Matcher is much clear - regexp defines what you are
looking for and extract the value you need. Also there is no need to split
`jstackOutput` into lines - the whole jstack output can be passed to
`Pattern.matcher`.
But I don't force you if you prefer to extract the address manually.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1473643552
Re: RFR: JDK-8318566: Heap walking functions should not use FilteredFieldStream
On Wed, 31 Jan 2024 21:28:34 GMT, Alex Menkov wrote:
> FilteredFieldStream used by heap walking functions to iterate through
> klass/superclasses/interfaces fields are known to have poor performance (see
> [JDK-8317692](https://bugs.openjdk.org/browse/JDK-8317692) for details).
> Heap walking API implementation is the last user of the klasses.
> The fix reworks iteration through klass/superclasses/interfaces fields and
> drops FilteredFieldStream-related code.
> Additionally removed/updated includes of reflectionUtils.hpp.
>
> Testing:
> - tier1..4;
> - test/hotspot/jtreg/vmTestbase/nsk/jvmti (contains tests for different
> heap walking functions);
> - new test from #17580 (now the test runs several times faster).
src/hotspot/share/prims/jvmtiTagMap.cpp line 453:
> 451: InstanceKlass* super_klass = ik->java_super();
> 452: if (super_klass != nullptr) {
> 453: start_index += add_instance_fields(super_klass, start_index);
Does hotspot have any rules against potentially very deep recursion that can
overflow the stack?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17661#discussion_r1473607788
Re: RFR: 8324845: management.properties text "interface name" is misleading [v2]
On Wed, 31 Jan 2024 21:29:14 GMT, Kevin Walls wrote: >> We have the text "host-or-interface-name" describing the >> com.sun.management.jmxremote.host property in management.properties, but >> interface names (like "eth0" or "lo", or "ens3"...) are not what it accepts. >> It should just say it needs to be a host name or address. >> >> This change only affects comment text in a properties file. >> >> (We don't currently mention this property in other docs, e.g. in the M >> guide.) > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > text update Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17615#pullrequestreview-1855055246
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v4]
> I noticed that "clhsdb jstack" seemed to hang when I attached to process with > a somewhat large heap. It had taken over 10 minutes when I finally decided to > have a look at the SA process (using bin/jstack), which came up with the > following in the stack: > > > at > sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) > at > sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) > at > sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) > at > sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/StackTrace.java:71) > > > This code is meant to print information about j.u.c. locks. It it works by > searching the entire java heap for instances of AbstractOwnableSynchronizer. > This is a very expensive operation because it means not only does SA need to > read in the entire java heap, but it needs to create a Klass mirror for every > heap object so it can determine its type. > > Our testing doesn't seem to run into this slowness problem because "clhsdb > jstack" only iterates over the heap if AbstractOwnableSynchronizer has been > loaded, which it won't be if no j.u.c. locks have been created. This seems to > be the case wherever we use "clhsdb jstack" in testing. We do have some tests > that likely result in j.u.c locks, but they all use "jhsdb jstack", which > doesn't have this issue (it requires use of the --locks argument to enable > printing of j.u.c locks). > > This issue was already called out in > [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am > proposing that "clhsdb jstack" not include j.u.c lock scanning by default, > and add the -l argument to enable it. This will make it similar to > bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has a > --locks argument (which maps internally to the Jstack.java -l argument). > > The same changes are also being made to "clhsdb pstack". > > Tested with all tier1, tier2, and tier5 svc tests. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: simplify regex passed to String.split(). - Changes: - all: https://git.openjdk.org/jdk/pull/17479/files - new: https://git.openjdk.org/jdk/pull/17479/files/6c24a9a2..929de70f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17479=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17479=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17479.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17479/head:pull/17479 PR: https://git.openjdk.org/jdk/pull/17479
RFR: JDK-8318566: Heap walking functions should not use FilteredFieldStream
FilteredFieldStream used by heap walking functions to iterate through klass/superclasses/interfaces fields are known to have poor performance (see [JDK-8317692](https://bugs.openjdk.org/browse/JDK-8317692) for details). Heap walking API implementation is the last user of the klasses. The fix reworks iteration through klass/superclasses/interfaces fields and drops FilteredFieldStream-related code. Additionally removed/updated includes of reflectionUtils.hpp. Testing: - tier1..4; - test/hotspot/jtreg/vmTestbase/nsk/jvmti (contains tests for different heap walking functions); - new test from #17580 (now the test runs several times faster). - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/17661/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17661=00 Issue: https://bugs.openjdk.org/browse/JDK-8318566 Stats: 290 lines in 10 files changed: 41 ins; 230 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/17661.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17661/head:pull/17661 PR: https://git.openjdk.org/jdk/pull/17661
Re: RFR: 8324845: management.properties text "interface name" is misleading [v2]
On Mon, 29 Jan 2024 15:24:45 GMT, Alan Bateman wrote: >> Kevin Walls has updated the pull request incrementally with one additional >> commit since the last revision: >> >> text update > > src/jdk.management.agent/share/conf/management.properties line 258: > >> 256: # >> 257: # com.sun.management.jmxremote.host= >> 258: # Specifies the address on which the JMX RMI agent will bind. > > The placeholder is "host-name-or-address" so it might be clearer to say the > local host name or IP address in the description. ok! - PR Review Comment: https://git.openjdk.org/jdk/pull/17615#discussion_r1473484519
Re: RFR: 8324845: management.properties text "interface name" is misleading [v2]
> We have the text "host-or-interface-name" describing the > com.sun.management.jmxremote.host property in management.properties, but > interface names (like "eth0" or "lo", or "ens3"...) are not what it accepts. > It should just say it needs to be a host name or address. > > This change only affects comment text in a properties file. > > (We don't currently mention this property in other docs, e.g. in the M > guide.) Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: text update - Changes: - all: https://git.openjdk.org/jdk/pull/17615/files - new: https://git.openjdk.org/jdk/pull/17615/files/d1eaf87b..58cdf0cf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17615=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17615=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17615/head:pull/17615 PR: https://git.openjdk.org/jdk/pull/17615
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v25]
> Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives (so bypass inlined > methods). > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Prior to that we can try to re-compile the method letting > compile broker to perform it taking new directives stack into account. > Re-compilation helps to prevent hot methods from execution in the interpreter. > > A new flag `-r` has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > puts methods that have any active non-default matching compiler directives to > re-compilation if possible, otherwise marks them for deoptimization. There is > currently no distinction which directives are found. In particular, this > means that if there are rules for inlining into some method, it will be > refreshed. On the other hand, if there are rules for a method and it was > inlined, top-level methods won't be refreshed, but this can be achieved by > having rules for them. > > In addition, a new diagnostic command `Compiler.replace_directives`, has been > added for ... Dmitry Chuyko has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 43 commits: - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Deopt osr, cleanups - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - ... and 33 more: https://git.openjdk.org/jdk/compare/5b9b176c...ea0322cd - Changes: https://git.openjdk.org/jdk/pull/14111/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14111=24 Stats: 381 lines in 15 files changed: 348 ins; 3 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/14111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111 PR: https://git.openjdk.org/jdk/pull/14111
RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
This change uses a claim token to allocate multi dimensional arrays rather than holding MultiArray_lock around metaspace allocation. We can't hold a mutex around metaspace allocation because it can create an OOM object and it can also call into JVMTI for a resource exhausted event. Also, we were creating mirrors and more metadata arrays while holding this lock. See the bug for more details and other ideas considered and rejected. Tested with tier1-7. - Commit messages: - Some more cleanups, and make token really recursive. - 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock Changes: https://git.openjdk.org/jdk/pull/17660/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17660=00 Issue: https://bugs.openjdk.org/browse/JDK-8308745 Stats: 156 lines in 10 files changed: 90 ins; 17 del; 49 mod Patch: https://git.openjdk.org/jdk/pull/17660.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17660/head:pull/17660 PR: https://git.openjdk.org/jdk/pull/17660
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v7]
On Wed, 31 Jan 2024 08:01:15 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Minor - fix alignment Marked as reviewed by gziemski (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1854147646
Re: RFR: 8325055: Rename Injector.h [v2]
> Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/Injector.h to Injector.hpp. > > Testing: mach5 tier1 Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: fix name in README - Changes: - all: https://git.openjdk.org/jdk/pull/17656/files - new: https://git.openjdk.org/jdk/pull/17656/files/ea9edad1..0ab5cf73 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17656=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17656=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17656.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17656/head:pull/17656 PR: https://git.openjdk.org/jdk/pull/17656
RFR: 8325055: Rename Injector.h
Please review this trivial change that renames the file test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/Injector.h to Injector.hpp. Testing: mach5 tier1 - Commit messages: - rename Injector.h Changes: https://git.openjdk.org/jdk/pull/17656/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17656=00 Issue: https://bugs.openjdk.org/browse/JDK-8325055 Stats: 9 lines in 4 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17656.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17656/head:pull/17656 PR: https://git.openjdk.org/jdk/pull/17656
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Wed, 31 Jan 2024 13:20:52 GMT, Martin Doerr wrote: >> Suchismith Roy has updated the pull request incrementally with one >> additional commit since the last revision: >> >> spelling > > src/hotspot/os/aix/os_aix.cpp line 1176: > >> 1174: strncpy(file_path,filename, buffer_length + 1); >> 1175: char* const pointer_to_dot = strrchr(file_path, '.'); >> 1176: assert(pointer_to_dot != nullptr, "Attempting to load a shared >> object without extension? %s", filename); > > This should not only be an assertion. I think the check could be used instead > of the strcmp below. I didn't follow that. You mean i need to keep a check if it is null and print it out ? - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1472963709
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
On Wed, 31 Jan 2024 13:17:21 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > spelling src/hotspot/os/aix/os_aix.cpp line 1176: > 1174: strncpy(file_path,filename, buffer_length + 1); > 1175: char* const pointer_to_dot = strrchr(file_path, '.'); > 1176: assert(pointer_to_dot != nullptr, "Attempting to load a shared object > without extension? %s", filename); This should not only be an assertion. I think the check could be used instead of the strcmp below. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1472813890
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v13]
> J2SE agent does not start and throws error when it tries to find the shared > library ibm_16_am. > After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load > fails.It fails to identify the shared library ibm_16_am.a shared archive file > on AIX. > Hence we are providing a function which will additionally search for .a file > on AIX ,when the search for .so file fails. Suchismith Roy has updated the pull request incrementally with one additional commit since the last revision: spelling - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/713e514b..af761abb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16604=12 - incr: https://webrevs.openjdk.org/?repo=jdk=16604=11-12 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16604/head:pull/16604 PR: https://git.openjdk.org/jdk/pull/16604
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v12]
> J2SE agent does not start and throws error when it tries to find the shared > library ibm_16_am. > After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load > fails.It fails to identify the shared library ibm_16_am.a shared archive file > on AIX. > Hence we are providing a function which will additionally search for .a file > on AIX ,when the search for .so file fails. Suchismith Roy has updated the pull request incrementally with one additional commit since the last revision: Clarify comment - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/257f5def..713e514b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16604=11 - incr: https://webrevs.openjdk.org/?repo=jdk=16604=10-11 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16604/head:pull/16604 PR: https://git.openjdk.org/jdk/pull/16604
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v11]
On Wed, 31 Jan 2024 07:42:49 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1166: >> >>> 1164: Search order: >>> 1165: libfilename-> load "libfilename.so" first,then load libfilename.a,on >>> failure. >>> 1166: In,OpenJ9,the libary with .so extension is loaded first and then .a >>> extension,on failure. >> >> Hi Suchi, I'm puzzled. Your comment implies for me, that load library gets a >> 'base' filename without 'lib' prefix and without extension (e.g. 'name'). >> Then the j9 code creates the filename 'libname.so' first and on failure >> 'libname.a' second. What about given libname.so explicitly (e.g. >> libname.so)? Does j9 really use 'libname.a' as a failure fallback in this >> case? > > The load library gets the entire library name, after construction from > dll_build_name. This is always a .so file name. When .so file name fails to > load, we fallback to .a filename. > Do i need to mention the filename as libfilename.so then ? Yes, I think this would make it clear. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1472683336
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v7]
On Wed, 31 Jan 2024 08:01:15 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Minor - fix alignment tier1, tier2 and tier3 testing of the latest state of this PR has passed both with and without the changes that were done in https://bugs.openjdk.org/browse/JDK-8300088. - PR Comment: https://git.openjdk.org/jdk/pull/17588#issuecomment-1918911434
Re: RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities
On Wed, 31 Jan 2024 10:01:37 GMT, Severin Gehwolf wrote: > Thanks! Please make sure that the tests actually ran. If, for example, docker > is not installed, they get skipped. Ah, good point. Running the tests did take some amount of time, so it felt like they did something. And by spamming `docker ps` while the tests are running I could see e.g. this: $ docker ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 179ff2470b18 jdk-internal:test-containers-docker-TestJFREvents-jfr-events "/jdk/bin/java -cp /…" 1 second agoUp Less than a second stoic_clarke - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1918778186
Re: RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities
On Tue, 30 Jan 2024 13:57:43 GMT, Severin Gehwolf wrote: >> 8307977: jcmd and jstack broken for target processes running with elevated >> capabilities > > `test/hotspot/jtreg/serviceability` tests would also be worth running. > Hi @jerboaa, thanks a lot for the hints! The container tests were new to me > at least. > > Just out of curiosity, are the container tests run as part of the tier1 tests > (`make test-tier1`)? I'm not sure if I'm looking at the right place, but I > get the feeling this is defined in `test/hotspot/jtreg/TEST.groups`, and the > answer is maybe "no" in that case. > > > Please run container tests, which do some jcmd testing across containers > > (host system runs jcmd and containers have the JVMs): > > `test/hotspot/jtreg/containers` See > > [testing.md](https://github.com/openjdk/jdk/blob/master/doc/testing.md#docker-tests) > > as to how to run them. I'll give this PR a spin as well. > > If I understand it correctly, this is the way to run them. Please correct me > if I'm wrong. > > ``` > $ make test TEST="jtreg:test/hotspot/jtreg/containers" > > ... > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg/containers 1414 0 0 > > == > TEST SUCCESS > ``` Thanks! Please make sure that the tests actually ran. If, for example, `docker` is not installed, they get skipped. - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1918774353
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v4]
On Tue, 30 Jan 2024 14:15:57 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge branch 'master' into jdk-FOB64 > - Move #include out of debug_util.h > - Restore AIX dirent64 et al defines > - Rollback AIX changes since they are now tracked in JDK-8324834 > - Remove superfluous setting of FOB64 > - Replace all foo64() with foo() for large-file functions in the JDK > - 8324539: Do not use LFS64 symbols in JDK libs After adding this additional patch I fully build fastdebug on AIX (hav to admit it does not look very nice). diff --git a/src/java.desktop/share/native/libawt/java2d/pipe/BufferedRenderPipe.c b/src/java.desktop/share/native/libawt/java2d/pipe/BufferedRenderPipe.c index 823475b0a23..ee0109b6806 100644 --- a/src/java.desktop/share/native/libawt/java2d/pipe/BufferedRenderPipe.c +++ b/src/java.desktop/share/native/libawt/java2d/pipe/BufferedRenderPipe.c @@ -31,6 +31,10 @@ #include "SpanIterator.h" #include "Trace.h" +#if defined(_AIX) && defined(open) +#undef open +#endif + /* The "header" consists of a jint opcode and a jint span count value */ #define INTS_PER_HEADER 2 #define BYTES_PER_HEADER 8 - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1918699480
Re: RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities
On Tue, 30 Jan 2024 17:00:16 GMT, Bernd Eckenfels wrote: > Is that actually safe to allow low priveledged user context to attach and > control to a higher prived? It can at least overwrite files, but probably > also inject code? On the native level a ptrace(2) would probably not be > allowed. It's a good question. For context, this has worked fine in JDK 8, and AFAIK it was never intentionally broken for security reasons. In some cases the opposite can also be true - that one needs root access to attach to a process is not acceptable or even possible. - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1918616533
Re: RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities
On Tue, 30 Jan 2024 13:57:43 GMT, Severin Gehwolf wrote: >> 8307977: jcmd and jstack broken for target processes running with elevated >> capabilities > > `test/hotspot/jtreg/serviceability` tests would also be worth running. Hi @jerboaa, thanks a lot for the hints! The container tests were new to me at least. Just out of curiosity, are the container tests run as part of the tier1 tests (`make test-tier1`)? I'm not sure if I'm looking at the right place, but I get the feeling this is defined in `test/hotspot/jtreg/TEST.groups`, and the answer is maybe "no" in that case. > Please run container tests, which do some jcmd testing across containers > (host system runs jcmd and containers have the JVMs): > `test/hotspot/jtreg/containers` See > [testing.md](https://github.com/openjdk/jdk/blob/master/doc/testing.md#docker-tests) > as to how to run them. I'll give this PR a spin as well. If I understand it correctly, this is the way to run them. Please correct me if I'm wrong. $ make test TEST="jtreg:test/hotspot/jtreg/containers" ... == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/hotspot/jtreg/containers 1414 0 0 == TEST SUCCESS > `test/hotspot/jtreg/serviceability` tests would also be worth running. $ make test TEST="jtreg:test/hotspot/jtreg/serviceability" ... == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/hotspot/jtreg/serviceability 368 368 0 0 == TEST SUCCESS - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1918576420
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v7]
> Can I please get a review of this change which proposes to address > https://bugs.openjdk.org/browse/JDK-8324668? > > This change proposes to fix the issue in jdwp where when launching a child > process (for the `launch=` option), it iterates over an extremely large > number of file descriptors to close each one of those. Details about the > issue and the proposed fixed are added in a subsequent comment in this PR > https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, > tier2 and tier3 testing is currently in progress with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Minor - fix alignment - Changes: - all: https://git.openjdk.org/jdk/pull/17588/files - new: https://git.openjdk.org/jdk/pull/17588/files/3d323f7d..afef982f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17588=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17588=05-06 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17588.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17588/head:pull/17588 PR: https://git.openjdk.org/jdk/pull/17588
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 06:28:05 GMT, David Holmes wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
>
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 100:
>
>> 98: if ((dp = opendir(FD_DIR)) == NULL) {
>> 99: ERROR_MESSAGE(("failed to open dir %s while determining"
>> 100: " file descriptors to close for process %d", FD_DIR,
>> getpid()));
>
> Nit: indent of second line is now off - please align "
Done. PR has been update to fix this.
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 139:
>
>> 137: ERROR_MESSAGE(("failed to close file descriptors of"
>> 138: " child process optimally, falling back to closing"
>> 139: " %d file descriptors sequentially", (max_fd - i +
>> 1)));
>
> Nit: please realign lines on "
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472430580
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472430298
