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

[David Holmes]

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.

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
>>>>
>>>
>>
>>
>