8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp #618

hseigel · 2020-10-12T20:34:25Z

Hi,
Please review this small change to simplify exception throwing in classFileParser.cpp The change was tested with tiers one and two on Windows, Linux x64, and Mac OSX, and with tiers 3-5 on Linux x64.

Thanks, Harold

Progress

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

Issue

JDK-8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp

Reviewers

Lois Foltan (@lfoltan - Reviewer) ⚠️ Review applies to feb343c
David Holmes (@dholmes-ora - Reviewer)
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to feb343c

Download

$ git fetch https://git.openjdk.java.net/jdk pull/618/head:pull/618
$ git checkout pull/618

…eParser.cpp

bridgekeeper · 2020-10-12T20:35:20Z

👋 Welcome back hseigel! 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 · 2020-10-12T20:37:09Z

@hseigel The following label will be automatically applied to this pull request:

hotspot-runtime

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

mlbridge · 2020-10-12T20:40:17Z

Webrevs

01: Full - Incremental (3c4d6e2)
00: Full (feb343c)

lfoltan

Looks good Harold!

openjdk · 2020-10-12T21:03:27Z

@hseigel 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:

8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp

Reviewed-by: lfoltan, dholmes, coleenp

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 26 new commits pushed to the master branch:

ba140b0: 8254748: Bad Copyright header format after JDK-8212218
db9dcdf: 8254668: JVMTI process frames on thread without started processing
dc262df: 8212218: [TESTBUG] runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryErrorInMetaspace.java timed out
9eeeb8a: 8254696: safepointMechanism_aix needs adaptation for JDK-8253180
b509e31: 8254602: compiler/debug/TestStressCM.java failed with "RuntimeException: got the same optimization stats for different seeds: expected 45"
9fe9b24: 8254575: C2: Clean up unused TRACK_PHI_INPUTS assertion code
31d9b7f: 8254252: Generic arraycopy stub overwrites callee-save rdi register on 64-bit Windows
a098037: 8254365: ciMethod.hpp should not include methodHandles.hpp
d50e0de: 8254722: bsd_zero builds broken after JDK-8253717
ba5dc67: 8254158: Consolidate per-platform stack overflow handling code
... and 16 more: https://git.openjdk.java.net/jdk/compare/2a4328ba7d4bf81a7695c1bda1215e57a33679e2...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.

dholmes-ora

Hi Harold,
Not sure why we couldn't use THROW_MSG in these cases? The helper functions are okay though.
But don't use CHECK macro here - just pass THREADS and then return. We know there is a pending exception.
Thanks,
David

dholmes-ora · 2020-10-12T21:40:20Z

src/hotspot/share/classfile/classFileParser.cpp

-        this_klass->external_name(),
-        super_ik->external_name());
-      return;
+      classfile_icce_error("class %s cannot inherit from sealed class %s", super_ik, CHECK);


You don't want CHECK there - you know there is an exception pending. Just pass THREAD and return afterwards. This goes for all the new callsites.

dholmes-ora · 2020-10-12T21:44:45Z

src/hotspot/share/classfile/classFileError.cpp

@@ -78,6 +78,25 @@ void ClassFileParser::classfile_parse_error(const char* msg,
                     msg, name, signature, _class_name->as_C_string());
 }

+void ClassFileParser::classfile_icce_error(const char* msg,


Is there a reason we can't just use THROW_MSG at the callsites rather than introducing these helper functions?

I think it's intended to match existing usage.

coleenp

Looks good to me. I agree with David's comment to just pass THREAD and return; Saves an implicit if statement in these places.

coleenp · 2020-10-12T22:16:31Z

src/hotspot/share/classfile/classFileError.cpp

@@ -78,6 +78,25 @@ void ClassFileParser::classfile_parse_error(const char* msg,
                     msg, name, signature, _class_name->as_C_string());
 }

