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

[Albert Noll]

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/20131024/c0a4e283/attachment-0001.html