Add check for throwDefaultConflict J9Method #21336

matthewhall2 · 2025-03-11T19:17:41Z

Part of fix for #20677

Checks for special initial J9Method throwDefaultConflict in linkToStaticSpecial(), and throws defaultConflict error (IncompatibleClassChangeError) accordingly.

changes vm initial method throwDefaultConflict:
- J9Method::extra field was changed from 0 to J9_JIT_NEVER_TRANSLATE. This fixes a crash in the JITed code where we assumed the method was compiled (since the last bit was 0). The case for when we try an invoke the default conflict method from the JIT is now handled in the vm BytecodeInterpreter.
- gave this fake method a fake bytecode field (with no bytecodes - just an empty method ROM method) to address issues in the stack walker when the error is thrown
Checks for special fake method in j2iTransition and handles error on same path a Native methods
Changes defaultConflictForMemberName error setup to be same as other Java level errors like NullPointer, WrongMethodType, ArrayIndex, etc.

runtime/vm/BytecodeInterpreter.hpp

babsingh · 2025-03-13T05:23:37Z

runtime/vm/BytecodeInterpreter.hpp

+#define nullCheck_j9bj(j9obj, fromJIT, dec_sp) \
+	do { \
+		if (J9_UNEXPECTED(NULL == j9obj)) { \
+			if (fromJIT) { \
+				_sp -= dec_sp;  \
+				buildJITResolveFrame(REGISTER_ARGS); \
+			} \
+			return THROW_NPE; \
+		} \
+ 	} while (0)


Instead of a define macro, an inlined function should be added for this code snippet.

If the macro is retained, the name's formatting should be fixed. It should be all caps (see #define RECORD_BYTECODE_PAIR(number) \ for reference).

babsingh · 2025-03-13T05:27:46Z

runtime/vm/BytecodeInterpreter.hpp

+		void *const jitReturnAddress = VM_JITInterface::fetchJITReturnAddress(_currentThread, _sp);
+		J9ROMMethod *const romMethod = J9_ROM_METHOD_FROM_RAM_METHOD(_sendMethod);
+		void *const exitPoint = j2iReturnPoint(J9ROMMETHOD_SIGNATURE(romMethod));


This file consistently follows void* const formatting throughout. So, we shouldn't modify it to void *const over here.

runtime/vm/BytecodeInterpreter.hpp

babsingh · 2025-03-13T13:18:30Z

I have provided feedback on this PR but am unsure about the correct guidelines for BytecodeInterpreter. @gacholio, could you take over the review and confirm whether my comments are valid or if a different approach is needed to address the issues?

matthewhall2 · 2025-03-17T14:51:25Z

thanks for the review @babsingh . I've made the changes you suggested. If you or @gacholio could take a provide more info about what buildGenericSpecialStackFrame and/or buildJITResolveFrame do (see #21336 (comment)) I would appreciate it.

gacholio · 2025-03-31T16:59:55Z

I will look at this in detail shortly - my feeling is that this should just work. Is the default conflict method marked as native (which affects the JIT transition)?

matthewhall2 · 2025-04-01T17:01:15Z

I will look at this in detail shortly - my feeling is that this should just work. Is the default conflict method marked as native (which affects the JIT transition)?

At the moment the only definition for this method is
J9Method cThrowDefaultConflict = { 0, 0, J9_BCLOOP_ENCODE_SEND_TARGET(J9_BCLOOP_SEND_TARGET_MEMBERNAME_DEFAULT_CONFLICT), 0 };
(so its marked as compiled).

