RFR 8146984: SIGBUS: bool Method::has_method_vptr(const void*)+0xc

[Coleen Phillimore]

On 2/3/16 10:33 AM, Jesper Wilhelmsson wrote:
> Den 3/2/16 kl. 16:14, skrev Daniel D. Daugherty:
>> On 2/3/16 8:10 AM, Markus Gronlund wrote:
>>> Hi Coleen,
>>>
>>> Thanks for looking into this. Maybe it could be simplified like:
>>
>> Very nicely done...
>>
>>>
>>> bool Method::has_method_vptr(const void* ptr) {
>>>    // Use SafeFetch to check if this is a valid pointer first
>>>    // This assumes that the vtbl pointer is the first word of a C++ 
>>> object.
>>>    // This assumption is also in universe.cpp patch_klass_vtable
>>>    if (1 == SafeFetchN((intptr_t*)ptr, intptr_t(1))) {
>>>      return false;
>>>    }
>>>    const Method m;
>>>    return dereference_vptr(&m) == ptr;
>>> }
>>>
>>> // Check that this pointer is valid by checking that the vtbl 
>>> pointer matches
>>> bool Method::is_valid_method() const {
>>>    if (this == NULL) {
>>
>> In keeping with our latest style migration:
>>
>>      if (NULL == this) {
>>
>> Yes, I know I have said I don't like it... but we're
>> going that way and it's starting to grow on me... :-)
>
> Don't give in Dan!
> I think this truly awful and everyone I've asked around here dislikes 
> this style. Is there a reason to turn things around like this?
> (Compilers from the Jurassic age cant detect "if (this = NULL)" is not 
> a valid argument.)

Oh good, I agree, I find this style visually distracting.  I won't give in.

Coleen

> /Jesper
>
>
>>
>> Dan
>>
>>
>>>      return false;
>>>    }
>>>    // Quick sanity check on pointer.
>>>    if ((intptr_t(this) & (wordSize-1)) != 0) {
>>
>> Spaces around the '-' please...
>>
>> Dan
>>
>>>      return false;
>>>    }
>>>
>>>    return has_method_vptr(this);
>>> }
>>>
>>> I think you also need:
>>>
>>> #include "runtime/stubRoutines.hpp
>>>
>>> Can't really comment on the use of SafeFetchN() as I am not familiar 
>>> with it?
>>> Do all platforms support this?
>>>
>>> Looks like a good improvement for stability.
>>>
>>> No need to see any updated reviews.
>>>
>>> Thanks
>>> Markus
>>>
>>>
>>> -----Original Message-----
>>> From: Coleen Phillimore
>>> Sent: den 2 februari 2016 18:54
>>> To: hotspot-dev at openjdk.java.net; Lindenmaier, Goetz
>>> Subject: Re: RFR 8146984: SIGBUS: bool Method::has_method_vptr(const 
>>> void*)+0xc
>>>
>>>
>>> Goetz, Can you review this since it's using SafeFetchN?
>>>
>>> thanks,
>>> Coleen
>>>
>>> On 2/1/16 1:04 PM, Coleen Phillimore wrote:
>>>>
>>>> Thanks Dan!
>>>>
>>>> On 2/1/16 12:21 PM, Daniel D. Daugherty wrote:
>>>>> On 1/30/16 7:39 AM, Coleen Phillimore wrote:
>>>>>> I've moved the SafeFetch to has_method_vptr as suggested and 
>>>>>> retested.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coleenp/8146984.02/webrev/
>>>>> src/share/vm/oops/method.cpp
>>>>>      (old) L2114:     return has_method_vptr((const void*)this);
>>>>>      (new) L2120:       return has_method_vptr(this);
>>>>>          Just curious. I don't see anything that explains why the
>>>>>          cast is no longer needed (no type changes). Was this
>>>>>          simply cleaning up an unnecessary cast?
>>>> The cast is unnecessary.  I didn't add it back when I added the call
>>>> to has_method_vptr back.
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>>> Thumbs up.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 1/29/16 11:58 AM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> On 1/29/16 11:02 AM, Daniel D. Daugherty wrote:
>>>>>>>> On 1/29/16 5:46 AM, Coleen Phillimore wrote:
>>>>>>>>> Summary: Add address check and use SafeFetchN for Method* vptr
>>>>>>>>> access when Method* may be bad pointer.
>>>>>>>>>
>>>>>>>>> Tested with RBT and failing test case (reproduced 1 in 100 times)
>>>>>>>>> with fatal in the 'return's in the change to verify.
>>>>>>>>>
>>>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8146984/
>>>>>>>> This one caught my eye because it has to do with sampling...
>>>>>>> I should mention sampling in all my RFRs then!
>>>>>>>> src/share/vm/oops/method.cpp
>>>>>>>>      The old code checked "!is_metaspace_object()" and used
>>>>>>>>      has_method_vptr((const void*)this).
>>>>>>>>
>>>>>>>>      The new code skips the "!is_metaspace_object()" check even
>>>>>>>> after sanity
>>>>>>>>      checking the pointer, but you don't really explain why 
>>>>>>>> that's OK.
>>>>>>> is_metaspace_object is a very expensive check.   It has to traverse
>>>>>>> all the metaspace mmap chunks.   The new code is more robust in
>>>>>>> that it sanity checks the pointer first but uses Safefetch to get
>>>>>>> the vptr.
>>>>>>>
>>>>>>>
>>>>>>>>      The new code also picks up parts of Method::has_method_vptr()
>>>>>>>> which
>>>>>>>>      makes me wonder if that's the right place for the fix. Won't
>>>>>>>> other
>>>>>>>>      callers to Method::has_method_vptr() be subject to the same
>>>>>>>> crashing
>>>>>>>>      mode? Or was the crashing mode only due to the
>>>>>>>> "!is_metaspace_object()"
>>>>>>>>      check...
>>>>>>> I should have moved the SafeFetch in to the has_method_vptr. I
>>>>>>> can't remember why I copied it now.  It crashed because the pointer
>>>>>>> was in metaspace (is_metaspace_object returned true) but wasn't
>>>>>>> aligned, but the pointer could come from anywhere.
>>>>>>>
>>>>>>> Thanks,   I'll test out this fix and resend it.
>>>>>>> Coleen
>>>>>>>
>>>>>>>> Dan
>>>>>>>>
>>>>>>>>
>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8146984
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen
>>>>>>>>>
>>