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.

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

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

Closed

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Nov 23, 2020

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1390/head:pull/1390
$ git checkout pull/1390

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2020

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Nov 23, 2020

@tstuefe The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Nov 23, 2020
@tstuefe tstuefe force-pushed the JDK-8256864-improve-mapping-tracing branch from 392ccce to 7bba557 Compare Nov 23, 2020
@tstuefe tstuefe marked this pull request as ready for review Nov 23, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 23, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 23, 2020

Webrevs

@tstuefe
Copy link
Member Author

tstuefe commented Nov 24, 2020

/cc hotspot-dev

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 24, 2020
@openjdk
Copy link

openjdk bot commented Nov 24, 2020

@tstuefe
The hotspot label was successfully added.

@tstuefe
Copy link
Member Author

tstuefe commented Nov 27, 2020

Gentle ping.

This is needed to get further information on the CDS/Metaspace initialization errors (https://bugs.openjdk.java.net/browse/JDK-8256729)

iklam
iklam approved these changes Dec 1, 2020
Copy link
Member

@iklam iklam left a comment

LGTM

@openjdk
Copy link

openjdk bot commented Dec 1, 2020

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256864: [windows] Improve tracing for mapping errors

Reviewed-by: iklam, rrich

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 33 new commits pushed to the master branch:

  • 2508bc7: 8257140: Crash in JvmtiTagMap::flush_object_free_events()
  • cfb50a9: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
  • 287b829: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
  • 1fd0ea7: 8256382: Use try_lock for hs_err EventLog printing
  • bff68f1: 8257533: legacy-jre-image includes jpackage and jlink tools
  • 9a60413: 8248736: [aarch64] runtime/signal/TestSigpoll.java failed "fatal error: not an ldr (literal) instruction."
  • e7ca0c4: 8257224: JDK-8251549 didn't update building.html
  • 7e37c7c: 8257471: fatal error: Fatal exception in JVMCI: Exception during JVMCI compiler initialization
  • 3e3745c: 8256008: UL does not report anything if disk writing fails
  • fb139cf: 8257467: [TESTBUG] -Wdeprecated-declarations is reported at sigset() in exesigtest.c
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/021dced22a7856b6ff72a63ff4d71e1ac3327c00...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2020
@tstuefe
Copy link
Member Author

tstuefe commented Dec 1, 2020

LGTM

Thanks, Ioi!

@tstuefe
Copy link
Member Author

tstuefe commented Dec 1, 2020

@iklam Could you please take a small look at the last commit? After your Windbg exercises in JDK-8255917 I decided to buff the printout of the mapping. Nothing too complicated.

@tstuefe
Copy link
Member Author

tstuefe commented Dec 1, 2020

mapping-printout.txt
This is an example of the mapping printout for a full process

src/hotspot/os/windows/os_windows.cpp Outdated Show resolved Hide resolved
iklam
iklam approved these changes Dec 1, 2020
iklam
iklam approved these changes Dec 1, 2020
Copy link
Contributor

@reinrich reinrich left a comment

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.

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
Contributor

@reinrich reinrich Dec 2, 2020

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.

@@ -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
Contributor

@reinrich reinrich Dec 2, 2020

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

@tstuefe tstuefe Dec 2, 2020

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).

if (first_line) {
char buf[MAX_PATH];
int dummy;
if (os::dll_address_to_library_name(allocation_base, buf, sizeof(buf), &dummy)) {
Copy link
Contributor

@reinrich reinrich Dec 2, 2020

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

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
Contributor

@reinrich reinrich Dec 2, 2020

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

@tstuefe tstuefe Dec 2, 2020

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.

// 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
Contributor

@reinrich reinrich Dec 2, 2020

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?

@@ -6046,7 +6105,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
Contributor

@reinrich reinrich Dec 2, 2020

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.

@@ -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
Contributor

@reinrich reinrich Dec 2, 2020

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.

char* buf = NEW_C_HEAP_ARRAY(char, buflen, mtInternal);
buf[0] = '\0';
stringStream ss(buf, buflen);
if (start != NULL) {
Copy link
Contributor

@reinrich reinrich Dec 2, 2020

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.

}

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

@reinrich reinrich Dec 2, 2020

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

@tstuefe tstuefe Dec 2, 2020

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 :)

@tstuefe
Copy link
Member Author

tstuefe commented Dec 2, 2020

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.

Yes, see link in the PR description.

Thanks, Thomas

@tstuefe
Copy link
Member Author

tstuefe commented Dec 2, 2020

Hi @reinrich

thanks a lot for your review. Please take a look at the new version.

I modified the test to create a test mapping, just for showing that the snippets are printed out okay. See below, 64- and 32-bit-versions (look for "ABCD" to find the mapping).

printout-32.txt
printout-64.txt

Cheers, Thomas

Copy link
Contributor

@reinrich reinrich left a comment

LGTM, thanks :)

// the kernel merges multiple mappings.
if (first_line) {
char buf[MAX_PATH];
if (os::dll_address_to_library_name(allocation_base, buf, sizeof(buf), NULL)) {
Copy link
Contributor

@reinrich reinrich Dec 2, 2020

Choose a reason for hiding this comment

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

You forgot one NULL

@tstuefe
Copy link
Member Author

tstuefe commented Dec 3, 2020

Thanks @iklam and @reinrich . All tests went through sucessfully tonight.

/integrate

@openjdk openjdk bot closed this Dec 3, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 3, 2020
@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@tstuefe Since your change was applied there have been 47 commits pushed to the master branch:

  • ae1eb28: 8257604: JNI_ArgumentPusherVaArg leaks valist
  • 4169d96: 8257143: Enable JVMCI code installation tests on AArch64
  • a5a034b: 8257617: TestLinkPlatform fails with new Java source version
  • d80ae05: 8166596: TLS support for the EdDSA signature algorithm
  • 3932527: 8257466: Improve enum iteration
  • 02a0a02: 8257563: Remove excessive include of klass.inline.hpp
  • cc1915b: 8253821: Improve ByteBuffer performance with GCM
  • 3da30e9: 8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes
  • 7104400: 8257164: Share LambdaForms for VH linkers/invokers
  • 3e89981: 8257623: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java shouldn't use timeout
  • ... and 37 more: https://git.openjdk.java.net/jdk/compare/021dced22a7856b6ff72a63ff4d71e1ac3327c00...master

Your commit was automatically rebased without conflicts.

Pushed as commit b44a329.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@tstuefe tstuefe deleted the JDK-8256864-improve-mapping-tracing branch Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
3 participants