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

[John Rose]

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> 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/
> 
> 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> wrote:
>> 
>>> Forgot to adjust the comment. This version should be fine.
>>> 
>>> http://cr.openjdk.java.net/~anoll/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/6efbeedf/attachment-0001.html