Add test for BSM resolution on error throw #21730
Conversation
matthewhall2
force-pushed
the
test_makeConat_resolve_recursive
branch
from
fbd3a9c to
b5191c5
Compare
|
This test appears to be designed to recurse without limit, which it will until the stack overflows. However, your method lookup for After correcting the method lookup, the test runs but ultimately crashes on the pending push assert and the test fails. This is JDK 21. Is this the behaviour you are expecting? Please clarify what you hope to demonstrate with this test. Depending on a stack overflow in a test is often dubious as it may not always occur when you expect it to, leading to intermittent behaviour in the test. |
|
General comment: please make sure the indentation and spacing is consistent throughout. |
| } catch (NoSuchMethodException e) { | ||
| // ... | ||
| } catch (IllegalAccessException e) { | ||
| // ... | ||
| } catch (java.lang.reflect.InvocationTargetException e) { | ||
| // ... |
There was a problem hiding this comment.
Each of these exceptions should be handled as test failures.
| try { | ||
| Thread.sleep(30000); // let thread run for 30s | ||
| tester.stop(); | ||
| thread.join(); | ||
| } catch (InterruptedException e) {} | ||
| System.out.println("Ran with no errors"); | ||
| } |
There was a problem hiding this comment.
Please correct the indentation here (only use tabs).
matthewhall2
force-pushed
the
test_makeConat_resolve_recursive
branch
2 times, most recently
from
623104e to
5406db6
Compare
matthewhall2
changed the title
matthewhall2
force-pushed
the
test_makeConat_resolve_recursive
branch
3 times, most recently
from
26e7ba9 to
85d8646
Compare
When an error is thrown during resolveInvokeDynamic, we return a special error-throwing MethodHandle. This MH has no paramters, which can cause problems for OSR Bookkeeping since no arguments will be popped off the stack, causing the assertion failure in ILGen "cannot create a pending push temporary that exceeds that number of slots for this method" since we are trying to push the return value onto the stack when it is already full This commit tests arguments are popped and that the assertion does not occur. Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
matthewhall2
force-pushed
the
test_makeConat_resolve_recursive
branch
from
85d8646 to
496f378
Compare
|
Made some significant changes. Since this assertion fail can happen any time an error is thrown during resolveInvokeDynamic, changed the test to use a custom bootstrap method that throws a given error instead of trying to produce a stack overflow. |
| @@ -65,6 +65,7 @@ | |||
| <include name="**/indyn/Helper.java" /> | |||
| <include name="**/indyn/SimpleIndyGenerator.java" /> | |||
| <include name="**/CrossPackageHelper.java"/> | |||
| <include name="**/indy/IndyOSRTest.java"/> | |||
There was a problem hiding this comment.
Please correct the path and keep the elements sorted by name.
| @@ -512,4 +535,4 @@ SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0-only WITH Classpath-ex | |||
| <impl>ibm</impl> | |||
| </impls> | |||
| </test> | |||
| </playlist> | |||
| </playlist> | |||
There was a problem hiding this comment.
Please restore the final newline (every non-empty text file should end with one).
| <test name="jsr292_InDyn_OSRTest"> | ||
| <classes> | ||
| <class name="com.ibm.j9.jsr292.indyn.IndyOSRTest"/> | ||
| </classes> | ||
| </test> |
| </classes> | ||
| </test> | ||
|
|
||
| </suite> |
There was a problem hiding this comment.
Please restore the final newline.
| @@ -0,0 +1,150 @@ | |||
| /* | |||
| * Copyright IBM Corp. and others 2001 | |||
There was a problem hiding this comment.
The year should be 2025 for a new file.
| // attempt once in the interpreter | ||
| try { | ||
| if (t == null){ | ||
| Assert.assertTrue(cls.getMethod("dummy").invoke(null).equals("bootstrap1,2,3,4")); |
There was a problem hiding this comment.
Please use assertEquals() (also line 136).
| if (t == null) { | ||
| Assert.fail("Bootsrap method should not throw error when parameter t is null"); | ||
| } |
There was a problem hiding this comment.
Please use assertNotNull()
Assert.assertNotNull(t, "Bootsrap method should not throw error when parameter t is null");Also line 146.
| } else { | ||
| cls.getMethod("dummy").invoke(null); | ||
| } | ||
| } catch(IllegalAccessException e) { |
|
|
||
| // attempt once in the interpreter | ||
| try { | ||
| if (t == null){ |
| } | ||
| } |
There was a problem hiding this comment.
Please indent only with tabs.
When an error is thrown during resolveInvokeDynamic, we return a special error-throwing MethodHandle. This MH has no parameters, which can cause problems for OSR Bookkeeping since no arguments will be popped off the stack, causing the assertion failure in ILGen "cannot create a pending push temporary that exceeds that number of slots for this method" since we are trying to push the return value onto the stack when it is already full.
This commit tests arguments are popped and that the assertion does not occur.