Re: [External] : Re: RFC: Extend DCmd(Diagnostic-Command) framework to support Java level DCmd
Hi Denghui, Yes, there are other ways the same thing could be accomplished like sockets or signals, but all of this is outside of the purview of the JDK, and therefore we don't become responsible for its design, maintenance, and potential security concerns. EnableUserLevelDCmd doesn't really fix any of these concerns, because an app can just always launch with this flag enabled. It really should be reserved for launching a JVM for the specific purpose of gathering some extra diagnostic data, but there is no way to enforce that. Anyway, I'm not the gatekeeper on this. Just expressing some of my concerns. Others have done the same. I think we've seen a lack of enthusiasm in favor of doing this except from you. I would be good to see input from others that would like this feature in place. cheers, Chris On 11/1/21 8:09 PM, Denghui Dong wrote: Hi Chris, Thank you for the comments. Yes, we have no good way to restrict the user registration commands to only include diagnosis-related operations, but in my opinion, this does not seem to be a problem that must be solved perfectly. The following are my thoughts. This extension is an entry that triggers the operation that the user wants to perform (similar to the Signal Handler mechanism but with a name and parameters). Even without this extension, the user can have other ways to achieve the same goal. On the one hand, we could standardize the usage scenarios of the API on the document(Indeed, users can still write programs not in accordance with the specifications, for example, users can implement multiple calls to the same object's hachCode method to return different values or make an object alive again during finalize method executing). On the other hand, we can add some restrictions to help users make better use of this extension. e.g we can add a new VM option, such as EnableUserLevelDCmd, the application can only register customer commands when this option is enabled. Or from another perspective, can we allow users to do some non-diagnostic-related operations in custom commands? Best, Denghui -- From:Chris Plummer Send Time:2021年11月2日(星期二) 03:35 To:董登辉(卓昂) ; serviceability-dev ; hotspot-dev Subject:Re: RFC: Extend DCmd(Diagnostic-Command) framework to support Java level DCmd I have similar concerns to those others have expressed, so I'll try to add something new to the discussion and not just repeat. DCMDs have historically been very VM centric. That's not to say they aren't useful for debugging applications, but they do so by providing VM related info like stack traces, heap dumps, and class histograms. Also hotspot has been the gatekeeper for new DCMDs, meaning that new ones do not get added without going through the hotspot review process. Allowing any application or framework to add a DCMD changes this VM centric view in a way that concerns me. This approach allows a DCMD to pretty much do anything (java security not withstanding). App writers could even use them to provide a user facing interface. For example, if an app has some sort internal database, it could allow users to query it via a DCMD, and maybe even suggest that users write simple shell scripts that use jcmd to do these queries. Allowing this type of non-diagnostic usage seems like a path we don't want to go down, yet I don't see how it can be prevented once you allow applications to add DCMDs. Chris On 10/25/21 1:37 AM, Denghui Dong wrote: Hi there! We'd like to discuss a proposal for extending the current DCmd framework to support Java level DCmd. At present, DCmd only allows the VM to register commands, which can be called through jcmd or JMX. It would be beneficial if the user could create their own commands. The idea of this extension originally came from our internal Java agent that detects the misusage of Unsafe API. This agent can collect the call sites that allocate or free direct memory in the application(NMT could not do it IMO
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 19:19:11 GMT, Daniel D. Daugherty wrote: > It's actually fairly common to have Mac-specific stuff in the BSD files. The > macOS port was built on top of the BSD port and the BSD port was built by > copying a LOT of code from Linux into BSD specific files with modifications > as needed. I always wondered whether anyone actually builds the BSDs in head. I assume Oracle does not, right? I know there are downstream porters somewhere but only for old releases, or? > > If I pushed this change down into MachDecoder, then I would have to lose the > `ShouldNotReachHere()` call in order to not assert in non-release bits. I > don't think I want to do that since this may not be the only place that calls > the 6-arg version of decode(). Fair enough, thanks for the clarification. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 19:27:32 GMT, Daniel D. Daugherty wrote: > I took a quick look at the other calls to `dladdr()` in > src/hotspot/os/bsd/os_bsd.cpp and I'm not comfortable with changing those > uses without having a specific test case that I can use to investigate those > code paths. > > We are fairly early in our testing on macOS12 so I may run into a reason to > revisit this choice down the road. Okay! - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. Marked as reviewed by stuefe (Reviewer). Marked as reviewed by stuefe (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6193
Integrated: 8276615: Update CR number of some tests in ProblemList-zgc.txt
On Thu, 4 Nov 2021 00:46:49 GMT, Yasumasa Suenaga wrote: > Following tests are referred to > [JDK-8220624](https://bugs.openjdk.java.net/browse/JDK-8220624), however it > has been closed due to all subtasks were resolved. > > * resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java > * serviceability/sa/CDSJMapClstats.java > * serviceability/sa/ClhsdbJhisto.java > > We will work these problems in > [JDK-8276539](https://bugs.openjdk.java.net/browse/JDK-8276539), so we need > to update CR number of them. This pull request has now been integrated. Changeset: 558ee40a Author:Yasumasa Suenaga URL: https://git.openjdk.java.net/jdk/commit/558ee40a4ac158e5be8269c87b7e18af77dd14c5 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8276615: Update CR number of some tests in ProblemList-zgc.txt Reviewed-by: dholmes - PR: https://git.openjdk.java.net/jdk/pull/6244
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Thu, 4 Nov 2021 02:15:45 GMT, Ioi Lam wrote: >> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs >> owned by a specific user. No one uses these APIs, and they don't work anyway. >> >> The current code is very confusing to look at. Since we're likely to change >> code in this area for further container support, it's better to clean up the >> code now. >> >> - Remove all APIs that take a user name >> - Also removed PerfDataFile.getFile() methods that are unused >> - Cleaned up the code that looks up the hsperfdata_xxx files >> - Fix comments to explain what's happening >> - Avoid using Matcher.reset which is not thread-safe >> - Renamed confusing variables such as `userFilter` to make the code more >> readable >> - LocalVmManager.activeVms() probably doesn't need to be synchronized, but >> I kept it anyway to avoid unnecessary risks. >> >> Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers >> and have extensive use of the management tools). > > Ioi Lam 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 three additional commits since > the last revision: > > - Merge branch 'master' into > 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code > - @kevinjwalls and @plummercj review - (1) restore > PerfDataFile.userDirNamePattern, etc. (2) Fixed comments > - 8275185: Remove dead code and clean up jvmstat LocalVmManager Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Tue, 19 Oct 2021 22:02:58 GMT, Chris Plummer wrote:
>> Ioi Lam 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 three additional commits
>> since the last revision:
>>
>> - Merge branch 'master' into
>> 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code
>> - @kevinjwalls and @plummercj review - (1) restore
>> PerfDataFile.userDirNamePattern, etc. (2) Fixed comments
>> - 8275185: Remove dead code and clean up jvmstat LocalVmManager
>
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/LocalVmManager.java
> line 75:
>
>> 73: // 1.4.1 (or earlier?): the files are stored directly under
>> {tmpdir}/ with
>> 74: // the following pattern.
>> 75: Pattern oldtmpFilePattern =
>> Pattern.compile("^hsperfdata_[0-9]+(_[1-2]+)?$");
>
> So this pattern optionally has `_` followed by a sequence of 1's and 2's at
> the end? Seems odd.
I restored this line to use PerfDataFile.tmpFileNamePattern, as before my
changes. Yes, that's an odd way of naming a file.
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/LocalVmManager.java
> line 105:
>
>> 103:
>> 104:
>> 105: // 1.4.2 and later: Look for the files
>> {tmpdir}/hsperfdata_{any_user_name}/[0-0]+
>
> should be `[0-9]+`
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Tue, 26 Oct 2021 07:48:05 GMT, Kevin Walls wrote: > Nice cleanup - although was it not better for PerfDataFile.java to "own" the > definitions of what a perfdata filename looks like? That might be what Chris > was hinting at. There isn't really that _much_ flipping between two files. 8-) Hi Kevin, thanks for the review. I've moved the filename patterns back to PerfDataFile.java as you suggested. - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Wed, 20 Oct 2021 02:23:25 GMT, Serguei Spitsyn wrote: > Hi Ioi, It looks good to me except the line 105 commented by Chris. I wonder > how should this be tested to make sure there are no regressions. Do we really > care about pre 1.4.2 formats? If so, what is the way to test it? We have a > little lack of knowledge in this area. I guess, we need the performance team > to get involved into this review. Thanks, Serguei I am not sure about 1.4.2 formats. I guess we don't really care, but we should probably remove it in a separate RFE. I am trying to keep this PR from having any functional changes, and will just remove dead code. For testing, I am running tiers 1-4. Will that be enough? I'll ping the performance team. Thanks - Ioi - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs > owned by a specific user. No one uses these APIs, and they don't work anyway. > > The current code is very confusing to look at. Since we're likely to change > code in this area for further container support, it's better to clean up the > code now. > > - Remove all APIs that take a user name > - Also removed PerfDataFile.getFile() methods that are unused > - Cleaned up the code that looks up the hsperfdata_xxx files > - Fix comments to explain what's happening > - Avoid using Matcher.reset which is not thread-safe > - Renamed confusing variables such as `userFilter` to make the code more > readable > - LocalVmManager.activeVms() probably doesn't need to be synchronized, but > I kept it anyway to avoid unnecessary risks. > > Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers > and have extensive use of the management tools). Ioi Lam 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 three additional commits since the last revision: - Merge branch 'master' into 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code - @kevinjwalls and @plummercj review - (1) restore PerfDataFile.userDirNamePattern, etc. (2) Fixed comments - 8275185: Remove dead code and clean up jvmstat LocalVmManager - Changes: - all: https://git.openjdk.java.net/jdk/pull/5923/files - new: https://git.openjdk.java.net/jdk/pull/5923/files/c2f3937b..bbeb7c16 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5923&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5923&range=00-01 Stats: 43203 lines in 1054 files changed: 33107 ins; 6126 del; 3970 mod Patch: https://git.openjdk.java.net/jdk/pull/5923.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5923/head:pull/5923 PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8276615: Update CR number of some tests in ProblemList-zgc.txt
On Thu, 4 Nov 2021 00:46:49 GMT, Yasumasa Suenaga wrote: > Following tests are referred to > [JDK-8220624](https://bugs.openjdk.java.net/browse/JDK-8220624), however it > has been closed due to all subtasks were resolved. > > * resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java > * serviceability/sa/CDSJMapClstats.java > * serviceability/sa/ClhsdbJhisto.java > > We will work these problems in > [JDK-8276539](https://bugs.openjdk.java.net/browse/JDK-8276539), so we need > to update CR number of them. Looks good and trivial. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6244
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 16:05:21 GMT, Daniel D. Daugherty wrote:
>> Yes, please
>
> The rationale for removing the is_exiting() check from `java_suspend()` was
> that it
> was redundant because the handshake code detected and handled the
> `is_exiting()`
> case so we didn't need to do that work twice.
>
> If we look at `HandshakeState::resume()` there is no logic for detecting or
> handling
> the possibility of an exiting thread. That being said, we have to look closer
> at what
> `HandshakeState::resume()` does and whether that logic can be harmful if
> executed
> on an exiting thread.
>
> Here's the code:
>
> bool HandshakeState::resume() {
> if (!is_suspended()) {
> return false;
> }
> MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
> if (!is_suspended()) {
> assert(!_handshakee->is_suspended(), "cannot be suspended without a
> suspend request");
> return false;
> }
> // Resume the thread.
> set_suspended(false);
> _lock.notify();
> return true;
> }
>
>
> I'm not seeing anything in `HandshakeState::resume()` that
> worries me with respect to an exiting thread. Of course, the
> proof is in the testing so I'll rerun the usual testing after
> deleting that code.
A suspended thread cannot be exiting - else the suspend logic is broken. So,
given you can call `resume()` on a not-suspended thread, as long as the
handshake code checks for `is_supended()` (which it does) then no explicit
`is_exiting` check is needed.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 15:45:06 GMT, Daniel D. Daugherty wrote:
>> src/hotspot/share/runtime/thread.cpp line 446:
>>
>>> 444: Thread* current_thread = nullptr;
>>> 445: if (checkTLHOnly) {
>>> 446: current_thread = Thread::current();
>>
>> This seems redundant due to line 463. You can just have a `if
>> (!checkTLHOnly)` block here.
>
> I suspect that the way that git is displaying the diffs is confusing you.
>
> We need `current_thread` set if we get to line 474 so we have to init
> `current_thread` on line 446 for the `checkTLHOnly == true` case and
> on line 463 for the `checkTLHOnly == false` case.
>
> I could simplify the logic by always setting current thread when it is
> declared on 444, but I was trying to avoid the call to `Thread::current()`
> until I actually needed it. I thought `Thread::current()` can be expensive.
> Is this no longer the case?
Sorry I missed that line 463 is still within the else from line 447.
Thread::current() is a compiler-defined thread-local access so should be
relatively cheap these days, but I have no numbers.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
RFR: 8276615: Update CR number of some tests in ProblemList-zgc.txt
Following tests are referred to [JDK-8220624](https://bugs.openjdk.java.net/browse/JDK-8220624), however it has been closed due to all subtasks were resolved. * resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java * serviceability/sa/CDSJMapClstats.java * serviceability/sa/ClhsdbJhisto.java We will work these problems in [JDK-8276539](https://bugs.openjdk.java.net/browse/JDK-8276539), so we need to update CR number of them. - Commit messages: - 8276615: Update CR number of some tests in ProblemList-zgc.txt Changes: https://git.openjdk.java.net/jdk/pull/6244/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6244&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276615 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6244.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6244/head:pull/6244 PR: https://git.openjdk.java.net/jdk/pull/6244
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. Looks good, just a few optional nitpicks that I personally would have done, if it were me doing the change. src/hotspot/os/bsd/os_bsd.cpp line 929: > 927: #endif > 928: > 929: #if defined(__APPLE__) Why not just do: `#else` here instead and collapse these 3 lines into 1? src/hotspot/os/bsd/os_bsd.cpp line 930: > 928: > 929: #if defined(__APPLE__) > 930: char localbuf[MACH_MAXSYMLEN]; This `__APPLE__` section is the only one, that I can see, using `MACH_MAXSYMLEN`, why not move: #if defined(__APPLE__) #define MACH_MAXSYMLEN 256 #endif here (i.e. just the `#define MACH_MAXSYMLEN 256` and minimize the need for` __APPLE__` sections? src/hotspot/os/bsd/os_bsd.cpp line 964: > 962: if (offset) *offset = -1; > 963: return false; > 964: } Do we need this here? Wouldn't the earlier call to `Decoder::decode(addr, localbuf, MACH_MAXSYMLEN, offset, dlinfo.dli_fbase))` catch this with `ShouldNotReachHere`? - Marked as reviewed by gziemski (Committer). PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 19:08:38 GMT, Daniel D. Daugherty wrote:
>> src/hotspot/os/bsd/os_bsd.cpp line 902:
>>
>>> 900: return false;
>>> 901: }
>>> 902: #endif
>>
>> We use dladdr() in several places in this code. I wonder whether it would
>> make sense to fix all of those with a wrapper instead:
>>
>> static int my_dladdr(const void* addr, Dl_info* info) {
>> if (addr != (void*)-1) {
>> return dladdr(addr, info);
>> } else {
>> // add comment here
>> return 0;
>> }
>> }
>> #define dladdr my_dladdr
>
> I'll take a look at the other calls to dladdr(). I'm trying to limit what I
> change
> here to things that actually failed in our test on macOS12 on X64 and aarch64.
I took a quick look at the other calls to `dladdr()` in
src/hotspot/os/bsd/os_bsd.cpp
and I'm not comfortable with changing those uses without having a specific test
case that I can use to investigate those code paths.
We are fairly early in our testing on macOS12 so I may run into a reason to
revisit
this choice down the road.
-
PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 17:49:40 GMT, Thomas Stuefe wrote:
>> macOS12 has changed the dladdr() function to accept "-1" as a valid address
>> and
>> we have functions that use dladdr() to convert DLL addresses into function or
>> library names. We also have a gtest that verifies that "-1" is not a valid
>> value to use
>> as a symbol address.
>>
>> As you might imagine, existing code that uses
>> `os::dll_address_to_function_name()`
>> or `os::dll_address_to_library_name()` can get quite confused (and sometimes
>> crash)
>> if an `addr` parameter of `-1` was allowed to be used.
>>
>> I've also made two cleanup changes as part of this fix:
>>
>> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that
>> should
>>be properly `#ifdef`'ed. There is also some code that makes sense for ELF
>> format
>>files, but not for Mach-O format files so that code needs to be excluded
>> on macOS.
>>
>> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a
>> comment on an
>> `#endif` that I fixed. That typo does not appear anywhere else in the
>> HotSpot code
>> base so I'd like to fix it with this bug ID since I'm in related areas.
>>
>> This fix has been tested with Mach5 Tier[1-6].
>
> src/hotspot/os/bsd/os_bsd.cpp line 902:
>
>> 900: return false;
>> 901: }
>> 902: #endif
>
> We use dladdr() in several places in this code. I wonder whether it would
> make sense to fix all of those with a wrapper instead:
>
> static int my_dladdr(const void* addr, Dl_info* info) {
> if (addr != (void*)-1) {
> return dladdr(addr, info);
> } else {
> // add comment here
> return 0;
> }
> }
> #define dladdr my_dladdr
I'll take a look at the other calls to dladdr(). I'm trying to limit what I
change
here to things that actually failed in our test on macOS12 on X64 and aarch64.
> src/hotspot/os/bsd/os_bsd.cpp line 918:
>
>> 916: // ranges like ELF. That makes sense since Mach-O can contain
>> binaries for
>> 917: // than one instruction set so there can be more than one address
>> range for
>> 918: // each "file".
>
> Small nit, it seems confusing to have a Mac-specific comment in the BSD
> section.
>
> Maybe this would be better in MachDecoder? E.g. implement the 6-arg version
> of decode() but stubbed out returning false, and with your comment there.
It's actually fairly common to have Mac-specific stuff in the BSD files. The
macOS
port was built on top of the BSD port and the BSD port was built by copying a
LOT
of code from Linux into BSD specific files with modifications as needed.
If I pushed this change down into MachDecoder, then I would have to lose the
`ShouldNotReachHere()` call in order to not assert in non-release bits. I don't
think I want to do that since this may not be the only place that calls the
6-arg version of decode().
-
PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. @tstuefe - Thanks for your review. - PR: https://git.openjdk.java.net/jdk/pull/6193
Integrated: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output"
On Mon, 1 Nov 2021 04:39:27 GMT, Jakob Cornell wrote: > This will fix a few issues with the tests added in #5290: > > - [x] intermittent failures > - [x] tests should use `failure` method to report problems rather than > throwing `AssertionError` This pull request has now been integrated. Changeset: c7f070f5 Author:Jakob Cornell Committer: Chris Plummer URL: https://git.openjdk.java.net/jdk/commit/c7f070f5f17dad661cc3296f2e3cd7a1cd5fc742 Stats: 25 lines in 2 files changed: 1 ins; 0 del; 24 mod 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output" Reviewed-by: cjplummer, iklam - PR: https://git.openjdk.java.net/jdk/pull/6182
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote:
> macOS12 has changed the dladdr() function to accept "-1" as a valid address
> and
> we have functions that use dladdr() to convert DLL addresses into function or
> library names. We also have a gtest that verifies that "-1" is not a valid
> value to use
> as a symbol address.
>
> As you might imagine, existing code that uses
> `os::dll_address_to_function_name()`
> or `os::dll_address_to_library_name()` can get quite confused (and sometimes
> crash)
> if an `addr` parameter of `-1` was allowed to be used.
>
> I've also made two cleanup changes as part of this fix:
>
> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that
> should
>be properly `#ifdef`'ed. There is also some code that makes sense for ELF
> format
>files, but not for Mach-O format files so that code needs to be excluded
> on macOS.
>
> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment
> on an
> `#endif` that I fixed. That typo does not appear anywhere else in the
> HotSpot code
> base so I'd like to fix it with this bug ID since I'm in related areas.
>
> This fix has been tested with Mach5 Tier[1-6].
Hi Daniel,
looks good. Small remarks below. I leave it up to you if you take my
suggestions.
Cheers, Thomas
src/hotspot/os/bsd/os_bsd.cpp line 902:
> 900: return false;
> 901: }
> 902: #endif
We use dladdr() in several places in this code. I wonder whether it would make
sense to fix all of those with a wrapper instead:
static int my_dladdr(const void* addr, Dl_info* info) {
if (addr != (void*)-1) {
return dladdr(addr, info);
} else {
// add comment here
return 0;
}
}
#define dladdr my_dladdr
src/hotspot/os/bsd/os_bsd.cpp line 918:
> 916: // ranges like ELF. That makes sense since Mach-O can contain
> binaries for
> 917: // than one instruction set so there can be more than one address
> range for
> 918: // each "file".
Small nit, it seems confusing to have a Mac-specific comment in the BSD
section.
Maybe this would be better in MachDecoder? E.g. implement the 6-arg version of
decode() but stubbed out returning false, and with your comment there.
-
PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. Ping! Any takers? - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 09:50:08 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/handshake.cpp line 350:
>>
>>> 348: }
>>> 349:
>>> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
>>> tlh_p, JavaThread* target) {
>>
>> Nit: can we drop the `_p` part of `tlh_p` please.
>
> Yes, please.
Fixed in handshake.[ch]pp.
>> src/hotspot/share/runtime/thread.cpp line 1764:
>>
>>> 1762: guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */
>>> true),
>>> 1763: "missing ThreadsListHandle in calling context.");
>>> 1764: if (is_exiting()) {
>>
>> Can't we remove this the same as we did for `java_suspend()`?
>
> Yes, please
The rationale for removing the is_exiting() check from `java_suspend()` was
that it
was redundant because the handshake code detected and handled the `is_exiting()`
case so we didn't need to do that work twice.
If we look at `HandshakeState::resume()` there is no logic for detecting or
handling
the possibility of an exiting thread. That being said, we have to look closer
at what
`HandshakeState::resume()` does and whether that logic can be harmful if
executed
on an exiting thread.
Here's the code:
bool HandshakeState::resume() {
if (!is_suspended()) {
return false;
}
MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
if (!is_suspended()) {
assert(!_handshakee->is_suspended(), "cannot be suspended without a suspend
request");
return false;
}
// Resume the thread.
set_suspended(false);
_lock.notify();
return true;
}
I'm not seeing anything in `HandshakeState::resume()` that
worries me with respect to an exiting thread. Of course, the
proof is in the testing so I'll rerun the usual testing after
deleting that code.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 01:21:50 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/thread.cpp line 446:
>
>> 444: Thread* current_thread = nullptr;
>> 445: if (checkTLHOnly) {
>> 446: current_thread = Thread::current();
>
> This seems redundant due to line 463. You can just have a `if
> (!checkTLHOnly)` block here.
I suspect that the way that git is displaying the diffs is confusing you.
We need `current_thread` set if we get to line 474 so we have to init
`current_thread` on line 446 for the `checkTLHOnly == true` case and
on line 463 for the `checkTLHOnly == false` case.
I could simplify the logic by always setting current thread when it is
declared on 444, but I was trying to avoid the call to `Thread::current()`
until I actually needed it. I thought `Thread::current()` can be expensive.
Is this no longer the case?
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8275729: Qualified method names in CodeHeap Analytics
On Tue, 2 Nov 2021 23:17:30 GMT, Vladimir Kozlov wrote: >> This PR changes nmethods names in `METHOD NAMES for CodeHeap` section to be >> qualified. >> Testing: >> - `make test TEST="gtest"`: Passed >> - `make run-test TEST="tier1"`: Passed >> - `make run-test TEST="tier2"`: Passed >> - `make run-test >> TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java`: Passed > > BTW, you need to update Copyright year in file. @vnkozlov Created PR https://github.com/openjdk/jdk/pull/6234 - PR: https://git.openjdk.java.net/jdk/pull/6200
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 01:19:21 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/handshake.cpp line 350:
>
>> 348: }
>> 349:
>> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
>> tlh_p, JavaThread* target) {
>
> Nit: can we drop the `_p` part of `tlh_p` please.
Yes, please.
> src/hotspot/share/runtime/thread.cpp line 1764:
>
>> 1762: guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */
>> true),
>> 1763: "missing ThreadsListHandle in calling context.");
>> 1764: if (is_exiting()) {
>
> Can't we remove this the same as we did for `java_suspend()`?
Yes, please
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr2.patch Marked as reviewed by rehn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4677
