RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m
Hi,

Please review the fix made for issue

bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/

Unit tests: As its hard, none

Other tests: jprt.

Description of the issue:
A valid pc match in exception cache returning an invalid handler makes
assert to fail.
This happens as  ExceptionCache reads are lock free access.

As a fix for this i have put a storestore mem barrier before the count
is updated.

Best Regards,
Jamsheed
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Christian Thalinger-3
   if (count() < cache_size) {
     set_pc_at(count(),addr);
     set_handler_at(count(), handler);

Shouldn’t we read count() only once into a local variable to rule any odd race bugs down the road?

> On Jan 28, 2016, at 5:16 PM, Jamsheed C m <[hidden email]> wrote:
>
> Hi,
>
> Please review the fix made for issue
>
> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>
> Unit tests: As its hard, none
>
> Other tests: jprt.
>
> Description of the issue:
> A valid pc match in exception cache returning an invalid handler makes assert to fail.
> This happens as  ExceptionCache reads are lock free access.
>
> As a fix for this i have put a storestore mem barrier before the count is updated.
>
> Best Regards,
> Jamsheed

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m


On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>     if (count() < cache_size) {
>       set_pc_at(count(),addr);
>       set_handler_at(count(), handler);
>
> Shouldn’t we read count() only once into a local variable to rule any odd race bugs down the road?

write to cache is mutex lock protected. so this code is safe.

Issue is seen in weak memory order machines.  lockless read of exception
cache values fails as writes in cache get reordered.

Best Regards,
Jamsheed

>
>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m <[hidden email]> wrote:
>>
>> Hi,
>>
>> Please review the fix made for issue
>>
>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>
>> Unit tests: As its hard, none
>>
>> Other tests: jprt.
>>
>> Description of the issue:
>> A valid pc match in exception cache returning an invalid handler makes assert to fail.
>> This happens as  ExceptionCache reads are lock free access.
>>
>> As a fix for this i have put a storestore mem barrier before the count is updated.
>>
>> Best Regards,
>> Jamsheed

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Vladimir Kozlov
On 1/28/16 12:29 PM, Jamsheed C m wrote:
>
>
> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>     if (count() < cache_size) {
>>       set_pc_at(count(),addr);
>>       set_handler_at(count(), handler);
>>
>> Shouldn’t we read count() only once into a local variable to rule any odd race bugs down the road?

+1. As I understand, Chris is suggesting to do it in addition to storestore barrier.

Do we have other similar code?

Thanks,
Vladimir

>
> write to cache is mutex lock protected. so this code is safe.
>
> Issue is seen in weak memory order machines.  lockless read of exception cache values fails as writes in cache get
> reordered.
>
> Best Regards,
> Jamsheed
>>
>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> Please review the fix made for issue
>>>
>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>
>>> Unit tests: As its hard, none
>>>
>>> Other tests: jprt.
>>>
>>> Description of the issue:
>>> A valid pc match in exception cache returning an invalid handler makes assert to fail.
>>> This happens as  ExceptionCache reads are lock free access.
>>>
>>> As a fix for this i have put a storestore mem barrier before the count is updated.
>>>
>>> Best Regards,
>>> Jamsheed
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

dean.long
In reply to this post by Jamsheed C m
As you noticed, for this kind of bug the memory is going to consistent
by the time the core file is written.
So to help debug this assert it if happens again, could you change it to
something like:

#ifdef ASSERT
     address computed_address =
SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
force_unwind, true);
     vmassert(handler_address == computed_address, PTR_FORMAT " != "
PTR_FORMAT, p2i(handler_address), p2i(computed_address));
#endif

dl

On 1/28/2016 8:16 AM, Jamsheed C m wrote:

> Hi,
>
> Please review the fix made for issue
>
> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>
> Unit tests: As its hard, none
>
> Other tests: jprt.
>
> Description of the issue:
> A valid pc match in exception cache returning an invalid handler makes
> assert to fail.
> This happens as  ExceptionCache reads are lock free access.
>
> As a fix for this i have put a storestore mem barrier before the count
> is updated.
>
> Best Regards,
> Jamsheed

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m
Hi Dean,

On 1/29/2016 9:40 AM, Dean Long wrote:

> As you noticed, for this kind of bug the memory is going to consistent
> by the time the core file is written.
> So to help debug this assert it if happens again, could you change it
> to something like:
>
> #ifdef ASSERT
>     address computed_address =
> SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
> force_unwind, true);
>     vmassert(handler_address == computed_address, PTR_FORMAT " != "
> PTR_FORMAT, p2i(handler_address), p2i(computed_address));
> #endif
I got handler_address value in this case. This value was inconsistent
with value in ExceptionCache.
It was having initial value and that was helpful in figuring out what
would have went wrong.

I will make this change.

Best Regards,
Jamsheed

>
> dl
>
> On 1/28/2016 8:16 AM, Jamsheed C m wrote:
>> Hi,
>>
>> Please review the fix made for issue
>>
>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>
>> Unit tests: As its hard, none
>>
>> Other tests: jprt.
>>
>> Description of the issue:
>> A valid pc match in exception cache returning an invalid handler
>> makes assert to fail.
>> This happens as  ExceptionCache reads are lock free access.
>>
>> As a fix for this i have put a storestore mem barrier before the
>> count is updated.
>>
>> Best Regards,
>> Jamsheed
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m
In reply to this post by Vladimir Kozlov
Hi Vladimir,

On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:

> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>
>>
>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>     if (count() < cache_size) {
>>>       set_pc_at(count(),addr);
>>>       set_handler_at(count(), handler);
>>>
>>> Shouldn’t we read count() only once into a local variable to rule
>>> any odd race bugs down the road?
>
> +1. As I understand, Chris is suggesting to do it in addition to
> storestore barrier.
Ok.
>
>
> Do we have other similar code?

I am not sure, let me have a check.

Best Regards,
Jamsheed

>
> Thanks,
> Vladimir
>
>>
>> write to cache is mutex lock protected. so this code is safe.
>>
>> Issue is seen in weak memory order machines.  lockless read of
>> exception cache values fails as writes in cache get
>> reordered.
>>
>> Best Regards,
>> Jamsheed
>>>
>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Please review the fix made for issue
>>>>
>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>
>>>> Unit tests: As its hard, none
>>>>
>>>> Other tests: jprt.
>>>>
>>>> Description of the issue:
>>>> A valid pc match in exception cache returning an invalid handler
>>>> makes assert to fail.
>>>> This happens as  ExceptionCache reads are lock free access.
>>>>
>>>> As a fix for this i have put a storestore mem barrier before the
>>>> count is updated.
>>>>
>>>> Best Regards,
>>>> Jamsheed
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

dean.long
In reply to this post by Jamsheed C m
On 1/28/2016 10:36 PM, Jamsheed C m wrote:

> Hi Dean,
>
> On 1/29/2016 9:40 AM, Dean Long wrote:
>> As you noticed, for this kind of bug the memory is going to
>> consistent by the time the core file is written.
>> So to help debug this assert it if happens again, could you change it
>> to something like:
>>
>> #ifdef ASSERT
>>     address computed_address =
>> SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
>> force_unwind, true);
>>     vmassert(handler_address == computed_address, PTR_FORMAT " != "
>> PTR_FORMAT, p2i(handler_address), p2i(computed_address));
>> #endif
> I got handler_address value in this case. This value was inconsistent
> with value in ExceptionCache.
> It was having initial value and that was helpful in figuring out what
> would have went wrong.
>

In the bug report, you said all data in the core file was consistent, so
I'm just wondering where you saw
it inconsistent.   Just to confirm what was going wrong, you suspect
that _count was being updated before the handler?

dl

> I will make this change.
>
> Best Regards,
> Jamsheed
>>
>> dl
>>
>> On 1/28/2016 8:16 AM, Jamsheed C m wrote:
>>> Hi,
>>>
>>> Please review the fix made for issue
>>>
>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>
>>> Unit tests: As its hard, none
>>>
>>> Other tests: jprt.
>>>
>>> Description of the issue:
>>> A valid pc match in exception cache returning an invalid handler
>>> makes assert to fail.
>>> This happens as  ExceptionCache reads are lock free access.
>>>
>>> As a fix for this i have put a storestore mem barrier before the
>>> count is updated.
>>>
>>> Best Regards,
>>> Jamsheed
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m

Hi Dean,

On 1/30/2016 12:49 AM, Dean Long wrote:

> On 1/28/2016 10:36 PM, Jamsheed C m wrote:
>> Hi Dean,
>>
>> On 1/29/2016 9:40 AM, Dean Long wrote:
>>> As you noticed, for this kind of bug the memory is going to
>>> consistent by the time the core file is written.
>>> So to help debug this assert it if happens again, could you change
>>> it to something like:
>>>
>>> #ifdef ASSERT
>>>     address computed_address =
>>> SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
>>> force_unwind, true);
>>>     vmassert(handler_address == computed_address, PTR_FORMAT " != "
>>> PTR_FORMAT, p2i(handler_address), p2i(computed_address));
>>> #endif
>> I got handler_address value in this case. This value was inconsistent
>> with value in ExceptionCache.
>> It was having initial value and that was helpful in figuring out what
>> would have went wrong.
>>
>
> In the bug report, you said all data in the core file was consistent,
> so I'm just wondering where you saw
> it inconsistent.   Just to confirm what was going wrong, you suspect
> that _count was being updated before the handler?
i meant ExceptionCache(heap) and ExecptionHandlerTable(heap) contents
were consistent at the time core file was written.
handler_address(local variable) had already captured failing value.
handler_address(local variable) was inconsistent with
ExceptionCache(heap) hanlder_address in core file.

