Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v5]
> 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid > (Kubernetes debug container) Sebastian Lövdahl has updated the pull request incrementally with one additional commit since the last revision: Adapt code style - Changes: - all: https://git.openjdk.org/jdk/pull/19055/files - new: https://git.openjdk.org/jdk/pull/19055/files/c625411b..8b200fc7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19055&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19055&range=03-04 Stats: 27 lines in 1 file changed: 4 ins; 11 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19055.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19055/head:pull/19055 PR: https://git.openjdk.org/jdk/pull/19055
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v4]
On Fri, 28 Jun 2024 18:02:28 GMT, Kevin Walls wrote:
>> Sebastian Lövdahl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add test for the elevated privileges case
>
> src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java:
>
> It's going to be mostly style nits. 8-)
> I realise some of this was passed back and forth above (so apologies to you
> and to Larry for being a pain).
>
>
> line 281 line break before IOException
>
>
> 313
> There is no hard limit on line length, but when you add new longest lines
> it's probably diverged from the file's style.
> Could you make some of the new comments more terse?
>
>
> A lot of the new code is double-spaced, which is unusual for the file for
> HotSpot/JDK as a whole.
> Is it still readable to you if we delete at least these empty lines:
> 274, 276, 286, 296, 303, 312, 317, 319, 324, 326
>
> It may not quite get findTargetProcessTmpDirectory to fit on screen, but it
> will help. 8-)
Hi again @kevinjwalls,
Thanks a lot for the review! And no worries :) I'm happy to adapt the code
style to match the rest of the file and the JDK in general.
I pushed some changes in 8b200fc7f1d4bb00a4f18c90cd8e36374ade98ba. I removed a
lof of newlines and tried to make the comments more terse. The two `throw new
IOException(String.format("unable to attach, %s non-existent! process: %d
terminated", procPidPath, pid));` lines still stand out in terms of line
length, should I wrap them?
-
PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2199413193
RFR: 8313235: TestClhsdbJstackLock.java failed with '^\s+- waiting to lock <0x[0-9a-f]+> \(a java\.lang\.Class for LingeredAppWithLock\)$' missing from stdout/stderr
Once the main thread has detected that the spawned thread is in the BLOCKED state, the spawned thread's LingeredAppWithLock.lockMethod() should be visible on the top of the stack, but it is not, so the "waiting to lock" message is missing from the stack trace. I think the explanations mentioned in [JDK-8335124](https://bugs.openjdk.org/browse/JDK-8335124) and [JDK-8269881](https://bugs.openjdk.org/browse/JDK-8269881) apply here also. The state of the thread has moved to BLOCKED, but the thread still needs to finish making an OS call to actually become blocked and the thread become idle. During that time we may not be able to get an accurate stack trace. In this case probably SP has not been flushed, so we are not seeing the lockMethod() frame, which should appear at the top of the stack. A short delay has been added after all threads have entered the desired state to make sure they have fully reached the desired state and are now idle. Tested with Tier1 CI and all svc test tasks for tier2 and tier5. - Commit messages: - Add a 1/2 second delay to make sure the threads have finished moving to the right state and and idle. Changes: https://git.openjdk.org/jdk/pull/19953/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19953&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8313235 Stats: 18 lines in 2 files changed: 17 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19953.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19953/head:pull/19953 PR: https://git.openjdk.org/jdk/pull/19953
Re: RFR: 8335291: Problem list all SA core file tests on macosx-aarch64 due to JDK-8318754 [v2]
> On macosx-aarch64, sometimes the generated core file does not contain all > valid memory. This causes SA tests to fail with various address related java > exceptions. There's nothing SA can do to work around the problem, and these > failures over time have been just too noisy, so I'm problem listing all the > SA core files tests on macosx-aarch64. Note they are already problem listed > on macosx-x64 dues to > [JDK-8267433](https://bugs.openjdk.org/browse/JDK-8267433) (the tests time > out while generating the core dump because it takes way too long), so > unfortunately this means no more core file testing on macosx, at least for > now. > > Tested with tier1 and running all tier2 and tier5 SA test tasks. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Use proper CR in problem list - Changes: - all: https://git.openjdk.org/jdk/pull/19950/files - new: https://git.openjdk.org/jdk/pull/19950/files/33c20cdc..958429ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19950&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19950&range=00-01 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19950.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19950/head:pull/19950 PR: https://git.openjdk.org/jdk/pull/19950
Integrated: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container
On Mon, 11 Mar 2024 16:55:36 GMT, Severin Gehwolf wrote: > Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? This pull request has now been integrated. Changeset: 0a6ffa57 Author:Severin Gehwolf URL: https://git.openjdk.org/jdk/commit/0a6ffa57954ddf4f92205205a5a1bada813d127a Stats: 411 lines in 20 files changed: 305 ins; 79 del; 27 mod 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container Reviewed-by: stuefe, iklam - PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v8]
On Fri, 28 Jun 2024 15:41:48 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 18 commits: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - ... and 8 more: https://git.openjdk.org/jdk/compare/486aa11e...1017da35 Thank you for the review! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2199581201
Re: RFR: 8333891: Method excluded with directive is not compiled after removal of directive [v2]
On Sat, 22 Jun 2024 09:30:25 GMT, Evgeny Astigeevich
wrote:
>> A Java method can become non-compilable if there are issues with its
>> compilation or if its compiled version causes problems. Additionally, a
>> method can be marked as non-compilable using a compile command or a compiler
>> directive. Since compiler directives can be updated, a directive that
>> disables a method's compilation can be changed or removed.
>>
>> Currently, when a Java method is marked as non-compilable, the reason for
>> this status is unknown. If a change in a compiler directive makes the method
>> compilable again, we cannot clear the non-compilable status because we don't
>> know if the directive initially caused the method to become non-compilable.
>>
>> To resolve the issue two method flags are introduced: `is_c1_exclude` and
>> `is_c2_excluded`. They mean a Java method is excluded from compilation by a
>> directive. With these flags we can find out a Java method has been excluded
>> with a directive. If the directive changes and allows compilation of the
>> method we can detect this and clear the non-compilable status.
>>
>> As accesses to flags must be race free we have to remove getting a directive
>> from `CompileBroker::compile_method`. We combine two
>> `CompileBroker::compile_method` into one. We also move calculation whether
>> compilation is blocking into `CompileBroker::compile_method_base`. The
>> directive used for that calculation is passed to a compile task, so a
>> compile task does not need to get it again.
>>
>> A regression test is added.
>>
>> Tested fastdebug build on Linux AArch64, Linux x86_64 and Windows Server
>> 2019 x86_64:
>> - Tier1/2/3 passed.
>
> Evgeny Astigeevich has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix data race
Changes requested by dfenacci (Committer).
src/hotspot/share/compiler/compileBroker.cpp line 1336:
> 1334: }
> 1335:
> 1336: AbstractCompiler *comp = CompileBroker::compiler(comp_level);
Do we need to change this?
src/hotspot/share/compiler/compileBroker.cpp line 1563:
> 1561: // See if this compilation is not allowed.
> 1562: bool CompileBroker::compilation_is_prohibited(const methodHandle&
> method, int osr_bci, int comp_level) {
> 1563: assert(compiler(comp_level) != nullptr, "Ensure we have a compiler");
Are you sure `compiler(comp_level)` can never be `nullptr` (not even
potentially)? (it seems so to me too but just to make sure)
-
PR Review: https://git.openjdk.org/jdk/pull/19637#pullrequestreview-2148291624
PR Review Comment: https://git.openjdk.org/jdk/pull/19637#discussion_r1658904857
PR Review Comment: https://git.openjdk.org/jdk/pull/19637#discussion_r1660722626
Re: RFR: 8269881: SA stack dump fails to include stack trace for SteadyStateThread
On Fri, 28 Jun 2024 20:34:48 GMT, Chris Plummer wrote: > The completely unrelated fix to > [JDK-8335124](https://bugs.openjdk.org/browse/JDK-8335124) led me to believe > that the issue with sometimes not being able to get the stack trace of the > SteadyStateThread might be due to the thread being active for a short period > after being reported as in the Thread.State.BLOCKED state. Once set to that > state, the thread still needs to call a native OS API to block the thread so > it is truly idle. During this time the thread stack might be inconsistent and > not walk-able. The fix is to add a short sleep after the thread has moved to > the Thread.State.BLOCKED state to give it a chance to finish blocking. > > Tested with Tier1 CI and all svc test tasks for tier2 and tier5. Looks good, let's try it! Was wondering if for the failure in ClhsdbDumpheap.java, the missing text was too far from when LingeredApp was started. But if it's the first subtest, then it's the stacks in a dumpheap output where we don't find the required steadyState text. So the test only has to create the array of subtests and call the first one, before the LingeredApp thread has really blocked... Good to make this harmless test change so we get long term testing of it. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19951#pullrequestreview-2150892788
Re: RFR: 8313235: TestClhsdbJstackLock.java failed with '^\s+- waiting to lock <0x[0-9a-f]+> \(a java\.lang\.Class for LingeredAppWithLock\)$' missing from stdout/stderr
On Fri, 28 Jun 2024 22:30:52 GMT, Chris Plummer wrote: > Once the main thread has detected that the spawned thread is in the BLOCKED > state, the spawned thread's LingeredAppWithLock.lockMethod() should be > visible on the top of the stack, but it is not, so the "waiting to lock" > message is missing from the stack trace. > > I think the explanations mentioned in > [JDK-8335124](https://bugs.openjdk.org/browse/JDK-8335124) and > [JDK-8269881](https://bugs.openjdk.org/browse/JDK-8269881) apply here also. > The state of the thread has moved to BLOCKED, but the thread still needs to > finish making an OS call to actually become blocked and the thread become > idle. During that time we may not be able to get an accurate stack trace. In > this case probably SP has not been flushed, so we are not seeing the > lockMethod() frame, which should appear at the top of the stack. > > A short delay has been added after all threads have entered the desired state > to make sure they have fully reached the desired state and are now idle. > > Tested with Tier1 CI and all svc test tasks for tier2 and tier5. Looks good. I think its more productive to make these harmless test updates than to count nanoseconds and try to reason about some of these failures. Reading LingeredApp I notice waitAppReadyOrCrashed() is really "wait until alive, or crashed", it has no idea if LingeredApp is "ready". TestClhsdbJstackLock does not waiting, just relies on LingeredApp.startApp(), so this delay should help. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19953#pullrequestreview-2150951040
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v5]
On Mon, 1 Jul 2024 07:19:48 GMT, Sebastian Lövdahl wrote: >> 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid >> (Kubernetes debug container) > > Sebastian Lövdahl has updated the pull request incrementally with one > additional commit since the last revision: > > Adapt code style That all looks reasonable to me, thanks. We have a two reviewer requirement in hotspot generally so let's try and get further comment/review. - PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2199734377
Re: [jdk23] RFR: 8326820: Metadata artificially kept alive
On Thu, 27 Jun 2024 14:30:43 GMT, Axel Boldt-Christmas wrote: > Hi all, > > This pull request contains a backport of commit > [5909d541](https://github.com/openjdk/jdk/commit/5909d54147355dd7da5786ff39ead4c15816705c) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Axel Boldt-Christmas on 27 Jun > 2024 and was reviewed by Erik Österlund, Stefan Karlsson and Coleen > Phillimore. > > Thanks! Thanks for the review. This has been running through the JDK 24 CI over the weekend. No issues found. - PR Comment: https://git.openjdk.org/jdk/pull/19929#issuecomment-2199778367
[jdk23] Integrated: 8326820: Metadata artificially kept alive
On Thu, 27 Jun 2024 14:30:43 GMT, Axel Boldt-Christmas wrote: > Hi all, > > This pull request contains a backport of commit > [5909d541](https://github.com/openjdk/jdk/commit/5909d54147355dd7da5786ff39ead4c15816705c) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Axel Boldt-Christmas on 27 Jun > 2024 and was reviewed by Erik Österlund, Stefan Karlsson and Coleen > Phillimore. > > Thanks! This pull request has now been integrated. Changeset: e5fbc631 Author:Axel Boldt-Christmas URL: https://git.openjdk.org/jdk/commit/e5fbc631ca06b40a682149b0903221e190f592aa Stats: 80 lines in 6 files changed: 31 ins; 25 del; 24 mod 8326820: Metadata artificially kept alive Reviewed-by: stefank Backport-of: 5909d54147355dd7da5786ff39ead4c15816705c - PR: https://git.openjdk.org/jdk/pull/19929
Re: [jdk23] RFR: 8333542: Breakpoint in parallel code does not work
On Fri, 28 Jun 2024 12:14:55 GMT, Coleen Phillimore wrote: > Clean backport of JDK-8333542. After this, we need a backport for > JDK-8335134 to fix the test. Thank you Chris. - PR Comment: https://git.openjdk.org/jdk/pull/19938#issuecomment-210801
[jdk23] Integrated: 8333542: Breakpoint in parallel code does not work
On Fri, 28 Jun 2024 12:14:55 GMT, Coleen Phillimore wrote: > Clean backport of JDK-8333542. After this, we need a backport for > JDK-8335134 to fix the test. This pull request has now been integrated. Changeset: 7040de19 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/7040de19bdb29a3abacf2a39b7c7c30a07c61135 Stats: 516 lines in 16 files changed: 339 ins; 129 del; 48 mod 8333542: Breakpoint in parallel code does not work Reviewed-by: cjplummer Backport-of: b3bf31a0a08da679ec2fd21613243fb17b1135a9 - PR: https://git.openjdk.org/jdk/pull/19938
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v5]
On Mon, 1 Jul 2024 07:19:48 GMT, Sebastian Lövdahl wrote: >> 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid >> (Kubernetes debug container) > > Sebastian Lövdahl has updated the pull request incrementally with one > additional commit since the last revision: > > Adapt code style Looks good to me, thanks! (!havePidNSes && nsPid > 1) I didn't get this at first, I think it's because PID 1 can't have a parent? (in the same namespace) - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19055#pullrequestreview-2151324535 PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2200071296
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v5]
On 7/1/24 5:59 AM, Kevin Walls wrote: On Mon, 1 Jul 2024 07:19:48 GMT, Sebastian Lövdahl wrote: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) Sebastian Lövdahl has updated the pull request incrementally with one additional commit since the last revision: Adapt code style Looks good to me, thanks! (!havePidNSes && nsPid > 1) I didn't get this at first, I think it's because PID 1 can't have a parent? (in the same namespace) that's correct - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19055#pullrequestreview-2151324535 PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2200071296
Re: RFR: 8333891: Method excluded with directive is not compiled after removal of directive [v2]
On Sat, 22 Jun 2024 09:30:25 GMT, Evgeny Astigeevich wrote: >> A Java method can become non-compilable if there are issues with its >> compilation or if its compiled version causes problems. Additionally, a >> method can be marked as non-compilable using a compile command or a compiler >> directive. Since compiler directives can be updated, a directive that >> disables a method's compilation can be changed or removed. >> >> Currently, when a Java method is marked as non-compilable, the reason for >> this status is unknown. If a change in a compiler directive makes the method >> compilable again, we cannot clear the non-compilable status because we don't >> know if the directive initially caused the method to become non-compilable. >> >> To resolve the issue two method flags are introduced: `is_c1_exclude` and >> `is_c2_excluded`. They mean a Java method is excluded from compilation by a >> directive. With these flags we can find out a Java method has been excluded >> with a directive. If the directive changes and allows compilation of the >> method we can detect this and clear the non-compilable status. >> >> As accesses to flags must be race free we have to remove getting a directive >> from `CompileBroker::compile_method`. We combine two >> `CompileBroker::compile_method` into one. We also move calculation whether >> compilation is blocking into `CompileBroker::compile_method_base`. The >> directive used for that calculation is passed to a compile task, so a >> compile task does not need to get it again. >> >> A regression test is added. >> >> Tested fastdebug build on Linux AArch64, Linux x86_64 and Windows Server >> 2019 x86_64: >> - Tier1/2/3 passed. > > Evgeny Astigeevich has updated the pull request incrementally with one > additional commit since the last revision: > > Fix data race Hi @dafedafe, Thank you for reviewing. - PR Review: https://git.openjdk.org/jdk/pull/19637#pullrequestreview-2151784002
Re: RFR: 8333891: Method excluded with directive is not compiled after removal of directive [v2]
On Fri, 28 Jun 2024 15:15:31 GMT, Damon Fenacci wrote:
>> Evgeny Astigeevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix data race
>
> src/hotspot/share/compiler/compileBroker.cpp line 1336:
>
>> 1334: }
>> 1335:
>> 1336: AbstractCompiler *comp = CompileBroker::compiler(comp_level);
>
> Do we need to change this?
Not really. I'll change it back to the original code.
> src/hotspot/share/compiler/compileBroker.cpp line 1563:
>
>> 1561: // See if this compilation is not allowed.
>> 1562: bool CompileBroker::compilation_is_prohibited(const methodHandle&
>> method, int osr_bci, int comp_level) {
>> 1563: assert(compiler(comp_level) != nullptr, "Ensure we have a compiler");
>
> Are you sure `compiler(comp_level)` can never be `nullptr` (not even
> potentially)? (it seems so to me too but just to make sure)
Yes, I am sure.
This is a private member function which is only used in
`CompileBroker::compile_method`.
In `CompileBroker::compile_method` there has always been an assert checking
availability of a compiler. I added the assert here just in case the function
is used in any other functions where a compiler might not be available.
If we want to minimize changes, we can keep the original code where the
parameter `excluded` is only deleted. The checks `comp == nullptr` are
redundant in the original code because the function is invoked as follows:
if (comp == nullptr || compilation_is_prohibited(...)) {
return nullptr;
}
I can add a check `comp == nullptr` at the beginning of the function and return
`nullptr` if it is true.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19637#discussion_r1661286156
PR Review Comment: https://git.openjdk.org/jdk/pull/19637#discussion_r1661319021
[jdk23] RFR: 8335134: Test com/sun/jdi/BreakpointOnClassPrepare.java timeout
Clean backport. Need to backport this since it was introduced with the new test in [JDK-8333542](https://bugs.openjdk.org/browse/JDK-8333542), which has also been backpoted to 23. - Commit messages: - Backport 4e8cbf884ab1eee9c3110712ab62edc706e948ba Changes: https://git.openjdk.org/jdk/pull/19976/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19976&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335134 Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19976.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19976/head:pull/19976 PR: https://git.openjdk.org/jdk/pull/19976
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v5]
On Mon, 1 Jul 2024 12:55:24 GMT, Kevin Walls wrote: >(!havePidNSes && nsPid > 1) > I didn't get this at first, I think it's because PID 1 can't have a parent? > (in the same namespace) That was my assumption as well. Is that correct @larry-cable? Maybe it could be worth clarifying with a comment. - PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2200666442
Re: RFR: 8322812: Manpage for jcmd is missing JFR.view command
On Fri, 28 Jun 2024 14:51:57 GMT, Erik Gahlin wrote: > Could I have a review of a change to the jcmd man page? A typo was also fixed > for JFR.start. > > Testing: tier1 > > Thanks > Erik Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19942#pullrequestreview-2148543225
Re: RFR: 8322812: Manpage for jcmd is missing JFR.view command
On Fri, 28 Jun 2024 14:51:57 GMT, Erik Gahlin wrote: > Could I have a review of a change to the jcmd man page? A typo was also fixed > for JFR.start. > > Testing: tier1 > > Thanks > Erik Marked as reviewed by kevinw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19942#pullrequestreview-2151862195
RFR: 8322812: Manpage for jcmd is missing JFR.view command
Could I have a review of a change to the jcmd man page? A typo was also fixed for JFR.start. Testing: tier1 Thanks Erik - Commit messages: - Don't touch version - Review feedback - Initial Changes: https://git.openjdk.org/jdk/pull/19942/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19942&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322812 Stats: 50 lines in 1 file changed: 49 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19942.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19942/head:pull/19942 PR: https://git.openjdk.org/jdk/pull/19942
Re: RFR: 8269881: SA stack dump fails to include stack trace for SteadyStateThread
On Mon, 1 Jul 2024 09:31:12 GMT, Kevin Walls wrote: >> The completely unrelated fix to >> [JDK-8335124](https://bugs.openjdk.org/browse/JDK-8335124) led me to believe >> that the issue with sometimes not being able to get the stack trace of the >> SteadyStateThread might be due to the thread being active for a short period >> after being reported as in the Thread.State.BLOCKED state. Once set to that >> state, the thread still needs to call a native OS API to block the thread so >> it is truly idle. During this time the thread stack might be inconsistent >> and not walk-able. The fix is to add a short sleep after the thread has >> moved to the Thread.State.BLOCKED state to give it a chance to finish >> blocking. >> >> Tested with Tier1 CI and all svc test tasks for tier2 and tier5. > > Looks good, let's try it! > > Was wondering if for the failure in ClhsdbDumpheap.java, the missing text was > too far from when LingeredApp was started. But if it's the first subtest, > then it's the stacks in a dumpheap output where we don't find the required > steadyState text. So the test only has to create the array of subtests and > call the first one, before the LingeredApp thread has really blocked... > > Good to make this harmless test change so we get long term testing of it. @kevinjwalls Actually in all cases after launching LingeredApp and waiting for the the SteadyStateThread to be "ready", there is still then the launching of the clhsdb tool, which is going to take some time. Seems hard to believe that the SteadyStateThread would ever lose out on that race. I get the feeling that maybe there is more going on here than I initially thought. Almost all of these failures are on Windows (about 22 out of 25) with the other 3 on linux-arm. Maybe sometimes there is some sort of OS hiccup that is delaying the SteadyStateThread. In any case, no real harm with this fix, and hopefully it helps - PR Comment: https://git.openjdk.org/jdk/pull/19951#issuecomment-2200769458
Re: [jdk23] RFR: 8335134: Test com/sun/jdi/BreakpointOnClassPrepare.java timeout
On Mon, 1 Jul 2024 17:10:44 GMT, Chris Plummer wrote: > Clean backport. Need to backport this since it was introduced with the new > test in [JDK-8333542](https://bugs.openjdk.org/browse/JDK-8333542), which has > also been backpoted to 23. Backport looks accurate. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19976#pullrequestreview-2152315588
Re: [jdk23] RFR: 8335134: Test com/sun/jdi/BreakpointOnClassPrepare.java timeout
On Mon, 1 Jul 2024 17:10:44 GMT, Chris Plummer wrote: > Clean backport. Need to backport this since it was introduced with the new > test in [JDK-8333542](https://bugs.openjdk.org/browse/JDK-8333542), which has > also been backpoted to 23. Thanks David! Integrating now since without this backport we might start seeing some new CI failures due to [JDK-8333542](https://bugs.openjdk.org/browse/JDK-8333542) having been backported earlier today. - PR Comment: https://git.openjdk.org/jdk/pull/19976#issuecomment-2201260668
[jdk23] Integrated: 8335134: Test com/sun/jdi/BreakpointOnClassPrepare.java timeout
On Mon, 1 Jul 2024 17:10:44 GMT, Chris Plummer wrote: > Clean backport. Need to backport this since it was introduced with the new > test in [JDK-8333542](https://bugs.openjdk.org/browse/JDK-8333542), which has > also been backpoted to 23. This pull request has now been integrated. Changeset: 9d744b0e Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/9d744b0e04cdf4be8d9e3d5435f8bc108bda0206 Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod 8335134: Test com/sun/jdi/BreakpointOnClassPrepare.java timeout Reviewed-by: dholmes Backport-of: 4e8cbf884ab1eee9c3110712ab62edc706e948ba - PR: https://git.openjdk.org/jdk/pull/19976
Re: RFR: 8332124: Jcmd processing should accept the "help" sub option as command argument [v3]
On Fri, 28 Jun 2024 14:39:12 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Updating copyright header >> - Modifying usage to --help and -help. Updated ensuing test case to test >> both >> - Updating copyright headers > > One question. With the current implementation, this is the check in place to > offer help: > ```strncmp(args, " -help", 6) == 0 || strncmp(args, " --help", 7)``` > > I could change this to do `strncmp(args, " -h", 3)` instead but this is a bit > problematic as it would block any future flags that start out with `-h` from > working. > > I've been giving this some thought on how to implement a restrictive way to > check only for the arguments `-h` or `-help` while also allowing for other > arguments to be passed afterwards that should be ignored. > > I thought about perhaps using some type of regex matching with > ```std:regex``` but hotspot doesn't allow the use of global operators new and > delete. I don't want to overengineer this piece of logic so I was wondering > if there was a regex matching utility that I could leverage in HotSpot? > > The alternative would be to make the check more restrictive and not allow for > arguments after `-h` has been issued i.e. `strcmp(args, "-h") == 0`. > > Thanks for your help! Hi @SoniaZaldana sorry for the delay, I'm snowed in atm. > One question. With the current implementation, this is the check in place to > offer help: `strncmp(args, " -help", 6) == 0 || strncmp(args, " --help", 7)` > > I could change this to do `strncmp(args, " -h", 3)` instead but this is a bit > problematic as it would block any future flags that start out with `-h` from > working. Well, we restrict future compatibility no matter what we do. > > I've been giving this some thought on how to implement a restrictive way to > check only for the arguments `-h` or `-help` while also allowing for other > arguments to be passed afterwards that should be ignored. > > I thought about perhaps using some type of regex matching with `std:regex` > but hotspot doesn't allow the use of global operators new and delete. God no :) > I don't want to overengineer this piece of logic so I was wondering if there > was a regex matching utility that I could leverage in HotSpot? No need, you can do this with normal C tools. with p pointing to start of arguments: - forward p and walk all spaces, break out for \0 - strncmp with "-h", size 2 - if follow-up char is either blank or \0, its a help command. If we agree to just ignore follow up arguments, you can now just rebuild the command as "help commandname". - bonus for writing a warning if there are arguments after help > > The alternative would be to make the check more restrictive and not allow for > arguments after `-h` has been issued i.e. `strcmp(args, "-h") == 0`. > > Thanks for your help! Cheers - PR Comment: https://git.openjdk.org/jdk/pull/19776#issuecomment-2202046212
