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> >
