8376400: C2: folding ifs may cause incorrect execution when trap is taken

src/hotspot/share/opto/callnode.hpp

@@ -728,19 +728,22 @@ class CallJavaNode : public CallNode {
 // calls and optimized virtual calls, plus calls to wrappers for run-time
 // routines); generates static stub.
 class CallStaticJavaNode : public CallJavaNode {
+  // If this is an uncommon trap guarded by some condition, is it safe to change the condition to a narrower condition?
+  // See comment in PhaseIdealLoop::do_split_if()
+  bool _safe_for_fold_compare;
   virtual bool cmp( const Node &n ) const;
   virtual uint size_of() const; // Size is bigger
 public:
   CallStaticJavaNode(Compile* C, const TypeFunc* tf, address addr, ciMethod* method)
-    : CallJavaNode(tf, addr, method) {
+    : CallJavaNode(tf, addr, method), _safe_for_fold_compare(true) {
     init_class_id(Class_CallStaticJava);
     if (C->eliminate_boxing() && (method != nullptr) && method->is_boxing_method()) {
       init_flags(Flag_is_macro);
       C->add_macro_node(this);
     }
   }
   CallStaticJavaNode(const TypeFunc* tf, address addr, const char* name, const TypePtr* adr_type)
-    : CallJavaNode(tf, addr, nullptr) {
+    : CallJavaNode(tf, addr, nullptr), _safe_for_fold_compare(true) {
     init_class_id(Class_CallStaticJava);
     // This node calls a runtime stub, which often has narrow memory effects.
     _adr_type = adr_type;
@@ -763,6 +766,14 @@ class CallStaticJavaNode : public CallJavaNode {
   virtual int         Opcode() const;
   virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
 
+  void clear_safe_for_fold_compare() {
+    _safe_for_fold_compare = false;
+  }
+
+  bool safe_for_fold_compare() const {
+    return _safe_for_fold_compare;
+  }
+
 #ifndef PRODUCT
   virtual void        dump_spec(outputStream *st) const;
   virtual void        dump_compact_spec(outputStream *st) const;

src/hotspot/share/opto/cfgnode.hpp

@@ -438,6 +438,7 @@ class IfNode : public MultiBranchNode {
 #ifndef PRODUCT
   virtual void dump_spec(outputStream *st) const;
 #endif
+  void mark_projections_unsafe_for_fold_compare() const;
 };
 
 class RangeCheckNode : public IfNode {

src/hotspot/share/opto/ifnode.cpp

@@ -843,6 +843,10 @@ bool IfNode::has_only_uncommon_traps(ProjNode* proj, ProjNode*& success, ProjNod
         return false;
       }
 
+      if (!dom_unc->safe_for_fold_compare()) {
+        return false;
+      }
+
       // See merge_uncommon_traps: the reason of the uncommon trap
       // will be changed and the state of the dominating If will be
       // used. Checked that we didn't apply this transformation in a
@@ -1601,6 +1605,57 @@ Node* IfNode::search_identical(int dist) {
   return prev_dom;
 }
 
+void IfNode::mark_projections_unsafe_for_fold_compare() const {
+  // With the following code pattern
+  //
+  // if (some_condition) {
+  //     v = 0;
+  // } else {
+  //     v = 1;
+  // } // v is Phi(0, 1)
+  // if (v == 0) {
+  //     uncommon_trap(); // reexecutes the "if (v == 0) {" above, captures v as stack argument to ifeq bytecode
+  // }
+  // if (some_other_condition) {
+  //     uncommon_trap(); // reexecutes the "if (some_other_condition) {"
+  // }
+  //
+  // if the second if is split thru Phi, the result is:
+  //
+  // if (some_condition) {
+  //     uncommon_trap(); // reexecutes the "if (v == 0) {" that was removed above, captures v = 0 as stack argument to ifeq bytecode
+  // }
+  // if (some_other_condition) {
+  //     uncommon_trap(); // reexecutes the "if (some_other_condition) {"
+  // }
+  //
+  // some_condition and some_other_condition could be folded into
+  // a single new condition that is narrower than some_condition
+  // (done by IfNode::fold_compares(), for instance):
+  //
+  // if (combined_narrower_condition) {
+  //     uncommon_trap(); // reexecutes the "if (v == 0) {" that was removed, captures v = 0 as stack argument to ifeq bytecode
+  // }
+  //
+  // Then combined_narrower_condition is true for some input value for
+  // which some_condition is false. When such an input value is used
+  // at runtime, the trap is taken which causes "if (v == 0) {" to be
+  // reexecuted with v = 0 even though some_condition is wrong, causing
+  // the wrong branch to be executed.
+  //
+  // Mark the uncommon trap nodes to prevent such a transformation
+  // from happening.
+  IfProjNode* true_projection = proj_out(1)->as_IfProj();
+  IfProjNode* false_projection = proj_out(0)->as_IfProj();
+  CallStaticJavaNode* unc = true_projection->is_uncommon_trap_proj(Deoptimization::Reason_none);
+  if (unc != nullptr) {
+    unc->clear_safe_for_fold_compare();
+  }
+  unc = false_projection->is_uncommon_trap_proj(Deoptimization::Reason_none);
+  if (unc != nullptr) {
+    unc->clear_safe_for_fold_compare();
+  }
+}
 
 static int subsuming_bool_test_encode(Node*);
 

src/hotspot/share/opto/split_if.cpp

@@ -598,6 +598,7 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio
     tty->print_cr("SplitIf");
   }
 
+  iff->as_If()->mark_projections_unsafe_for_fold_compare();
   C->set_major_progress();
   RegionNode *region = iff->in(0)->as_Region();
   Node *region_dom = idom(region);