there were two failing case.
1) Only one entry in exception cache and failing

         -here i suspect handler_address in exception cache write code
got reordered well below count and and even ExceptioCache pointer update
in nm.
2)Two entries in exception cache for an exception and second entry
causing failure.

         - here i suspect handler_address in exception cache write code
got reordered below count.

These reordering happens in very small window, as this is code is
already lock protected ( and has a mem barrier below).

Best,
Jamsheed

>
> dl
>
>> I will make this change.
>>
>> Best Regards,
>> Jamsheed
>>>
>>> dl
>>>
>>> On 1/28/2016 8:16 AM, Jamsheed C m wrote:
>>>> Hi,
>>>>
>>>> Please review the fix made for issue
>>>>
>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>
>>>> Unit tests: As its hard, none
>>>>
>>>> Other tests: jprt.
>>>>
>>>> Description of the issue:
>>>> A valid pc match in exception cache returning an invalid handler
>>>> makes assert to fail.
>>>> This happens as  ExceptionCache reads are lock free access.
>>>>
>>>> As a fix for this i have put a storestore mem barrier before the
>>>> count is updated.
>>>>
>>>> Best Regards,
>>>> Jamsheed
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m


On 1/30/2016 9:38 AM, Jamsheed C m wrote:

>
> Hi Dean,
>
> On 1/30/2016 12:49 AM, Dean Long wrote:
>> On 1/28/2016 10:36 PM, Jamsheed C m wrote:
>>> Hi Dean,
>>>
>>> On 1/29/2016 9:40 AM, Dean Long wrote:
>>>> As you noticed, for this kind of bug the memory is going to
>>>> consistent by the time the core file is written.
>>>> So to help debug this assert it if happens again, could you change
>>>> it to something like:
>>>>
>>>> #ifdef ASSERT
>>>>     address computed_address =
>>>> SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
>>>> force_unwind, true);
>>>>     vmassert(handler_address == computed_address, PTR_FORMAT " != "
>>>> PTR_FORMAT, p2i(handler_address), p2i(computed_address));
>>>> #endif
>>> I got handler_address value in this case. This value was
>>> inconsistent with value in ExceptionCache.
>>> It was having initial value and that was helpful in figuring out
>>> what would have went wrong.
>>>
>>
>> In the bug report, you said all data in the core file was consistent,
>> so I'm just wondering where you saw
>> it inconsistent.   Just to confirm what was going wrong, you suspect
>> that _count was being updated before the handler?
> i meant ExceptionCache(heap) and ExecptionHandlerTable(heap) contents
> were consistent at the time core file was written.
> handler_address(local variable) had already captured failing value.
> handler_address(local variable) was inconsistent with
> ExceptionCache(heap) hanlder_address in core file.
>
> there were two failing case.
> 1) Only one entry in exception cache and failing
>
>         -here i suspect handler_address in exception cache write code
> got reordered well below count and and even ExceptioCache pointer
> update in nm.
> 2)Two entries in exception cache for an exception and second entry
> causing failure.
>
>         - here i suspect handler_address in exception cache write code
> got reordered below count.
>
> These reordering happens in very small window, as this code is already
> lock protected ( and has a mem barrier below).
i have removed the ambiguity in the bug report.

