[vmkit-commits] [PATCH] Better handle virtual calls through casted 'this' pointers.

[Nicolas Geoffray]

That's a great catch Will. Thanks very much for the test case. Please apply!

On Wed, Nov 16, 2011 at 8:29 PM, Will Dietz <wdietz2 at illinois.edu> wrote:

> Inlined below.
>
> I opted to do the assignable check, as opposed to just handling the
> case when customizeFor->lookupMethod fails, because if the classes
> aren't assignable


I think you're right. If we're casting 'this' to something unrelated, we'll
try to do bogus clever stuff. We won't it though, because of a runtime
CheckCastException, but, like you, I prefer to be on the safe side.


> then I'm concerned lookupMethod could conceivably
> return the *wrong* method, although I can't make a test case
> demonstrating that.
>

A way to have a test case is to write two files, one with class A and one
with class B extends A. Write the maybeFoo method like in the
InstanceofThisTest. And then change the file with class B to not extend A.

Then, I believe vmkit will try to do the 'bogus clever stuff'.

I don't have that framework in place for tests like that, so it's fine no
to think too much about it.


> This fixes the test case I checked in earlier, as well as the
> inspiration for that test case: java/awt/Toolkit.getDesktopProperty()
> (in OpenJDK anyway).
>
> Thoughts welcome, and thanks for your time! :)
>
> ~Will
>
> >From 6295ae789713baf56d5cc49500f2193812151db6 Mon Sep 17 00:00:00 2001
> From: Will Dietz <w at wdtz.org>
> Date: Wed, 16 Nov 2011 13:21:45 -0600
> Subject: [PATCH] Better handle virtual calls through casted 'this'
> pointers.
>
> * Being a 'thisReference' doesn't mean we don't need to dynamically resolve
> * Only customize if the call is valid for the current customization class.
>    This is checked by "if(customizeFor instanceof calleeClass)"
> ---
>  lib/J3/Compiler/JavaJIT.cpp |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/J3/Compiler/JavaJIT.cpp b/lib/J3/Compiler/JavaJIT.cpp
> index 202c1dc..0b87881 100644
> --- a/lib/J3/Compiler/JavaJIT.cpp
> +++ b/lib/J3/Compiler/JavaJIT.cpp
> @@ -121,10 +121,10 @@ void JavaJIT::invokeVirtual(uint16 index) {
>   bool customized = false;
>   bool thisReference =
>     isThisReference(stackSize() - signature->getNumberOfSlots() - 1);
> -  if (thisReference) {
> -    assert(meth != NULL);
> +  if (thisReference && meth) {
>

Please add a comment on why 'meth' may be NULL (you could just reference
the test case).


>     isCustomizable = true;
> -    if (customizeFor != NULL) {
> +    if ((customizeFor != NULL)
> +        && cl->isAssignableFrom(customizeFor)) {
>

And another comment on why we check if it's assignable :)


>       meth = customizeFor->lookupMethodDontThrow(
>           meth->name, meth->type, false, true, NULL);
>       assert(meth);
> --
> 1.7.5.1
> _______________________________________________
> vmkit-commits mailing list
> vmkit-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/vmkit-commits/attachments/20111116/8587854d/attachment.html>