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

Tue Oct 1 06:41:07 UTC 2013

Hello,

I've looked over the 05 version of the webrev.

The code on line 494 in modifier is incorrect:

  484     /**
  485      * Return an {@code int} value OR-ing together the source language
  486      * modifiers that can be applied to a parameter.
  487      * @return an {@code int} value OR-ing together the source 
language
  488      * modifiers that can be applied to a parameter.
  489      *
  490      * @jls 8.4.1 Formal Parameters
  491      * @since 1.8
  492      */
  493     public static int parameterModifiers() {
  494         return Modifier.FINAL | Modifier.SYNTHETIC | 
Modifier.MANDATED;
  495     }

Synthetic and mandated are *not* source language modifiers. The bit mask 
you want will have to be constructed based on parameterModifiers rather 
than solely as parameterModifiers.

It would be more on topic for the list of malformed conditions in 
Executable:

  294      * <p>This implementation will throw a {@code
  295      * MalformedParametersException} if the MethodParameters attribute
  296      * describing the parameters contains invalid data.  The following
  297      * is a list of conditions under which this can occur:
  298      * <ul>
  299      * <li> The number of parameters (parameter_count) is wrong 
for the method
  300      * <li> A constant pool index is out of bounds.
  301      * <li> A constant pool index does not refer to a UTF-8 entry
  302      * <li> A parameter's name is "", or contains an illegal character
  303      * <li> The flags field contains an illegal flag (something 
other than
  304      *     FINAL, SYNTHETIC, or MANDATED)
  305      * </ul>

to instead be detailed in MalformedParametersException.java.

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

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>