RFR: JDK-8294902: Undefined Behavior in C2 regalloc with null references [v3]
Vladimir Ivanov
vlivanov at openjdk.org
Thu Nov 3 22:30:57 UTC 2022
On Thu, 3 Nov 2022 16:27:07 GMT, Andrew Haley <aph at openjdk.org> wrote:
>> src/hotspot/share/opto/node.hpp line 1528:
>>
>>> 1526: public:
>>> 1527: Node_Array(Arena* a, uint max = OptoNodeListSize) : _a(a), _max(max) {
>>> 1528: if (a != NULL) {
>>
>> Add `assert(a != NULL, "...")` here?
>
> 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 ®nd ) {
+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,®nd);
+ 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 ®nd, 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, ®nd, two_adr != k);
}
// Unallocated Nodes define no registers
-------------
PR: https://git.openjdk.org/jdk/pull/10920
More information about the hotspot-dev
mailing list