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

8261479: CDS runtime code should check exceptions #3097

Closed

Conversation

@calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Mar 19, 2021

Proposed changes include:

  • For the functions which could throw exceptions, the callers should pass in CHECK instead of THREAD.

  • For the functions which won't throw exceptions, put THREAD as the first parameter.
    E.g.static bool add_unregistered_class(Thread* current, InstanceKlass* k);

  • For the functions which won't throw exceptions and don't use THREAD, just simply get rid of the TRAPS parameter.

Testing: passed tiers 1 and 2, in-progress tiers 3 and 4.


Progress

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

Issue

  • JDK-8261479: CDS runtime code should check exceptions

Reviewers

Download

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

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

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 19, 2021

/label add hotspot-runtime

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 19, 2021

👋 Welcome back ccheung! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

Loading

@calvinccheung calvinccheung marked this pull request as ready for review Mar 19, 2021
@openjdk openjdk bot added the rfr label Mar 19, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 19, 2021

Loading

yminqi
yminqi approved these changes Mar 19, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LGTM.

Loading

@@ -108,7 +108,7 @@ class ClassLoaderExt: public ClassLoader { // AllStatic
}

static void record_result(const s2 classpath_index,
InstanceKlass* result, TRAPS);
InstanceKlass* result);
Copy link
Contributor

@yminqi yminqi Mar 19, 2021

Choose a reason for hiding this comment

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

110/111 can merge into one line.

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 20, 2021

Choose a reason for hiding this comment

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

@yminqi Thanks for your review.
I'll also make similar change in classLoaderExt.cpp.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2021

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

8261479: CDS runtime code should check exceptions

Reviewed-by: minqi, dholmes, iklam

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

  • d7268fa: 8251942: PrintStream specification is not clear which flush method is automatically invoked
  • 8fa34e4: 8241619: (fs) Files.newByteChannel(path, Set.of(CREATE_NEW, READ)) does not throw a FileAlreadyExistsException when the file exists
  • e9321cd: 8263964: Redundant check in ObjectStartArray::object_starts_in_range
  • bd7a184: 8263442: Potential bug in jdk.internal.net.http.common.Utils.CONTEXT_RESTRICTED
  • 2335362: 8264032: Improve thread safety of Runtime.version()
  • 8c1ab38: 8263766: Confusing specification of JEditorPaneAccessibleHypertextSupport constructor
  • 5bc382f: 8263976: Remove block allocation from BasicHashtable
  • fbd57bd: 8263260: [s390] Support latest hardware (z14 and z15)
  • de2ff25: 8263974: Move SystemDictionary::verify_protection_domain
  • 9dad857: 8263080: Obsolete relationship in MulticastSocket API documentation.
  • ... and 67 more: https://git.openjdk.java.net/jdk/compare/e333b6e15343535789353f0017721b6509eb4f00...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.

Loading

@openjdk openjdk bot added the ready label Mar 19, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Calvin,

Welcome to my nightmare :)

There are a lot of inconsistencies with what you have done. Sometimes TRAPS is replaced by "Thread* current" and other times it is just gone and we have to reify Thread::current again later. Still lots of places where THREAD is used in a non-exception context.

Cheers,
David

Loading

Thread* thread = Thread::current();
char* path = NEW_RESOURCE_ARRAY_IN_THREAD(thread, char, path_len);
Copy link
Member

@dholmes-ora dholmes-ora Mar 20, 2021

Choose a reason for hiding this comment

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

No need to introduce a local when only one use.

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 22, 2021

Choose a reason for hiding this comment

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

Fixed.

Loading

