That's a great catch Will. Thanks very much for the test case. Please apply!<div><br><div class="gmail_quote">On Wed, Nov 16, 2011 at 8:29 PM, Will Dietz <span dir="ltr"><<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Inlined below.<br>
<br>
I opted to do the assignable check, as opposed to just handling the<br>
case when customizeFor->lookupMethod fails, because if the classes<br>
aren't assignable </blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">then I'm concerned lookupMethod could conceivably<br>
return the *wrong* method, although I can't make a test case<br>
demonstrating that.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>Then, I believe vmkit will try to do the 'bogus clever stuff'.</div><div><br></div><div>I don't have that framework in place for tests like that, so it's fine no to think too much about it. </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
This fixes the test case I checked in earlier, as well as the<br>
inspiration for that test case: java/awt/Toolkit.getDesktopProperty()<br>
(in OpenJDK anyway).<br>
<br>
Thoughts welcome, and thanks for your time! :)<br>
<br>
~Will<br>
<br>
>From 6295ae789713baf56d5cc49500f2193812151db6 Mon Sep 17 00:00:00 2001<br>
From: Will Dietz <<a href="mailto:w@wdtz.org">w@wdtz.org</a>><br>
Date: Wed, 16 Nov 2011 13:21:45 -0600<br>
Subject: [PATCH] Better handle virtual calls through casted 'this' pointers.<br>
<br>
* Being a 'thisReference' doesn't mean we don't need to dynamically resolve<br>
* Only customize if the call is valid for the current customization class.<br>
This is checked by "if(customizeFor instanceof calleeClass)"<br>
---<br>
lib/J3/Compiler/JavaJIT.cpp | 6 +++---<br>
1 files changed, 3 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/lib/J3/Compiler/JavaJIT.cpp b/lib/J3/Compiler/JavaJIT.cpp<br>
index 202c1dc..0b87881 100644<br>
--- a/lib/J3/Compiler/JavaJIT.cpp<br>
+++ b/lib/J3/Compiler/JavaJIT.cpp<br>
@@ -121,10 +121,10 @@ void JavaJIT::invokeVirtual(uint16 index) {<br>
bool customized = false;<br>
bool thisReference =<br>
isThisReference(stackSize() - signature->getNumberOfSlots() - 1);<br>
- if (thisReference) {<br>
- assert(meth != NULL);<br>
+ if (thisReference && meth) {<br></blockquote><div><br></div><div>Please add a comment on why 'meth' may be NULL (you could just reference the test case).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
isCustomizable = true;<br>
- if (customizeFor != NULL) {<br>
+ if ((customizeFor != NULL)<br>
+ && cl->isAssignableFrom(customizeFor)) {<br></blockquote><div><br></div><div>And another comment on why we check if it's assignable :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
meth = customizeFor->lookupMethodDontThrow(<br>
meth->name, meth->type, false, true, NULL);<br>
assert(meth);<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.7.5.1<br>
_______________________________________________<br>
vmkit-commits mailing list<br>
<a href="mailto:vmkit-commits@cs.uiuc.edu">vmkit-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits</a><br>
</font></span></blockquote></div><br></div>