Thanks, Joe. I reverted Modifier, and removed the list (I thought I had done that already).
I will push after a successful test run. On 10/02/13 15:54, Joe Darcy wrote: > Hi Eric, > > Please revert the change to j.l.r.Modifer. The fix can be pushed with > just that modification; however, I strongly recommend also removing the > "here is everything that can go wrong" list from j.l.r.Executable. Core > reflection generally doesn't delve into such details in the main-line > API docs and calling the details out in the exception type should be > sufficient. > > Cheers, > > -Joe > > On 10/2/2013 11:55 AM, Eric McCorkle wrote: >> I've updated the test, switched to an in-memory class loader, and added >> a test case. Please review. >> >> On 10/01/13 16:27, Eric McCorkle wrote: >>> On 10/01/13 02:41, Joe Darcy wrote: >>> >>> (Suggested changes have been applied) >>> >>>> I think the test is acceptable as-is, but an RFE could be filed for >>>> some >>>> refactoring (having each bad class be represented as a diff from a base >>>> byte[], avoiding sending the bytes through the file system). >>>> >>> Better yet: I've filed an RFE for a framework for generating bad class >>> files. >>> >>> Webrev has been updated, please review: >>> http://cr.openjdk.java.net/~emc/8020981/ >>> >>>> Cheers, >>>> >>>> -Joe >>>> >>>> On 9/24/2013 12:15 PM, Eric McCorkle wrote: >>>>> Webrev updated to address these issues. >>>>> >>>>> On 09/24/13 07:51, Joel Borggren-Franck wrote: >>>>> >>>>>> 364 try { >>>>>> 365 tmp = getParameters0(); >>>>>> 366 } catch(IllegalArgumentException e) { >>>>>> 367 // Rethrow ClassFormatErrors >>>>>> 368 throw new MalformedParametersException("Invalid >>>>>> constant pool index"); >>>>>> 369 } >>>>>> >>>>>> What is causing the IAE? Have you considered having >>>>>> MalformedParametersExcepton taking a Throwable cause and having >>>>>> the IAE >>>>>> as cause? >>>>>> >>>>> The IAE comes from hotspot itself. There is a standard constant pool >>>>> index verifying function that throws IAE if given a bad index. >>>>> >>>>>> On 2013-09-19, Eric McCorkle wrote: >>>>>>> The webrev has been updated with Joe's comments addressed. >>>>>>> >>>>>>> On 09/19/13 00:11, David Holmes wrote: >>>>>>>> On 19/09/2013 9:59 AM, Eric McCorkle wrote: >>>>>>>>> This still needs to be reviewed. >>>>>>>> You seem to have ignored Joe's comments regarding the change to >>>>>>>> Modifier >>>>>>>> being incorrect. >>>>>>>> >>>>>>>> David >>>>>>>> >>>>>>>>> On 09/16/13 14:50, Eric McCorkle wrote: >>>>>>>>>> I pulled the class files into byte array constants, as a >>>>>>>>>> temporary >>>>>>>>>> measure until a viable method for testing bad class files is >>>>>>>>>> developed. >>>>>>>>>> >>>>>>>>>> The webrev has been refreshed. The class files will be taken out >>>>>>>>>> before >>>>>>>>>> I push. >>>>>>>>>> >>>>>>>>>> http://cr.openjdk.java.net/~emc/8020981/ >>>>>>>>>> >>>>>>>>>> On 09/13/13 20:48, Joe Darcy wrote: >>>>>>>>>>> To avoid storing binaries in Hg, you could try something like: >>>>>>>>>>> >>>>>>>>>>> * uuencode / ascii armor the class file >>>>>>>>>>> * convert to byte array in the test >>>>>>>>>>> * load classes from byte array >>>>>>>>>>> >>>>>>>>>>> -Joe >>>>>>>>>>> >>>>>>>>>>> On 09/13/2013 11:54 AM, Eric McCorkle wrote: >>>>>>>>>>>> I did it by hand with emacs. >>>>>>>>>>>> >>>>>>>>>>>> I would really rather tackle the bad class files for testing >>>>>>>>>>>> issue >>>>>>>>>>>> once >>>>>>>>>>>> and for all, the Right Way (tm). But with ZBB looming, now is >>>>>>>>>>>> not the >>>>>>>>>>>> time to do it. >>>>>>>>>>>> >>>>>>>>>>>> Hence, I have created this task >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024674 >>>>>>>>>>>> >>>>>>>>>>>> I also just created this one: >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024812 >>>>>>>>>>>> >>>>>>>>>>>> On 09/13/13 13:54, Peter Levart wrote: >>>>>>>>>>>>> Hi Eric, >>>>>>>>>>>>> >>>>>>>>>>>>> How did you create those class files? By hand using a HEX >>>>>>>>>>>>> editor? Did >>>>>>>>>>>>> you create a program that patched the original class file? >>>>>>>>>>>>> If the >>>>>>>>>>>>> later >>>>>>>>>>>>> is the case, you could pack that patching logic inside a >>>>>>>>>>>>> custom >>>>>>>>>>>>> ClassLoader... >>>>>>>>>>>>> >>>>>>>>>>>>> To hacky? Dependent on future changes of javac? At least >>>>>>>>>>>>> the "bad >>>>>>>>>>>>> name" >>>>>>>>>>>>> patching could be performed trivially and reliably, I think: >>>>>>>>>>>>> search and >>>>>>>>>>>>> replace with same-length string. >>>>>>>>>>>>> >>>>>>>>>>>>> Regards, Peter >>>>>>>>>>>>> >>>>>>>>>>>>> On 09/13/2013 07:35 PM, Eric McCorkle wrote: >>>>>>>>>>>>>> Ugh. Byte arrays of class file data is really a horrible >>>>>>>>>>>>>> solution. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I have already filed a task for test development post ZBB to >>>>>>>>>>>>>> develop a >>>>>>>>>>>>>> solution for generating bad class files. I'd prefer to >>>>>>>>>>>>>> file a >>>>>>>>>>>>>> follow-up >>>>>>>>>>>>>> to this to add the bad class file tests when that's done. >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 09/13/13 10:55, Joel Borggrén-Franck wrote: >>>>>>>>>>>>>>> I think the right thing to do is to include the original >>>>>>>>>>>>>>> compiling >>>>>>>>>>>>>>> source in a comment, together with a comment on how you >>>>>>>>>>>>>>> modify >>>>>>>>>>>>>>> them, and then the result as a byte array. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> IIRC I have seen test of that kind before somewhere in our >>>>>>>>>>>>>>> repo. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> cheers >>>>>>>>>>>>>>> /Joel >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Sep 13, 2013, at 4:49 PM, Eric McCorkle >>>>>>>>>>>>>>> <eric.mccor...@oracle.com> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> There is no simple means of generating bad class files for >>>>>>>>>>>>>>>> testing. >>>>>>>>>>>>>>>> This is a huge deficiency in our testing abilities. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> If these class files shouldn't go in, then I'm left with no >>>>>>>>>>>>>>>> choice >>>>>>>>>>>>>>>> but >>>>>>>>>>>>>>>> to check in no test for this patch. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> However, anyone can run the test I've provided with the >>>>>>>>>>>>>>>> class >>>>>>>>>>>>>>>> files and >>>>>>>>>>>>>>>> see that it works. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 09/13/13 09:55, Joel Borggrén-Franck wrote: >>>>>>>>>>>>>>>>> Hi Eric, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> IIRC we don't check in classfiles into the repo. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I'm not sure how we handle testing of broken class-files >>>>>>>>>>>>>>>>> in jdk, >>>>>>>>>>>>>>>>> but ASM might be an option, or storing the class file >>>>>>>>>>>>>>>>> as an >>>>>>>>>>>>>>>>> embedded byte array in the test. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> cheers >>>>>>>>>>>>>>>>> /Joel >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Sep 13, 2013, at 3:40 PM, Eric McCorkle >>>>>>>>>>>>>>>>> <eric.mccor...@oracle.com> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> A new webrev is posted (and crucible updated), which >>>>>>>>>>>>>>>>>> actually >>>>>>>>>>>>>>>>>> validates >>>>>>>>>>>>>>>>>> parameter names correctly. Apologies for the last one. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 09/12/13 16:02, Eric McCorkle wrote: >>>>>>>>>>>>>>>>>>> Hello, >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Please review this patch, which implements correct >>>>>>>>>>>>>>>>>>> behavior for >>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>> Parameter Reflection API in the case of malformed class >>>>>>>>>>>>>>>>>>> files. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> The bug report is here: >>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8020981 >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> The webrev is here: >>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8020981/ >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> This review is also on crucible. The ID is >>>>>>>>>>>>>>>>>>> CR-JDK8TL-182. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>>>>> Eric >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> <eric_mccorkle.vcf> >