u1* ClassPathZipEntry::open_entry(const char* name, jint* filesize, bool nul_terminate) {
// enable call to C land
JavaThread* thread = THREAD->as_Java_thread();
JavaThread* thread = JavaThread::current();
Copy link
Member

@dholmes-ora dholmes-ora Mar 20, 2021

Choose a reason for hiding this comment

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

Is there a reason you didn't continue to pass in the thread rather than having to reify it again with JavaThread::current()?

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 22, 2021

Choose a reason for hiding this comment

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

Because the caller of the above function doesn't need the thread. I've changed my patch so that the thread will be passed along as Thread* current.

Loading

@@ -1295,7 +1296,7 @@ char* ClassLoader::skip_uri_protocol(char* source) {

// Record the shared classpath index and loader type for classes loaded
// by the builtin loaders at dump time.
void ClassLoader::record_result(InstanceKlass* ik, const ClassFileStream* stream, TRAPS) {
void ClassLoader::record_result(Thread* current, InstanceKlass* ik, const ClassFileStream* stream) {
Copy link
Member

@dholmes-ora dholmes-ora Mar 20, 2021

Choose a reason for hiding this comment

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

So here you converted TRAPS to "Thread* current" but you didn't in the above functions - why not? This is inconsistent and seems arbitrary.

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 22, 2021

Choose a reason for hiding this comment

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

Please refer to my reply above. Hopefully, the latest change is more consistent.

Loading

@@ -216,7 +216,7 @@ InstanceKlass* KlassFactory::create_from_stream(ClassFileStream* stream,

#if INCLUDE_CDS
if (Arguments::is_dumping_archive()) {
ClassLoader::record_result(result, stream, THREAD);
ClassLoader::record_result(THREAD, result, stream);
Copy link
Member

@dholmes-ora dholmes-ora Mar 20, 2021

Choose a reason for hiding this comment

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

There are many non-exception related uses of THREAD in this method. This seems like a candidate for introducing:
JavaThread* current = THREAD->as_Java_thread()
and using "current" throughout instead of THREAD in those non-exception cases.

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 22, 2021

Choose a reason for hiding this comment

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

I've made the changes as you suggested.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Mar 24, 2021

Choose a reason for hiding this comment

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

Just for completeness I reverted my position on this due to objections from Coleen about this kind of change (made elsewhere). So we stick with THREAD here.

Loading

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 22, 2021

Hi Calvin,

Welcome to my nightmare :)

Thanks for your review.

There are a lot of inconsistencies with what you have done. Sometimes TRAPS is replaced by "Thread* current" and other times it is just gone and we have to reify Thread::current again later. Still lots of places where THREAD is used in a non-exception context.

Hope you'll like the latest patch better.

Thanks,
Calvin

Cheers,
David

Loading

Copy link
Member

@iklam iklam left a comment

I think this should also be be changed to a CHECK:

void MetaspaceShared::post_initialize(TRAPS) {
  if (UseSharedSpaces) {
    int size = FileMapInfo::get_number_of_shared_paths();
    if (size > 0) {
      SystemDictionaryShared::allocate_shared_data_arrays(size, THREAD); <<<<

Loading

}
if (use_full_module_graph()) {
HeapShared::reset_archived_object_states(THREAD);
if (HAS_PENDING_EXCEPTION) {
Copy link
Member

@iklam iklam Mar 22, 2021

Choose a reason for hiding this comment

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

I think this is not necessary. If an exception is thrown, it will be be printed by MetaspaceShared::preload_and_dump()

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 22, 2021

Choose a reason for hiding this comment

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

Do you mean I should keep the original code as follows?

 712   if (use_full_module_graph()) {
 713     HeapShared::reset_archived_object_states(CHECK);
 714   }

Loading

Copy link
Member

@iklam iklam Mar 22, 2021

Choose a reason for hiding this comment

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

Yes.

Loading

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 22, 2021

I think this should also be be changed to a CHECK:

void MetaspaceShared::post_initialize(TRAPS) {
  if (UseSharedSpaces) {
    int size = FileMapInfo::get_number_of_shared_paths();
    if (size > 0) {
      SystemDictionaryShared::allocate_shared_data_arrays(size, THREAD); <<<<

It was already changed to a CHECK in the webrev:

219     int size = FileMapInfo::get_number_of_shared_paths();
220     if (size > 0) {
221       SystemDictionaryShared::allocate_shared_data_arrays(size, CHECK);

Loading

@iklam
Copy link
Member

@iklam iklam commented Mar 22, 2021

I think this should also be be changed to a CHECK:

void MetaspaceShared::post_initialize(TRAPS) {
  if (UseSharedSpaces) {
    int size = FileMapInfo::get_number_of_shared_paths();
    if (size > 0) {
      SystemDictionaryShared::allocate_shared_data_arrays(size, THREAD); <<<<

It was already changed to a CHECK in the webrev:

219     int size = FileMapInfo::get_number_of_shared_paths();
220     if (size > 0) {
221       SystemDictionaryShared::allocate_shared_data_arrays(size, CHECK);

Oh, you're right. Sorry I missed that :-(

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Calvin,

This looks much better - thanks!

A couple of comments below, but only one of substance. I'll approve as-is but I think Coleen would want changes per my comment.

David

Loading

// enable call to C land
JavaThread* thread = THREAD->as_Java_thread();
JavaThread* thread = current->as_Java_thread();
Copy link
Member

@dholmes-ora dholmes-ora Mar 23, 2021

Choose a reason for hiding this comment

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

No need to introduce a local for a single use.

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 23, 2021

Choose a reason for hiding this comment

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

This is an existing issue but I've made the change and also fixed the indentation of the comment at line 289.

Loading

assert(THREAD->is_Java_thread(), "must be a JavaThread");

ResourceMark rm(THREAD);
HandleMark hm(THREAD);
JavaThread* current = THREAD->as_Java_thread();
Copy link
Member

@dholmes-ora dholmes-ora Mar 23, 2021

Choose a reason for hiding this comment

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

You can delete the assert at line 174 as the same assert is inside as_Java_thread().

But I think this is the kind of change that Coleen objects to: introducing a new local variable just to avoid using THREAD. The changes in this file can probably be reverted.

Loading

Copy link
Member Author

@calvinccheung calvinccheung Mar 23, 2021

Choose a reason for hiding this comment

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

Ok. I've reverted the changes.

Loading

iklam
iklam approved these changes Mar 23, 2021
Copy link
Member

@iklam iklam left a comment

Latest version LGTM.

Loading

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 23, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

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

  • 087c8bf: 8264041: Incorrect comments for ParallelCompactData::summarize_dense_prefix
  • c087f3e: 8263995: Incorrect double-checked locking in Types.arraySuperType()
  • d7268fa: 8251942: PrintStream specification is not clear which flush method is automatically invoked
  • 8fa34e4: 8241619: (fs) Files.newByteChannel(path, Set.of(CREATE_NEW, READ)) does not throw a FileAlreadyExistsException when the file exists
  • e9321cd: 8263964: Redundant check in ObjectStartArray::object_starts_in_range
  • bd7a184: 8263442: Potential bug in jdk.internal.net.http.common.Utils.CONTEXT_RESTRICTED
  • 2335362: 8264032: Improve thread safety of Runtime.version()
  • 8c1ab38: 8263766: Confusing specification of JEditorPaneAccessibleHypertextSupport constructor
  • 5bc382f: 8263976: Remove block allocation from BasicHashtable
  • fbd57bd: 8263260: [s390] Support latest hardware (z14 and z15)
  • ... and 69 more: https://git.openjdk.java.net/jdk/compare/e333b6e15343535789353f0017721b6509eb4f00...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1c9817b.

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

Loading

@calvinccheung calvinccheung deleted the 8261479-cds-check-exceptions branch Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants