Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JDK-8256864: [windows] Improve tracing for mapping errors #1390

Closed
wants to merge 8 commits into from
Closed

JDK-8256864: [windows] Improve tracing for mapping errors #1390

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7bba557
Initial patch
tstuefe Nov 23, 2020
71d8ef9
Merge
tstuefe Nov 27, 2020
be7acf6
Fix gtest for MacOS and AIX
tstuefe Nov 30, 2020
59ec1db
Merge
tstuefe Dec 1, 2020
e2b7165
Print a short snippet for unknown mappings
tstuefe Dec 1, 2020
de70507
Feedback Ioi
tstuefe Dec 1, 2020
89b5906
Feedback Richard
tstuefe Dec 2, 2020
bed9505
the last NULL is null
tstuefe Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 96 additions & 15 deletions src/hotspot/os/windows/os_windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3215,9 +3215,10 @@ void os::split_reserved_memory(char *base, size_t size, size_t split) {
(attempt_reserve_memory_at(base, split) != NULL) &&
(attempt_reserve_memory_at(split_address, size - split) != NULL);
if (!rc) {
log_warning(os)("os::split_reserved_memory failed for [" RANGE_FORMAT ")",
log_warning(os)("os::split_reserved_memory failed for " RANGE_FORMAT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I agree. Good point with the editors. Exclusive end is the usual default anyway.

I'll rework this in a follow up if there is a follow up (there probably will be one).

RANGE_FORMAT_ARGS(base, size));
assert(false, "os::split_reserved_memory failed for [" RANGE_FORMAT ")",
os::print_memory_mappings((char*)base, size, tty);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type cast for base is redundant.

assert(false, "os::split_reserved_memory failed for " RANGE_FORMAT,
RANGE_FORMAT_ARGS(base, size));
}

Expand Down Expand Up @@ -5989,19 +5990,54 @@ bool os::win32::find_mapping(address addr, mapping_info_t* mi) {
return rc;
}

// Helper for print_one_mapping: print n words, both as hex and ascii.
// Use Safefetch for all values.
static void print_snippet(const void* p, outputStream* st) {
static const int num_words = LP64_ONLY(3) NOT_LP64(6);
static const int num_bytes = num_words * sizeof(int);
intptr_t v[num_words];
const int errval = 0xDE210244;
for (int i = 0; i < num_words; i++) {
v[i] = SafeFetch32((int*)p + i, errval);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is a stupid mistake. I alternated several times between SafeFetchN and SafeFetch32. I must have committed an inconsistent version. Good catch.

if (v[i] == errval) {
tstuefe marked this conversation as resolved.
Show resolved Hide resolved
return;
}
}
st->put('[');
for (int i = 0; i < num_words; i++) {
st->print(INTPTR_FORMAT " ", v[i]);
}
const char* b = (char*)v;
st->put('\"');
for (int i = 0; i < num_bytes; i++) {
st->put(::isgraph(b[i]) ? b[i] : '.');
}
st->put('\"');
st->put(']');
}

// Helper function for print_memory_mappings:
// Given a MEMORY_BASIC_INFORMATION, containing information about a non-free region:
// print out all regions in that allocation. If any of those regions
// fall outside the given range [start, end), indicate that in the output.
// Return the pointer to the end of the allocation.
static address print_one_mapping(MEMORY_BASIC_INFORMATION* minfo, address start, address end, outputStream* st) {
assert(start != NULL && end != NULL && end > start, "Sanity");
// Print it like this:
//
// Base: <xxxxx>: [xxxx - xxxx], state=MEM_xxx, prot=x, type=MEM_xxx (region 1)
// [xxxx - xxxx], state=MEM_xxx, prot=x, type=MEM_xxx (region 2)
assert(minfo->State != MEM_FREE, "Not inside an allocation.");
address allocation_base = (address)minfo->AllocationBase;
address last_region_end = NULL;
st->print_cr("AllocationBase: " PTR_FORMAT ":", allocation_base);
#define IS_IN(p) (p >= start && p < end)
bool first_line = true;
bool is_dll = false;
for(;;) {
if (first_line) {
st->print("Base " PTR_FORMAT ": ", p2i(allocation_base));
} else {
st->print_raw(NOT_LP64 (" ")
LP64_ONLY(" "));
}
address region_start = (address)minfo->BaseAddress;
address region_end = region_start + minfo->RegionSize;
assert(region_end > region_start, "Sanity");
Expand All @@ -6014,19 +6050,40 @@ static address print_one_mapping(MEMORY_BASIC_INFORMATION* minfo, address start,
}
st->print("[" PTR_FORMAT "-" PTR_FORMAT "), state=", p2i(region_start), p2i(region_end));
switch (minfo->State) {
case MEM_COMMIT: st->print("MEM_COMMIT"); break;
case MEM_FREE: st->print("MEM_FREE"); break;
case MEM_RESERVE: st->print("MEM_RESERVE"); break;
case MEM_COMMIT: st->print_raw("MEM_COMMIT "); break;
case MEM_FREE: st->print_raw("MEM_FREE "); break;
case MEM_RESERVE: st->print_raw("MEM_RESERVE"); break;
default: st->print("%x?", (unsigned)minfo->State);
}
st->print(", prot=%x, type=", (unsigned)minfo->AllocationProtect);
st->print(", prot=%3x, type=", (unsigned)minfo->Protect);
switch (minfo->Type) {
case MEM_IMAGE: st->print("MEM_IMAGE"); break;
case MEM_MAPPED: st->print("MEM_MAPPED"); break;
case MEM_PRIVATE: st->print("MEM_PRIVATE"); break;
case MEM_IMAGE: st->print_raw("MEM_IMAGE "); break;
case MEM_MAPPED: st->print_raw("MEM_MAPPED "); break;
case MEM_PRIVATE: st->print_raw("MEM_PRIVATE"); break;
default: st->print("%x?", (unsigned)minfo->State);
}
// At the start of every allocation, print some more information about this mapping.
// Notes:
// - this could be beefed up a lot, similar to os::print_location
// - for now we just query the allocation start point. This may be confusing for cases where
// the kernel merges multiple mappings.
if (first_line) {
char buf[MAX_PATH];
int dummy;
if (os::dll_address_to_library_name(allocation_base, buf, sizeof(buf), &dummy)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could pass nullptr instead of &dummy

st->print(", %s", buf);
is_dll = true;
}
}
// If memory is accessible, and we do not know anything else about it, print a snippet
if (!is_dll &&
minfo->State == MEM_COMMIT &&
!(minfo->Protect & PAGE_NOACCESS || minfo->Protect & PAGE_GUARD)) {
st->print_raw(", ");
print_snippet(region_start, st);
}
st->cr();
// Next region...
bool rc = checkedVirtualQuery(region_end, minfo);
if (rc == false || // VirtualQuery error, end of allocation?
(minfo->State == MEM_FREE) || // end of allocation, free memory follows
Expand All @@ -6035,6 +6092,7 @@ static address print_one_mapping(MEMORY_BASIC_INFORMATION* minfo, address start,
{
return region_end;
}
first_line = false;
}
#undef IS_IN
ShouldNotReachHere();
Expand All @@ -6046,7 +6104,14 @@ void os::print_memory_mappings(char* addr, size_t bytes, outputStream* st) {
address start = (address)addr;
address end = start + bytes;
address p = start;
while (p < end) {
if (p == NULL) { // Lets skip the zero pages.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

p += os::vm_allocation_granularity();
}
address p2 = p; // guard against wraparounds
int fuse = 0;

while (p < end && p >= p2) {
p2 = p;
// Probe for the next mapping.
if (checkedVirtualQuery(p, &minfo)) {
if (minfo.State != MEM_FREE) {
Expand All @@ -6064,8 +6129,24 @@ void os::print_memory_mappings(char* addr, size_t bytes, outputStream* st) {
p = region_end;
}
} else {
// advance probe pointer.
p += os::vm_allocation_granularity();
// MSDN doc on VirtualQuery is unclear about what it means if it returns an error.
// In particular, whether querying an address outside any mappings would report
// a MEM_FREE region or just return an error. From experiments, it seems to return
// a MEM_FREE region for unmapped areas in valid address space and an error if we
// are outside valid address space.
// Here, we advance the probe pointer by alloc granularity. But if the range to print
// is large, this may take a long time. Therefore lets stop right away if the address
// is outside of what we know are valid addresses on Windows. Also, add a loop fuse.
static const address end_phys = (address)(LP64_ONLY(0x7ffffffffffULL) NOT_LP64(3*G));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (p >= end_phys) {
break;
} else {
// Advance probe pointer, but with a fuse to break long loops.
if (fuse++ == 100000) {
break;
}
p += os::vm_allocation_granularity();
}
}
}
}
5 changes: 5 additions & 0 deletions src/hotspot/share/runtime/os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1737,6 +1737,11 @@ bool os::release_memory(char* addr, size_t bytes) {
return res;
}

// Prints all mappings
void os::print_memory_mappings(outputStream* st) {
os::print_memory_mappings(NULL, (size_t)-1, st);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

void os::pretouch_memory(void* start, void* end, size_t page_size) {
for (volatile char *p = (char*)start; p < (char*)end; p += page_size) {
*p = 0;
Expand Down
2 changes: 2 additions & 0 deletions src/hotspot/share/runtime/os.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,8 @@ class os: AllStatic {

// A diagnostic function to print memory mappings in the given range.
static void print_memory_mappings(char* addr, size_t bytes, outputStream* st);
// Prints all mappings
static void print_memory_mappings(outputStream* st);

// Touch memory pages that cover the memory range from start to end (exclusive)
// to make the OS back the memory range with actual memory.
Expand Down
32 changes: 27 additions & 5 deletions test/hotspot/gtest/runtime/test_os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
*/

#include "precompiled.hpp"
#include "memory/allocation.hpp"
#include "memory/resourceArea.hpp"
#include "runtime/os.hpp"
#include "utilities/globalDefinitions.hpp"
Expand Down Expand Up @@ -494,11 +495,32 @@ TEST_VM(os, release_one_mapping_multi_commits) {
PRINT_MAPPINGS("D");
}

TEST_VM(os, show_mappings_1) {
// Display an arbitrary large address range. Make this works, does not hang, etc.
char dummy[16 * K]; // silent truncation is fine, we don't care.
stringStream ss(dummy, sizeof(dummy));
os::print_memory_mappings((char*)0x1000, LP64_ONLY(1024) NOT_LP64(3) * G, &ss);
static void test_show_mappings(address start, size_t size) {
// Note: should this overflow, thats okay. stream will silently truncate. Does not matter for the test.
const size_t buflen = 4 * M;
char* buf = NEW_C_HEAP_ARRAY(char, buflen, mtInternal);
buf[0] = '\0';
stringStream ss(buf, buflen);
if (start != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

os::print_memory_mappings((char*)start, size, &ss);
} else {
os::print_memory_mappings(&ss); // prints full address space
}
// Still an empty implementation on MacOS and AIX
#if defined(LINUX) || defined(_WIN32)
EXPECT_NE(buf[0], '\0');
#endif
// buf[buflen - 1] = '\0';
// tty->print_raw(buf);
FREE_C_HEAP_ARRAY(char, buf);
}

TEST_VM(os, show_mappings_small_range) {
test_show_mappings((address)0x100000, 2 * G);
}

TEST_VM(os, show_mappings_full_range) {
test_show_mappings(NULL, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh.

I have an unreasonable dislike against nullptr and the churn that causes (NULL is nullptr anyway post C++11). But I'll be a good boy and change all those :)

}

#ifdef _WIN32
Expand Down