Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
The enhanced metadata spec says nothing at all about an IAE. That is an implementation detail, possibly subject to change at any point, and it *should* not be leaked. On 09/24/13 17:28, Paul Benedict wrote: Eric, Should MalformedParametersException save IAE as the root cause? Or is that an internal detail you don't want leaked? 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. Cheers, Paul
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
Updated webrev here: http://cr.openjdk.java.net/~emc/8020981/ Are there any more comments, or is this good to go? On 09/19/13 18:15, 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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
Hi Eric, Some feedback: Executable.java: 299 * (i) The number of parameters (parameter_count) is wrong for the method What is wrong in this case? Do you mean inconsistent with the signature? 302 * (iv) A parameter's name is , or contains an illegal character [0] What does [0] mean in this case? A placeholder for an in-mail reference or a null byte in a modified utf8 array? 299 * (i) The number of parameters (parameter_count) is wrong for the method 300 * (ii) A constant pool index is out of bounds. 301 * (iii) A constant pool index does not refer to a UTF-8 entry 302 * (iv) A parameter's name is , or contains an illegal character [0] 303 * (v) The flags field contains an illegal flag (something other than 304 * FINAL, SYNTHETIC, or MANDATED) The new markup looks odd. I think you should use ul or ol to be consistent j.l.Class. See Class.getMethods() for an example. 306 * @throws MalformedParametersException if the class file contains 307 * a MethodParameters attribute that is improperly formatted. 308 * @return an array of {@code Parameter} objects representing all 309 * the parameters to the executable this object represents @throws ends with a period, @return does not. I'm not sure what the convention is but I think you want to be consistent. 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? MalformedParametersException.java: 42 /** 43 * Use serialVersionUID from JDK 1.1.X for interoperability 44 */ 45 private static final long serialVersionUID = 20130919L; Was this type present in 1.1.x? This comment makes no sense to me. Please explain. BadClassFiles.java: Thanks for encoding the class-files. Please include the original source above the bytes. cheers /Joel 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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
Eric, Should MalformedParametersException save IAE as the root cause? Or is that an internal detail you don't want leaked? 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. Cheers, Paul
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
This still needs to be reviewed. 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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
MalformedParametersException should receive a @since tag. Additionally, the javadoc doesn't describe what it means for a parameter to be malformed. The answer doesn't need to be exhaustive, but I think some examples would help developers if they catch one and need to dig into class files. Or if there's a reference point to the JLS, that would be good too. Here's a good sample from Eric: http://mail.openjdk.java.net/pipermail/enhanced-metadata-spec-discuss/2013-August/000242.html PS: I think the consistent exception hierarchy for reflective operations should remain in tact. Although it's annoying checked exceptions exist, my preference would be to extend from ReflectiveOperationException rather than RuntimeException. -- Cheers, Paul
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
On 09/13/13 09:53, Paul Benedict wrote: MalformedParametersException should receive a @since tag. Additionally, the javadoc doesn't describe what it means for a parameter to be malformed. The answer doesn't need to be exhaustive, but I think some examples would help developers if they catch one and need to dig into class files. Or if there's a reference point to the JLS, that would be good too. Here's a good sample from Eric: http://mail.openjdk.java.net/pipermail/enhanced-metadata-spec-discuss/2013-August/000242.html I updated the javadoc comments with a list taken from my earlier email you referenced, and I added more comments to MalformedParametersException. PS: I think the consistent exception hierarchy for reflective operations should remain in tact. Although it's annoying checked exceptions exist, my preference would be to extend from ReflectiveOperationException rather than RuntimeException. If you have spec issues, please take them up on enhanced-metadata-discuss. I am not the spec owner.
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
On 09/12/2013 01:02 PM, 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 Hi Eric, The update to j.l.reflect.Modifiers is incorrect; the field in question is only supposed to encode *source* modifiers. The new exception types needs to declare a serialVersionUID field since it is a serializable type (thanks RMI for making all Throwables serializable!). -Joe
Re: JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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
JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions
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