On Mon, 28 Oct 2024 14:40:44 GMT, Simon Tooke <sto...@openjdk.org> wrote:
>> This is a port of #16301 to macOS. >> >> System.map and System.dump_map are implemented using the macOS API and >> provide roughly the same information in the same format. Most of the heavy >> lifting was implemented by @tstuefe in >> https://github.com/openjdk/jdk/pull/16301 - this PR adds the macOS >> implementation and enables the common code for macOS 64 bit. >> >> The System.map tests are also reworked to be cleaner for the three >> implementations. >> >> [Sample >> output](https://github.com/user-attachments/files/17517412/sample.txt) > > Simon Tooke has updated the pull request incrementally with one additional > commit since the last revision: > > changes from review Looking good, small nits remain. Could you share an example output (maybe one run with G1, one with ZGC?) src/hotspot/os/bsd/memMapPrinter_macosx.cpp line 79: > 77: char buf[PATH_MAX]; > 78: buf[0] = 0; > 79: proc_regionfilename(getpid(), (uint64_t) _address, buf, > sizeof(buf)); I'd probably guard after the API call, just to be sure, with `buf[sizeof(buf)-1] = '\0'`. I could not find a description of this API anywhere, so no idea what its rules about zero termination are when buffer overflows. src/hotspot/os/bsd/memMapPrinter_macosx.cpp line 101: > 99: if (valid_share_mode) { > 100: int share_mode = rinfo.pri_share_mode; > 101: //share_mode = share_mode == SM_LARGE_PAGE || share_mode == > SM_PRIVATE_ALIASED ? SM_PRIVATE; remnant? src/hotspot/os/bsd/memMapPrinter_macosx.cpp line 196: > 194: > 195: bool is_private = region_info.pri_share_mode == SM_PRIVATE || > region_info.pri_share_mode == SM_PRIVATE_ALIASED; > 196: bool is_shared = region_info.pri_share_mode == SM_SHARED || > region_info.pri_share_mode == SM_SHARED_ALIASED || region_info.pri_share_mode > == SM_TRUESHARED || region_info.pri_share_mode == SM_COW; give us a line break :-) ? src/hotspot/os/bsd/memMapPrinter_macosx.cpp line 212: > 210: } > 211: > 212: st->print_cr("Number of mappings: %u", _num_mappings); If you reshuffle the lines (move this up), you can combine the conditions for err_vm ==/!= KERN_SUCCESS into one statement, would be easier to read src/hotspot/os/bsd/memMapPrinter_macosx.cpp line 336: > 334: } > 335: > 336: #endif // __APPLE__ whitespace error? test/hotspot/jtreg/serviceability/dcmd/vm/SystemMapTestBase.java line 180: > 178: macOSbase + macprivate + space + someNumber + space + > "META.*", > 179: // parts of metaspace should be uncommitted > 180: //regexBase + "-" + space + "META.*", ?? remnant ------------- PR Review: https://git.openjdk.org/jdk/pull/20953#pullrequestreview-2401027472 PR Review Comment: https://git.openjdk.org/jdk/pull/20953#discussion_r1820295899 PR Review Comment: https://git.openjdk.org/jdk/pull/20953#discussion_r1820296647 PR Review Comment: https://git.openjdk.org/jdk/pull/20953#discussion_r1820297843 PR Review Comment: https://git.openjdk.org/jdk/pull/20953#discussion_r1820299881 PR Review Comment: https://git.openjdk.org/jdk/pull/20953#discussion_r1820291338 PR Review Comment: https://git.openjdk.org/jdk/pull/20953#discussion_r1820301511
