RFR: JDK-8277029: JMM GetDiagnosticXXXInfo APIs should verify output array sizes
jmm_GetDiagnosticCommandArgumentsInfo and jmm_GetDiagnosticCommandInfo are used to query the hotspot about diagnostic commands. They provide output arrays for the information: void jmm_GetDiagnosticCommandArgumentsInfo(JNIEnv *env, jstring command, dcmdArgInfo* infoArray) but array size is implicitly assumed to be known to both caller and callee. Caller and callee negotiate those sizes in prior steps, but things can go wrong. E.g. I recently hunted a bug where `DCmd::number_arguments()` was off - did not reflect the real number of its jcmd parameters - which led to a hidden memory overwriter. Thankfully, JDK-8264565 rewrote the dcmd framework to deal with this particular issue (The VM I analyzed was older). Still, it would be good if we had additional safety measures here. - Testing: - manual tests with artificially induced error in one dcmd for debug, release - GHAs (which include tier1 serviceability jcmd tests which use JMX and exercise these APIs) - Commit messages: - explicitly pass output array size and check it in hotspot Changes: https://git.openjdk.java.net/jdk/pull/6363/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6363&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277029 Stats: 18 lines in 3 files changed: 8 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/6363.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6363/head:pull/6363 PR: https://git.openjdk.java.net/jdk/pull/6363
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 17:48:36 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > Marked as reviewed by cjplummer (Reviewer). Thanks for the reviews @plummercj, @sspitsyn, @schmelter-sap! - PR: https://git.openjdk.java.net/jdk/pull/5849
Integrated: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend
On Thu, 7 Oct 2021 13:50:49 GMT, Richard Reingruber wrote: > This change fixes deadlocks described in the JBS-bug by: > > * Releasing `handlerLock` before waiting on `threadLock` in > `blockOnDebuggerSuspend()` > > * Notifying on `threadLock` in `threadControl_reset()` > > Also the actions in handleAppResumeBreakpoint() are moved/deferred until > doPendingTasks() runs. This is necessary because an event handler must not > release handlerLock first and foremost because handlers are called while > iterating the handler chain for an event type which is protected by > handlerLock > (see https://github.com/openjdk/jdk/pull/5805) > > The first commit delays the cleanup actions after leaving the loop in > `debugLoop_run()`. It allows to reproduce the deadlock running the dispose003 > test with the command > > > make run-test > TEST=test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003 > > > The second commit adds a new test that reproduces the deadlock when calling > threadControl_resumeThread() while a thread is waiting in > blockOnDebuggerSuspend(). > > The third commit contains the fix described above. With it the deadlocks > cannot be reproduced anymore. > > The forth commit removes the diagnostic code introduced with the first commit > again. > > The fix passed > > test/hotspot/jtreg/serviceability/jdwp > test/jdk/com/sun/jdi > test/hotspot/jtreg/vmTestbase/nsk/jdwp > test/hotspot/jtreg/vmTestbase/nsk/jdi This pull request has now been integrated. Changeset: ca2efb73 Author:Richard Reingruber URL: https://git.openjdk.java.net/jdk/commit/ca2efb73f59112d9be2ec29db405deb4c58dd435 Stats: 333 lines in 2 files changed: 314 ins; 14 del; 5 mod 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend Reviewed-by: cjplummer, sspitsyn, rschmelter - PR: https://git.openjdk.java.net/jdk/pull/5849
Integrated: JDK-8276983: Small fixes to DumpAllocStat::print_stats
On Thu, 11 Nov 2021 06:15:32 GMT, Thomas Stuefe wrote: > When looking at CDS code in the context of Lilliput, I had to spend some time > in DumpAllocStat::print(). I noticed two small things which can be fixed > independently: > > - the divide-by-zero check at lines 45ff is not needed, since `percent_of` > does this already. It also can cause the asserts at the end of the function > to fire wrongly. > > - About those asserts: it makes sense to flush the debug message before scope > end, otherwise, we won't see the debug message if the asserts fire. If they > fire, the debug message would be helpful. > > I also improved the assert message somewhat. I noticed that all sizes in this > method are `int`, but left it that way because changing it would have too > many ripple effects. I guess we won't get CDS archives beyond int_max any > time soon. > > Thanks, Thomas This pull request has now been integrated. Changeset: 296780c7 Author:Thomas Stuefe URL: https://git.openjdk.java.net/jdk/commit/296780c7ae5c129d24997007600f428b697d3365 Stats: 13 lines in 1 file changed: 3 ins; 8 del; 2 mod 8276983: Small fixes to DumpAllocStat::print_stats Reviewed-by: dholmes, iklam - PR: https://git.openjdk.java.net/jdk/pull/6347
Re: RFR: JDK-8276983: Small fixes to DumpAllocStat::print_stats
On Mon, 15 Nov 2021 00:39:53 GMT, David Holmes wrote: >> When looking at CDS code in the context of Lilliput, I had to spend some >> time in DumpAllocStat::print(). I noticed two small things which can be >> fixed independently: >> >> - the divide-by-zero check at lines 45ff is not needed, since `percent_of` >> does this already. It also can cause the asserts at the end of the function >> to fire wrongly. >> >> - About those asserts: it makes sense to flush the debug message before >> scope end, otherwise, we won't see the debug message if the asserts fire. If >> they fire, the debug message would be helpful. >> >> I also improved the assert message somewhat. I noticed that all sizes in >> this method are `int`, but left it that way because changing it would have >> too many ripple effects. I guess we won't get CDS archives beyond int_max >> any time soon. >> >> Thanks, Thomas > > Hi Thomas, > > Seems okay. Relying on percent_of rather than passing in 1 does seem to > change what will be output, but AFAICS we should never be able to pass zero > in the first place. > > Thanks, > David Thanks @dholmes-ora and @iklam ! - PR: https://git.openjdk.java.net/jdk/pull/6347
Re: RFR: JDK-8276983: Small fixes to DumpAllocStat::print_stats
On Thu, 11 Nov 2021 06:15:32 GMT, Thomas Stuefe wrote: > When looking at CDS code in the context of Lilliput, I had to spend some time > in DumpAllocStat::print(). I noticed two small things which can be fixed > independently: > > - the divide-by-zero check at lines 45ff is not needed, since `percent_of` > does this already. It also can cause the asserts at the end of the function > to fire wrongly. > > - About those asserts: it makes sense to flush the debug message before scope > end, otherwise, we won't see the debug message if the asserts fire. If they > fire, the debug message would be helpful. > > I also improved the assert message somewhat. I noticed that all sizes in this > method are `int`, but left it that way because changing it would have too > many ripple effects. I guess we won't get CDS archives beyond int_max any > time soon. > > Thanks, Thomas LGTM. Good clean up. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6347
Re: RFR: JDK-8276983: Small fixes to DumpAllocStat::print_stats
On Thu, 11 Nov 2021 06:15:32 GMT, Thomas Stuefe wrote: > When looking at CDS code in the context of Lilliput, I had to spend some time > in DumpAllocStat::print(). I noticed two small things which can be fixed > independently: > > - the divide-by-zero check at lines 45ff is not needed, since `percent_of` > does this already. It also can cause the asserts at the end of the function > to fire wrongly. > > - About those asserts: it makes sense to flush the debug message before scope > end, otherwise, we won't see the debug message if the asserts fire. If they > fire, the debug message would be helpful. > > I also improved the assert message somewhat. I noticed that all sizes in this > method are `int`, but left it that way because changing it would have too > many ripple effects. I guess we won't get CDS archives beyond int_max any > time soon. > > Thanks, Thomas Hi Thomas, Seems okay. Relying on percent_of rather than passing in 1 does seem to change what will be output, but AFAICS we should never be able to pass zero in the first place. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6347
