RFA: 8168996: C2 crash at postaloc.cpp:140 : assert(false) failed: unexpected yanked node

[Yangfei (Felix)]

Hi Andrew,

  Thanks for the quick reply :-)

> -----Original Message-----
> From: Andrew Dinn [mailto:adinn at redhat.com]
> Sent: Friday, January 8, 2021 5:32 PM
> To: Yangfei (Felix) <felix.yang at huawei.com>
> Cc: jdk8u-dev at openjdk.java.net
> Subject: Re: RFA: 8168996: C2 crash at postaloc.cpp:140 : assert(false) failed:
> unexpected yanked node
> 
> Hi Felix,
> 
> On 08/01/2021 06:17, Yangfei (Felix) wrote:
> > Hi,
> >
> >    I am witnessing a known issue [1] when running jcstress test with 8u
> aarch64 fastdebug build.
> >    This issue can be easily reproduced during a jcstress run on my aarch64
> linux server.
> 
> This certainly looks like the right fix for the right 'known issue' i.e.
> the one reported in JDK-8168996. However, I don't really understand how
> you came to the conclusion that 8168996 was the issue that is causing the
> problem, given that the bug is private.
> 
> Are you basing that assumption on the brief description in the email thread?
> Or the fact that the fix makes the problem go away? Or both? Or maybe
> something else?
> 
> I would prefer simply to approve this patch but it would help to have more
> background if you have any.

Sorry, I should have mentioned more details about the diagnosis for 8u in my previous mail.

There is some description about C2 graph in the original RFR thread:
  "The problem is that there is a MemBarAcquire node from a volatile object field load that keeps otherwise dead LoadN nodes alive (see graph at [1]).
    This is very similar to JDK-8048879 [2] with the difference that the MemBarAcquire -> DecodeN is not directly connected to the LoadN but there is a Phi in between. "

Given that the bug is private, I think what we can do is capturing the C2 graph for our scenario and compare it with this description.
We witnessed that the crash always triggers for the same java method: 
  Current CompileTask:
  C2:   2287   88             org.openjdk.jcstress.tests.locks.stamped.StampedLockPairwiseTests_tRLt_URs_aWL_U_jcstress::lambda$sanityCheck_Footprints$2 (120 bytes)

Given the concurrency execution of jcstress test, we need some local hacking [1] on 8u JVM in order to get the C2 graph on the crash site.
Two purposes for the local hacking:
1. Add process id to the C2 graph dump file name so we won't have two jvm process dumping to the same file.
2. Enable C2 graph dump for specific java method so that the dump file won't be too large in size.

Then the JVM command line becomes something like:
$ java -XX:-BackgroundCompilation -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:PrintIdealGraphLevel=3 -XX:PrintIdealGraphFile=ideal-%p-%t.xml -XX:CompileCommand="print *jcstress::lambda\$sanityCheck_Footprints\$2" -jar jcstress.jar -c 64 -f 1 -iters 1 -t org.openjdk.jcstress.tests.locks.stamped.StampedLockPairwiseTests

With that we managed to get the C2 graph after "Optimized finished" phase.  The subgraph looks like: LoadN  -->  DecodeN  --> MemBarAcquire
Here, the MemBarAcquire node from a volatile object field was keeping the otherwise dead LoadN nodes alive.
But we don't have a Phi in between. This is slight different with the C2 graph in the original RFR thread.
I think this does not affect the application of the original fix.  Is this enough to validate the 8u backport?

Thanks,
Felix


[1] local modification to capture the C2 graph
diff --git a/hotspot/src/share/vm/opto/idealGraphPrinter.cpp b/hotspot/src/share/vm/opto/idealGraphPrinter.cpp
index 65c1ac97b..5a4762eb7 100644
--- a/hotspot/src/share/vm/opto/idealGraphPrinter.cpp
+++ b/hotspot/src/share/vm/opto/idealGraphPrinter.cpp
@@ -130,10 +130,10 @@ IdealGraphPrinter::IdealGraphPrinter() {
       } else {
         st.print("%s%d", PrintIdealGraphFile, _file_count);
       }
