Pop extra args on BootstrapMethodError for invokedynamic #21720

matthewhall2 · 2025-04-24T19:30:16Z

When an error is caught during resolveInvokeDynamic (MethodHandleResolver.java), a MethodHandle that throws a BootstrapMethodError is returned, which has no arguments. This can cause an issue for OSR where we run out of pending push temp slots during OSR BookKeeping since we are not popping all the args off the operand stack.

This change recognizes when we have not popped all the arguments and pops them accordingly

Fixes: #21419

jdmpapin

There are some typos at end of the commit message:

all the ~~arguemnts~~ → arguments and pops ~~then~~ → them ~~accordinly~~ → accordingly.

I see that you've already corrected them in the PR description

runtime/compiler/ilgen/Walker.cpp

matthewhall2 · 2025-05-12T14:52:46Z

Thanks @jdmpapin , I've made the requested changes. Can I get another review please?

jdmpapin

This looks like it works properly now. Just a couple of formatting comments, and the "arguemnts" typo is still there in the commit message

runtime/compiler/ilgen/Walker.cpp

When an error is caught during resolveInvokeDynamic (MethodHandleResolver.java), a MethodHandle that throws a BootstrapMethodError is returned, which has no arguments. This can cause an issue for OSR where we run out of pending push temp slots since we are not popping all the args off the operand stack. This commit recognizes when we have not popped all the arguments and pops them accordingly. Fixes: eclipse-openj9#21419 Signed-off-by: Matthew Hall <[email protected]>

jdmpapin · 2025-05-12T19:29:48Z

@hzongaro, could you please review as well?

hzongaro

I think the changes look good. I had some cosmetic comments, but I don't want them to hold up the fix; I will approve this change and ask you to address them when you have the opportunity.

hzongaro · 2025-05-15T13:53:59Z

runtime/compiler/ilgen/J9ByteCodeIlGenerator.hpp

@@ -149,14 +149,15 @@ class TR_J9ByteCodeIlGenerator : public TR_IlGenerator, public TR_J9ByteCodeIter

   bool         runMacro(TR::SymbolReference *);
   bool         runFEMacro(TR::SymbolReference *);
-   TR::Node *    genInvoke(TR::SymbolReference *, TR::Node *indirectCallFirstChild, TR::Node *invokedynamicReceiver = NULL);
+   TR::Node *    genInvoke(TR::SymbolReference *, TR::Node *indirectCallFirstChild, TR::Node *invokedynamicReceiver = NULL, int32_t numExpectedArgs = -1);


May I ask you to add Doxygen comments for this method? I know it's not a new method, but hopefully over time we can get broader coverage.

hzongaro · 2025-05-15T13:54:14Z

runtime/compiler/ilgen/J9ByteCodeIlGenerator.hpp


-   TR::Node *    genInvokeDirect(TR::SymbolReference *symRef){ return genInvoke(symRef, NULL); }
+   TR::Node *    genInvokeDirect(TR::SymbolReference *symRef, int32_t numExpectedArgs = -1){ return genInvoke(symRef, NULL, NULL, numExpectedArgs); }


Again, may I ask you to add Doxygen comments for this method?

hzongaro · 2025-05-15T13:54:28Z

runtime/compiler/ilgen/Walker.cpp

@@ -3200,6 +3200,29 @@ static char *suffixedName(char *baseName, char typeSuffix, char *buf, int32_t bu
   return methodName;
   }

+static int32_t countParams(unsigned char *sig)


May I ask you to add Doxygen comments for this method?

hzongaro · 2025-05-15T13:55:55Z

runtime/compiler/ilgen/Walker.cpp

