On 23/03/2021 9:54 pm, Coleen Phillimore wrote:
On Tue, 23 Mar 2021 05:52:51 GMT, Ioi Lam <ik...@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
