It looks good. I have three requests.
Regarding this comment:
+ * See MethodSupplier.java to see how to produce these bytes.
+ * They encode that class, except that method m is private, not public.
The recipe is incomplete, since it does not say which bits to tweak to make m
private. Please add that step to the comments more explicitly, or if possible
to the code (so bogusMethodSupplier is a clean copy of the od output). I
suppose you could ask sed to change the byte for you. That will make this test
a better example for future tests, and make it easier to maintain if javac
output changes. The high road to use would be asm, although for a single byte
tweak od works OK.
Also, this bug has two twins. The same thing will happen with NoSuchMethodE*
and NoSuchFieldE*. Can you please make fixes for those guys also?
FTR, see MemberName.makeAccessException() for logic which maps the other way,
from *Error to *Exception. I don't see a direct way to avoid the double
mapping (Error=>Exception=>Error) when it occurs.
For the initCause bit we should do this:
} catch (IllegalAccessException ex) {
Error err = new IllegalAccessError(ex.getMessage());
err.initCause(ex.getCause()); // reuse underlying cause of ex
throw err;
} ... and for NSME and NSFE...
That way users can avoid the duplicate exception but can see any underlying
causes that the JVM may include.
Thanks for untangling this!
— John
On Sep 12, 2013, at 12:17 PM, David Chase <[email protected]> wrote:
> The test is adapted from the test in the bug report.
> The headline on the bug report is wrong -- because it uses reflection in the
> test to do the invocation,
> the thrown exception is wrapped with InvocationTargetException, which is
> completely correct.
> HOWEVER, the exception inside the wrapper is the wrong one.
>
> The new test checks the exception in both the reflective-invocation and
> plain-invocation cases,
> and also checks both a method handle call (which threw the wrong exception)
> and a plain
> call (which threw, and throws, the right exception).
>
> The test also uses a tweaked classloader to substitute the borked bytecodes
> necessary to get
> the error, so it is not only shell-free, it can also be run outside jtreg.
>
> On 2013-09-12, at 2:34 PM, Christian Thalinger
> <[email protected]> wrote:
>
>>
>> On Sep 12, 2013, at 11:28 AM, David Chase <[email protected]> wrote:
>>
>>> New webrev, commented line removed:
>>> http://cr.openjdk.java.net/~drchase/8022701/webrev.03/
>>
>> I think the change is good given that the tests work now. Is your test
>> derived from the test in the bug report?
>>
>> And it would be good if John could also have a quick look (just be to be
>> sure).
>>
>> -- Chris
>>
>>>
>>> On 2013-09-12, at 1:53 PM, David Chase <[email protected]> wrote:
>>>
>>>> I believe it produced extraneous output -- it was not clear to me that it
>>>> was either necessary or useful to fully describe all the converted
>>>> exceptions that lead to the defined exception being thrown. The commented
>>>> line should have just been removed (I think).
>>>>
>>>> On 2013-09-12, at 1:24 PM, Christian Thalinger
>>>> <[email protected]> wrote:
>>>>
>>>>> + // err.initCause(ex);
>>>>>
>>>>> Why is this commented?
>>>>>
>>>>> -- Chris
>>>
>>
>
> _______________________________________________
> mlvm-dev mailing list
> [email protected]
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
