-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8324833: Signed integer overflows in ABS #17617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5d548e3
7d0ea58
f110e39
91d5302
e9333b6
d7b951c
074679a
00d4d84
96a1987
18dd071
272fb16
b942242
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -802,14 +802,14 @@ bool PhaseIdealLoop::create_loop_nest(IdealLoopTree* loop, Node_List &old_new) { | |
| // The number of iterations for the integer count loop: guarantee no | ||
| // overflow: max_jint - stride_con max. -1 so there's no need for a | ||
| // loop limit check if the exit test is <= or >=. | ||
| int iters_limit = max_jint - ABS(stride_con) - 1; | ||
| int iters_limit = max_jint - uabs(stride_con) - 1; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears iters_limit can become -1 or -2 here, depending on the value of stride_con. See below for the problem. |
||
| #ifdef ASSERT | ||
| if (bt == T_LONG && StressLongCountedLoop > 0) { | ||
| iters_limit = iters_limit / StressLongCountedLoop; | ||
| } | ||
| #endif | ||
| // At least 2 iterations so counted loop construction doesn't fail | ||
| if (iters_limit/ABS(stride_con) < 2) { | ||
| if (iters_limit/uabs(stride_con) < 2) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously negative values of iters_limit would cause us to return false here. Now the comparison is unsigned, so we can get a different result: |
||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -1106,8 +1106,8 @@ int PhaseIdealLoop::extract_long_range_checks(const IdealLoopTree* loop, jlong s | |
| jlong scale = 0; | ||
| if (loop->is_range_check_if(if_proj, this, T_LONG, phi, range, offset, scale) && | ||
| loop->is_invariant(range) && loop->is_invariant(offset) && | ||
| original_iters_limit / ABS(scale * stride_con) >= min_iters) { | ||
| reduced_iters_limit = MIN2(reduced_iters_limit, original_iters_limit/ABS(scale)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixing signed and unsigned can cause compiler warnings and a change in semantics. And again, checked_cast is not better than a raw cast here. I think it would be better to make min_iters, reduced_iters_limit, original_iters_limit all unsigned. |
||
| original_iters_limit / uabs(scale * stride_con) >= min_iters) { | ||
| reduced_iters_limit = MIN2(reduced_iters_limit, checked_cast<jlong>(original_iters_limit / uabs(scale))); | ||
| range_checks.push(c); | ||
| } | ||
| } | ||
|
|
@@ -2341,7 +2341,7 @@ Node* PhaseIdealLoop::exact_limit( IdealLoopTree *loop ) { | |
| CountedLoopNode *cl = loop->_head->as_CountedLoop(); | ||
| assert(cl->is_valid_counted_loop(T_INT), ""); | ||
|
|
||
| if (ABS(cl->stride_con()) == 1 || | ||
| if (uabs(cl->stride_con()) == 1 || | ||
| cl->limit()->Opcode() == Op_LoopLimit) { | ||
| // Old code has exact limit (it could be incorrect in case of int overflow). | ||
| // Loop limit is exact with stride == 1. And loop may already have exact limit. | ||
|
|
@@ -2559,20 +2559,18 @@ Node *LoopLimitNode::Ideal(PhaseGVN *phase, bool can_reshape) { | |
|
|
||
| const TypeInt* init_t = phase->type(in(Init) )->is_int(); | ||
| const TypeInt* limit_t = phase->type(in(Limit))->is_int(); | ||
| int stride_p; | ||
| jlong lim, ini; | ||
| julong max; | ||
| if (stride_con > 0) { | ||
| stride_p = stride_con; | ||
| lim = limit_t->_hi; | ||
| ini = init_t->_lo; | ||
| max = (julong)max_jint; | ||
| } else { | ||
| stride_p = -stride_con; | ||
| lim = init_t->_hi; | ||
| ini = limit_t->_lo; | ||
| max = (julong)min_jint; | ||
| } | ||
| uint stride_p = uabs(stride_con); | ||
| julong range = lim - ini + stride_p; | ||
| if (range <= max) { | ||
| // Convert to integer expression if it is not overflow. | ||
|
|
@@ -2951,9 +2949,9 @@ void OuterStripMinedLoopNode::adjust_strip_mined_loop(PhaseIterGVN* igvn) { | |
| CountedLoopEndNode* inner_cle = inner_cl->loopexit(); | ||
|
|
||
| int stride = inner_cl->stride_con(); | ||
| jlong scaled_iters_long = ((jlong)LoopStripMiningIter) * ABS(stride); | ||
| jlong scaled_iters_long = ((jlong)LoopStripMiningIter) * uabs(stride); | ||
| int scaled_iters = (int)scaled_iters_long; | ||
| int short_scaled_iters = LoopStripMiningIterShortLoop* ABS(stride); | ||
| int short_scaled_iters = LoopStripMiningIterShortLoop * uabs(stride); | ||
| const TypeInt* inner_iv_t = igvn->type(inner_iv_phi)->is_int(); | ||
| jlong iter_estimate = (jlong)inner_iv_t->_hi - (jlong)inner_iv_t->_lo; | ||
| assert(iter_estimate > 0, "broken"); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.