However this will be changing in another PR: the extra field will be changed to (void *)J9_JIT_NEVER_TRANSLATE).
That change is to fix a crash in the JITed code resulting from the default conflict method being marked as compiled, but causes other issues on some platforms (#21540 - this symptom was also discussed over slack)
@gacholio

gacholio · 2025-04-03T15:05:11Z

I think I understand the issue now - the conflict method is too fake - it's not native, but has no bytecodes.

I suspect if we turn this method into an INL native and possibly mark it as never translate in the count field. throwForDefaultConflict already builds a stack frame, so it may be enough just to change the definition of the fake method.

@babsingh @fengxue-IS What do you think?

gacholio · 2025-04-03T15:47:32Z

I'm not quite sure how we get to the default conflict method, so it may not be practical to mark it native (which is a property of the ROM method). Perhaps we can use a new code in the count field to indicate that this method should take the native path when transitioning into the interpreter. Any negative value in count will prevent translation.

gacholio · 2025-04-03T16:40:07Z

I see there's already checks for the default conflict method - for now, if we just make the transition code take the native path when this is detected (and update the count to never translate) that may be sufficient.

matthewhall2 · 2025-04-03T17:44:06Z

I see there's already checks for the default conflict method - for now, if we just make the transition code take the native path when this is detected (and update the count to never translate) that may be sufficient.

this works on Z but will not work on x86 since it has JitDispatchJ9Method enabled, which causes an Invalid JIT Return address fail when we try and dispatch.

This doesn't happen on Z since the call to the fake method goes through the INL method linkToStaticSpecial before we do j2i

gacholio · 2025-04-04T11:22:10Z

What does JitDispatchJ9Method imply? Does it convert linkTo* calls into normal dispatch? If so, putting the default check in j2iTransition would catch both cases.

runtime/vm/BytecodeInterpreter.hpp

gacholio · 2025-04-07T16:53:38Z

It may be useful or necessary to make the conflict send target use the INL native convention (currently it build a generic method frame).

runtime/vm/BytecodeInterpreter.hpp

Checks that the J9Method being invoked is not the special J9Method vm->initialMethods.throwDefaultConflict. This fixes a segfault that was happening when trying to load a bytecodes field containing 0x0 Fixes: eclipse-openj9#20677 Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>

matthewhall2 · 2025-04-16T14:45:06Z

@gacholio can I get another review please?
I noticed that other Java level errors thrown from the various linkTo* methods (like NullPtr) don't build special frames, so I changed the handling for the default conflict error (MethodHandle case only - IncompatibleClassChangeError) to use the set setup as the other errors (and build a jitResolveFrame before handling like we do for NullPtr).

All my perosnal tests are passing, both on jenkins and locally (since this case almost never happens on Jenkins). Most of the jobs that show as failed are false - no actual fails show in the results. The few real fails unrelated

gacholio · 2025-04-17T11:19:25Z

Apologies, I've been out sick all week. There should be no need for a resolve frame - the native transition will take care of the stack.

gacholio · 2025-04-22T13:55:13Z

runtime/vm/BytecodeInterpreter.hpp

@@ -648,8 +652,13 @@ class INTERPRETER_CLASS
 #endif /* J9SW_NEEDS_JIT_2_INTERP_CALLEE_ARG_POP */
 			/* Set the flag indicating that the caller was the JIT */
 			_currentThread->jitStackFrameFlags = J9_SSF_JIT_NATIVE_TRANSITION_FRAME;
-			/* If a stop request has been posted, handle it instead of running the native */
+			if (isMethodDefaultConflictForMethodHandle) {


I don't think this necessary - see the suggested change to the throwing code.

Is building a JIT Resolve Frame is not necessary? Without it, even with the suggestions below, the wrong method is used to throw the error:
In gdb we get

Thread 2 "main" received signal SIGSEGV, Segmentation fault. dropPendingSendPushes (currentThread=0x17000) at /root/jdks/openj9-openjdk-jdk21/openj9/runtime/vm/drophelp.c:58 58 UDATA slotCount = J9_ARG_COUNT_FROM_ROM_METHOD(romMethod) + J9_TEMP_COUNT_FROM_ROM_METHOD(romMethod); => 0x00007ffff7a6f138 <dropPendingSendPushes+104>: 0f b6 42 fd movzbl -0x3(%rdx),%eax (gdb) print method $1 = (J9Method *) 0x7fff7ac0015c (gdb) gcore warning: target file /proc/1136059/cmdline contained unexpected null characters warning: Memory read failed for corefile section, 4096 bytes at 0xffffffffff600000. Saved corefile core.1136059 (gdb) print method->bytecodes $2 = (U_8 *) 0x9eb00685297eb <error: Cannot access memory at address 0x9eb00685297eb> (gdb)

which is not the fake method, and has inaccessible bytecodes. The method we get at that address is

(kca) j9m 0x7fff7ac0015c J9Method for JIT Method: {java/lang/invoke/LambdaForm$DMH/0x00000000f0b76c20.invokeSpecialIFC} Method {ClassPath/Name.MethodName}: {java/lang/invoke/LambdaForm$DMH/0x00000000f0b76c20.invokeSpecialIFC}

which is the Lambda Form used to call the (in this case fake) method.

NullPointerExceptions within the various linkTo* methods for LambdaForms build a JIT resolve frame so it may be necessary here too.

gacholio · 2025-04-22T13:58:57Z

runtime/vm/BytecodeInterpreter.hpp

@@ -9915,12 +9925,10 @@ class INTERPRETER_CLASS
 	VMINLINE VM_BytecodeAction
 	throwDefaultConflictForMemberName(REGISTER_ARGS_LIST)
 	{
-		/* Load the conflicting method and error message from this special target */
-		buildGenericSpecialStackFrame(REGISTER_ARGS, 0);


Leave this in but use the jit frame flags:

buildGenericSpecialStackFrame(REGISTER_ARGS, jitStackFrameFlags(REGISTER_ARGS, 0));

gacholio · 2025-04-22T14:00:35Z

runtime/vm/BytecodeInterpreter.hpp

 		updateVMStruct(REGISTER_ARGS);
+		prepareForExceptionThrow(_currentThread);


Not sure this is needed given the special stack frame build above.

gacholio · 2025-04-25T22:46:02Z

You don't appear to have made any changes since the last review request.

matthewhall2 mentioned this pull request Mar 11, 2025

jsr335tests FAILED Segmentation error vmState=0x00000000 Compiled_method=java/lang/invoke/LambdaForm$DMH/0x000000007df2b9f0.invokeSpecialIFC(Ljava/lang/Object;Ljava/lang/Object;)I #20677

Open

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch from 425f427 to 681ad93 Compare March 12, 2025 12:26

babsingh reviewed Mar 13, 2025

View reviewed changes

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch 2 times, most recently from 1976ac1 to 11e99cc Compare March 14, 2025 21:22

matthewhall2 requested a review from babsingh March 17, 2025 14:52

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch from 11e99cc to 58a22ad Compare March 17, 2025 15:33

babsingh mentioned this pull request Mar 18, 2025

Enable yielding of pinned continuations by default #21389

Merged

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch 2 times, most recently from 0781756 to 93634d9 Compare April 4, 2025 15:07

gacholio reviewed Apr 4, 2025

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch from 93634d9 to 6c2d620 Compare April 4, 2025 15:30

gacholio reviewed Apr 4, 2025

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved

gacholio reviewed Apr 4, 2025

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch 4 times, most recently from 3f9d86a to f97a110 Compare April 8, 2025 18:46

matthewhall2 requested a review from gacholio April 8, 2025 18:48

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch from f97a110 to f998f9a Compare April 8, 2025 20:31

gacholio reviewed Apr 9, 2025

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch from f998f9a to 72a0da9 Compare April 11, 2025 12:20

matthewhall2 requested a review from gacholio April 11, 2025 12:28

matthewhall2 force-pushed the vm_throwDefaultConflict_j9method_check branch from 72a0da9 to 5ec2adb Compare April 11, 2025 12:48

gacholio reviewed Apr 22, 2025

View reviewed changes

matthewhall2 requested a review from gacholio April 25, 2025 19:35

		updateVMStruct(REGISTER_ARGS);
		prepareForExceptionThrow(_currentThread);

Add check for throwDefaultConflict J9Method #21336

Are you sure you want to change the base?

Add check for throwDefaultConflict J9Method #21336

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

babsingh Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

babsingh Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

babsingh commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gacholio commented Mar 31, 2025

Uh oh!

matthewhall2 commented Apr 1, 2025

Uh oh!

gacholio commented Apr 3, 2025

Uh oh!

gacholio commented Apr 3, 2025

Uh oh!

gacholio commented Apr 3, 2025

Uh oh!

matthewhall2 commented Apr 3, 2025

Uh oh!

gacholio commented Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gacholio commented Apr 7, 2025

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gacholio commented Apr 17, 2025

Uh oh!

gacholio Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

matthewhall2 Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gacholio Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

gacholio commented Apr 25, 2025

Uh oh!

Uh oh!

matthewhall2 commented Mar 11, 2025 •

edited

Loading

babsingh commented Mar 13, 2025 •

edited

Loading

matthewhall2 commented Mar 17, 2025 •

edited

Loading

matthewhall2 commented Apr 16, 2025 •

edited

Loading

gacholio Apr 22, 2025 •

edited

Loading