Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Mon, 3 Jan 2022 16:11:02 GMT, Daniel D. Daugherty wrote: >> @dcubed-ojdk: I just ran into this in the Julia runtime and reached a >> similar conclusion/fix. Did you find out anything more about why this >> happens? At a glance, I didn't see any blatant `addr == -1`-style checks in >> the latest dyld open-source dump. > > @dnadlinger - No I haven't chased this any further. > > Also please see this note in the bug report: > > https://bugs.openjdk.java.net/browse/JDK-8273967?focusedCommentId=14455403&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14455403 @dcubed-ojdk: Thanks for taking the time to replyr regardless! The dlopen/dladdr/… man pages all show up fine on my machine, by the way (macOS 12.1). - 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 [v2]
On Fri, 31 Dec 2021 23:54:22 GMT, David Nadlinger wrote: >> @gerard-ziemski - Thanks for the re-review! > > @dcubed-ojdk: I just ran into this in the Julia runtime and reached a similar > conclusion/fix. Did you find out anything more about why this happens? At a > glance, I didn't see any blatant `addr == -1`-style checks in the latest dyld > open-source dump. @dnadlinger - No I haven't chased this any further. Also please see this note in the bug report: https://bugs.openjdk.java.net/browse/JDK-8273967?focusedCommentId=14455403&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14455403 - 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 [v2]
On Fri, 5 Nov 2021 14:56:44 GMT, Daniel D. Daugherty wrote: >> Thank you Dan for the fix! > > @gerard-ziemski - Thanks for the re-review! @dcubed-ojdk: I just ran into this in the Julia runtime and reached a similar conclusion/fix. Did you find out anything more about why this happens? At a glance, I didn't see any blatant `addr == -1`-style checks in the latest dyld open-source dump. - 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 [v2]
On Fri, 5 Nov 2021 14:49:45 GMT, Gerard Ziemski wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Thank you Dan for the fix! @gerard-ziemski - Thanks for the re-review! - 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 [v2]
On Thu, 4 Nov 2021 22:59:39 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]. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8273967.cr1.patch Marked as reviewed by gziemski (Committer). Thank you Dan for the fix! - 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 [v2]
On Fri, 5 Nov 2021 05:16:28 GMT, Thomas Stuefe wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Looks good! @tstuefe - Thanks for the re-review! - 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 [v2]
On Thu, 4 Nov 2021 22:59:39 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]. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8273967.cr1.patch Looks good! - Marked as reviewed by stuefe (Reviewer). 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 [v2]
> 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]. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8273967.cr1.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/6193/files - new: https://git.openjdk.java.net/jdk/pull/6193/files/f57d7f0d..ecb230d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6193&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6193&range=00-01 Stats: 48 lines in 1 file changed: 16 ins; 28 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6193/head:pull/6193 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]. This version has been tested with Mach5 Tier1 and with runs of the specific tests using release and debug bits on macosx-aarch64 and macosx-x64 test machines running macOS12. - 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 [v2]
On Thu, 4 Nov 2021 05:13:29 GMT, Thomas Stuefe wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Marked as reviewed by stuefe (Reviewer). @tstuefe and @gerard-ziemski - please re-review when you get the chance. - 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 21:14:17 GMT, Gerard Ziemski 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 929: > >> 927: #endif >> 928: >> 929: #if defined(__APPLE__) > > Why not just do: > > `#else` > > here instead and collapse these 3 lines into 1? Hmmm... I'll take a look at doing that. > 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? Hmmm I'll take a look at cleaning this up a bit. > 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`? That's the 5-parameter version of decode() and it doesn't have `ShouldNotReachHere`. So if that code site is called and returns `false`, then we get into `dll_address_to_library_name()` and reach this `dladdr()` call which will accept the "-1"... - 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 Thu, 4 Nov 2021 05:12:48 GMT, Thomas Stuefe 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. > >> 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! I've had a chance to think about this overnight and I'm not liking my duplication of code so I'm going to look at adding a wrapper that is called by the two calls sites where know I need the special handling. - 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 closing the loop on my previous replies. @gerard-ziemski - Thanks for the review! I'm going to make more tweaks to this fix and will update the PR after my test cycle is complete. - 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 Thu, 4 Nov 2021 05:12:32 GMT, Thomas Stuefe 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. >> >> 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(). > >> 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. Oracle does not build BSD in head. At one point, Dmitry Samersoff used to build BSD in his lab, but I don't know if he still does that. - 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: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
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
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
RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
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]. - Commit messages: - 8273967.cr0.patch Changes: https://git.openjdk.java.net/jdk/pull/6193/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6193&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273967 Stats: 50 lines in 4 files changed: 43 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6193/head:pull/6193 PR: https://git.openjdk.java.net/jdk/pull/6193