Best Regards,
Jamsheed

>
> Best,
> Jamsheed
>
>>
>> dl
>>
>>> I will make this change.
>>>
>>> Best Regards,
>>> Jamsheed
>>>>
>>>> dl
>>>>
>>>> On 1/28/2016 8:16 AM, Jamsheed C m wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the fix made for issue
>>>>>
>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>
>>>>> Unit tests: As its hard, none
>>>>>
>>>>> Other tests: jprt.
>>>>>
>>>>> Description of the issue:
>>>>> A valid pc match in exception cache returning an invalid handler
>>>>> makes assert to fail.
>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>
>>>>> As a fix for this i have put a storestore mem barrier before the
>>>>> count is updated.
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

dean.long
On 1/29/2016 9:19 PM, Jamsheed C m wrote:

>
>
> On 1/30/2016 9:38 AM, Jamsheed C m wrote:
>>
>> Hi Dean,
>>
>> On 1/30/2016 12:49 AM, Dean Long wrote:
>>> On 1/28/2016 10:36 PM, Jamsheed C m wrote:
>>>> Hi Dean,
>>>>
>>>> On 1/29/2016 9:40 AM, Dean Long wrote:
>>>>> As you noticed, for this kind of bug the memory is going to
>>>>> consistent by the time the core file is written.
>>>>> So to help debug this assert it if happens again, could you change
>>>>> it to something like:
>>>>>
>>>>> #ifdef ASSERT
>>>>>     address computed_address =
>>>>> SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
>>>>> force_unwind, true);
>>>>>     vmassert(handler_address == computed_address, PTR_FORMAT " !=
>>>>> " PTR_FORMAT, p2i(handler_address), p2i(computed_address));
>>>>> #endif
>>>> I got handler_address value in this case. This value was
>>>> inconsistent with value in ExceptionCache.
>>>> It was having initial value and that was helpful in figuring out
>>>> what would have went wrong.
>>>>
>>>
>>> In the bug report, you said all data in the core file was
>>> consistent, so I'm just wondering where you saw
>>> it inconsistent.   Just to confirm what was going wrong, you suspect
>>> that _count was being updated before the handler?
>> i meant ExceptionCache(heap) and ExecptionHandlerTable(heap) contents
>> were consistent at the time core file was written.
>> handler_address(local variable) had already captured failing value.
>> handler_address(local variable) was inconsistent with
>> ExceptionCache(heap) hanlder_address in core file.
>>
>> there were two failing case.
>> 1) Only one entry in exception cache and failing
>>
>>         -here i suspect handler_address in exception cache write code
>> got reordered well below count and and even ExceptioCache pointer
>> update in nm.
>> 2)Two entries in exception cache for an exception and second entry
>> causing failure.
>>
>>         - here i suspect handler_address in exception cache write
>> code got reordered below count.
>>
>> These reordering happens in very small window, as this code is
>> already lock protected ( and has a mem barrier below).
> i have removed the ambiguity in the bug report.
>

