RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v2]

[David Holmes]

On 23/03/2021 9:54 pm, Coleen Phillimore wrote:
> On Tue, 23 Mar 2021 05:52:51 GMT, Ioi Lam <iklam at openjdk.org> wrote:
> 
>>> src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 484:
>>>
>>>> 482:   void rewrite_cp_refs_in_method(methodHandle method,
>>>> 483:     methodHandle * new_method_p, TRAPS);
>>>> 484:   bool rewrite_cp_refs_in_methods(InstanceKlass* scratch_class, TRAPS);
>>>
>>> This method clears any pending exception and so should not be a TRAPS method.
>>
>> `VM_RedefineClasses::load_new_class_versions` also seems to never throw. These functions should be changed to take a `Thread*` parameter, and should use `HandleMark em(thread);` to guarantee that an exception never leaves the function.
> 
> Both of these functions are good examples of the convention that we're trying to agree on.   In, load_new_class_versions, TRAPS is passed to materialize THREAD.  THREAD is used then in a lot of places, and also to pass to SystemDictionary::parse_stream(...TRAPS), which does have an exceptional return that must be handled.
> Removing TRAPS then adding:
> JavaThread* current = JavaThread::current();
> changing THREAD to current in most of the places seems ok, but passing 'current' to SystemDictionary::resolve_from_stream loses the information visually that this function returns an exception that must be handled.

Okay ...

Only a function that upon return may have directly, or indirectly, 
caused an exception to be pending should be declared with TRAPS. The 
caller is then expected to use the CHECK macros under most conditions.

If a function is going to call a TRAPS function but clear the exception, 
then it should manifest a THREAD variable and pass that, both to 
indicate the called function is TRAPS and to allow its own use of the 
exception macros that depend on THREAD. But that function should not 
itself declare TRAPS just to get THREAD.

How to manifest THREAD depends on the exact context, and we already have 
these cases today:
- Thread* THREAD = Thread::current();
- Thread* THREAD = <pre-existing current thread variable by some name>

In this case I would expect to see:

jvmtiError VM_RedefineClasses::load_new_class_versions(Thread* current) {
...
  ResourceMark rm(current);
  JvmtiThreadState *state = 
JvmtiThreadState::state_for(current->as_Java_thread());
...
   HandleMark hm(current);
...
   Handle the_class_loader(current, the_class->class_loader());
   Handle protection_domain(current, the_class->protection_domain());
...
   Thread* THREAD = current; // For exception processing
   InstanceKlass* scratch_class = SystemDictionary::parse_stream(
                                                       the_class_sym,
                                                       the_class_loader,
                                                       &st,
                                                       cl_info,
                                                       THREAD);
...
   if (HAS_PENDING_EXCEPTION) {
...
       the_class->link_class(THREAD);
       if (HAS_PENDING_EXCEPTION) {
...


I'll point out, to be clear that I recognise it, that the existing CATCH 
macro does not fit in with these conventions as you only apply CATCH to 
a function that never lets an exception escape, and such a function 
should not be declared TRAPS and should never be passed THREAD, but you 
need to manifest THREAD to use CATCH. I consider the CATCH macro to be a 
well-intentioned mistake.

> We need some meta-writeup rather than these decisions made in pull requests, because if we made these decisions collectively, I missed out.

Once we've fleshed things out we can propose them for the style guide. 
But it is easier to discuss concrete examples.

The important things (to me at least) at the moment are:

- we get rid of TRAPS from non-exception throwing code
- we pave the way to allow changing of TRAPS to declare "JavaThread* 
THREAD" so that functions that always expect to be called on a 
JavaThread can explicitly indicate that. (And that goes for non-TRAPS 
functions too.)
- we simplify/clarify code that uses an arbitrary mix of names for the 
current thread, and establish simple conventions to use going forward 
and to apply a-posteri on a time available basis when we do cleanups

Cheers,
David
-----

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3141
>