RFR 8062116: JVMTI GetClassMethods is Slow

[Coleen Phillimore]

On 11/5/14, 8:11 PM, serguei.spitsyn at oracle.com wrote:
>
> On 11/5/14 4:35 PM, Jeremy Manson wrote:
>>
>>
>> On Wed, Nov 5, 2014 at 3:40 PM, Coleen Phillimore 
>> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>> 
>> wrote:
>>
>>
>>     On 11/5/14, 6:13 PM, Jeremy Manson wrote:
>>>
>>>
>>>     On Tue, Nov 4, 2014 at 8:56 PM, serguei.spitsyn at oracle.com
>>>     <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
>>>     <mailto:serguei.spitsyn at oracle.com>> wrote:
>>>
>>>         The fix looks good in general.
>>>
>>>         src/share/vm/oops/method.cpp
>>>
>>>         1785   bool contains(Method** m) {
>>>         1786     if (m == NULL) return false;
>>>         1787     for (JNIMethodBlockNode* b = &_head; b != NULL; b = b->_next) {
>>>         1788       if (b->_methods <= m && m < b->_methods + b->_number_of_methods) {
>>>         *1789         ptrdiff_t idx = m - b->_methods;**
>>>         **1790         if (b->_methods + idx == m) {**
>>>         1791           return true;
>>>         1792         }*
>>>         1793       }
>>>         1794     }
>>>         1795     return false;  // not found
>>>         1796   }
>>>
>>>
>>>         Just noticed that the lines 1789-1792 can be replaced with
>>>         one liner:
>>>         *        return true;*
>>>
>>>
>>>     Ah, you have found our crappy workaround for wild pointers to
>>>     non-aligned places in the middle of _methods.
>>
>>     Can you explain this?  Why are there wild pointers?
>>
>>
>> My belief was that end user code could pass any old garbage to this 
>> function.  It's called by Method::is_method_id, which is called 
>> by jniCheck::validate_jmethod_id.  The idea was that this would help 
>> check jni deliver useful information in the case of the end user 
>> inputting garbage that happened to be in the right memory range.
>>
>> Having said that, at a second glance, it looks as if it that call is 
>> protected by a call to is_method() (in checked_resolve_jmethod_id), 
>> so the program will probably crash before it gets to this check.
>>
>> The other point about it was that the result of >= and < is 
>> technically unspecified; if it were ever implemented as anything 
>> other than a binary comparison between integers (which it never is, 
>> now that no one has a segmented architecture), the comparison could 
>> pass spuriously, so checking would be a good thing.  Of course, the 
>> comparison could fail spuriously, too.
>>
>> Anyway, I'm happy to leave it in as belt-and-suspenders (and add a 
>> comment, obviously, since it has caused confusion), or take it out.  
>> Your call.
>
> I'm still confused.
>
> How this code could possibly check anything?
>    ptrdiff_t idx = m - b->_methods;
>    if (b->_methods + idx == m) {
>
> The condition above always gives true:
>    b->_methods + (idx) == b->_methods + (m - b->_methods) == 
> (b->_methods- b->_methods) + m == (0 + m) == m
>
> Even if m was unaligned then at the end we compare m with m which is 
> still true.
> Do I miss anything?

If 'm' is unaligned we would fail this comparison:

(gdb) print &methods->_data[2]
$34 = (Method **) 0x7fffe0022440
(gdb) print &methods->_data[0]
$35 = (Method **) 0x7fffe0022430
(gdb) print 0x7fffe0022444 - 0x7fffe0022430
$32 = 20
(gdb) print 20/8
$33 = 2

if m is misaligned 0x7fffe0022444 the idx would be 2 and the expression 
(b->_methods + idx) would evaluate to the aligned 0xfffe0022440  so not 
equal m.

But the code could check for misaligned m instead (or it would have 
already crashed).  I think all bets are off if the address space is 
segmented.

The comment Jeremy added is:

       if (b->_methods <= m && m < b->_methods + b->_number_of_methods) {
         // This is a bit of extra checking, for two reasons.  One is
         // that contains() deals with pointers that are passed in by
         // JNI code, so making sure that the pointer is aligned
         // correctly is valuable.  The other is that <= and > are
         // technically not defined on pointers, so the if guard can
         // pass spuriously; no modern compiler is likely to make that
         // a problem, though (and if one did, the guard could also
         // fail spuriously, which would be bad).
         ptrdiff_t idx = m - b->_methods;
         if (b->_methods + idx == m) {
           return true;
         }

Coleen
>
>
> Thanks,
> Serguei
>
> **
>>
>> Jeremy
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141105/3a1bf0b9/attachment-0001.html>