On Sep 19, 2013, at 9:57 AM, David Chase <david.r.ch...@oracle.com> wrote:
> Recommended changes made: > > http://cr.openjdk.java.net/~drchase/8022701/webrev.04/ Looks good. Thanks for using ASM. > > Test with jtreg (for pass and for induced failure) on MacOS, > not sure what additional other testing is desired since it is entirely in the > libraries. > > I included code to handle the case of a broken field-referencing methodhandle, > but did not test it because there's no syntax for these in Java, nor is their > creation > well-documented. > > David > > > On 2013-09-13, at 12:02 AM, John Rose <john.r.r...@oracle.com> wrote: > >> On Sep 12, 2013, at 5:44 PM, David Chase <david.r.ch...@oracle.com> wrote: >> >>> Do these sibling bugs have numbers? >> >> Yes, 8022701. I just updated the bug to explain their common genesis. >> >>> And I take it you would like to see >>> >>> 1) a test enhanced with ASM to do all 3 variants of this >> >> Yes, please. The case of "no such field" does not have a direct cause from >> lambda expressions AFAIK, but it can occur with "raw" >> CONSTANT_MethodHandles. (It would be reasonable for javac to emit >> CONSTANT_MH's for fields in some very limited circumstances, but I'll bet it >> doesn't.) >> >>> 2) DO attach the underlying cause >> >> Yes. Read the javadoc for ExceptionInInitializerError for an example of why >> users want the underlying cause for linkage errors. >> >> (Golly, I wonder what happens if resolution of a CONSTANT_MethodHandle tries >> to touch a class with a booby-trapped <clinit>. I hope it's the case that >> the ExceptionInInitializerError just passes straight up the stack. But if >> it were to get wrapped in a ROE of some sort, it would probably bubble up as >> an IAE. Could be a charming exploration. Actually, no, I don't want to go >> in there. Getting all possible linkage errors correct is fine, but we have >> more important things to do.) >> >> — John >> >>> David >>> >>> On 2013-09-12, at 5:35 PM, John Rose <john.r.r...@oracle.com> wrote: >>> >>>> 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 <david.r.ch...@oracle.com> 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 >>>>> <christian.thalin...@oracle.com> wrote: >>>>> >>>>>> >>>>>> On Sep 12, 2013, at 11:28 AM, David Chase <david.r.ch...@oracle.com> >>>>>> 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 <david.r.ch...@oracle.com> 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 >>>>>>>> <christian.thalin...@oracle.com> wrote: >>>>>>>> >>>>>>>>> + // err.initCause(ex); >>>>>>>>> >>>>>>>>> Why is this commented? >>>>>>>>> >>>>>>>>> -- Chris >>>>>>> >>>>>> >>>>> >>>>> _______________________________________________ >>>>> mlvm-dev mailing list >>>>> mlvm-...@openjdk.java.net >>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >>>> >>> >> > > _______________________________________________ > mlvm-dev mailing list > mlvm-...@openjdk.java.net > http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
