Request for review (S) 7012088: jump to 0 address because of lack of memory ordering in SignatureHandler::add

[Coleen Phillimore]

On 2/1/2011 7:41 PM, David Holmes wrote:
> Coleen Phillimore said the following on 02/02/11 05:48:
>> On 2/1/2011 2:42 PM, Dmitry Samersoff wrote:
>>> Coleen,
>>>
>>> Should we NULL-check _handlers in the code also, before accessing it?
>> No, because we create and initialize it just before the code.  If 
>> 'new' comes back as null, it'll vm_exit_out_of_memory().
>>
>> see:  http://cr.openjdk.java.net/~coleenp/7012088_2/
>
> Here:
>
> 1195 #ifdef ASSERT
> 1196   int handler_index = 0;
> 1197   int fingerprint_index = 0;
> 1198   {
> 1199     // '_handlers' and '_fingerprints' are 'GrowableArray's and 
> are NOT synchronized
> 1200     // in any way if accessed from multiple threads. To avoid 
> races with another
> 1201     // thread which may change the arrays in the above, mutex 
> protected block, we
> 1202     // have to protect this read access here with the same mutex 
> as well!
> 1203     MutexLocker mu(SignatureHandlerLibrary_lock);
> 1204     if (_handlers != NULL) {
> 1205       handler_index = _handlers->find(method->signature_handler());
> 1206       fingerprint_index = 
> _fingerprints->find(Fingerprinter(method).fingerprint());
> 1207     }
> 1208   }
> 1209   assert(method->signature_handler() == 
> Interpreter::slow_signature_handler() ||
> 1210          handler_index == fingerprint_index, "sanity check");
> 1211 #endif // ASSERT
>
> should we set handler_index and fingerprint_index to different initial 
> values (-1 and -2 ?) so that the assert doesn't accidentally pass? I 
> assume that if _handlers is null then we should have invalid indices 
> but the first clause of the assert should pass.

Yeah, that makes sense.

Thanks,
Coleen
>
> David
>
>> Thanks,
>> Coleen
>>>
>>> -Dmitry
>>>
>>>
>>> On 2011-02-01 21:20, Coleen Phillimore wrote:
>>>>
>>>> Dmitry,   You are absolutely right.  We should check _handlers 
>>>> under the
>>>> lock too.  It could be created in a separate thread.
>>>>
>>>> Thanks!
>>>> Coleen
>>>>
>>>> On 2/1/2011 1:14 PM, Dmitry Samersoff wrote:
>>>>> Coleen,
>>>>>
>>>>> assert code is not clean for me:
>>>>>
>>>>> if (_handlers != NULL){
>>>>> }
>>>>>
>>>>> _handlers never checked for NULL before e.g. during printing we
>>>>> use _handlers->length() etc.
>>>>>
>>>>> if we are fighting against race I guess we should take a lock before
>>>>> null check.
>>>>>
>>>>> i.e.
>>>>>
>>>>> MutexLocker mu(SignatureHandlerLibrary_lock);
>>>>> if (_handlers != NULL) {
>>>>> ....
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2011-02-01 19:48, Coleen Phillimore wrote:
>>>>>> Summary: Write method signature handler under lock to prevent 
>>>>>> race with
>>>>>> growable array resizing
>>>>>>
>>>>>> Also includes a fix to 6704010 which was broken with
>>>>>> -XX:-UseFastSignatureHandlers
>>>>>>
>>>>>> This change was submitted and tested by the customer.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/7012088/
>>>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7012088
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>
>>>>
>>>
>>>
>>