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

[Christian Thalinger]

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> 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/
> 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> 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/20131024/0b0d0823/attachment.html