Set InterfaceHandle correctly for methods inherited from Object #1554
Conversation
| if (*methodIndex == -1) { | ||
| method = NULL; | ||
| if (J9_ARE_ANY_BITS_SET(J9_CLASS_FROM_METHOD(method)->romClass->modifiers, J9_JAVA_INTERFACE)) { | ||
| *methodIndex = getITableIndexForMethod(method, lookupClass); |
There was a problem hiding this comment.
We should still be setting method to null if getITableIndexForMethod() returned -1.
There was a problem hiding this comment.
Also, is there a path through this code (or the caller) that allows returning with a null J9Method without having set an exception?
There was a problem hiding this comment.
Updated to return null and throw an exception.
lookupImpl() throws an exception when it returns null.
getMethodForSpecialSend() asserts notNull on the result
|
@DanHeidinga any other comments? |
| *methodIndex = getITableIndexForMethod(method, lookupClass); | ||
| if (-1 == *methodIndex) { | ||
| method = NULL; | ||
| vmFuncs->setCurrentException(currentThread, J9VMCONSTANTPOOL_JAVALANGNOSUCHMETHODERROR, NULL); |
There was a problem hiding this comment.
Can you sent the method name & signature as the message for this exception?
| if (J9_ARE_ANY_BITS_SET(J9_CLASS_FROM_METHOD(method)->romClass->modifiers, J9_JAVA_INTERFACE)) { | ||
| *methodIndex = getITableIndexForMethod(method, lookupClass); | ||
| if (-1 == *methodIndex) { | ||
| char msg[64]; /* long enough for most name and signature combinations */ |
There was a problem hiding this comment.
Let's write the code to allocate a buffer if this assumption is invalid. Not uncommon for ~40+ chars to be consumed by the package name. We've got examples of that pattern in exceptionsupport.c (I think)
pdbain-ibm
force-pushed
the
interface
branch
2 times, most recently
from
297666c to
7df1abb
Compare
|
Dan |
| + J9UTF8_LENGTH(methodName) + J9UTF8_LENGTH(sig) | ||
| + 4 /* period, parentheses, and terminating null */; | ||
| char *msg = (char *)j9mem_allocate_memory(nameAndSigLength, OMRMEM_CATEGORY_VM); | ||
| Assert_JCL_notNull(msg); |
There was a problem hiding this comment.
We don't assert on the return of j9mem_allocate_memory. The code can either throw the NSME or it can throw a NativeOOM but crashing the VM isn't the right choice.
| size_t nameAndSigLength = J9UTF8_LENGTH(className) | ||
| + J9UTF8_LENGTH(methodName) + J9UTF8_LENGTH(sig) | ||
| + 4 /* period, parentheses, and terminating null */; | ||
| char *msg = (char *)j9mem_allocate_memory(nameAndSigLength, OMRMEM_CATEGORY_VM); |
There was a problem hiding this comment.
The (char *) cast isn't required for the return value of j9mem_allocate_memory() in C code.
| @@ -78,6 +78,7 @@ static UDATA | |||
| lookupInterfaceMethod(J9VMThread *currentThread, J9Class *lookupClass, J9UTF8 *name, J9UTF8 *signature, UDATA *methodIndex) | |||
| { | |||
| J9Method *method; | |||
| PORT_ACCESS_FROM_VMC(currentThread); | |||
There was a problem hiding this comment.
This can be moved to the smallest enclosing the scope - the one using j9mem_allocate_memory
| J9ROMMethod *romMethod = J9_ROM_METHOD_FROM_RAM_METHOD(method); | ||
| J9UTF8 *methodName = J9ROMMETHOD_NAME(romMethod); | ||
| J9UTF8 *sig = J9ROMMETHOD_SIGNATURE(romMethod); | ||
| size_t nameAndSigLength = J9UTF8_LENGTH(className) |
There was a problem hiding this comment.
Can you adjust the formatting on this? The lines seem excessively tabbed over
Something like this is more readable
size_t nameAndSigLength = J9UTF8_LENGTH(className)
+ J9UTF8_LENGTH(methodName) + J9UTF8_LENGTH(sig)
+ 4 /* period, parentheses, and terminating null */;
600edca to
d259692
Compare
|
Thank you. Formatting, declarations, and allocation failure updated accordingly. |
| + 4 /* period, parentheses, and terminating null */; | ||
| char *msg = j9mem_allocate_memory(nameAndSigLength, OMRMEM_CATEGORY_VM); | ||
| if (NULL != msg) { | ||
| j9str_printf(PORTLIB, msg, nameAndSigLength, "%s.%s(%s)", |
There was a problem hiding this comment.
The format string isn't correct. It needs to be %.*s.%.*s(%.*s) to handle the non-null terminated UTF8s.
When calling lookup().getVirtual() on an interface for a method in java.lang.Object, don't check the itables for the method. Instead, set the methodIndex to -1 and return the concrete method. If the method cannot be found, throw an exception. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>
|
Fixed format string. |
|
Jenkins test sanity plinux |
|
@pdbain-ibm Can you create another PR to add your simple test case from #1524 to the JSR 292 test bucket? |
|
Issue for test case: #1593. |
When calling lookup().getVirtual() on an interface for a method in
java.lang.Object, don't check the itables for the method. Instead, set the
methodIndex to -1 and return the concrete method.
Fixes #1524
Signed-off-by: Peter Bain peter_bain@ca.ibm.com