RFR: JDK-8318636: Add jcmd to print annotated process memory map [v3]

[Johan Sjölen]

On Thu, 26 Oct 2023 15:02:53 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Analysts and supporters often use /proc/xx/maps to make sense of the memory footprint of a process.
>> 
>> Interpreting the memory map correctly can help when used as a complement to other tools (e.g. NMT). There even exist tools out there that attempt to annotate the process memory map with JVM information.
>> 
>> That, however, can be more accurately done from within the JVM, at least for mappings originating from hotspot. After all, we can correlate the mapping information in NMT with the VMA map.
>> 
>> Example output (from a spring petstore run):
>> 
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>> 
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have an experimental patch that works with malloc'ed memory, but it is not ready yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Merge master and solve merge conflicts
>  - small fixes
>  - start from VM op; show more thread details
>  - start

Hi,

Thank you for this PR. These comments are just a first pass, I haven't finished going through the code.

src/hotspot/os/linux/memMapPrinter_linux.cpp line 32:

> 30: #include "utilities/globalDefinitions.hpp"
> 31: 
> 32: struct proc_maps_info_t {

`struct ProcMapsInfo` please. The `_t` is apparently reserved for POSIX, and I prefer our structs and classes to not look like they're coming from a C library anyway.

src/hotspot/os/linux/memMapPrinter_linux.cpp line 34:

> 32: struct proc_maps_info_t {
> 33:   unsigned long long from = 0;
> 34:   unsigned long long to = 0;

Can we not use `uintptr_t` here? scanf directive: https://stackoverflow.com/questions/12228227/how-to-get-pointer-sized-integer-using-scanf

src/hotspot/os/linux/memMapPrinter_linux.cpp line 39:

> 37:   char dev[20 + 1];
> 38:   char inode[20 + 1];
> 39:   char filename[1024 + 1];

Maybe use `PATH_MAX` here? Potentially +1 to account for null byte.

src/hotspot/os/linux/memMapPrinter_linux.cpp line 47:

> 45:     if (items_read < 2) {
> 46:       return false;
> 47:     }

Not necessary thanks to the other return value!

src/hotspot/os/linux/memMapPrinter_linux.cpp line 72:

> 70:       "from                 to                 "
> 71: #else
> 72:       "from         to         "

16+1 and 8+1 spaces, depending on how long the addresses are I assume? Your choice, but might be good to note that.

src/hotspot/os/linux/memMapPrinter_linux.cpp line 81:

> 79:   FILE* f = os::fopen("/proc/self/maps", "r");
> 80:   if (f != nullptr) {
> 81:     char line[1024];

This looks like a bug: A line may at most be 1024 characters, but `scan_proc_maps_line` may clearly read more than 1024 characters. Yes, it won't read out of bounds, but it limits the parser unnecessarily.

src/hotspot/share/services/memMapPrinter.hpp line 43:

> 41:   const void* to() const { return _to; }
> 42:   virtual void print_details_1(outputStream* st) const {} // To be printed before VM annotations
> 43:   virtual void print_details_2(outputStream* st) const {} // To be printed before VM annotations

May we please have better names :)?

test/hotspot/jtreg/serviceability/dcmd/vm/SystemMapTest.java line 48:

> 46:         if (output.getOutput().contains("please enable Native Memory Tracking")) {
> 47:             have_nmt = false;
> 48:         }

`boolean have_nmt = output.getOutput().contains....`

-------------

PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1701276028
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374325262
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374327488
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374255012
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374253123
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374318000
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374322993
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374313507
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374311277