RFR: 8261479: CDS runtime code should check exceptions [v2]

[Calvin Cheung]

On Sat, 20 Mar 2021 02:48:20 GMT, David Holmes <dholmes at openjdk.org> wrote:

> 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

> src/hotspot/share/classfile/classLoader.cpp line 245:
> 
>> 243:   size_t path_len = strlen(_dir) + strlen(name) + strlen(os::file_separator()) + 1;
>> 244:   Thread* thread = Thread::current();
>> 245:   char* path = NEW_RESOURCE_ARRAY_IN_THREAD(thread, char, path_len);
> 
> No need to introduce a local when only one use.

Fixed.

> src/hotspot/share/classfile/classLoader.cpp line 291:
> 
>> 289: u1* ClassPathZipEntry::open_entry(const char* name, jint* filesize, bool nul_terminate) {
>> 290:     // enable call to C land
>> 291:   JavaThread* thread = JavaThread::current();
> 
> Is there a reason you didn't continue to pass in the thread rather than having to reify it again with JavaThread::current()?

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

> src/hotspot/share/classfile/classLoader.cpp line 1299:
> 
>> 1297: // Record the shared classpath index and loader type for classes loaded
>> 1298: // by the builtin loaders at dump time.
>> 1299: void ClassLoader::record_result(Thread* current, InstanceKlass* ik, const ClassFileStream* stream) {
> 
> So here you converted TRAPS to "Thread* current" but you didn't in the above functions - why not? This is inconsistent and seems arbitrary.

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

> src/hotspot/share/classfile/klassFactory.cpp line 219:
> 
>> 217: #if INCLUDE_CDS
>> 218:   if (Arguments::is_dumping_archive()) {
>> 219:     ClassLoader::record_result(THREAD, result, stream);
> 
> 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.

I've made the changes as you suggested.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3097