-      fileStream *stream = new (ResourceObj::C_HEAP, mtCompiler) fileStream(st.as_string());
+      fileStream *stream = new (ResourceObj::C_HEAP, mtCompiler) fileStream(st.as_string(), true);
       _output = stream;
     } else {
-      fileStream *stream = new (ResourceObj::C_HEAP, mtCompiler) fileStream(PrintIdealGraphFile);
+      fileStream *stream = new (ResourceObj::C_HEAP, mtCompiler) fileStream(PrintIdealGraphFile, true);
       _output = stream;
     }
     _file_count++;
diff --git a/hotspot/src/share/vm/utilities/ostream.cpp b/hotspot/src/share/vm/utilities/ostream.cpp
index 587b839b9..670f25b13 100644
--- a/hotspot/src/share/vm/utilities/ostream.cpp
+++ b/hotspot/src/share/vm/utilities/ostream.cpp
@@ -705,6 +705,18 @@ void test_snprintf() {
 }
 #endif // PRODUCT

+fileStream::fileStream(const char* file_name, bool is_need_transform) {
+  if(is_need_transform)
+    file_name = make_log_name(file_name, NULL);
+  _file = fopen(file_name, "w");
+  if (_file != NULL) {
+    _need_close = true;
+  } else {
+    warning("Cannot open file %s due to %s\n", file_name, strerror(errno));
+    _need_close = false;
+  }
+}
+
 fileStream::fileStream(const char* file_name) {
   _file = fopen(file_name, "w");
   if (_file != NULL) {
diff --git a/hotspot/src/share/vm/utilities/ostream.hpp b/hotspot/src/share/vm/utilities/ostream.hpp
index c69289fb5..1fdd52102 100644
--- a/hotspot/src/share/vm/utilities/ostream.hpp
+++ b/hotspot/src/share/vm/utilities/ostream.hpp
@@ -200,6 +200,7 @@ class fileStream : public outputStream {
  public:
   fileStream() { _file = NULL; _need_close = false; }
   fileStream(const char* file_name);
+    fileStream(const char *file_name, bool is_need_transform);
   fileStream(const char* file_name, const char* opentype);
   fileStream(FILE* file, bool need_close = false) { _file = file; _need_close = need_close; }
   ~fileStream();

diff --git a/hotspot/src/share/vm/opto/compile.cpp b/hotspot/src/share/vm/opto/compile.cpp
index 540cae5d6..f2f85915f 100644
--- a/hotspot/src/share/vm/opto/compile.cpp
+++ b/hotspot/src/share/vm/opto/compile.cpp
@@ -866,7 +866,7 @@ Compile::Compile( ciEnv* ci_env, C2Compiler* compiler, ciMethod* target, int osr
   // Drain the list.
   Finish_Warm();
 #ifndef PRODUCT
-  if (_printer) {
+  if (_printer && CompilerOracle::should_print(C->method()->get_Method())) {
     _printer->print_inlining(this);
   }
 #endif
diff --git a/hotspot/src/share/vm/opto/compile.hpp b/hotspot/src/share/vm/opto/compile.hpp
index 93fa11737..2a368a5fb 100644
--- a/hotspot/src/share/vm/opto/compile.hpp
+++ b/hotspot/src/share/vm/opto/compile.hpp
@@ -639,7 +639,7 @@ class Compile : public Phase {

   void begin_method() {
 #ifndef PRODUCT
-    if (_printer) _printer->begin_method(this);
+    if (_printer && CompilerOracle::should_print(C->method()->get_Method())) _printer->begin_method(this);
 #endif
     C->_latest_stage_start_counter.stamp();
   }
@@ -656,7 +656,7 @@ class Compile : public Phase {


 #ifndef PRODUCT
-    if (_printer) _printer->print_method(this, CompilerPhaseTypeHelper::to_string(cpt), level);
+    if (_printer && CompilerOracle::should_print(C->method()->get_Method())) _printer->print_method(this, CompilerPhaseTypeHelper::to_string(cpt), level);
 #endif
     C->_latest_stage_start_counter.stamp();
   }
@@ -671,7 +671,7 @@ class Compile : public Phase {
       event.commit();
     }
 #ifndef PRODUCT
-    if (_printer) _printer->end_method();
+     if (_printer && CompilerOracle::should_print(C->method()->get_Method())) _printer->end_method();
 #endif
   }