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

[Christian Thalinger]

Usually we call them I2C/C2I “adapters” but it doesn’t matter.  The change is good.

On Oct 25, 2013, at 1:26 AM, Albert Noll <albert.noll at oracle.com> wrote:

> 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/
> 
> 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> 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/20131025/02695f76/attachment.html