RFR(S): 8153267: nmethod's exception cache not multi-thread safe

[Doerr, Martin]

Hi Jamsheed,

thanks for pointing me to it. Interesting that you have found such a problem so shortly before me :)

My webrev addresses some aspects which are not covered by your fix:

-          add_handler_for_exception_and_pc adds a new ExceptionCache instance in the other case. They need to get released as well.

-          The readers of the _exception_cache field are not safe, yet. As Andrew Haley pointed out, optimizers may modify load accesses for non-volatile fields.

So I think my change is still needed.

And after taking a closer look at your change, I think the _count field which is addressed by your fix needs to be volatile as well. I can incorporate that in my change if you like.
Would you agree?

Best regards,
Martin


From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Jamsheed C m
Sent: Montag, 4. April 2016 08:14
To: hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR(S): 8153267: nmethod's exception cache not multi-thread safe

Hi Martin,

"nmethod's exception cache not multi-thread safe"  bug is fixed in b107
bug id: https://bugs.openjdk.java.net/browse/JDK-8143897
fix changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/f918c20107d9
discussion link: http://openjdk.5641.n7.nabble.com/RFR-XS-8143897-Weblogic12medrec-assert-handler-address-SharedRuntime-compute-compiled-exc-handler-nme-td255611.html

Best Regards,
Jamsheed
On 4/1/2016 6:07 PM, Doerr, Martin wrote:
Hello everyone,

we have found a concurrency problem with the nmethod's exception cache. Readers of the cache may read stale data on weak memory platforms.

The writers of the cache are synchronized by locks, but there may be concurrent readers: The compiler runtimes use nmethod::handler_for_exception_and_pc to access the cache without locking.
Therefore, the nmethod's field _exception_cache needs to be volatile and adding new entries must be done by releasing stores. (Loading seems to be fine without acquire because there's an address dependency from the load of the cache to the usage of its contents which is sufficient to ensure ordering on all openjdk platforms.)

I also added a minor cleanup: I changed nmethod::is_alive to read the volatile field _state only once. It is certainly undesired to force the compiler to load it from memory twice.

Webrev is here:
http://cr.openjdk.java.net/~mdoerr/8153267_exception_cache/webrev.00/

Please review. I will also need a sponsor.

Best regards,
Martin


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160405/078ad910/attachment.html>