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

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 10/25/13 1:26 AM, Albert Noll 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/ <http://cr.openjdk.java.net/%7Eanoll/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 <mailto: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/ <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
>>>>>>>>> **
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>