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

Wed Oct 2 19:54:12 UTC 2013

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