RFR 8062116: JVMTI GetClassMethods is Slow

[David Holmes]

On 6/11/2014 1:00 PM, Coleen Phillimore wrote:
>
> 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

I was confused about this too. What we have here is pointer arithmetic, 
not regular arithmetic, so I'm assuming an unaligned value has to be 
adjusted before the actual difference is computed. So in practice:

m - b->_methods

is really

adjusted_for_alignment(m) - b->_methods

David
-----

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