+void ClassFileParser::classfile_icce_error(const char* msg,


I think it's intended to match existing usage.

…eParser.cpp

hseigel · 2020-10-13T17:14:10Z

Thanks for looking at this change! Please review the updated change that replaces CHECK* arguments with THREAD, followed by 'return' statements. This change was made for calls to the new methods and existing calls to classfile_parse_error().

coleenp · 2020-10-13T17:38:13Z

This looks like a good improvement. It might make the size of the generated code in cfp smaller. Thanks!

dholmes-ora

Looks good - thanks for the extended cleanup. This is why we have the THROW macros to handle the return. :)

hseigel · 2020-10-14T12:21:30Z

/integrate

openjdk · 2020-10-14T12:23:33Z

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

ba140b0: 8254748: Bad Copyright header format after JDK-8212218
db9dcdf: 8254668: JVMTI process frames on thread without started processing
dc262df: 8212218: [TESTBUG] runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryErrorInMetaspace.java timed out
9eeeb8a: 8254696: safepointMechanism_aix needs adaptation for JDK-8253180
b509e31: 8254602: compiler/debug/TestStressCM.java failed with "RuntimeException: got the same optimization stats for different seeds: expected 45"
9fe9b24: 8254575: C2: Clean up unused TRACK_PHI_INPUTS assertion code
31d9b7f: 8254252: Generic arraycopy stub overwrites callee-save rdi register on 64-bit Windows
a098037: 8254365: ciMethod.hpp should not include methodHandles.hpp
d50e0de: 8254722: bsd_zero builds broken after JDK-8253717
ba5dc67: 8254158: Consolidate per-platform stack overflow handling code
... and 16 more: https://git.openjdk.java.net/jdk/compare/2a4328ba7d4bf81a7695c1bda1215e57a33679e2...master

Your commit was automatically rebased without conflicts.

Pushed as commit 95e68c6.

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

8254586: Replace fthrow() calls with simpler method calls in classFil…

feb343c

…eParser.cpp

openjdk bot added the rfr Pull request is ready for review label Oct 12, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 12, 2020

lfoltan approved these changes Oct 12, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 12, 2020

dholmes-ora suggested changes Oct 12, 2020

View reviewed changes

coleenp approved these changes Oct 12, 2020

View reviewed changes

8254586: Replace fthrow() calls with simpler method calls in classFil…

3c4d6e2

…eParser.cpp

dholmes-ora approved these changes Oct 14, 2020

View reviewed changes

openjdk bot closed this Oct 14, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 14, 2020

8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp #618

8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp #618

hseigel commented Oct 12, 2020 •

edited by openjdk bot

bridgekeeper bot commented Oct 12, 2020

openjdk bot commented Oct 12, 2020

mlbridge bot commented Oct 12, 2020 •

edited

lfoltan left a comment

openjdk bot commented Oct 12, 2020 •

edited

dholmes-ora left a comment

dholmes-ora Oct 12, 2020

dholmes-ora Oct 12, 2020

coleenp Oct 12, 2020

coleenp left a comment

coleenp Oct 12, 2020

hseigel commented Oct 13, 2020

coleenp commented Oct 13, 2020

dholmes-ora left a comment

hseigel commented Oct 14, 2020

openjdk bot commented Oct 14, 2020

8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp #618

8254586: Replace fthrow() calls with simpler method calls in classFileParser.cpp #618

Conversation

hseigel commented Oct 12, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Oct 12, 2020

openjdk bot commented Oct 12, 2020

mlbridge bot commented Oct 12, 2020 • edited

Webrevs

lfoltan left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 12, 2020 • edited

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Oct 12, 2020

Choose a reason for hiding this comment

dholmes-ora Oct 12, 2020

Choose a reason for hiding this comment

coleenp Oct 12, 2020

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

coleenp Oct 12, 2020

Choose a reason for hiding this comment

hseigel commented Oct 13, 2020

coleenp commented Oct 13, 2020

dholmes-ora left a comment

Choose a reason for hiding this comment

hseigel commented Oct 14, 2020

openjdk bot commented Oct 14, 2020

hseigel commented Oct 12, 2020 •

edited by openjdk bot

mlbridge bot commented Oct 12, 2020 •

edited

openjdk bot commented Oct 12, 2020 •

edited