RFR 8062116: JVMTI GetClassMethods is Slow
David Holmes
david.holmes at oracle.com
Thu Nov 6 05:35:10 UTC 2014
On 6/11/2014 3:02 PM, Coleen Phillimore wrote:
>
> David and Serguei (and Jeremy), see below. Summary: I think Jeremy's
> code and comments are good.
>
> On 11/5/14, 10:11 PM, David Holmes wrote:
>> 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
>
> It's not adjusted for alignment:
Right - now I get it. Pointer difference is an algebraic subtraction
with "div sizeof what is pointed to". For aligned pointers there will be
no remainder and adding back the difference to the initial pointer will
yield the end pointer. But if one of the pointers is not aligned that is
not the case.
All rather icky.
Thanks,
David
----
> #include <cstddef>
>
> extern "C" int printf(const char *,...);
> class Method {
> int i ; int j; int k;
> };
>
> Method* array[10] = { new Method(),new Method(),new Method(),new
> Method(),new Method(),n
> ew Method(),new Method(),new Method(),new Method(),new Method() };
>
> void test(Method** m) {
> printf("m is 0x%p ", m);
> ptrdiff_t idx = m - array;
> if (array + idx == m) {
> printf("true %ld\n", idx);
> } else {
> printf("false %ld\n", idx);
> }
> }
> main() {
> Method** xx = &array[3];
> test(xx);
> test((Method**)(((char*)xx) - 2));
> }
>
> cphilli% a.out
> m is 0x0x601098 true 3
> m is 0x0x601096 false 2
>
>
> Coleen
>
>>
>> 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
>>>>
>>>
>
More information about the serviceability-dev
mailing list