From ea734e2ac201fabf1d025fcced9832dc01a9db23 Mon Sep 17 00:00:00 2001 From: Dmitry Stogov Date: Mon, 4 Sep 2017 19:11:17 +0300 Subject: [PATCH] Fixed incorect constant conditional jump elimination --- NEWS | 3 + ext/opcache/Optimizer/block_pass.c | 46 ++++++++++++++ ext/opcache/Optimizer/sccp.c | 87 ++++++++++++++++++++++++++ ext/opcache/Optimizer/zend_optimizer.c | 34 ---------- ext/opcache/Optimizer/zend_ssa.c | 86 +++++++++++++------------ ext/opcache/Optimizer/zend_ssa.h | 1 + ext/opcache/tests/jmp_elim_001.phpt | 18 ++++++ 7 files changed, 202 insertions(+), 73 deletions(-) create mode 100644 ext/opcache/tests/jmp_elim_001.phpt diff --git a/NEWS b/NEWS index f5c9cac6de..4fcd94ff20 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,9 @@ PHP NEWS - GD: . Fixed bug #75139 (libgd/gd_interpolation.c:1786: suspicious if ?). (cmb) +- Opcache + . Fixed incorect constant conditional jump elimination. (Dmitry) + - OpenSSL . Automatically load OpenSSL configuration file. (Jakub Zelenka) diff --git a/ext/opcache/Optimizer/block_pass.c b/ext/opcache/Optimizer/block_pass.c index 41934f192b..738e283806 100644 --- a/ext/opcache/Optimizer/block_pass.c +++ b/ext/opcache/Optimizer/block_pass.c @@ -195,6 +195,52 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array VAR_SOURCE(op1) = NULL; literal_dtor(&ZEND_OP1_LITERAL(src)); MAKE_NOP(src); + switch (opline->opcode) { + case ZEND_JMPZ: + if (zend_is_true(&ZEND_OP1_LITERAL(opline))) { + MAKE_NOP(opline); + DEL_SOURCE(block, block->successors[0]); + block->successors_count = 1; + block->successors[0] = block->successors[1]; + } else { + opline->opcode = ZEND_JMP; + COPY_NODE(opline->op1, opline->op2); + DEL_SOURCE(block, block->successors[1]); + block->successors_count = 1; + } + break; + case ZEND_JMPNZ: + if (zend_is_true(&ZEND_OP1_LITERAL(opline))) { + opline->opcode = ZEND_JMP; + COPY_NODE(opline->op1, opline->op2); + DEL_SOURCE(block, block->successors[1]); + block->successors_count = 1; + } else { + MAKE_NOP(opline); + DEL_SOURCE(block, block->successors[0]); + block->successors_count = 1; + block->successors[0] = block->successors[1]; + } + break; + case ZEND_JMPZNZ: + if (zend_is_true(&ZEND_OP1_LITERAL(opline))) { + zend_op *target_opline = ZEND_OFFSET_TO_OPLINE(opline, opline->extended_value); + ZEND_SET_OP_JMP_ADDR(opline, opline->op1, target_opline); + DEL_SOURCE(block, block->successors[0]); + block->successors[0] = block->successors[1]; + } else { + zend_op *target_opline = ZEND_OP2_JMP_ADDR(opline); + ZEND_SET_OP_JMP_ADDR(opline, opline->op1, target_opline); + DEL_SOURCE(block, block->successors[0]); + } + block->successors_count = 1; + opline->op1_type = IS_UNUSED; + opline->extended_value = 0; + opline->opcode = ZEND_JMP; + break; + default: + break; + } } else { zval_ptr_dtor_nogc(&c); } diff --git a/ext/opcache/Optimizer/sccp.c b/ext/opcache/Optimizer/sccp.c index 24af78ee5d..e061e209c7 100644 --- a/ext/opcache/Optimizer/sccp.c +++ b/ext/opcache/Optimizer/sccp.c @@ -234,6 +234,93 @@ static zend_bool try_replace_op1( zval zv; ZVAL_COPY(&zv, value); if (zend_optimizer_update_op1_const(ctx->scdf.op_array, opline, &zv)) { + /* Reconstruct SSA */ + int num; + zend_basic_block *block; + + switch (opline->opcode) { + case ZEND_JMPZ: + if (zend_is_true(&zv)) { + MAKE_NOP(opline); + num = ctx->scdf.ssa->cfg.map[opline - ctx->scdf.op_array->opcodes]; + block = &ctx->scdf.ssa->cfg.blocks[num]; + if (block->successors_count == 2) { + if (block->successors[1] != block->successors[0]) { + zend_ssa_remove_predecessor(ctx->scdf.ssa, num, block->successors[0]); + } + block->successors_count = 1; + block->successors[0] = block->successors[1]; + } + } else { + opline->opcode = ZEND_JMP; + COPY_NODE(opline->op1, opline->op2); + num = ctx->scdf.ssa->cfg.map[opline - ctx->scdf.op_array->opcodes]; + block = &ctx->scdf.ssa->cfg.blocks[num]; + if (block->successors_count == 2) { + if (block->successors[1] != block->successors[0]) { + zend_ssa_remove_predecessor(ctx->scdf.ssa, num, block->successors[1]); + } + block->successors_count = 1; + } + } + break; + case ZEND_JMPNZ: + if (zend_is_true(&zv)) { + opline->opcode = ZEND_JMP; + COPY_NODE(opline->op1, opline->op2); + num = ctx->scdf.ssa->cfg.map[opline - ctx->scdf.op_array->opcodes]; + block = &ctx->scdf.ssa->cfg.blocks[num]; + if (block->successors_count == 2) { + if (block->successors[1] != block->successors[0]) { + zend_ssa_remove_predecessor(ctx->scdf.ssa, num, block->successors[1]); + } + block->successors_count = 1; + } + } else { + MAKE_NOP(opline); + num = ctx->scdf.ssa->cfg.map[opline - ctx->scdf.op_array->opcodes]; + block = &ctx->scdf.ssa->cfg.blocks[num]; + if (block->successors_count == 2) { + if (block->successors[1] != block->successors[0]) { + zend_ssa_remove_predecessor(ctx->scdf.ssa, num, block->successors[0]); + } + block->successors_count = 1; + block->successors[0] = block->successors[1]; + } + } + break; + case ZEND_JMPZNZ: + if (zend_is_true(&zv)) { + zend_op *target_opline = ZEND_OFFSET_TO_OPLINE(opline, opline->extended_value); + ZEND_SET_OP_JMP_ADDR(opline, opline->op1, target_opline); + num = ctx->scdf.ssa->cfg.map[opline - ctx->scdf.op_array->opcodes]; + block = &ctx->scdf.ssa->cfg.blocks[num]; + if (block->successors_count == 2) { + if (block->successors[1] != block->successors[0]) { + zend_ssa_remove_predecessor(ctx->scdf.ssa, num, block->successors[0]); + } + block->successors_count = 1; + block->successors[0] = block->successors[1]; + } + } else { + zend_op *target_opline = ZEND_OP2_JMP_ADDR(opline); + ZEND_SET_OP_JMP_ADDR(opline, opline->op1, target_opline); + num = ctx->scdf.ssa->cfg.map[opline - ctx->scdf.op_array->opcodes]; + block = &ctx->scdf.ssa->cfg.blocks[num]; + if (block->successors_count == 2) { + if (block->successors[1] != block->successors[0]) { + zend_ssa_remove_predecessor(ctx->scdf.ssa, num, block->successors[1]); + } + block->successors_count = 1; + } + } + opline->op1_type = IS_UNUSED; + opline->extended_value = 0; + opline->opcode = ZEND_JMP; + break; + default: + break; + } return 1; } else { // TODO: check the following special cases ??? diff --git a/ext/opcache/Optimizer/zend_optimizer.c b/ext/opcache/Optimizer/zend_optimizer.c index f8f42a1277..cc197c0e52 100644 --- a/ext/opcache/Optimizer/zend_optimizer.c +++ b/ext/opcache/Optimizer/zend_optimizer.c @@ -258,41 +258,7 @@ int zend_optimizer_update_op1_const(zend_op_array *op_array, zend_op *opline, zval *val) { - zend_op *target_opline; - switch (opline->opcode) { - case ZEND_JMPZ: - if (zend_is_true(val)) { - MAKE_NOP(opline); - } else { - opline->opcode = ZEND_JMP; - COPY_NODE(opline->op1, opline->op2); - opline->op2_type = IS_UNUSED; - } - zval_ptr_dtor_nogc(val); - return 1; - case ZEND_JMPNZ: - if (zend_is_true(val)) { - opline->opcode = ZEND_JMP; - COPY_NODE(opline->op1, opline->op2); - opline->op2_type = IS_UNUSED; - } else { - MAKE_NOP(opline); - } - zval_ptr_dtor_nogc(val); - return 1; - case ZEND_JMPZNZ: - if (zend_is_true(val)) { - target_opline = ZEND_OFFSET_TO_OPLINE(opline, opline->extended_value); - } else { - target_opline = ZEND_OP2_JMP_ADDR(opline); - } - ZEND_SET_OP_JMP_ADDR(opline, opline->op1, target_opline); - opline->op1_type = IS_UNUSED; - opline->extended_value = 0; - opline->opcode = ZEND_JMP; - zval_ptr_dtor_nogc(val); - return 1; case ZEND_FREE: MAKE_NOP(opline); zval_ptr_dtor_nogc(val); diff --git a/ext/opcache/Optimizer/zend_ssa.c b/ext/opcache/Optimizer/zend_ssa.c index e100be426c..3811165948 100644 --- a/ext/opcache/Optimizer/zend_ssa.c +++ b/ext/opcache/Optimizer/zend_ssa.c @@ -1339,6 +1339,52 @@ void zend_ssa_remove_uses_of_var(zend_ssa *ssa, int var_num) /* {{{ */ } /* }}} */ +void zend_ssa_remove_predecessor(zend_ssa *ssa, int from, int to) /* {{{ */ +{ + zend_basic_block *next_block = &ssa->cfg.blocks[to]; + zend_ssa_block *next_ssa_block = &ssa->blocks[to]; + zend_ssa_phi *phi; + int j; + + /* Find at which predecessor offset this block is referenced */ + int pred_offset = -1; + int *predecessors = &ssa->cfg.predecessors[next_block->predecessor_offset]; + + for (j = 0; j < next_block->predecessors_count; j++) { + if (predecessors[j] == from) { + pred_offset = j; + break; + } + } + + /* If there are duplicate successors, the predecessors may have been removed in + * a previous iteration already. */ + if (pred_offset == -1) { + return; + } + + /* For phis in successor blocks, remove the operands associated with this block */ + for (phi = next_ssa_block->phis; phi; phi = phi->next) { + if (phi->pi >= 0) { + if (phi->pi == from) { + zend_ssa_remove_uses_of_var(ssa, phi->ssa_var); + zend_ssa_remove_phi(ssa, phi); + } + } else { + ZEND_ASSERT(phi->sources[pred_offset] >= 0); + zend_ssa_remove_phi_source(ssa, phi, pred_offset, next_block->predecessors_count); + } + } + + /* Remove this predecessor */ + next_block->predecessors_count--; + if (pred_offset < next_block->predecessors_count) { + predecessors = &ssa->cfg.predecessors[next_block->predecessor_offset + pred_offset]; + memmove(predecessors, predecessors + 1, (next_block->predecessors_count - pred_offset) * sizeof(uint32_t)); + } +} +/* }}} */ + void zend_ssa_remove_block(zend_op_array *op_array, zend_ssa *ssa, int i) /* {{{ */ { zend_basic_block *block = &ssa->cfg.blocks[i]; @@ -1369,45 +1415,7 @@ void zend_ssa_remove_block(zend_op_array *op_array, zend_ssa *ssa, int i) /* {{{ } for (s = 0; s < block->successors_count; s++) { - zend_basic_block *next_block = &ssa->cfg.blocks[block->successors[s]]; - zend_ssa_block *next_ssa_block = &ssa->blocks[block->successors[s]]; - zend_ssa_phi *phi; - - /* Find at which predecessor offset this block is referenced */ - int pred_offset = -1; - predecessors = &ssa->cfg.predecessors[next_block->predecessor_offset]; - for (j = 0; j < next_block->predecessors_count; j++) { - if (predecessors[j] == i) { - pred_offset = j; - break; - } - } - - /* If there are duplicate successors, the predecessors may have been removed in - * a previous iteration already. */ - if (pred_offset == -1) { - continue; - } - - /* For phis in successor blocks, remove the operands associated with this block */ - for (phi = next_ssa_block->phis; phi; phi = phi->next) { - if (phi->pi >= 0) { - if (phi->pi == i) { - zend_ssa_remove_uses_of_var(ssa, phi->ssa_var); - zend_ssa_remove_phi(ssa, phi); - } - } else { - ZEND_ASSERT(phi->sources[pred_offset] >= 0); - zend_ssa_remove_phi_source(ssa, phi, pred_offset, next_block->predecessors_count); - } - } - - /* Remove this predecessor */ - next_block->predecessors_count--; - if (pred_offset < next_block->predecessors_count) { - predecessors = &ssa->cfg.predecessors[next_block->predecessor_offset + pred_offset]; - memmove(predecessors, predecessors + 1, (next_block->predecessors_count - pred_offset) * sizeof(uint32_t)); - } + zend_ssa_remove_predecessor(ssa, i, block->successors[s]); } /* Remove successors of predecessors */ diff --git a/ext/opcache/Optimizer/zend_ssa.h b/ext/opcache/Optimizer/zend_ssa.h index f5855f2453..b281305de2 100644 --- a/ext/opcache/Optimizer/zend_ssa.h +++ b/ext/opcache/Optimizer/zend_ssa.h @@ -139,6 +139,7 @@ int zend_build_ssa(zend_arena **arena, const zend_script *script, const zend_op_ int zend_ssa_compute_use_def_chains(zend_arena **arena, const zend_op_array *op_array, zend_ssa *ssa); int zend_ssa_unlink_use_chain(zend_ssa *ssa, int op, int var); +void zend_ssa_remove_predecessor(zend_ssa *ssa, int from, int to); void zend_ssa_remove_instr(zend_ssa *ssa, zend_op *opline, zend_ssa_op *ssa_op); void zend_ssa_remove_phi(zend_ssa *ssa, zend_ssa_phi *phi); void zend_ssa_remove_uses_of_var(zend_ssa *ssa, int var_num); diff --git a/ext/opcache/tests/jmp_elim_001.phpt b/ext/opcache/tests/jmp_elim_001.phpt new file mode 100644 index 0000000000..349816e3c3 --- /dev/null +++ b/ext/opcache/tests/jmp_elim_001.phpt @@ -0,0 +1,18 @@ +--TEST-- +Edge-cases in constant conditional jump elimination +--SKIPIF-- + + +--FILE-- + PHP_OS, + 'phpversion' => phpversion(), + 'arch' => $archName, + 'webserver' =>$webserver); +header('Content-Type: application/json'); +echo json_encode($info) . "\n"; +--EXPECT-- +{"os":"Linux","phpversion":"7.2.0-dev","arch":"64","webserver":"Apache"} -- 2.40.0