RFR (S): 8026478: -XX:+VerifyAdapterSharing is broken

[Albert Noll]

Hi Chris,

thanks for looking at this again. I moved the !VerifyAdapterCalls as you 
proposed.
That seems more logical to me as well.

I also updated the comment that describes the contains_all_checks variable.
If you do not understand it, it is not well decribed, but it should be. 
Please let
me know if it is clearer now. If not, I'll update the comment.

*+      // StubRoutines::code2() is initialized after this function can be called. As a result,*
*+     // VerifyAdapterCalls and VerifyAdapterSharing can fail if we re-use code that generated*
*+     // prior to StubRoutines::code2() being set. Checks refer to checks generated in an I2C*
*+     // stub that ensure that an I2C stub is called from an interpreter frame.*


Here is the updated webrev:
http://cr.openjdk.java.net/~anoll/8026478/webrev.05/ 
<http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.05/>

Best,
Albert

On 24.10.2013 18:24, Christian Thalinger wrote:
> *src/share/vm/runtime/sharedRuntime.cpp:*
>   *+     bool contains_all_checks = (StubRoutines::code2() != NULL) || !VerifyAdapterCalls;*
> Could you move the *!VerifyAdapterCalls *check to here:
> *+     if (contains_all_checks) {*
> And I still don’t know what “checks” means in this context.  Anyway, 
> this looks good.
>
> On Oct 24, 2013, at 5:30 AM, Albert Noll <albert.noll at oracle.com 
> <mailto:albert.noll at oracle.com>> wrote:
>
>> Hi John,
>>
>> thank you for looking at this. You are right, the changes to 
>> free_entry are not correct when -XX:VerifyAdapterSharing is
>> specified.
>>
>> An alternative way to solve the problem is to add entrys only if the 
>> entry contains all checks and
>> -XX:+VerifyAdapterCalls is specified. With this version, we also do 
>> not need the _contains_all_checks
>> field in AdapterHandlerEntry.
>>
>> What do you think about this version?
>> http://cr.openjdk.java.net/~anoll/8026478/webrev.04/ 
>> <http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.04/>
>> Best,
>> Albert
>>
>>
>> On 23.10.2013 18:59, John Rose wrote:
>>> I have a question:  The amended free_entry function indirectly 
>>> unlinks the selected 'entry', but may also unlink other entries with 
>>> the same hash and fingerprint.  Isn't this working against the use 
>>> for free_entry, where a pre-existing shared_entry is to be favored 
>>> and returned?  Won't the shared_entry be unlinked also?  That would 
>>> seem to lead to a storage leak.
>>>
>>> (Minor style nit:  You code the hash comparison and key comparison 
>>> as nested ifs; they would be just as readable connected by an '&&', 
>>> and with less nesting.)
>>>
>>> — John
>>>
>>> On Oct 23, 2013, at 6:49 AM, Albert Noll <albert.noll at oracle.com 
>>> <mailto:albert.noll at oracle.com>> wrote:
>>>
>>>> Hi,
>>>>
>>>> unfortunately, the patch was not correct, as revealed by jprt.
>>>> The problem was that entries into the hashmap are not removed
>>>> correctly.
>>>>
>>>> This version fixes the problem. The patch is currently in jprt.
>>>> So far, everything looks good.
>>>>
>>>> Many thanks for another round. Here is the updated webrev:
>>>> http://cr.openjdk.java.net/~anoll/8026478/webrev.03/ 
>>>> <http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.03/>
>>>>
>>>> Best,
>>>> Albert
>>>>
>>>>
>>>> On 16.10.2013 19:43, Christian Thalinger wrote:
>>>>> It would be nice to have a comment on this guy:
>>>>> *+   bool    _contains_all_checks;*
>>>>> No need for another webrev if you decide to add the comment.
>>>>>
>>>>> Looks good.
>>>>>
>>>>> On Oct 15, 2013, at 11:02 PM, Albert Noll <albert.noll at oracle.com 
>>>>> <mailto:albert.noll at oracle.com>> wrote:
>>>>>
>>>>>> Forgot to adjust the comment. This version should be fine.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~anoll/8026478/webrev.02/ 
>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.02/>
>>>>>>
>>>>>> Albert
>>>>>>
>>>>>> On 15.10.2013 21:03, Vladimir Kozlov wrote:
>>>>>>> Albert,
>>>>>>>
>>>>>>> contains_all_stubs name is incorrect. It should be 
>>>>>>> contains_all_checks. Adapter code does not have stubs in it.
>>>>>>>
>>>>>>> I think the next check should be:
>>>>>>>
>>>>>>> bool contains_all_checks = (StubRoutines::code2() != NULL) || 
>>>>>>> !VerifyAdapterCalls;
>>>>>>>
>>>>>>> because code does not change when VerifyAdapterCalls is false.
>>>>>>> Then you don't need to check VerifyAdapterCalls in next condition:
>>>>>>>
>>>>>>> +       if ((VerifyAdapterCalls || VerifyAdapterSharing) && 
>>>>>>> !entry->contains_all_checks()) {
>>>>>>>
>>>>>>> And we need to regenerate adapter regardless 
>>>>>>> VerifyAdapterSharing because, as you said, VerifyAdapterCalls 
>>>>>>> potentially reports a false error if check for code2 is not 
>>>>>>> generated.
>>>>>>>
>>>>>>> So the condition should be simple "if 
>>>>>>> (!entry->contains_all_checks())".
>>>>>>>
>>>>>>> Could you leave next lines unchanged (use 2 lines for arguments)?:
>>>>>>> ! buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
>>>>>>>
>>>>>>> sizeof(buffer_locs)/sizeof(relocInfo));
>>>>>>>         MacroAssembler _masm(&buffer);
>>>>>>>
>>>>>>> Cleanup is fine.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 10/15/13 4:14 AM, Albert Noll wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> could I have reviews for this small patch?
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8026478
>>>>>>>> webrev: http://cr.openjdk.java.net/~anoll/8026478/webrev.00/
>>>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.00/>
>>>>>>>>
>>>>>>>> Problem: This flag is broken. The reason is that adapters
>>>>>>>> (gen_i2c_adapter()) that are generated during VM startup do not 
>>>>>>>> include
>>>>>>>> the range for StubRoutines::code2() if -XX:+VerifyAdapterCalls is
>>>>>>>> specified. StubRoutines::code2() is only allocated later during 
>>>>>>>> startup;
>>>>>>>> consequently, code2() is NULL. As a result, the size of the 
>>>>>>>> generated
>>>>>>>> adapter as well as the code itself does not mactch.
>>>>>>>>
>>>>>>>> Solution: Defer check (VerifyAdapterSharing) until VM is 
>>>>>>>> initialzied.
>>>>>>>> Note that this patch also fixes potential issues with
>>>>>>>> -XX:+VerifyAdapterCalls, which is a dignostic flag. The patch also
>>>>>>>> removes some unused field of VerifyAdapterSharing.
>>>>>>>>
>>>>>>>>
>>>>>>>> Many thanks in advance,
>>>>>>>> Albert
>>>>>>>> **
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20131025/b47d2452/attachment-0001.html