RFR 8062116: JVMTI GetClassMethods is Slow

[serguei.spitsyn at oracle.com]

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?


Thanks,
Serguei

**
>
> Jeremy

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