Review request for 8058313: Mismatch of method descriptor and MethodParameters.parameters_count should cause MalformedParameterException
Eric McCorkle
eric.mccorkle at oracle.com
Mon Nov 10 19:03:22 UTC 2014
I ran the test list in tonga, with a patched and unpatched checkout. No
difference in failures.
On 11/07/14 15:26, Coleen Phillimore wrote:
>
> Eric,
>
> I reviewed this also. This is a bit confusing storing a 0 for
> method_parameters_length=0 in ConstMethod but I think it's fine with the
> comments.
>
> I have bad news though, you may need to make serviceability agent
> changes. You can tell if you run nsk.sajdi.testlist in ute.
>
> Coleen
>
> On 11/7/14, 3:03 PM, Eric McCorkle wrote:
>> Ah, ok, thanks.
>>
>> In that case, I need a capital-R reviewer to look at this, please?
>>
>> Thanks,
>> Eric
>>
>> On 11/07/14 14:49, Jiangli Zhou wrote:
>>> Eric,
>>>
>>> I'm not a "R"eviewer yet. You need at least two reviewers, within them
>>> at least one should be "R"eviewer.
>>>
>>> Thanks,
>>> Jiangli
>>>
>>> On 11/07/2014 11:40 AM, Eric McCorkle wrote:
>>>> Thanks,
>>>>
>>>> Are you a capital R? If not, wouldn't that mean I need two?
>>>>
>>>> On 11/07/14 13:33, Jiangli Zhou wrote:
>>>>> Hi Eric,
>>>>>
>>>>> Looks okay. You also need a capital R reviewer for the change.
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>> On 11/07/2014 05:07 AM, Eric McCorkle wrote:
>>>>>> On 11/06/14 18:53, Jiangli Zhou wrote:
>>>>>>
>>>>>>> Could you please point to the updated webrev? I don't see the
>>>>>>> update in
>>>>>>> http://cr.openjdk.java.net/~emc/8058313/webrev.01/src/share/vm/prims/jvm.cpp.sdiff.html.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> I made a mistake uploading it. It's here:
>>>>>> http://cr.openjdk.java.net/~emc/8058313/webrev.02/
>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>>> On 11/03/2014 01:35 PM, Eric McCorkle wrote:
>>>>>>>>>> Please review this issue so that it can go in along with 8058322.
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> On 10/30/14 19:40, Eric McCorkle wrote:
>>>>>>>>>>> Thank you for the pointers. I have applied your changes and
>>>>>>>>>>> refreshed
>>>>>>>>>>> the webrev.
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8058313/
>>>>>>>>>>>
>>>>>>>>>>> Also, I have posted the test for this and another patch here:
>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8062556/
>>>>>>>>>>>
>>>>>>>>>>> On 10/30/14 13:51, Jiangli Zhou wrote:
>>>>>>>>>>>> Hi Eric,
>>>>>>>>>>>>
>>>>>>>>>>>> On 10/30/2014 07:56 AM, Eric McCorkle wrote:
>>>>>>>>>>>>> On 10/29/14 20:36, Jiangli Zhou wrote:
>>>>>>>>>>>>>> Hi Eric,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I wonder if we could specialize this particular case and
>>>>>>>>>>>>>> avoid
>>>>>>>>>>>>>> changing
>>>>>>>>>>>>>> the parsing code. How about setting the
>>>>>>>>>>>>>> _has_method_parameters
>>>>>>>>>>>>>> flag in
>>>>>>>>>>>>>> the ConstMethod when encounter such MethodParameter, and
>>>>>>>>>>>>>> changing
>>>>>>>>>>>>>> JVM_GetMethodParameters() to return non-NULL value for such
>>>>>>>>>>>>>> case
>>>>>>>>>>>>>> when
>>>>>>>>>>>>>> _has_method_parameters is true but method_parameters_length
>>>>>>>>>>>>>> is 0.
>>>>>>>>>>>>>> Would
>>>>>>>>>>>>>> that work?
>>>>>>>>>>>>> Which parser are you talking about? The inline tables
>>>>>>>>>>>>> parser, or
>>>>>>>>>>>>> the
>>>>>>>>>>>>> class file parser. The class file parser has to change,
>>>>>>>>>>>>> because it
>>>>>>>>>>>>> was
>>>>>>>>>>>>> previously ignoring MethodParameters attributes with
>>>>>>>>>>>>> parameter_count 0.
>>>>>>>>>>>> It's the class parsing changes that I was referring to, mostly
>>>>>>>>>>>> relate to
>>>>>>>>>>>> the initialization and checking against
>>>>>>>>>>>> method_parameters_length.
>>>>>>>>>>>> It's a
>>>>>>>>>>>> bit awkward to include the 0 case but also skipping it in the
>>>>>>>>>>>> loop. For
>>>>>>>>>>>> example, the following code in classFileParser.cpp changed
>>>>>>>>>>>> ">" to
>>>>>>>>>>>> ">="
>>>>>>>>>>>> in the if check, but has no real effect and is not need.
>>>>>>>>>>>>
>>>>>>>>>>>> 2486 // Copy method parameters
>>>>>>>>>>>> 2487 if (method_parameters_length >= 0) {
>>>>>>>>>>>> 2488 MethodParametersElement* elem =
>>>>>>>>>>>> m->constMethod()->method_parameters_start();
>>>>>>>>>>>> 2489 for (int i = 0; i < method_parameters_length; i++) {
>>>>>>>>>>>> 2490 elem[i].name_cp_index =
>>>>>>>>>>>> Bytes::get_Java_u2(method_parameters_data);
>>>>>>>>>>>> 2491 method_parameters_data += 2;
>>>>>>>>>>>> 2492 elem[i].flags =
>>>>>>>>>>>> Bytes::get_Java_u2(method_parameters_data);
>>>>>>>>>>>> 2493 method_parameters_data += 2;
>>>>>>>>>>>> 2494 }
>>>>>>>>>>>> 2495 }
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> I don't think your proposal will work. The inline tables'
>>>>>>>>>>>>> offsets are
>>>>>>>>>>>>> all dependent on what inline tables are actually present. If
>>>>>>>>>>>>> _has_method_parameters is set, then the inline tables code
>>>>>>>>>>>>> expects the
>>>>>>>>>>>>> last u2 of the inline tables to be a u2 indicating the
>>>>>>>>>>>>> number of
>>>>>>>>>>>>> method
>>>>>>>>>>>>> parameters entries, preceeded by the array of method
>>>>>>>>>>>>> parameters
>>>>>>>>>>>>> data.
>>>>>>>>>>>>> If _has_method_parameters is false, then it expects that there
>>>>>>>>>>>>> is no
>>>>>>>>>>>>> method parameters information at all (including no length
>>>>>>>>>>>>> field). If
>>>>>>>>>>>>> you were to set _has_method_parameters, but not store any
>>>>>>>>>>>>> information in
>>>>>>>>>>>>> the inline table, then it would cause errors for all the rest
>>>>>>>>>>>>> of the
>>>>>>>>>>>>> inline tables.
>>>>>>>>>>>> Thank you for reminding me of the complexity of the inlined
>>>>>>>>>>>> table
>>>>>>>>>>>> calculation in the ConstMethod. My proposal would require
>>>>>>>>>>>> tweaks in
>>>>>>>>>>>> that
>>>>>>>>>>>> area to correctly compute the table sizes. As it's easy to
>>>>>>>>>>>> introduce
>>>>>>>>>>>> bugs in that area, it's not worth to change the table
>>>>>>>>>>>> calculation
>>>>>>>>>>>> code
>>>>>>>>>>>> for this purpose. I agree my proposal is not a better choice in
>>>>>>>>>>>> this
>>>>>>>>>>>> case.
>>>>>>>>>>>>
>>>>>>>>>>>>> What I do for the parameter_count = 0 case is just
>>>>>>>>>>>>> store
>>>>>>>>>>>>> a 0 u2 for zero-length method parameters information, and no
>>>>>>>>>>>>> data.
>>>>>>>>>>>>> All
>>>>>>>>>>>>> the existing inline tables code works fine with this case, so
>>>>>>>>>>>>> there
>>>>>>>>>>>>> aren't any serious changes to the inline tables code (other
>>>>>>>>>>>>> than
>>>>>>>>>>>>> allowing method parameters information to be stored when the
>>>>>>>>>>>>> array is
>>>>>>>>>>>>> length 0). But you have to make some change to the inline
>>>>>>>>>>>>> table
>>>>>>>>>>>>> code,
>>>>>>>>>>>>> otherwise the information won't be stored.
>>>>>>>>>>>> Ok. Could you please add comments to the change in
>>>>>>>>>>>> constMethod.cpp to
>>>>>>>>>>>> explain above?
>>>>>>>>>>>>
>>>>>>>>>>>> In jvm.cpp, since -1 represents no method parameter now. Maybe
>>>>>>>>>>>> checking
>>>>>>>>>>>> against explicity and add comments for the 0-length case.
>>>>>>>>>>>>
>>>>>>>>>>>> JVM_ENTRY(jobjectArray, JVM_GetMethodParameters(JNIEnv *env,
>>>>>>>>>>>> jobject
>>>>>>>>>>>> method))
>>>>>>>>>>>> {
>>>>>>>>>>>> ...
>>>>>>>>>>>> // No method parameter
>>>>>>>>>>>> if (num_params == -1) {
>>>>>>>>>>>> return (jobjectArray)NULL;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> /* handle the rest here */
>>>>>>>>>>>> // make sure all the symbols are properly formatted
>>>>>>>>>>>> for (int i = 0; i < num_params; i++) {
>>>>>>>>>>>> ...
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Jiangli
>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Jiangli
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 10/29/2014 03:39 PM, Eric McCorkle wrote:
>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please review this fix for parameter reflection which
>>>>>>>>>>>>>>> addresses
>>>>>>>>>>>>>>> hotspot
>>>>>>>>>>>>>>> falsely ignoring zero-length MethodParameter attributes.
>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>> JVMS
>>>>>>>>>>>>>>> allows a MethodParameters attribute with parameter_count
>>>>>>>>>>>>>>> = 0,
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> parameter reflection spec states that a
>>>>>>>>>>>>>>> MalformedParametersException
>>>>>>>>>>>>>>> should be thrown if parameter_count does not match the
>>>>>>>>>>>>>>> number of
>>>>>>>>>>>>>>> real
>>>>>>>>>>>>>>> parameters to a method. Hotspot currently ignores
>>>>>>>>>>>>>>> MethodParameters
>>>>>>>>>>>>>>> attributes with parameter_count = 0; however, in a case
>>>>>>>>>>>>>>> where a
>>>>>>>>>>>>>>> (bad)
>>>>>>>>>>>>>>> MethodParameters attribute has parameter_count = 0, but the
>>>>>>>>>>>>>>> method
>>>>>>>>>>>>>>> has a
>>>>>>>>>>>>>>> nonzero number of real parameters, hotspot will return null
>>>>>>>>>>>>>>> from
>>>>>>>>>>>>>>> JVM_GetMethodParameters, the result being that a
>>>>>>>>>>>>>>> MalformedParametersException is not thrown (rather, the
>>>>>>>>>>>>>>> reflection API
>>>>>>>>>>>>>>> acts like there is no MethodParameters attribute).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This patch causes hotspot to record the fact that a
>>>>>>>>>>>>>>> zero-length
>>>>>>>>>>>>>>> MethodParameters attribute does exist, causing the exception
>>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>>> thrown when it should be.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The bug is here:
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8058313
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The webrev is here:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8058313/
>
More information about the hotspot-dev
mailing list