David, Thank you so much for finding a way to do this. We do think this is important to get in for 8. And thank you for a way to check the hotspot changes in without waiting for the jdk changes.
Code looks good. Couple of minor comments: 1. universe.cpp - when the hotspot change gets in and the jdk change isn't in yet, are we going to see the tty->print_cr message? 2. style: Method* rather than Method space * 3. Head's up that ASM is being updated for default method support - visitMethoInsn will take an additional argument at the end for "isinterface" to determine if it needs a methodref or an interfacemethodref in the constant pool. I think that is in b117 (?) so we don't have it yet - so this will need modifying later - don't let that stop you from pushing the fix. See Lois's test webrev http://javaweb.us.oracle.com/~lfoltan/webrev/defmeth_asm_interfacemethodef_2/src/vm/runtime/defmeth/shared/ClassFileGenerator.java.sdiff.html thanks, Karen On Nov 22, 2013, at 2:07 PM, David Chase wrote: > Updated request. > > This is for a bug that it "deferred" by compilers, > but runtime really wants it fixed because they > are working in the same area and don't like it at all. > In particular, they want it committed to hotspot-rt ASAP > (don't want to wait for the multiweek turnaround) > and thus the diffs and testing are against hotspot-rt. > > There are jdk and hotspot webrevs; the hotspot webrev > is designed to allow it to run (with old behavior) in the > absence of the jdk change. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8016839 > Related test bug: https://bugs.openjdk.java.net/browse/JDK-8027733 > (confidential, sorry) > > Webrev(s): > http://cr.openjdk.java.net/~drchase/8016839/webrev-hotspot.01/ > http://cr.openjdk.java.net/~drchase/8016839/webrev-jdk.01/ > > Testing: > ute vm.quick.testlist A/B, no (new) failures > jtreg compiler runtime sanity, no (new) failures > defmeth, no new failures, fewer old failures > > New test, specifically tested on Sparc, Intel, embedded > -Xint, -Xcomp, plain, and with jdb on embedded, and > Netbeans on x86. (this was specifically to address the main > uncertainty, which was calling a zero-arg exception-throwing > method instead of the expected perhaps-multi-arg inaccessible > method; it is possible to devise calling conventions that will > make that not work, but we appear not to have done this. The test > includes 0- and 11-arg instances of the not-legally-accessible > methods). > > Problem: > Various patterns of inheritance and faulty overriding are supposed to > throw IllegalAccessError (IAE). At the time the bug was filed, they > instead threw NullPointerException (NPE) but after a related bug was > fixed the wrong exception changed to AbstractMethodError (AME). > > The root cause of the error is that (1) by convention, if an itable > entry is null, AME is thrown, and (2) in the case of inaccessible > methods overriding other methods that implement interface methods, a > null was entered into the itable (leading to throws of AME on code paths > that use the itable). > > This is a long-standing problem dating back to at least JDK 6, but made > much easier to observe by the introduction of method handles. Before > methodhandles this bug was more difficult to observe because resolution > of invokeinterface would aggressively evaluate errors and leave the > invoke unresolved if there was an error; thus repeated testing with > error receivers would always throw the proper error. If, however, a > good receiver is used for the invokeinterface, then resolution succeeds, > and subsequent invocations with bad receivers throws the wrong error > (AME instead of NPE) because the resolved code goes directly to the > itable and sees the null method. > > With Methodhandles, resolution and invocation are separate, and it > always throws the wrong error. > > Fix: > > I considered (and discussed with John Rose and Coleen Phillimore and > Karen Kinnear) use of a "second null value", where instead of using 0x0 > for the missing method, I would use 0x1, and then special case the null > checks (in triplicate, across all platforms) to instead unsigned compare > against a small integer and also special-case the trap handler for this > value. > > This seemed like a big change, potentially confusing to C programmers, > hacky, and somewhat hateful. > > I next considered creating a "bridge to nowhere" stub that would always > throw IAE, and fill that Method * in instead of null. Unfortunately, > the need for this stub is not discovered until after constant pool > rewriting has occurred, and adding new methods and CPC entries after > the rewriting looked very hairy. > > The last choice was to define a helper method in sun.misc.Unsafe that > would always throw IllegalAccessError when called, and use that Method * > in the should-throw-IAE case. The risk here is that the helper method > is zero-arg, where the method about to be called (and whose parameters > had been pushed on the stack) could have a different set of arguments, > and perhaps there is some platform where this does not work cleanly. > However, the existing code that checks for null and throws AME seems to > not be special-cased for number of args (that is, it branches to a > generic exception thrower with no additional cleanup). In addition, I > enhanced the test to try faulty invocations with both zero and 11 args, > and to run under jtreg in several modes, and I added iteration to help > force code to the compiled case as well, and it has performed properly > on Sparc, Intel, and embedded. >
