8264004: Don't use TRAPS if no exceptions are thrown #3141

coleenp · 2021-03-23T01:04:00Z

Removed the TRAPS in function declarations in jvmtiRedefineClasses and in ConstantPool merging functions.
Tested with vmTestbase/nsk/jvmti and tier1 (in progress).

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8264004: Don't use TRAPS if no exceptions are thrown

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Ioi Lam (@iklam - Reviewer)
Harold Seigel (@hseigel - Reviewer) ⚠️ Review applies to 3600616
Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to 9134ce0

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3141/head:pull/3141
$ git checkout pull/3141

To update a local copy of the PR:
$ git checkout pull/3141
$ git pull https://git.openjdk.java.net/jdk pull/3141/head

bridgekeeper · 2021-03-23T01:04:47Z

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-03-23T01:06:14Z

@coleenp The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-03-23T01:10:46Z

Webrevs

dholmes-ora

Hi Coleen,

This looks great! Good to see all those false TRAPS usages disappear. I found one more.

Thanks,
David

PS. Annoying that we often needs TRAPS through a call chain just because some leaf method may trigger an OOME. No escaping that unfortunately.

dholmes-ora · 2021-03-23T04:59:10Z

src/hotspot/share/prims/jvmtiRedefineClasses.hpp

  bool rewrite_cp_refs_in_permitted_subclasses_attribute(InstanceKlass* scratch_class);
+
  void rewrite_cp_refs_in_method(methodHandle method,
    methodHandle * new_method_p, TRAPS);
  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.
We need some meta-writeup rather than these decisions made in pull requests, because if we made these decisions collectively, I missed out.

openjdk · 2021-03-23T05:03:10Z

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264004: Don't use TRAPS if no exceptions are thrown

Reviewed-by: dholmes, iklam, hseigel, dcubed

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 36 new commits pushed to the master branch:

8d63bb6: 8260565: JFR: Fix copyright header in tests
0b2aa1b: 8263978: Clarify why 0 argument is ignored in SecureRandom::setSeed
15bcf6d: 8264055: backout JDK-8248904 in order to resubmit with additional attribution.
2425462: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
35102cb: 8263992: Remove dead code NativeLookup::base_library_lookup
91d86e6: 8263572: Output from jstack mixed mode is misaligned
47ef038: 8263905: Remove finalize methods for SocketInput/OutputStream
1c9817b: 8261479: CDS runtime code should check exceptions
087c8bf: 8264041: Incorrect comments for ParallelCompactData::summarize_dense_prefix
c087f3e: 8263995: Incorrect double-checked locking in Types.arraySuperType()
... and 26 more: https://git.openjdk.java.net/jdk/compare/f62b1008ed136dcbe7ee17191cc9b77ca2b70334...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

iklam

LGTM. I think suggested TRAPS changes could be done in a separate REF.

mlbridge · 2021-03-23T13:03:42Z

Mailing list message from David Holmes on serviceability-dev:

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

src/hotspot/share/oops/constantPool.cpp

hseigel

The changes look good!
Thanks, Harold

hseigel

Latest changes look good.
Thanks, Harold

coleenp · 2021-03-23T15:45:30Z

In your comments above, this makes sense to me:
Thread* THREAD = current; // For exception processing
This way we can keep the existing macros HAS_PENDING_EXCEPTION, etc.
I'll make this change here.

dcubed-ojdk

Looks good.

You should also do JDI test runs for this changeset and you should wait
to hear from the Serviceability team before integration.

src/hotspot/share/oops/constantPool.cpp

coleenp · 2021-03-23T19:15:38Z

I ran the jvmti and jdi tests, as well as the serviceability/jvmti/RedefineClasses tests, and jdk/java/lang/instrument tests.

iklam

Thread* THREAD = current; // for exception processing

is used only when the current method does not declare TRAPS, which means it should never throw. As a convention, I think the above code should be accompanied with by

HandleMark em(THREAD);

to ensure that the exceptions are not unintentionally leaked.

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

dholmes-ora

Updates look good - thanks.

I agree with Ioi about adding ExceptionMark as part of this usage pattern - it captures the intent that no exceptions are allowed to escape.

Thanks,
David

dholmes-ora

Looks good!

Thanks,
David

coleenp · 2021-03-24T12:14:34Z

Thanks for reviewing!
/integrate

openjdk · 2021-03-24T12:16:32Z

@coleenp Since your change was applied there have been 48 commits pushed to the master branch:

