8262368: wrong verifier message for bogus return type #2757

hseigel · 2021-02-26T20:07:49Z

Please review this small change to fix some verifier error messages. If a method's descriptor has a void return signature, but contains a bytecode such as 'ireturn' or 'areturn'. then it will get a VerifyError exception with the message "Method descriptor has a void return type". Conversely, if a method's descriptor has a non-void return signature, but contains a 'return' bytecode then it will get a VerifyError exception with the message "Method descriptor has a non-void return type".

A sample error message looks like:
Error: Unable to initialize main class Hi
Caused by: java.lang.VerifyError: Method descriptor has a void return type
Exception Details:
Location:
Hi.noRet(I)V @1: ireturn
Reason:
Type integer (current frame, stack[0]) is not assignable to top (from method signature)
Current Frame:
bci: @1
flags: { }
locals: { integer }
stack: { integer }
Bytecode:
0000000: 08ac

The fix was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, tiers 3-5 on Linux x64. and the JCK Lang and VM tests 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-8262368: wrong verifier message for bogus return type

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Download

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

bridgekeeper · 2021-02-26T20:08:56Z

👋 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 · 2021-02-26T20:10:20Z

@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 · 2021-02-26T20:14:03Z

Webrevs

01: Full - Incremental (fd83748)
00: Full (a810515)

dholmes-ora

Hi Harold,

Sorry I don't like the new messages. See below.

Thanks,
David

dholmes-ora · 2021-03-01T02:31:29Z

src/hotspot/share/classfile/verifier.cpp

          if (return_type != VerificationType::bogus_type()) {
            verify_error(ErrorContext::bad_code(bci),
-                         "Method expects a return value");
+                         "Method descriptor has a non-void return type");


I prefer the original text. The new text makes me have to think about what the actual problem is too much.

src/hotspot/share/classfile/verifier.cpp

coleenp

Looks good with the fixed message. I agree that it took me an extra minute to understand the new messages, so the old ones (corrected) are fine. Nice simple test.

coleenp · 2021-03-01T16:22:46Z

test/hotspot/jtreg/runtime/verifier/ReturnMsgs.java

+ * @test
+ * @bug 8262368
+ * @summary Test that VerifyError messages are correct when return bytecodes
+ *          andd method signatures do not match.


openjdk · 2021-03-01T16:27:15Z

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

8262368: wrong verifier message for bogus return type

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

044e2a2: 8183569: Assert the same limits are used in parse_xss and globals.hpp
5de0f4b: 8260869: Test java/foreign/TestHandshake.java fails intermittently
c9097a6: 8262893: Enable more doclint checks in javadoc build
40bdf52: 8262096: Vector API fails to work due to VectorShape initialization exception
93ffe6a: 8262892: minor typo in implSpec comment
4f4d0f5: 8261969: SNIHostName should check if the encoded hostname conform to RFC 3490
c92f3bc: 8262876: Shenandoah: Fix comments regarding VM_ShenandoahOperation inheritances
20b9ba5: 8262875: doccheck: empty paragraphs, etc in java.base module
f304b74: 8261859: gc/g1/TestStringDeduplicationTableRehash.java failed with "RuntimeException: 'Rehash Count: 0' found in stdout"
f18c019: 8247373: ArraysSupport.newLength doc, test, and exception message
... and 42 more: https://git.openjdk.java.net/jdk/compare/de3f519dc90c8ca8f0879c21705afd2889eadd54...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

You fixed the wrong one.

Cheers,
David

dholmes-ora · 2021-03-02T05:37:52Z

src/hotspot/share/classfile/verifier.cpp

    verify_error(ErrorContext::bad_type(bci,
        current_frame->stack_top_ctx(), TypeOrigin::signature(return_type)),
-        "Method expects a return value");
+        "Method does not expect a return value");


You've changed the wrong line. This one does expect a return value

Hi David,
If a method's descriptor has a void return type, but the method contains a bytecode such as areturn, then I think the message "Method does not expect a return type" is correct. Perhaps the word 'expect' is ambiguous? If so, then I think a more explicit message, similar to my original suggestion, should be used.
Harold

Okay I see now the original bug report was incorrect and the referenced code is correct. But the above code is actually incorrect and now fixed.

coleenp · 2021-03-02T17:02:24Z

