Mandy, thank you for reviewing this change. Comments inline.
On 11/05/18 11:10 PM, mandy chung wrote:
With your patch, IAE thrown with empty message is less desirable even
though it does not provide less information than the current message
(NPE::toString). I wonder if we could define an exception message in
MethodAccessorImpl for this IAE or even better null parameter is
passed in.
I'm open to adding an exception message (just like what the Native
method accessor does right now). I'll update the patch and send a
updated version soon.
Furthermore, this code has been written for a long time and not many
one is close to it. It would require someone to study the
implementation and generated code to review your patch properly. I'll
be on vacation next week. I may be able to help when I return.
Sure. I too am pretty new to this code (and OpenJDK in general) so would
like more reviews, especially the part where this introduces an
additional "aload" instruction.
-Jaikiran
Mandy
On 5/10/18 8:21 PM, Jaikiran Pai wrote:
Any reviews/sponsors please?
-Jaikiran
On 03/05/18 6:24 PM, Jaikiran Pai wrote:
Hi,
This mail contains a potential patch to the issue[1] that I had
reported a couple of years back. The issue is still valid and
reproducible with latest upstream JDK.
In brief, if a certain code has:
public void doSomething(String foo) {...}
and then it gets invoked through reflection:
thatMethod = // get hold of that method through reflection
thatMethod.invoke(validObjectInstance, null);
then as per the javadoc of the Method#invoke[2] you would expect an
IllegalArgumentException (since the method expects 1 parameter and
we are sending none). This does throw an IllegalArgumentException,
but when the invocation happens through a (dynamically generated)
MethodAccessor, instead of a native MethodAccessor, the
IllegalArgumentException that gets thrown is due to a NPE that
happens and the NPE's toString() output is contained as a message of
the IllegalArgumentException, as noted in the JIRA. This happens
even for Constructor invocations, through ConstructorAccessor.
The commit in the attached patch, adds bytecode instructions to
check if the method arguments being passed is null and if so,
doesn't attempt an arraylength instruction and instead just stores 0
on to the stack. This prevents the NullPointerException being
reported, enclosed as a message within the IllegalArgumentException
and instead throws back a proper IllegalArgumentException (as you
would expect if the invocation had happened over a native
MethodAccessor).
The goal of the patch _isn't_ to match the exception message with
what the native MethodAccessor throws but just prevent the NPE from
being playing a role in the IllegalArgumentException. i.e. this
change _doesn't_ throw a new IllegalArgumentException("wrong number
of arguments").
The patch also contains a new testcase which reproduces this against
the current upstream and verifies this patch works.
This is the first time I'm fiddling with bytecode generation, so I
would appreciate some feedback on the changed bytecode and if
there's a different/better way to do it. Furthermore, although I do
have a signed and approved OCA, I will need a sponsor for this
patch. Anyone willing to review and sponsor, please?
[1] https://bugs.openjdk.java.net/browse/JDK-8159797
[2]
https://docs.oracle.com/javase/10/docs/api/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)
-Jaikiran
