RFR: JDK-8294902: Undefined Behavior in C2 regalloc with null references [v3]

Vladimir Ivanov vlivanov at openjdk.org
Thu Nov 3 23:59:27 UTC 2022 

On Thu, 3 Nov 2022 22:25:38 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> This change is to allow the creation of a null sentinel. See `Node_List::_empty_list((Arena*)NULL)`
>
> Oh, I see it now...
> 
>  Considering `Node_List::_empty_list` is effectively unusable (except for `is_null()` query), I'd prefer to see `postaloc.cpp` migrated away from references to pointers when it comes to `Node_List`. It already does ugly things like [1] which your patch doesn't handle yet. 
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/postaloc.cpp#L260
> 
> How about the following patch for `postaloc.cpp`? Does it solve your problem? 
> 
> diff --git a/src/hotspot/share/opto/postaloc.cpp b/src/hotspot/share/opto/postaloc.cpp
> index 96c30a122bb..10c9d1f90ae 100644
> --- a/src/hotspot/share/opto/postaloc.cpp
> +++ b/src/hotspot/share/opto/postaloc.cpp
> @@ -77,7 +77,7 @@ bool PhaseChaitin::may_be_copy_of_callee( Node *def ) const {
>  
>  //------------------------------yank-----------------------------------
>  // Helper function for yank_if_dead
> -int PhaseChaitin::yank( Node *old, Block *current_block, Node_List *value, Node_List *regnd ) {
> +int PhaseChaitin::yank(Node *old, Block *current_block, Node_List *value, Node_List *regnd) {
>    int blk_adjust=0;
>    Block *oldb = _cfg.get_block_for_node(old);
>    oldb->find_remove(old);
> @@ -87,9 +87,9 @@ int PhaseChaitin::yank( Node *old, Block *current_block, Node_List *value, Node_
>    }
>    _cfg.unmap_node_from_block(old);
>    OptoReg::Name old_reg = lrgs(_lrg_map.live_range_id(old)).reg();
> -  if( regnd && (*regnd)[old_reg]==old ) { // Instruction is currently available?
> -    value->map(old_reg,NULL);  // Yank from value/regnd maps
> -    regnd->map(old_reg,NULL);  // This register's value is now unknown
> +  if (regnd != NULL && regnd->at(old_reg) == old) { // Instruction is currently available?
> +    value->map(old_reg, NULL); // Yank from value/regnd maps
> +    regnd->map(old_reg, NULL); // This register's value is now unknown
>    }
>    return blk_adjust;
>  }
> @@ -161,7 +161,7 @@ int PhaseChaitin::yank_if_dead_recurse(Node *old, Node *orig_old, Block *current
>  // Use the prior value instead of the current value, in an effort to make
>  // the current value go dead.  Return block iterator adjustment, in case
>  // we yank some instructions from this block.
> -int PhaseChaitin::use_prior_register( Node *n, uint idx, Node *def, Block *current_block, Node_List &value, Node_List &regnd ) {
> +int PhaseChaitin::use_prior_register( Node *n, uint idx, Node *def, Block *current_block, Node_List *value, Node_List *regnd ) {
>    // No effect?
>    if( def == n->in(idx) ) return 0;
>    // Def is currently dead and can be removed?  Do not resurrect
> @@ -207,7 +207,7 @@ int PhaseChaitin::use_prior_register( Node *n, uint idx, Node *def, Block *curre
>    _post_alloc++;
>  
>    // Is old def now dead?  We successfully yanked a copy?
> -  return yank_if_dead(old,current_block,&value,&regnd);
> +  return yank_if_dead(old,current_block,value,regnd);
>  }
>  
>  
> @@ -229,7 +229,7 @@ Node *PhaseChaitin::skip_copies( Node *c ) {
>  
>  //------------------------------elide_copy-------------------------------------
>  // Remove (bypass) copies along Node n, edge k.
> -int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List &value, Node_List &regnd, bool can_change_regs ) {
> +int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List *value, Node_List *regnd, bool can_change_regs ) {
>    int blk_adjust = 0;
>  
>    uint nk_idx = _lrg_map.live_range_id(n->in(k));
> @@ -253,12 +253,13 @@ int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List &v
>  
>    // Phis and 2-address instructions cannot change registers so easily - their
>    // outputs must match their input.
> -  if( !can_change_regs )
> +  if (!can_change_regs) {
>      return blk_adjust;          // Only check stupid copies!
> -
> +  }
>    // Loop backedges won't have a value-mapping yet
> -  if( &value == NULL ) return blk_adjust;
> -
> +  if (value == NULL) {
> +    return blk_adjust;
> +  }
>    // Skip through all copies to the _value_ being used.  Do not change from
>    // int to pointer.  This attempts to jump through a chain of copies, where
>    // intermediate copies might be illegal, i.e., value is stored down to stack
> @@ -273,10 +274,11 @@ int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List &v
>    // See if it happens to already be in the correct register!
>    // (either Phi's direct register, or the common case of the name
>    // never-clobbered original-def register)
> -  if (register_contains_value(val, val_reg, n_regs, value)) {
> -    blk_adjust += use_prior_register(n,k,regnd[val_reg],current_block,value,regnd);
> -    if( n->in(k) == regnd[val_reg] ) // Success!  Quit trying
> -      return blk_adjust;
> +  if (register_contains_value(val, val_reg, n_regs, *value)) {
> +    blk_adjust += use_prior_register(n,k,regnd->at(val_reg),current_block,value,regnd);
> +    if (n->in(k) == regnd->at(val_reg)) {
> +      return blk_adjust; // Success!  Quit trying
> +    }
>    }
>  
>    // See if we can skip the copy by changing registers.  Don't change from
> @@ -304,7 +306,7 @@ int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List &v
>        if (ignore_self) continue;
>      }
>  
> -    Node *vv = value[reg];
> +    Node *vv = value->at(reg);
>      // For scalable register, number of registers may be inconsistent between
>      // "val_reg" and "reg". For example, when "val" resides in register
>      // but "reg" is located in stack.
> @@ -325,7 +327,7 @@ int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List &v
>          last = (n_regs-1); // Looking for the last part of a set
>        }
>        if ((reg&last) != last) continue; // Wrong part of a set
> -      if (!register_contains_value(vv, reg, n_regs, value)) continue; // Different value
> +      if (!register_contains_value(vv, reg, n_regs, *value)) continue; // Different value
>      }
>      if( vv == val ||            // Got a direct hit?
>          (t && vv && vv->bottom_type() == t && vv->is_Mach() &&
> @@ -333,9 +335,9 @@ int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List &v
>        assert( !n->is_Phi(), "cannot change registers at a Phi so easily" );
>        if( OptoReg::is_stack(nk_reg) || // CISC-loading from stack OR
>            OptoReg::is_reg(reg) || // turning into a register use OR
> -          regnd[reg]->outcnt()==1 ) { // last use of a spill-load turns into a CISC use
> -        blk_adjust += use_prior_register(n,k,regnd[reg],current_block,value,regnd);
> -        if( n->in(k) == regnd[reg] ) // Success!  Quit trying
> +          regnd->at(reg)->outcnt()==1 ) { // last use of a spill-load turns into a CISC use
> +        blk_adjust += use_prior_register(n,k,regnd->at(reg),current_block,value,regnd);
> +        if( n->in(k) == regnd->at(reg) ) // Success!  Quit trying
>            return blk_adjust;
>        } // End of if not degrading to a stack
>      } // End of if found value in another register
> @@ -535,7 +537,7 @@ void PhaseChaitin::post_allocate_copy_removal() {
>        Block* pb = _cfg.get_block_for_node(block->pred(j));
>        // Remove copies along phi edges
>        for (uint k = 1; k < phi_dex; k++) {
> -        elide_copy(block->get_node(k), j, block, *blk2value[pb->_pre_order], *blk2regnd[pb->_pre_order], false);
> +        elide_copy(block->get_node(k), j, block, blk2value[pb->_pre_order], blk2regnd[pb->_pre_order], false);
>        }
>        if (blk2value[pb->_pre_order]) { // Have a mapping on this edge?
>          // See if this predecessor's mappings have been used by everybody
> @@ -691,7 +693,7 @@ void PhaseChaitin::post_allocate_copy_removal() {
>  
>        // Remove copies along input edges
>        for (k = 1; k < n->req(); k++) {
> -        j -= elide_copy(n, k, block, value, regnd, two_adr != k);
> +        j -= elide_copy(n, k, block, &value, &regnd, two_adr != k);
>        }
>  
>        // Unallocated Nodes define no registers

Sorry, missed a couple of null checks.  The following patch on top of the previous one passes hs-tier1/2:

diff --git a/src/hotspot/share/opto/postaloc.cpp b/src/hotspot/share/opto/postaloc.cpp
index 10c9d1f90ae..b39a78eef48 100644
--- a/src/hotspot/share/opto/postaloc.cpp
+++ b/src/hotspot/share/opto/postaloc.cpp
@@ -87,7 +87,8 @@ int PhaseChaitin::yank(Node *old, Block *current_block, Node_List *value, Node_L
   }
   _cfg.unmap_node_from_block(old);
   OptoReg::Name old_reg = lrgs(_lrg_map.live_range_id(old)).reg();
-  if (regnd != NULL && regnd->at(old_reg) == old) { // Instruction is currently available?
+  assert(value != NULL || regnd == NULL, "sanity");
+  if (value != NULL && regnd != NULL && regnd->at(old_reg) == old) { // Instruction is currently available?
     value->map(old_reg, NULL); // Yank from value/regnd maps
     regnd->map(old_reg, NULL); // This register's value is now unknown
   }
@@ -257,7 +258,8 @@ int PhaseChaitin::elide_copy( Node *n, int k, Block *current_block, Node_List *v
     return blk_adjust;          // Only check stupid copies!
   }
   // Loop backedges won't have a value-mapping yet
-  if (value == NULL) {
+  assert(regnd != NULL || value == NULL, "sanity");
+  if (value == NULL || regnd == NULL) {
     return blk_adjust;
   }
   // Skip through all copies to the _value_ being used.  Do not change from

-------------

PR: https://git.openjdk.org/jdk/pull/10920

More information about the hotspot-dev mailing list