src/hotspot/share/classfile/verifier.cpp

 void ClassVerifier::verify_return_value(
    VerificationType return_type, VerificationType type, u2 bci,
    StackMapFrame* current_frame, TRAPS) {
  if (return_type == VerificationType::bogus_type()) {


return_type is the type expected from the function, which here is void (I guess void == bogus_type, but ok).
So the message is correct.

mlbridge · 2021-03-02T22:54:41Z

Mailing list message from David Holmes on hotspot-runtime-dev:

On 3/03/2021 1:26 am, Harold Seigel wrote:

On Tue, 2 Mar 2021 05:37:52 GMT, David Holmes <dholmes at openjdk.org> wrote:

Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:

changed error message text

src/hotspot/share/classfile/verifier.cpp line 3152:

3150: verify_error(ErrorContext::bad_type(bci,
3151: current_frame->stack_top_ctx(), TypeOrigin::signature(return_type)),
3152: "Method does not expect a return value");

You've changed the wrong line. This one does expect a return value

Hi David,
If a method's descriptor has a void return type, but the method contains a bytecode such as areturn, then I think the message "Method does not expect a return type" is correct. Perhaps the word 'expect' is ambiguous? If so, then I think a more explicit message, similar to my original suggestion, should be used.

First the bug report does not concern the line you changed! The bug
report is about:

"There is a typo in an error message in verifier.cpp on line 1678"

1675: case Bytecodes::_return :
1676: if (return_type != VerificationType::bogus_type()) {
1677: verify_error(ErrorContext::bad_code(bci),
1678: "Method expects a return value");
1679: return;
1680: }

Here we have a plain _return bytecode, but the return-type is not void
as expected. So we have a return value where we do not expect one. Hence
the message on line 1678 is wrong, the method does not expect a return
value.

Now looking at the code you changed:

if (return_type == VerificationType::bogus_type()) {
verify_error(ErrorContext::bad_type(bci,
current_frame->stack_top_ctx(),
TypeOrigin::signature(return_type)),
"Method does not expect a return value");
return;
}

We have found a void return and this is an error. That means a void
return was not expected i.e. the method DOES expect a return value. And
the original message is correct.

Is there some weird double-negative logic being applied in all this that
is somehow inverting the sense of everything ???

Cheers,
David
-----

coleenp · 2021-03-03T02:48:12Z

Let's use javac to help us with the error message:
$ more X.java
class X {
void foo() { return obj; }
int bar() { return; }
}
$ javac X.java
X.java:2: error: incompatible types: unexpected return value
void foo() { return obj; } <=== This is line 3152: "Method does not expect a return value"
^
X.java:3: error: incompatible types: missing return value
int bar() { return; } <=== This is line 1678: "Method expects a return value"
^
2 errors

mlbridge · 2021-03-03T03:58:40Z

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Coleen,

On 3/03/2021 12:50 pm, Coleen Phillimore wrote:

On Tue, 2 Mar 2021 05:38:31 GMT, David Holmes <dholmes at openjdk.org> wrote:

Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:

changed error message text

You fixed the wrong one.

Cheers,
David

Let's use javac to help us with the error message:
$ more X.java
class X {
void foo() { return obj; }
int bar() { return; }
}
$ javac X.java
X.java:2: error: incompatible types: unexpected return value
void foo() { return obj; } <=== This is line 3152
^
X.java:3: error: incompatible types: missing return value
int bar() { return; } <=== This is line 1678
^
2 errors

Thank you! I see now where I was reading things in an inverted sense:

1675: case Bytecodes::_return :
1676: if (return_type != VerificationType::bogus_type()) {
1677: verify_error(ErrorContext::bad_code(bci),
1678: "Method expects a return value");
1679: return;
1680: }

Here we have a plain "return" but the method's return-type is not-void,
so it is an error to have the plain "return" because the "Method expects
a return value". So the actual initial bug report was in fact wrong.

As Harold discovered the true problem is with the other piece of code.

Sorry for the confusion Harold!

Thanks,
David

dholmes-ora

Thanks,
David

dholmes-ora · 2021-03-03T04:44:56Z

src/hotspot/share/classfile/verifier.cpp

    verify_error(ErrorContext::bad_type(bci,
        current_frame->stack_top_ctx(), TypeOrigin::signature(return_type)),
-        "Method expects a return value");
+        "Method does not expect a return value");


Okay I see now the original bug report was incorrect and the referenced code is correct. But the above code is actually incorrect and now fixed.

hseigel · 2021-03-03T13:18:03Z

Thanks Coleen and David for the reviews!

/integrate

openjdk · 2021-03-03T13:19:01Z

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

6d3c858: 8259235: javac crashes while attributing super method invocation
bf90e85: 8262926: JDK-8260966 broke AIX build
54dfd79: 8262256: C2 intrinsincs should not modify IR when bailing out
0265ab6: 8262466: linux libsaproc/DwarfParser.cpp delete DwarfParser object in early return
c15801e: 8261142: AArch64: Incorrect instruction encoding when right-shifting vectors with shift amount equals to the element width
044e2a2: 8183569: Assert the same limits are used in parse_xss and globals.hpp
5de0f4b: 8260869: Test java/foreign/TestHandshake.java fails intermittently
c9097a6: 8262893: Enable more doclint checks in javadoc build
40bdf52: 8262096: Vector API fails to work due to VectorShape initialization exception
93ffe6a: 8262892: minor typo in implSpec comment
... and 47 more: https://git.openjdk.java.net/jdk/compare/de3f519dc90c8ca8f0879c21705afd2889eadd54...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3d3eb5c.

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

8262368: wrong verifier message for bogus return type

a810515

openjdk bot added the rfr Pull request is ready for review label Feb 26, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 26, 2021

dholmes-ora suggested changes Mar 1, 2021

View reviewed changes

changed error message text

fd83748

coleenp approved these changes Mar 1, 2021

View reviewed changes

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

dholmes-ora suggested changes Mar 2, 2021

View reviewed changes

coleenp reviewed Mar 2, 2021

View reviewed changes

dholmes-ora approved these changes Mar 3, 2021

View reviewed changes

openjdk bot closed this Mar 3, 2021

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 Mar 3, 2021

8262368: wrong verifier message for bogus return type #2757

8262368: wrong verifier message for bogus return type #2757

Uh oh!

Conversation

hseigel commented Feb 26, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Feb 26, 2021

Uh oh!

openjdk bot commented Feb 26, 2021

Uh oh!

mlbridge bot commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

hseigel Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

coleenp Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Mar 2, 2021

Uh oh!

coleenp commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Mar 3, 2021

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

hseigel commented Mar 3, 2021

Uh oh!

openjdk bot commented Mar 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

hseigel commented Feb 26, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 26, 2021 •

edited

Loading

openjdk bot commented Mar 1, 2021 •

edited

Loading

coleenp commented Mar 3, 2021 •

edited

Loading