From e3c7f43298f9861fb2fce2dd58fef13d347e7c67 Mon Sep 17 00:00:00 2001
From: Thomas Schatzl <tschatzl@openjdk.org>
Date: Wed, 22 Jan 2020 10:00:13 +0100
Subject: [PATCH] 8235305: Corrupted oops embedded in nmethods due to parallel
 modification during optional evacuation

During optional evacuation it is possible that G1 modifies oops embedded in nmethods in parallel. One source are oop* gathered by a previous evacuation phase in the optional roots, the other the region's strong code roots list. Since these oops may be unaligned on x64, this can result in them being corrupted. The fix is to not gather embedded oops in the optional roots list as the strong code roots list contains them already.

Co-authored-by: Erik Osterlund <erik.osterlund@oracle.com>
Co-authored-by: Stefan Johansson <stefan.johansson@oracle.com>
Co-authored-by: Stefan Karlsson <stefan.karlsson@oracle.com>
Reviewed-by: sjohanss, stefank
---
 src/hotspot/share/gc/g1/g1OopClosures.hpp        |  3 ++-
 src/hotspot/share/gc/g1/g1OopClosures.inline.hpp |  2 +-
 src/hotspot/share/gc/g1/g1SharedClosures.hpp     | 11 ++++++++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/hotspot/share/gc/g1/g1OopClosures.hpp b/src/hotspot/share/gc/g1/g1OopClosures.hpp
index b2dd0405ec1..ae967681c18 100644
--- a/src/hotspot/share/gc/g1/g1OopClosures.hpp
+++ b/src/hotspot/share/gc/g1/g1OopClosures.hpp
@@ -152,7 +152,8 @@ class G1ParCopyHelper : public OopClosure {
 
 enum G1Barrier {
   G1BarrierNone,
-  G1BarrierCLD
+  G1BarrierCLD,
+  G1BarrierNoOptRoots  // Do not collect optional roots.
 };
 
 enum G1Mark {
diff --git a/src/hotspot/share/gc/g1/g1OopClosures.inline.hpp b/src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
index b7162ac117e..2274f907c41 100644
--- a/src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
+++ b/src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
@@ -246,7 +246,7 @@ void G1ParCopyClosure<barrier, do_mark_object>::do_oop_work(T* p) {
   } else {
     if (state.is_humongous()) {
       _g1h->set_humongous_is_live(obj);
-    } else if (state.is_optional()) {
+    } else if ((barrier != G1BarrierNoOptRoots) && state.is_optional()) {
       _par_scan_state->remember_root_into_optional_region(p);
     }
 
diff --git a/src/hotspot/share/gc/g1/g1SharedClosures.hpp b/src/hotspot/share/gc/g1/g1SharedClosures.hpp
index af77223cfa8..9a705ff2faf 100644
--- a/src/hotspot/share/gc/g1/g1SharedClosures.hpp
+++ b/src/hotspot/share/gc/g1/g1SharedClosures.hpp
@@ -40,6 +40,14 @@ class G1SharedClosures {
 public:
   G1ParCopyClosure<G1BarrierNone, Mark> _oops;
   G1ParCopyClosure<G1BarrierCLD,  Mark> _oops_in_cld;
+  // We do not need (and actually should not) collect oops from nmethods into the
+  // optional collection set as we already automatically collect the corresponding
+  // nmethods in the region's strong code roots set. So set G1BarrierNoOptRoots in
+  // this closure.
+  // If these were present there would be opportunity for multiple threads to try
+  // to change this oop* at the same time. Since embedded oops are not necessarily
+  // word-aligned, this could lead to word tearing during update and crashes.
+  G1ParCopyClosure<G1BarrierNoOptRoots, Mark> _oops_in_nmethod;
 
   G1CLDScanClosure                _clds;
   G1CodeBlobClosure               _codeblobs;
@@ -47,6 +55,7 @@ class G1SharedClosures {
   G1SharedClosures(G1CollectedHeap* g1h, G1ParScanThreadState* pss, bool process_only_dirty) :
     _oops(g1h, pss),
     _oops_in_cld(g1h, pss),
+    _oops_in_nmethod(g1h, pss),
     _clds(&_oops_in_cld, process_only_dirty),
-    _codeblobs(pss->worker_id(), &_oops, needs_strong_processing()) {}
+    _codeblobs(pss->worker_id(), &_oops_in_nmethod, needs_strong_processing()) {}
 };