OK thanks.  The change looks good to me.

dl

> Best Regards,
> Jamsheed
>>
>> Best,
>> Jamsheed
>>
>>>
>>> dl
>>>
>>>> I will make this change.
>>>>
>>>> Best Regards,
>>>> Jamsheed
>>>>>
>>>>> dl
>>>>>
>>>>> On 1/28/2016 8:16 AM, Jamsheed C m wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the fix made for issue
>>>>>>
>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>>
>>>>>> Unit tests: As its hard, none
>>>>>>
>>>>>> Other tests: jprt.
>>>>>>
>>>>>> Description of the issue:
>>>>>> A valid pc match in exception cache returning an invalid handler
>>>>>> makes assert to fail.
>>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>>
>>>>>> As a fix for this i have put a storestore mem barrier before the
>>>>>> count is updated.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jamsheed
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m


On 2/2/2016 1:24 AM, Dean Long wrote:

> On 1/29/2016 9:19 PM, Jamsheed C m wrote:
>>
>>
>> On 1/30/2016 9:38 AM, Jamsheed C m wrote:
>>>
>>> Hi Dean,
>>>
>>> On 1/30/2016 12:49 AM, Dean Long wrote:
>>>> On 1/28/2016 10:36 PM, Jamsheed C m wrote:
>>>>> Hi Dean,
>>>>>
>>>>> On 1/29/2016 9:40 AM, Dean Long wrote:
>>>>>> As you noticed, for this kind of bug the memory is going to
>>>>>> consistent by the time the core file is written.
>>>>>> So to help debug this assert it if happens again, could you
>>>>>> change it to something like:
>>>>>>
>>>>>> #ifdef ASSERT
>>>>>>     address computed_address =
>>>>>> SharedRuntime::compute_compiled_exc_handler(nm, pc, exception,
>>>>>> force_unwind, true);
>>>>>>     vmassert(handler_address == computed_address, PTR_FORMAT " !=
>>>>>> " PTR_FORMAT, p2i(handler_address), p2i(computed_address));
>>>>>> #endif
>>>>> I got handler_address value in this case. This value was
>>>>> inconsistent with value in ExceptionCache.
>>>>> It was having initial value and that was helpful in figuring out
>>>>> what would have went wrong.
>>>>>
>>>>
>>>> In the bug report, you said all data in the core file was
>>>> consistent, so I'm just wondering where you saw
>>>> it inconsistent.   Just to confirm what was going wrong, you
>>>> suspect that _count was being updated before the handler?
>>> i meant ExceptionCache(heap) and ExecptionHandlerTable(heap)
>>> contents were consistent at the time core file was written.
>>> handler_address(local variable) had already captured failing value.
>>> handler_address(local variable) was inconsistent with
>>> ExceptionCache(heap) hanlder_address in core file.
>>>
>>> there were two failing case.
>>> 1) Only one entry in exception cache and failing
>>>
>>>         -here i suspect handler_address in exception cache write
>>> code got reordered well below count and and even ExceptioCache
>>> pointer update in nm.
>>> 2)Two entries in exception cache for an exception and second entry
>>> causing failure.
>>>
>>>         - here i suspect handler_address in exception cache write
>>> code got reordered below count.
>>>
>>> These reordering happens in very small window, as this code is
>>> already lock protected ( and has a mem barrier below).
>> i have removed the ambiguity in the bug report.
>>
>
> OK thanks.  The change looks good to me.
>
Thanks Dean.

