RFR: JDK-8256864: [windows] Improve tracing for mapping errors [v5]

[Richard Reingruber]

On Tue, 1 Dec 2020 20:34:09 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> To analyze JDK-8256729 further, we need more tracing:
>> 
>> 1) We should print all mappings inside the split area if os::split_reserved_memory() fails
>> 
>> 2) The print-mapping code on windows has some shortcomings:
>> - should not probe for mappings outside of what we know are valid address ranges for Windows
>> - should handle wrap-arounds - it should be possible to print the whole address space
>> - Protection information is printed wrong (MEMORY_BASIC_INFORMATION.Protect would be the correct member)
>> - should be printed in a more compact manner - base address should be on the same line as the first region
>> - maybe adorned with some basic range info, e.g. library mappings
>> 
>> Tested in our nightlies for a week now, works fine.
>> 
>> Example output see https://github.com/openjdk/jdk/files/5622718/mapping-printout.txt
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Feedback Ioi

Hi Thomas,

I've got a few minor comments for you.

Have you got a sample of the trace you could share in this PR?

Thanks, Richard.

src/hotspot/os/windows/os_windows.cpp line 3220:

> 3218:     log_warning(os)("os::split_reserved_memory failed for " RANGE_FORMAT,
> 3219:                     RANGE_FORMAT_ARGS(base, size));
> 3220:     os::print_memory_mappings((char*)base, size, tty);

The type cast for `base` is redundant.

src/hotspot/os/windows/os_windows.cpp line 3218:

> 3216:                   (attempt_reserve_memory_at(split_address, size - split) != NULL);
> 3217:   if (!rc) {
> 3218:     log_warning(os)("os::split_reserved_memory failed for " RANGE_FORMAT,

I'm not very much in favor of the closing ")". It will confuse editors trying to match brackets/parenthesis when reading logs. Also I think it is redundant because an inclusive end does not make much sense without specifying how many bytes are included at the end address.

I see there are already uses of that style so I won't veto if you prefer to keep it.

src/hotspot/os/windows/os_windows.cpp line 6074:

> 6072:       char buf[MAX_PATH];
> 6073:       int dummy;
> 6074:       if (os::dll_address_to_library_name(allocation_base, buf, sizeof(buf), &dummy)) {

You could pass nullptr instead of &dummy

src/hotspot/os/windows/os_windows.cpp line 6001:

> 5999:   const int errval = 0xDE210244;
> 6000:   for (int i = 0; i < num_words; i++) {
> 6001:     v[i] = SafeFetch32((int*)p + i, errval);

I think you will get a sign extend of the loaded 4 bytes to 8 bytes intptr_t on 64 bit. This will be confusing. What about using int[] for v and INTX_FORMAT for printing?
Alternatively you could leave it like this and use SafeFetchN(). I'd think that would be best.

src/hotspot/os/windows/os_windows.cpp line 6141:

> 6139:       //  is large, this may take a long time. Therefore lets stop right away if the address
> 6140:       //  is outside of what we know are valid addresses on Windows. Also, add a loop fuse.
> 6141:       static const address end_phys = (address)(LP64_ONLY(0x7ffffffffffULL) NOT_LP64(3*G));

This is a virtual address, right? So better name it end_virt?

src/hotspot/os/windows/os_windows.cpp line 6108:

> 6106:   address end = start + bytes;
> 6107:   address p = start;
> 6108:   if (p == NULL) { // Lets skip the zero pages.

According to hotspot-style.md nullptr should be preferred.

src/hotspot/share/runtime/os.cpp line 1742:

> 1740: // Prints all mappings
> 1741: void os::print_memory_mappings(outputStream* st) {
> 1742:   os::print_memory_mappings(NULL, (size_t)-1, st);

According to hotspot-style.md nullptr should be preferred.

test/hotspot/gtest/runtime/test_os.cpp line 504:

> 502:   buf[0] = '\0';
> 503:   stringStream ss(buf, buflen);
> 504:   if (start != NULL) {

According to hotspot-style.md nullptr should be preferred.

test/hotspot/gtest/runtime/test_os.cpp line 523:

> 521: 
> 522: TEST_VM(os, show_mappings_full_range) {
> 523:   test_show_mappings(NULL, 0);

According to hotspot-style.md nullptr should be preferred.

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

PR: https://git.openjdk.java.net/jdk/pull/1390