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

[Albert Noll]

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/20131023/a5a53429/attachment.html