+   /* We need to get the expected number of parameters from the signature. There is a case where findOrCreateDynamicMethodSymbol()
+    * returns an error-throwing MethodHandle that takes 0 arguments (occurs when an error is caught during resolveInvokeDynamic()). We cannot use
+    * TR::Method::numberOfExplicitParameters() since that fetches the number of parameters of targetMethodSymRefs (the MH actually returned)
+    * instead of whats expected the invokedynamic call. This can be a problem since the expcected of args are already on the stack and won't be


Typos:

"whats expected the invokedynamic" -> "what's expected for the invokedynamic"

"expcected" -> "expected"

hzongaro · 2025-05-15T13:56:38Z

runtime/compiler/ilgen/Walker.cpp

+   if (numPopped < numExpectedArgs)
+      {
+      if (comp()->getOption(TR_TraceILGen))
+         traceMsg(comp(), "InvokeDynamic recieved error throwing MethodHandle. Popping extra args.\n");


Typo: "recieved" -> "received"

hzongaro · 2025-05-15T13:59:15Z

Jenkins test sanity.functional,sanity.openjdk all jdk17,jdk21

hzongaro · 2025-05-16T14:44:41Z

Testing completed successful and reviews approved. Merging.

pshipton · 2025-05-16T18:45:21Z

This seems to be causing crashes on jdk8, reverting it via #21902

jdmpapin · 2025-05-16T18:55:23Z

@matthewhall2, I'm guessing the extra popping was kicking in when it shouldn't. If that's the case, it should be possible to fix it by only doing extra pops when numExpectedArgs was explicitly specified, i.e. skip this part

if (numExpectedArgs == -1)
      numExpectedArgs = numArgs;

and then later on we can restrict the condition for popping:

if (numExpectedArgs > 0 && numPopped < numExpectedArgs)
   {
   ...
   }

matthewhall2 force-pushed the resolveInvokeDynamic_stackOverflowFix branch from 390d85c to 2cd5462 Compare May 2, 2025 19:38

jdmpapin mentioned this pull request May 2, 2025

System.Logger Triggers J9 Crash on OpenJDK 17 and 21 #21419

Closed

matthewhall2 marked this pull request as draft May 2, 2025 20:03

matthewhall2 force-pushed the resolveInvokeDynamic_stackOverflowFix branch from 2cd5462 to 3ecd2b3 Compare May 2, 2025 20:13

matthewhall2 marked this pull request as ready for review May 2, 2025 20:18

jdmpapin self-requested a review May 2, 2025 20:19

matthewhall2 changed the title ~~Add StackOverflow check in resolveInvokeDynamic~~ Pop extra args on BootstrapMethodError for invokedynamic May 2, 2025

jdmpapin reviewed May 5, 2025

View reviewed changes

matthewhall2 marked this pull request as draft May 8, 2025 14:33

matthewhall2 force-pushed the resolveInvokeDynamic_stackOverflowFix branch 5 times, most recently from dda7f01 to da57af0 Compare May 12, 2025 14:41

matthewhall2 marked this pull request as ready for review May 12, 2025 14:42

matthewhall2 requested a review from jdmpapin May 12, 2025 14:42

jdmpapin reviewed May 12, 2025

View reviewed changes

runtime/compiler/ilgen/Walker.cpp Show resolved Hide resolved

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

matthewhall2 force-pushed the resolveInvokeDynamic_stackOverflowFix branch from da57af0 to e8af1a6 Compare May 12, 2025 18:50

matthewhall2 requested a review from jdmpapin May 12, 2025 18:52

matthewhall2 force-pushed the resolveInvokeDynamic_stackOverflowFix branch from e8af1a6 to 743a0ce Compare May 12, 2025 18:57

jdmpapin requested a review from hzongaro May 12, 2025 19:29

jdmpapin approved these changes May 12, 2025

View reviewed changes

hzongaro approved these changes May 15, 2025

View reviewed changes

hzongaro added the comp:jit label May 15, 2025

hzongaro self-assigned this May 15, 2025

hzongaro merged commit 2e8cf57 into eclipse-openj9:master May 16, 2025
51 checks passed

pshipton mentioned this pull request May 16, 2025

Revert "Pop extra args on BootstrapMethodError for invokedynamic" #21902

Merged

This was referenced May 16, 2025

Update CRC32C acceleration for off-heap #21852

Merged

Update IntegerToChars evaluators for off-heap #21854

Merged

matthewhall2 mentioned this pull request May 27, 2025

Pop extra args on BootstrapMethodError for invokedynamic #21961

Merged


		TR::Node * genInvokeDirect(TR::SymbolReference *symRef){ return genInvoke(symRef, NULL); }
		TR::Node * genInvokeDirect(TR::SymbolReference *symRef, int32_t numExpectedArgs = -1){ return genInvoke(symRef, NULL, NULL, numExpectedArgs); }

Pop extra args on BootstrapMethodError for invokedynamic #21720

Pop extra args on BootstrapMethodError for invokedynamic #21720

Uh oh!

Conversation

matthewhall2 commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdmpapin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthewhall2 commented May 12, 2025

Uh oh!

jdmpapin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jdmpapin commented May 12, 2025

Uh oh!

hzongaro left a comment

Choose a reason for hiding this comment

Uh oh!

hzongaro May 15, 2025

Choose a reason for hiding this comment

Uh oh!

hzongaro May 15, 2025

Choose a reason for hiding this comment

Uh oh!

hzongaro May 15, 2025

Choose a reason for hiding this comment

Uh oh!

hzongaro May 15, 2025

Choose a reason for hiding this comment

Uh oh!

hzongaro May 15, 2025

Choose a reason for hiding this comment

Uh oh!

hzongaro commented May 15, 2025

Uh oh!

hzongaro commented May 16, 2025

Uh oh!

Uh oh!

pshipton commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdmpapin commented May 16, 2025

Uh oh!

Uh oh!

matthewhall2 commented Apr 24, 2025 •

edited

Loading

pshipton commented May 16, 2025 •

edited

Loading