> dl
>
>> Best Regards,
>> Jamsheed
>>>
>>> Best,
>>> Jamsheed
>>>
>>>>
>>>> dl
>>>>
>>>>> I will make this change.
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>>>>
>>>>>> dl
>>>>>>
>>>>>> On 1/28/2016 8:16 AM, Jamsheed C m wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the fix made for issue
>>>>>>>
>>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>>>
>>>>>>> Unit tests: As its hard, none
>>>>>>>
>>>>>>> Other tests: jprt.
>>>>>>>
>>>>>>> Description of the issue:
>>>>>>> A valid pc match in exception cache returning an invalid handler
>>>>>>> makes assert to fail.
>>>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>>>
>>>>>>> As a fix for this i have put a storestore mem barrier before the
>>>>>>> count is updated.
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jamsheed
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m
In reply to this post by Vladimir Kozlov
Hi Vladimir, Chris,

On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:

> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>
>>
>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>     if (count() < cache_size) {
>>>       set_pc_at(count(),addr);
>>>       set_handler_at(count(), handler);
>>>
>>> Shouldn’t we read count() only once into a local variable to rule
>>> any odd race bugs down the road?
>
> +1. As I understand, Chris is suggesting to do it in addition to
> storestore barrier.
I have made the suggested change.
http://cr.openjdk.java.net/~thartmann/8143897/webrev.01/

Best Regards,
Jamsheed

>
> Do we have other similar code?
>
> Thanks,
> Vladimir
>
>>
>> write to cache is mutex lock protected. so this code is safe.
>>
>> Issue is seen in weak memory order machines.  lockless read of
>> exception cache values fails as writes in cache get
>> reordered.
>>
>> Best Regards,
>> Jamsheed
>>>
>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Please review the fix made for issue
>>>>
>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>
>>>> Unit tests: As its hard, none
>>>>
>>>> Other tests: jprt.
>>>>
>>>> Description of the issue:
>>>> A valid pc match in exception cache returning an invalid handler
>>>> makes assert to fail.
>>>> This happens as  ExceptionCache reads are lock free access.
>>>>
>>>> As a fix for this i have put a storestore mem barrier before the
>>>> count is updated.
>>>>
>>>> Best Regards,
>>>> Jamsheed
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Vladimir Kozlov
Looks good.

Thanks,
Vladimir

On 2/3/16 1:56 AM, Jamsheed C m wrote:

> Hi Vladimir, Chris,
>
> On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:
>> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>>
>>>
>>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>>     if (count() < cache_size) {
>>>>       set_pc_at(count(),addr);
>>>>       set_handler_at(count(), handler);
>>>>
>>>> Shouldn’t we read count() only once into a local variable to rule any odd race bugs down the road?
>>
>> +1. As I understand, Chris is suggesting to do it in addition to storestore barrier.
> I have made the suggested change.
> http://cr.openjdk.java.net/~thartmann/8143897/webrev.01/
>
> Best Regards,
> Jamsheed
>
>>
>> Do we have other similar code?
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> write to cache is mutex lock protected. so this code is safe.
>>>
>>> Issue is seen in weak memory order machines.  lockless read of exception cache values fails as writes in cache get
>>> reordered.
>>>
>>> Best Regards,
>>> Jamsheed
>>>>
>>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m <[hidden email]> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please review the fix made for issue
>>>>>
>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>
>>>>> Unit tests: As its hard, none
>>>>>
>>>>> Other tests: jprt.
>>>>>
>>>>> Description of the issue:
>>>>> A valid pc match in exception cache returning an invalid handler makes assert to fail.
>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>
>>>>> As a fix for this i have put a storestore mem barrier before the count is updated.
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m
Thanks Vladimir.

Best Regards,
Jamsheed

On 2/3/2016 11:31 PM, Vladimir Kozlov wrote:

> Looks good.
>
> Thanks,
> Vladimir
>
> On 2/3/16 1:56 AM, Jamsheed C m wrote:
>> Hi Vladimir, Chris,
>>
>> On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:
>>> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>>>
>>>>
>>>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>>>     if (count() < cache_size) {
>>>>>       set_pc_at(count(),addr);
>>>>>       set_handler_at(count(), handler);
>>>>>
>>>>> Shouldn’t we read count() only once into a local variable to rule
>>>>> any odd race bugs down the road?
>>>
>>> +1. As I understand, Chris is suggesting to do it in addition to
>>> storestore barrier.
>> I have made the suggested change.
>> http://cr.openjdk.java.net/~thartmann/8143897/webrev.01/
>>
>> Best Regards,
>> Jamsheed
>>
>>>
>>> Do we have other similar code?
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>> write to cache is mutex lock protected. so this code is safe.
>>>>
>>>> Issue is seen in weak memory order machines.  lockless read of
>>>> exception cache values fails as writes in cache get
>>>> reordered.
>>>>
>>>> Best Regards,
>>>> Jamsheed
>>>>>
>>>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m
>>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please review the fix made for issue
>>>>>>
>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>>
>>>>>> Unit tests: As its hard, none
>>>>>>
>>>>>> Other tests: jprt.
>>>>>>
>>>>>> Description of the issue:
>>>>>> A valid pc match in exception cache returning an invalid handler
>>>>>> makes assert to fail.
>>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>>
>>>>>> As a fix for this i have put a storestore mem barrier before the
>>>>>> count is updated.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jamsheed
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

dean.long
Jamsheed, it just occurred to me, why don't we need a LoadLoad barrier
when we do the lock-free reads?

dl

On 2/4/2016 1:04 AM, Jamsheed C m wrote:

> Thanks Vladimir.
>
> Best Regards,
> Jamsheed
>
> On 2/3/2016 11:31 PM, Vladimir Kozlov wrote:
>> Looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 2/3/16 1:56 AM, Jamsheed C m wrote:
>>> Hi Vladimir, Chris,
>>>
>>> On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:
>>>> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>>>>
>>>>>
>>>>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>>>>     if (count() < cache_size) {
>>>>>>       set_pc_at(count(),addr);
>>>>>>       set_handler_at(count(), handler);
>>>>>>
>>>>>> Shouldn’t we read count() only once into a local variable to rule
>>>>>> any odd race bugs down the road?
>>>>
>>>> +1. As I understand, Chris is suggesting to do it in addition to
>>>> storestore barrier.
>>> I have made the suggested change.
>>> http://cr.openjdk.java.net/~thartmann/8143897/webrev.01/
>>>
>>> Best Regards,
>>> Jamsheed
>>>
>>>>
>>>> Do we have other similar code?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> write to cache is mutex lock protected. so this code is safe.
>>>>>
>>>>> Issue is seen in weak memory order machines.  lockless read of
>>>>> exception cache values fails as writes in cache get
>>>>> reordered.
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>>>>
>>>>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m
>>>>>>> <[hidden email]> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the fix made for issue
>>>>>>>
>>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>>>
>>>>>>> Unit tests: As its hard, none
>>>>>>>
>>>>>>> Other tests: jprt.
>>>>>>>
>>>>>>> Description of the issue:
>>>>>>> A valid pc match in exception cache returning an invalid handler
>>>>>>> makes assert to fail.
>>>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>>>
>>>>>>> As a fix for this i have put a storestore mem barrier before the
>>>>>>> count is updated.
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jamsheed
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

Jamsheed C m

On 2/9/2016 4:49 AM, Dean Long wrote:
> Jamsheed, it just occurred to me, why don't we need a LoadLoad barrier
> when we do the lock-free reads?
There is data dependence in ExceptionCache read code.

Best Regards,
Jamsheed

>
> dl
>
> On 2/4/2016 1:04 AM, Jamsheed C m wrote:
>> Thanks Vladimir.
>>
>> Best Regards,
>> Jamsheed
>>
>> On 2/3/2016 11:31 PM, Vladimir Kozlov wrote:
>>> Looks good.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 2/3/16 1:56 AM, Jamsheed C m wrote:
>>>> Hi Vladimir, Chris,
>>>>
>>>> On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:
>>>>> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>>>>>
>>>>>>
>>>>>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>>>>>     if (count() < cache_size) {
>>>>>>>       set_pc_at(count(),addr);
>>>>>>>       set_handler_at(count(), handler);
>>>>>>>
>>>>>>> Shouldn’t we read count() only once into a local variable to
>>>>>>> rule any odd race bugs down the road?
>>>>>
>>>>> +1. As I understand, Chris is suggesting to do it in addition to
>>>>> storestore barrier.
>>>> I have made the suggested change.
>>>> http://cr.openjdk.java.net/~thartmann/8143897/webrev.01/
>>>>
>>>> Best Regards,
>>>> Jamsheed
>>>>
>>>>>
>>>>> Do we have other similar code?
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> write to cache is mutex lock protected. so this code is safe.
>>>>>>
>>>>>> Issue is seen in weak memory order machines.  lockless read of
>>>>>> exception cache values fails as writes in cache get
>>>>>> reordered.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jamsheed
>>>>>>>
>>>>>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the fix made for issue
>>>>>>>>
>>>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>>>>
>>>>>>>> Unit tests: As its hard, none
>>>>>>>>
>>>>>>>> Other tests: jprt.
>>>>>>>>
>>>>>>>> Description of the issue:
>>>>>>>> A valid pc match in exception cache returning an invalid
>>>>>>>> handler makes assert to fail.
>>>>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>>>>
>>>>>>>> As a fix for this i have put a storestore mem barrier before
>>>>>>>> the count is updated.
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Jamsheed
>>>>>>
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8143897 :Weblogic12medrec assert(handler_address == SharedRuntime::compute_compiled_exc_handler(nm, pc, exception, force_unwind, true)) failed: Must be the same

dean.long
OK thanks.

dl

On 2/9/2016 4:29 AM, Jamsheed C m wrote:

>
> On 2/9/2016 4:49 AM, Dean Long wrote:
>> Jamsheed, it just occurred to me, why don't we need a LoadLoad
>> barrier when we do the lock-free reads?
> There is data dependence in ExceptionCache read code.
>
> Best Regards,
> Jamsheed
>>
>> dl
>>
>> On 2/4/2016 1:04 AM, Jamsheed C m wrote:
>>> Thanks Vladimir.
>>>
>>> Best Regards,
>>> Jamsheed
>>>
>>> On 2/3/2016 11:31 PM, Vladimir Kozlov wrote:
>>>> Looks good.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 2/3/16 1:56 AM, Jamsheed C m wrote:
>>>>> Hi Vladimir, Chris,
>>>>>
>>>>> On 1/29/2016 6:22 AM, Vladimir Kozlov wrote:
>>>>>> On 1/28/16 12:29 PM, Jamsheed C m wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/29/2016 12:15 AM, Christian Thalinger wrote:
>>>>>>>>     if (count() < cache_size) {
>>>>>>>>       set_pc_at(count(),addr);
>>>>>>>>       set_handler_at(count(), handler);
>>>>>>>>
>>>>>>>> Shouldn’t we read count() only once into a local variable to
>>>>>>>> rule any odd race bugs down the road?
>>>>>>
>>>>>> +1. As I understand, Chris is suggesting to do it in addition to
>>>>>> storestore barrier.
>>>>> I have made the suggested change.
>>>>> http://cr.openjdk.java.net/~thartmann/8143897/webrev.01/
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>>>
>>>>>>
>>>>>> Do we have other similar code?
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>> write to cache is mutex lock protected. so this code is safe.
>>>>>>>
>>>>>>> Issue is seen in weak memory order machines.  lockless read of
>>>>>>> exception cache values fails as writes in cache get
>>>>>>> reordered.
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jamsheed
>>>>>>>>
>>>>>>>>> On Jan 28, 2016, at 5:16 PM, Jamsheed C m
>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review the fix made for issue
>>>>>>>>>
>>>>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8143897
>>>>>>>>> web rev: http://cr.openjdk.java.net/~thartmann/8143897/webrev.00/
>>>>>>>>>
>>>>>>>>> Unit tests: As its hard, none
>>>>>>>>>
>>>>>>>>> Other tests: jprt.
>>>>>>>>>
>>>>>>>>> Description of the issue:
>>>>>>>>> A valid pc match in exception cache returning an invalid
>>>>>>>>> handler makes assert to fail.
>>>>>>>>> This happens as  ExceptionCache reads are lock free access.
>>>>>>>>>
>>>>>>>>> As a fix for this i have put a storestore mem barrier before
>>>>>>>>> the count is updated.
>>>>>>>>>
>>>>>>>>> Best Regards,
>>>>>>>>> Jamsheed
>>>>>>>
>>>>>
>>>
>>
>