8366020: Assert that java code is not executed during the AOT assembly phase#30345
8366020: Assert that java code is not executed during the AOT assembly phase#30345matias9927 wants to merge 5 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back matsaave! A progress list of the required criteria for merging this PR into |
|
@matias9927 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: 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 183 new commits pushed to the
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 |
|
@matias9927 The following label will be automatically applied to this pull request:
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. |
Webrevs
|
vnkozlov
left a comment
There was a problem hiding this comment.
To avoid confusion may you should use "Java code" instead of "user code" in title and comments.
| return false; | ||
| } | ||
|
|
||
| #ifndef PRODUCT |
There was a problem hiding this comment.
Use #ifdef ASSERT instead if you guard for debug VM.
|
/author add @iklam |
|
@matias9927 Syntax:
User names can only be used for users in the census associated with this repository. For other authors you need to supply the full name and email address. |
|
/contributor add @iklam |
|
@matias9927 |
| if (ik->is_interface() && !ik->interface_needs_clinit_execution_as_super()) { | ||
| // TODO: why are these interfaces marked as initialized?? | ||
| } else { | ||
| assert(false, "cannot execute user code"); |
There was a problem hiding this comment.
I don't think they're marked as initialized, I think this case doesn't have Java code to execute.
So why can't this be
// Interfaces with default methods and <clinit> also execute Java code.
assert(!ik->is_interface() || !ik->interface_needs_clinit_execution_as_super(), "cannot execute Java code");
Can you capitalize Java?
There was a problem hiding this comment.
I think the assert should be:
assert(ik->is_interface() && !ik->interface_needs_clinit_execution_as_super(), "cannot execute Java code in assembly phase")
There was a problem hiding this comment.
Yes, this assert is the same logic, not mine.
| // in assembly phase. | ||
| if (ik->class_loader() != nullptr && !ik->is_hidden()) { | ||
| if (ik->is_interface() && !ik->interface_needs_clinit_execution_as_super()) { | ||
| // TODO: why are these interfaces marked as initialized?? |
There was a problem hiding this comment.
I think we can use this version that explains why we allow interfaces that are !interface_needs_clinit_execution_as_super().
I also added
- check for user code from
-Xbootclasspath - more comments about the different conditions that are being checked.
if (!ik->is_initialized() && !ik->is_being_initialized()) {
[...]
return false;
>>>>
} else {
// If code in ik is executed, then ik must be in the state of being_initialized or
// fully_initialized.
//
// Check that no user code is executed during the assembly phase. Otherwise the user
// code may introduce undesirable environment dependencies into the heap image.
if (AOTInitTestClass != nullptr || ArchiveHeapTestClass != nullptr) {
// OK: if any of the above two flags are set, we allow user code to be executed
// in the assembly phase. Note that these flags are strictly for the purpose
// of testing HotSpot and are not available in product builds.
} else {
if (ik->defined_by_boot_loader()) {
// We allow boot classes to be AOT-initialized, except for classes from
// -Xbootclasspath (cp index >= 1) be AOT-initialized, as such classes may be
// provided by the user application.
assert(ik->shared_classpath_index() <= 0,
"only boot classed loaded from the modules image can be AOT-initialized");
} else {
assert(ik->defined_by_platform_loader() || ik->defined_by_app_loader(),
"cannot AOT-initialized classed loaded by other loaders");
// Some classes from platform/app loaders are allowed to be initialized during
// the assembly phase.
if (ik->is_hidden()) {
// OK: hidden classes from platform/app loaders need to be AOT-initialized to
// support AOT-linking of lambdas. These hidden classes are generated by the
// VM and do not contain user code.
} else if (ik->is_interface() && !ik->interface_needs_clinit_execution_as_super()) {
// OK: ik is an interface used by a lambda. When AOT-linking lambdas, we only
// support interfaces that are not interface_needs_clinit_execution_as_super().
// See AOTConstantPoolResolver::check_lambda_metafactory_signature().
} else {
ResourceMark rm;
fatal("cannot execute user code in class %s", ik->external_name());
}
}
}
}
<<<<
| if (ik->is_interface() && !ik->interface_needs_clinit_execution_as_super()) { | ||
| // TODO: why are these interfaces marked as initialized?? | ||
| } else { | ||
| assert(false, "cannot execute user code"); |
coleenp
left a comment
There was a problem hiding this comment.
Okay, this makes sense with all the comments.
User code should not be run during the AOT assembly phase and there are already checks in place to assure this. This patch simply adds a debug mode only check. Verified with tier 1-5 tests.
Progress
Issue
Reviewers
Contributors
<iklam@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30345/head:pull/30345$ git checkout pull/30345Update a local copy of the PR:
$ git checkout pull/30345$ git pull https://git.openjdk.org/jdk.git pull/30345/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30345View PR using the GUI difftool:
$ git pr show -t 30345Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30345.diff
Using Webrev
Link to Webrev Comment