RFR: 8261479: CDS runtime code should check exceptions

[David Holmes]

On Fri, 19 Mar 2021 21:13:29 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

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

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

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.

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()?

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.

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.

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

Changes requested by dholmes (Reviewer).

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