9ee0b9a: 8264052: Shenandoah: Backout 8263832
e55aa41: 8263899: HttpClient throws NPE in AuthenticationFilter when parsing www-authenticate head
6c0fbf7: 8254196: jshell infinite loops when startup script contains System.exit call
a79f095: 8263721: Unify oop casting
329697b: 8263358: Update java.lang to use instanceof pattern variable
ae9af57: 8264001: JFR: Modernize implementation
fad8484: 8263411: Convert jshell tool to use Stream.toList()
06d46d6: 8264008: Incorrect metaspace statistics after JEP 387 when UseCompressedClassPointers is off
45e1bab: 8264091: Use the blessed modifier order in java.logging
cb776ed: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
... and 38 more: https://git.openjdk.java.net/jdk/compare/f62b1008ed136dcbe7ee17191cc9b77ca2b70334...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5d7e93c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Don't pass TRAPS to functions that don't throw exceptions.

c3f57eb

openjdk bot added the rfr Pull request is ready for review label Mar 23, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Mar 23, 2021

missed THREAD that should be CHECK_false argument.

1e3f00d

dholmes-ora approved these changes Mar 23, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 23, 2021

iklam approved these changes Mar 23, 2021

View reviewed changes

hseigel reviewed Mar 23, 2021

View reviewed changes

src/hotspot/share/oops/constantPool.cpp Show resolved Hide resolved

hseigel approved these changes Mar 23, 2021

View reviewed changes

Fix some obvious single use variables.

3600616

hseigel approved these changes Mar 23, 2021

View reviewed changes

Fix load_new_class_versions and remove more traps.

9134ce0

dcubed-ojdk approved these changes Mar 23, 2021

View reviewed changes

src/hotspot/share/oops/constantPool.cpp Show resolved Hide resolved

iklam suggested changes Mar 23, 2021

View reviewed changes

src/hotspot/share/prims/jvmtiRedefineClasses.cpp Show resolved Hide resolved

src/hotspot/share/prims/jvmtiRedefineClasses.cpp Outdated Show resolved Hide resolved

dholmes-ora approved these changes Mar 23, 2021

View reviewed changes

Add ExceptionMarks.

2080b94

dholmes-ora approved these changes Mar 24, 2021

View reviewed changes

iklam approved these changes Mar 24, 2021

View reviewed changes

openjdk bot closed this Mar 24, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Mar 24, 2021

openjdk bot removed the rfr Pull request is ready for review label Mar 24, 2021

coleenp deleted the traps branch March 24, 2021 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8264004: Don't use TRAPS if no exceptions are thrown #3141

8264004: Don't use TRAPS if no exceptions are thrown #3141

coleenp commented Mar 23, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 23, 2021

openjdk bot commented Mar 23, 2021

mlbridge bot commented Mar 23, 2021 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Mar 23, 2021

iklam Mar 23, 2021

coleenp Mar 23, 2021

openjdk bot commented Mar 23, 2021 •

edited

Loading

iklam left a comment

mlbridge bot commented Mar 23, 2021

hseigel left a comment

hseigel left a comment

coleenp commented Mar 23, 2021

dcubed-ojdk left a comment

coleenp commented Mar 23, 2021

iklam left a comment

dholmes-ora left a comment

dholmes-ora left a comment

coleenp commented Mar 24, 2021

openjdk bot commented Mar 24, 2021

8264004: Don't use TRAPS if no exceptions are thrown #3141

8264004: Don't use TRAPS if no exceptions are thrown #3141

Conversation

coleenp commented Mar 23, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Mar 23, 2021

openjdk bot commented Mar 23, 2021

mlbridge bot commented Mar 23, 2021 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Mar 23, 2021

Choose a reason for hiding this comment

iklam Mar 23, 2021

Choose a reason for hiding this comment

coleenp Mar 23, 2021

Choose a reason for hiding this comment

openjdk bot commented Mar 23, 2021 • edited Loading

iklam left a comment

Choose a reason for hiding this comment

mlbridge bot commented Mar 23, 2021

hseigel left a comment

Choose a reason for hiding this comment

hseigel left a comment

Choose a reason for hiding this comment

coleenp commented Mar 23, 2021

dcubed-ojdk left a comment

Choose a reason for hiding this comment

coleenp commented Mar 23, 2021

iklam left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Mar 24, 2021

openjdk bot commented Mar 24, 2021

coleenp commented Mar 23, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 23, 2021 •

edited

Loading

openjdk bot commented Mar 23, 2021 •

edited

Loading