JDK-8020981: Update methods of java.lang.reflect.Parameter to throw correct exceptions

Tue Oct 1 20:27:58 UTC 2013

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.mccorkle at 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.mccorkle at 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>
> 