RFR: JDK-8294902: Undefined Behavior in C2 regalloc with null references [v4]
Andrew Haley
aph at openjdk.org
Wed Nov 30 18:38:46 UTC 2022
On Thu, 3 Nov 2022 23:57:01 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:
>> 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
>
> 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
Done. If you're happy with this I'll push after tests. Thanks!
-------------
PR: https://git.openjdk.org/jdk/pull/10920
More information about the hotspot-dev
mailing list