On 11/5/14, 6:13 PM, Jeremy Manson wrote:
On Tue, Nov 4, 2014 at 8:56 PM, [email protected] <mailto:[email protected]> <[email protected] <mailto:[email protected]>> 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.
BTW, I've run the test sets suggested by Serguei and they all passed. Coleen
Jeremy
