Bug ID: JDK-8211394 CHECK_ must be used in the rhs of an assignment statement within a block

Versions (Unresolved/Resolved/Fixed)

The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.

JDK 12	JDK 8
12 b15Fixed	8u202Fixed

In utilities/exceptions.hpp, the CHECK_ macro expands into multiple statements, vis.

#define CHECK_(result)         THREAD); if (HAS_PENDING_EXCEPTION) { CLEAR_PENDING_EXCEPTION; return result; } (void)(0

Because of this, it and its derivative CHECK_* macros, can only correctly be used within a block statement, i.e., within squigly brackets { }, and, because the 'THREAD);' terminates a statement, effectively only on the right-hand-side of an assignment expression. Using these macros in expression contexts, e.g., in a return statement, results in the pending exception check being skipped, which can mask bugs, and is also surprising behavior to the programmer.

All uses of the CHECK_ macros in expression context should be fixed. E.g., in management.cpp, there are two instances of

return initialize_klass(k, CHECK_NULL);

which expand to

return initialize_klass(k, THREAD); if (HAS_PENDING_EXCEPTION) { CLEAR_PENDING_EXCEPTION; return NULL; } (void)(0);

The pending exception check is skipped. The code should be something like

InstanceKlass* ik = initialize_klass(k, CHECK_NULL);
return ik;

JDK-8062808 patched some uses of CHECK_NULL in return statements by replacing CHECK_NULL with THREAD, eliminating the pending exception check. It may happen to work because the callee may do the check, but it requires the programmer to know that, and violates encapsulation besides. That patch and any similar ones should be revisited.

Having examined the occurrences of this most cases involve calling a method of the same class, where knowing that it returns NULL/false etc on exception is quite clear and does not violate encapsulation in any way. Most of the changes done under JDK-8062808 but a couple are worth fixing.
07-10-2018
The use of return with CHECK_NULL is still pervasive in JDK8, but I haven't looked at all uses to ensure that THREAD could be used instead. In JDK12, there still exist a few places that use return with CHECK_NULL where THREAD could be used instead. E.g., in jvmciEnv.cpp, this code return elem_klass->array_klass(CHECK_NULL); If you drill down into array_klass (which took me 10 or so minutes), you find out that exception checks are done a few levels deep and that array_klass returns NULL if an exception happens, so THREAD is ok. One of the places you end up in is ObjArrayKlass::allocate_objArray_klass, which has CHECK_0's in it rather than CHECK_NULL's. A pedantic compiler might refuse to convert the zeros to NULL, but I suppose that's a separate issue.
02-10-2018
So would I, hence this bug report. :)
02-10-2018
All functions that can encounter exceptions should return a suitable error indicator (e.g. NULL) if exceptions occur. That's just the way all VM code should be written. But I would prefer to see: return foo(CHECK_NULL); rewritten as: T x = foo(CHECK_NULL); return x;
02-10-2018
Yes, the JDK-8062808 changes are technically correct, but the programmer must know (and do the research to find out, which can be time consuming) that the called method returns NULL when an exception occurs in order to change CHECK_NULL to THREAD. Part of the point of using the CHECK_ macros is to avoid having to figure that out. We could alternatively document every method that takes a THREAD argument as to whether or not it returns NULL (or 0, or whatever) if an exception occurs, but that's pretty fragile.
02-10-2018
I don't see any issue with the changes made by JDK-8062808 as long as the called function returns NULL when an exception occurs (which it should). The end result is we return NULL to the caller with an exception pending just as when using CHECK_NULL.
02-10-2018
I thought the compiler flag settings were now such that you could not misuse CHECK_ in this way. I also thought we had checked for and fixed all such issues in the past. (I recall fixing at least one myself).
02-10-2018