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

Round maxRequiredSize to match romSize calculation #7104

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Sep 17, 2019

Fixes: #5988
Fixes: #7056
Fixes: #5399

romSize was rounded here: #5301

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

@pshipton
Copy link
Member

jenkins test sanity,extended osx jdk11

@pshipton
Copy link
Member

Does this resolve #7056 ?

@pshipton
Copy link
Member

@pshipton
Copy link
Member

@theresa-m can you please look at the test failure, this isn't a failure I've seen in the nightly builds.
https://ci.eclipse.org/openj9/job/Test_openjdk11_j9_extended.functional_x86-64_mac_Personal/30/

@theresa-m
Copy link
Contributor Author

I have verified locally that this fixes #7056

Looking at the functional test failure..

@pshipton
Copy link
Member

This should also fix #5399 so I've added it to the list in the description.

* This mirrors ROM class padding in finishPrepareAndLaydown when the final ROM class size
* is calculated.
*/
maxRequiredSize += (sizeof(U_64) - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Could use the ROUND_UP_TO_POWEROF2 macro here.

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

How can we prevent this from occurring again? Is there some way we can track the allocated size vs the written size and assert if we're going to overrun the buffer?

@DanHeidinga
Copy link
Member

There's an existing assert for this: https://github.com/eclipse/openj9/blob/9d35d06dcbf62bb83d05b93dc980e201f6aeae9e/runtime/bcutil/ROMClassBuilder.cpp#L761 Why didn't it catch this issue?

@pshipton
Copy link
Member

Given the size of the file, there is likely a core file archived for the failures, like https://140-211-168-230-openstack.osuosl.org/artifactory/ci-eclipse-openj9/Test/Test_openjdk11_j9_extended.functional_x86-64_mac_Personal/30/functional_test_output.tar.gz

Probably doesn't matter, but the PR test failure occurred on osx1014-x86-1.
Running grinders I got some reproductions
https://ci.eclipse.org/openj9/view/Test/job/Test-Grinder/501 - osx1013-x86-4
https://ci.eclipse.org/openj9/view/Test/job/Test-Grinder/502 - osx1014-x86-2
https://ci.eclipse.org/openj9/view/Test/job/Test-Grinder/503 - osx1014-x86-3
https://ci.eclipse.org/openj9/view/Test/job/Test-Grinder/504 - osx1011-x86-2

Running grinders on last night's build without this change as well.

@pshipton
Copy link
Member

@pshipton
Copy link
Member

Trc_BCU_Assert_True is level 3 and not enabled by default.
TraceAssert=Trc_BCU_Assert_True NoEnv Overhead=1 Level=3 Assert="(P1)"

@pshipton
Copy link
Member

How can we prevent this from occurring again?

We can add a test that enables the BCU asserts.

@DanHeidinga
Copy link
Member

We can add a test that enables the BCU asserts.

I'd rather enable the BCU asserts by default and measure the performance hit. For reasons I don't yet understand, we're only seeing this issue with a particular class on OS X which a simple test wouldn't catch. It needs to be the default to catch this in the general case.

@pshipton
Copy link
Member

Created #7140 for the extended.functional failure, which is now also seen in nightly builds.

@DanHeidinga
Copy link
Member

I'd rather enable the BCU asserts by default and measure the performance hit. For reasons I don't yet understand, we're only seeing this issue with a particular class on OS X which a simple test wouldn't catch. It needs to be the default to catch this in the general case.

I don't want this PR which fixes memory corruption to be held up by this but it is something we should address. @theresa-m Can you open an issue for tracking changing the trace asserts to be level 1? That change will need some startup performance testing to ensure it doesn't regress startup.

@theresa-m
Copy link
Contributor Author

I've opened an issue #7143 for enabling TraceAsserts by default. I've also updated the commit to use the ROUND_UP_TO_POWEROF2 macro.

* This mirrors ROM class padding in finishPrepareAndLaydown when the final ROM class size
* is calculated.
*/
ROUND_UP_TO_POWEROF2(maxRequiredSize, sizeof(U_64));
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 this needs to be maxRequiredSize = ROUND_UP_TO_POWEROF2(maxRequiredSize, sizeof(U_64));

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@pshipton
Copy link
Member

jenkins test sanity osx jdk11

@pshipton pshipton merged commit 217b85d into eclipse-openj9:master Sep 19, 2019
@theresa-m theresa-m deleted the fix_5988 branch November 1, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment