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

8261480: MetaspaceShared::preload_and_dump should check exceptions #2925

Conversation

iklam
Copy link
Member

@iklam iklam commented Mar 10, 2021

MetaspaceShared::preload_and_dump(), as well as many of the functions that it calls, have extensive use of THREAD when calling functions that can potentially throw exceptions. Many of these call sites explicitly check for failure status, or assume that the caller would terminate the VM if an exception were to happen. This makes the code hard to understand and potentially unsafe.

I have make 3 types of changes:

  1. In most cases, replace THREAD with CHECK. Moved all the fatal error check to MetaspaceShared::preload_and_dump(), where it handles any uncaught exceptions and explicitly terminates the VM.
  2. If a TRAPS function can never throw, replace the TRAPS declaration with Thread* current
  3. In cases where an exception is handled and discarded, use THREAD immediately followed by HAS_PENDING_EXCEPTION -> CLEAR_PENDING_EXCEPTION to make the code easy to understand. HandleMark is used to assert that no exception escapes.

(See MetaspaceShared::try_link_class for an example of 2 and 3).

Tested with tiers 1-4.


Progress

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

Issue

  • JDK-8261480: MetaspaceShared::preload_and_dump should check exceptions

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2925/head:pull/2925
$ git checkout pull/2925

To update a local copy of the PR:
$ git checkout pull/2925
$ git pull https://git.openjdk.java.net/jdk pull/2925/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 10, 2021

👋 Welcome back iklam! 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 Mar 10, 2021

@iklam 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 Mar 10, 2021
@iklam iklam marked this pull request as ready for review March 16, 2021 01:54
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 16, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 16, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Ioi,

I started making comments below but then realized that I have a fundamental problem with this work - you are making the VMThread throw exceptions at dump time and the VMThread should not be throwing exceptions. The VMThread can't execute Java code and so it can only get away with creating a handful of exception types that the VM can construct directly without calling java code. This is somewhat of an abberration - the VMThread should not be creating exceptions and ideally would not have the mechanics to even throw or catch them (the fields would be in JavaThread not Thread). So while we have to deal with this aberration already I do not want to see the VMThread actually creating and "throwing" exceptions at dump time, only to eventually call vm_exit. So this change takes things in the wrong direction for me - the dumping code should not be using TRAPS in general and should be returning NULL up the call chain, or else calling vm_exit as appropriate.

Cheers,
David

// We might have an invalid class name or an bad class. Warn about it
// and keep going to the next line.
CLEAR_PENDING_EXCEPTION;
log_warning(cds)("Preload Warning: Cannot find %s", _class_name);
Copy link
Member

Choose a reason for hiding this comment

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

Including some information about the exception would be useful - it might not be what we think.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same as the current behavior -- all exceptions encountered during parsing of the classlist are printed as "Cannot find ...". Many of the test cases depend on this message.

I want to limit the size of this PR and stick to non-functional changes only. I plan to improve the message in do this in a separate RFE: JDK-8263469 "CDS log should print specific reasons for loading failures"

@@ -398,30 +453,27 @@ InstanceKlass* ClassListParser::load_class_from_source(Symbol* class_name, TRAPS
if (strncmp(_class_name, "java/", 5) == 0) {
log_info(cds)("Prohibited package for non-bootstrap classes: %s.class from %s",
_class_name, _source);
return NULL;
THROW_NULL(vmSymbols::java_lang_ClassNotFoundException());
Copy link
Member

Choose a reason for hiding this comment

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

Is that the right exception to use here? I'm not clear of the context for encountering this. What does regular classloading do in such a case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above. This will be improved in JDK-8263469.

@@ -471,10 +523,13 @@ bool ClassListParser::is_matching_cp_entry(constantPoolHandle &pool, int cp_inde
}
return true;
}
void ClassListParser::resolve_indy(Symbol* class_name_symbol, TRAPS) {

void ClassListParser::resolve_indy(Thread* current, Symbol* class_name_symbol) {
Copy link
Member

Choose a reason for hiding this comment

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

Should current be a JavaThread here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently all the current parameters will come from the THREAD/TRAPS of its caller, like

int ClassListParser::parse(TRAPS) {
    ...
    resolve_indy(THREAD, class_name_symbol);

I could change the caller to use THREAD->as_JavaThread(), but I think that's too much ceremony. It's better to change to JavaThread* after THREAD/TRAPS are changed to JavaThread (JDK-8252685)

Comment on lines 497 to 498
Klass* callee = pool->klass_at(callee_index, CHECK);
if (callee != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

AFAICS callee can never be NULL if no exception was thrown.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

THREAD->as_Java_thread()->get_thread_stat()->perf_timers_addr(),
PerfClassTraceTime::CLASS_LOAD);
stream = e->open_stream(file_name, CHECK_NULL);
THROW_NULL(vmSymbols::java_lang_ClassNotFoundException());
Copy link
Member

Choose a reason for hiding this comment

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

Again not obvious this is the right exception to throw. It is also not very informative as to what the actual problem was - isn't it the "source:" that was not found here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be improved in JDK-8263469.

log_warning(cds)("Preload Warning: Cannot find %s", class_name);
stream = e->open_stream(file_name, CHECK_NULL);
if (stream == NULL) {
THROW_NULL(vmSymbols::java_lang_ClassNotFoundException());
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be improved in JDK-8263469.

Comment on lines 284 to 288
return KlassFactory::create_from_stream(stream,
name,
loader_data,
cl_info,
THREAD);
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 I'd prefer to see a local introduced and then CHECK used rather than THREAD.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is our perennial problem: CHECK cannot be used on a return statement :-(

See JDK-8064811 (Use THREAD instead of CHECK_NULL in return statements)

Copy link
Member

Choose a reason for hiding this comment

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

InstanceKlass* k = KlassFactory::create_from_stream(stream, name, loader_data, cl_info, CHECK);
return k;

@@ -310,7 +295,7 @@ struct CachedClassPathEntry {

static GrowableArray<CachedClassPathEntry>* cached_path_entries = NULL;

ClassPathEntry* ClassLoaderExt::find_classpath_entry_from_cache(const char* path, TRAPS) {
ClassPathEntry* ClassLoaderExt::find_classpath_entry_from_cache(Thread* current, const char* path) {
Copy link
Member

Choose a reason for hiding this comment

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

If this is executed by the vmThread at dump time then I'd prefer to see current called vmThread.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is executed only in the main Java thread.

Comment on lines +324 to +327
ExceptionMark em(current);
Thread* THREAD = current; // For exception macros.
new_entry = create_class_path_entry(path, &st, /*throw_exception=*/false,
false, false, CATCH); // will never throw
Copy link
Member

Choose a reason for hiding this comment

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

Given we are actually dealing with the VMThread which can't (or shouldn't!) actually throw exceptions I'm concerned about this chunk of code. I don't think we have a good idiom for these cases where the VMThread calls potentially exception throwing code in a way that should never throw an exception. Ideally all the exception-related fields would be in JavaThread.

Copy link
Member Author

Choose a reason for hiding this comment

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

All code changed by this patch executes in the main Java thread.

@mlbridge
Copy link

mlbridge bot commented Mar 16, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 16/03/2021 2:54 pm, David Holmes wrote:

On Wed, 10 Mar 2021 20:37:17 GMT, Ioi Lam <iklam at openjdk.org> wrote:

`MetaspaceShared::preload_and_dump()`, as well as many of the functions that it calls, have extensive use of `THREAD` when calling functions that can potentially throw exceptions. Many of these call sites explicitly check for failure status, or assume that the caller would terminate the VM if an exception were to happen. This makes the code hard to understand and potentially unsafe.

I have make 3 types of changes:

1. In most cases, replace THREAD with CHECK. Moved all the fatal error check to `MetaspaceShared::preload_and_dump()`, where it handles any uncaught exceptions and explicitly terminates the VM.
2. If a `TRAPS` function can never throw, replace the `TRAPS` declaration with `Thread* current`
3. In cases where an exception is handled and discarded, use `THREAD` immediately followed by `HAS_PENDING_EXCEPTION` -> `CLEAR_PENDING_EXCEPTION` to make the code easy to understand. HandleMark is used to assert that no exception escapes.

(See `MetaspaceShared::try_link_class` for an example of 2 and 3).

Tested with tiers 1-4.

Hi Ioi,

I started making comments below but then realized that I have a fundamental problem with this work - you are making the VMThread throw exceptions at dump time and the VMThread should not be throwing exceptions. The VMThread can't execute Java code and so it can only get away with creating a handful of exception types that the VM can construct directly without calling java code. This is somewhat of an abberration - the VMThread should not be creating exceptions and ideally would not have the mechanics to even throw or catch them (the fields would be in JavaThread not Thread). So while we have to deal with this aberration already I do not want to see the VMThread actually creating and "throwing" exceptions at dump time, only to eventually call vm_exit. So this change takes things in the wrong direction for me - the dumping code should not be using TRAPS in general and should be returning NULL up the call chain, or else calling vm_exit as appropriate.

Mea culpa. Ioi pointed out to me that while the code is for dumping it
is not the code that is actually executed by the VMThread. So in all
cases we should be in the main JavaThread. In which case please look at
the comments and also consider where "Thread* current" could be
JavaThread* current".

Thanks,
David

@mlbridge
Copy link

mlbridge bot commented Mar 17, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Ioi,

All responses about future enhancements noted. A couple of specific
follow ups ...

On 17/03/2021 11:06 am, Ioi Lam wrote:

On Tue, 16 Mar 2021 04:09:40 GMT, David Holmes <dholmes at openjdk.org> wrote:

src/hotspot/share/classfile/classListParser.cpp line 527:

525: }
526:
527: void ClassListParser::resolve_indy(Thread* current, Symbol* class_name_symbol) {

Should current be a JavaThread here?

Currently all the `current` parameters will come from the THREAD/TRAPS of its caller, like

int ClassListParser::parse(TRAPS) {
...
resolve_indy(THREAD, class_name_symbol);

I could change the caller to use THREAD->as_JavaThread(), but I think that's too much ceremony. It's better to change to `JavaThread*` after THREAD/TRAPS are changed to `JavaThread` (JDK-8252685)

Okay I will take care of that one.

src/hotspot/share/classfile/classLoaderExt.cpp line 288:

286: loader_data,
287: cl_info,
288: THREAD);

I think I'd prefer to see a local introduced and then CHECK used rather than THREAD.

This is our perennial problem: CHECK cannot be used on a `return` statement :-(

See [JDK-8064811](https://bugs.openjdk.java.net/browse/JDK-8064811) (Use THREAD instead of CHECK_NULL in return statements)

Yes but my point is that rather than simply replace CHECK with THREAD I
would prefer to see a local variable introduced so that we can use CHECK ie.

RetType ret = foo(CHECK);
return ret;

rather than:

return foo(THREAD);

src/hotspot/share/classfile/classLoaderExt.cpp line 298:

296: static GrowableArray<CachedClassPathEntry>* cached_path_entries = NULL;
297:
298: ClassPathEntry* ClassLoaderExt::find_classpath_entry_from_cache(Thread* current, const char* path) {

If this is executed by the vmThread at dump time then I'd prefer to see current called vmThread.

This is executed only in the main Java thread.

Noted.

src/hotspot/share/classfile/classLoaderExt.cpp line 327:

325: Thread* THREAD = current; // For exception macros.
326: new_entry = create_class_path_entry(path, &st, /*throw_exception=*/false,
327: false, false, CATCH); // will never throw

Given we are actually dealing with the VMThread which can't (or shouldn't!) actually throw exceptions I'm concerned about this chunk of code. I don't think we have a good idiom for these cases where the VMThread calls potentially exception throwing code in a way that should never throw an exception. Ideally all the exception-related fields would be in JavaThread.

All code changed by this patch executes in the main Java thread.

Noted. I will re-examine.

Thanks,
David

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Ioi,

It is a bit hard to keep track of the refactorings, but this seems okay.

Thanks,
David

Comment on lines 284 to 288
return KlassFactory::create_from_stream(stream,
name,
loader_data,
cl_info,
THREAD);
Copy link
Member

Choose a reason for hiding this comment

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

InstanceKlass* k = KlassFactory::create_from_stream(stream, name, loader_data, cl_info, CHECK);
return k;

@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@iklam 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:

8261480: MetaspaceShared::preload_and_dump should check exceptions

Reviewed-by: dholmes, ccheung

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 2 new commits pushed to the master branch:

  • 50ff0d4: 8263756: Fix ZGC ProblemList entry for serviceability/sa/ClhsdbSymbol.java
  • 99b39aa: 8262807: Note assumptions of core reflection modeling and parameter handling

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Mar 17, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 17, 2021

Mailing list message from Ioi Lam on hotspot-runtime-dev:

On 3/16/21 6:34 PM, David Holmes wrote:

On Wed, 17 Mar 2021 01:03:26 GMT, Ioi Lam <iklam at openjdk.org> wrote:

src/hotspot/share/classfile/classLoaderExt.cpp line 288:

286: loader_data,
287: cl_info,
288: THREAD);
I think I'd prefer to see a local introduced and then CHECK used rather than THREAD.
This is our perennial problem: CHECK cannot be used on a `return` statement :-(

See [JDK-8064811](https://bugs.openjdk.java.net/browse/JDK-8064811) (Use THREAD instead of CHECK_NULL in return statements)
`InstanceKlass* k = KlassFactory::create_from_stream(stream, name, loader_data, cl_info, CHECK);`
`return k;`

Fixed.

Thanks
- Ioi

Comment on lines 275 to 277
PerfClassTraceTime vmtimer(perf_sys_class_lookup_time(),
THREAD->as_Java_thread()->get_thread_stat()->perf_timers_addr(),
PerfClassTraceTime::CLASS_LOAD);
Copy link
Member

Choose a reason for hiding this comment

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

Why was the above removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. It was removed by mistake and I restore it.

Copy link
Member

@calvinccheung calvinccheung left a comment

Choose a reason for hiding this comment

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

Looks good. One comment below.

Thanks,
Calvin

@iklam
Copy link
Member Author

iklam commented Mar 18, 2021

Thanks @dholmes-ora and @calvinccheung for the review.
/integrate

@openjdk openjdk bot closed this Mar 18, 2021
@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 Mar 18, 2021
@openjdk
Copy link

openjdk bot commented Mar 18, 2021

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

  • 81ba578: 8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()
  • 9225a23: 8263108: Class initialization deadlock in java.lang.constant
  • 5d5813a: 8263757: Remove serviceability/sa/ClhsdClasses.java from ZGC problem list
  • 50ff0d4: 8263756: Fix ZGC ProblemList entry for serviceability/sa/ClhsdbSymbol.java
  • 99b39aa: 8262807: Note assumptions of core reflection modeling and parameter handling

Your commit was automatically rebased without conflicts.

Pushed as commit 2b93ae0.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants