Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v2]
On Mon, 26 Jun 2023 05:06:13 GMT, Chris Plummer wrote:
>> Tom Rodriguez has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Flatten nested ifs
>> - Adjust ordering of message
>> - Adjust printing
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c line 302:
>
>> 300: // The base of the library is offset by a random amount which
>> ends up as a load command with a
>> 301: // filesize of 0. This must be ignored otherwise the base
>> address of the library is wrong.
>> 302: if (segcmd.filesize != 0 && add_map_info(ph, fd, segcmd.fileoff,
>> segcmd.vmaddr, segcmd.vmsize, segcmd.flags) == NULL) {
>
> I actually preferred this part with two `if` statements. Seems easier to
> read that way.
Ok. I reverted it to 2 ifs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14569#discussion_r1242483550
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v2]
On Fri, 23 Jun 2023 19:31:20 GMT, Tom Rodriguez wrote:
>> This is a minor fix to core file reading on macos x. I can confirm that
>> after this fix I can run the problem listed SA core file tests on Ventura.
>
> Tom Rodriguez has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Flatten nested ifs
> - Adjust ordering of message
> - Adjust printing
src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c line 302:
> 300: // The base of the library is offset by a random amount which ends
> up as a load command with a
> 301: // filesize of 0. This must be ignored otherwise the base address
> of the library is wrong.
> 302: if (segcmd.filesize != 0 && add_map_info(ph, fd, segcmd.fileoff,
> segcmd.vmaddr, segcmd.vmsize, segcmd.flags) == NULL) {
I actually preferred this part with two `if` statements. Seems easier to read
that way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14569#discussion_r1241570338
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v2]
On Fri, 23 Jun 2023 19:31:20 GMT, Tom Rodriguez wrote: >> This is a minor fix to core file reading on macos x. I can confirm that >> after this fix I can run the problem listed SA core file tests on Ventura. > > Tom Rodriguez has updated the pull request incrementally with three > additional commits since the last revision: > > - Flatten nested ifs > - Adjust ordering of message > - Adjust printing Update looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14569#pullrequestreview-1495996925
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v2]
> This is a minor fix to core file reading on macos x. I can confirm that > after this fix I can run the problem listed SA core file tests on Ventura. Tom Rodriguez has updated the pull request incrementally with three additional commits since the last revision: - Flatten nested ifs - Adjust ordering of message - Adjust printing - Changes: - all: https://git.openjdk.org/jdk/pull/14569/files - new: https://git.openjdk.org/jdk/pull/14569/files/9852ed16..2ebe4d09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14569=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14569=00-01 Stats: 12 lines in 1 file changed: 4 ins; 5 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14569.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14569/head:pull/14569 PR: https://git.openjdk.org/jdk/pull/14569
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v2]
On Wed, 21 Jun 2023 19:56:58 GMT, Chris Plummer wrote:
>> Tom Rodriguez has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Flatten nested ifs
>> - Adjust ordering of message
>> - Adjust printing
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c line 309:
>
>> 307: print_debug("LC_SEGMENT_64 added: nsects=%d fileoff=0x%llx
>> vmaddr=0x%llx vmsize=0x%llx filesize=0x%llx %s\n",
>> 308: segcmd.nsects, segcmd.fileoff, segcmd.vmaddr,
>> segcmd.vmsize,
>> 309: segcmd.filesize, [0]);
>
> It would be nice to include this print_debug for the `filesize == 0` case.
> Maybe you can move it outside of the `if` and print `added` or `skipped`
> conditional on `filesize`.
Good idea. I've flattened the ifs a little and fixed the message.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14569#discussion_r1240227363
