On 11/5/14 3:40 PM, Coleen Phillimore wrote:
On 11/5/14, 6:13 PM, Jeremy Manson wrote:
On Tue, Nov 4, 2014 at 8:56 PM, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com
<mailto:serguei.spit...@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?
It is because the condition *(b->_methods + idx == m)* is always
true. :)
Also, should we check the condition: **m != _free_method*** ?
What about the following ?:
* return (****m != _free_method***);*
I don't mind adding this, if Coleen thinks those are the semantics
this needs. It wasn't there before, of course.
The semantics weren't there before and the way this is called has
already checked that *m != _free_method. Would it be an improvement?
I don't think so. It seems that contains() should just check that the
Method** is contained in the methodID table. To be more correct,
is_method_id should check that it's not a freed methodID but the
caller verifies this already. So I don't think this should change.
Agreed.
Thank you for the explanation!
BTW, I've run the test sets suggested by Serguei and they all passed.
Nice!
Thanks,
Serguei
Coleen
Jeremy
