| Message ID | 20190110071913.GQ3170@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | PR88777, Out-of-range offsets building glibc test-tgmath2.c | expand |
On 1/10/19 12:19 AM, Alan Modra wrote: > bb-reorder is quite seriously broken if get_attr_min_length should > return INT_MAX, which it does for hppa on branches with r267666. Presumably you're referring to the overflows and such? > > I went the wrong way with my min_attr_value r267666 change. It seems > that it isn't important that a "can't calculate" value is returned > from recursive calls, but rather that it returns the minimum among > those the function can calculate, ie. a conservative minimum length. > That's what this patch does, going back to the behaviour of > min_attr_value prior to r267666, with an added comment to stop foolish > patches in future. > > Bootstrapped and regression tested powerpc64le-linux. OK? > > PR 88777 > PR 88614 > * genattrtab.c (min_fn): Don't translate values. > (min_attr_value): Return INT_MAX when the value can't be calculated. > Return minimum among any values that can be calculated. > (max_attr_value): Adjust. OK. Given you're likely done for the day I'm going to go ahead and install it momentarily to make my tester happy again :-) jeff
Jeff Law <law@redhat.com> writes: > On 1/10/19 12:19 AM, Alan Modra wrote: >> bb-reorder is quite seriously broken if get_attr_min_length should >> return INT_MAX, which it does for hppa on branches with r267666. > Presumably you're referring to the overflows and such? > > >> >> I went the wrong way with my min_attr_value r267666 change. It seems >> that it isn't important that a "can't calculate" value is returned >> from recursive calls, but rather that it returns the minimum among >> those the function can calculate, ie. a conservative minimum length. >> That's what this patch does, going back to the behaviour of >> min_attr_value prior to r267666, with an added comment to stop foolish >> patches in future. >> >> Bootstrapped and regression tested powerpc64le-linux. OK? >> >> PR 88777 >> PR 88614 >> * genattrtab.c (min_fn): Don't translate values. >> (min_attr_value): Return INT_MAX when the value can't be calculated. >> Return minimum among any values that can be calculated. >> (max_attr_value): Adjust. > OK. Given you're likely done for the day I'm going to go ahead and > install it momentarily to make my tester happy again :-) FWIW, I think this is papering over a deeper issue, but I guess it's OK to install anyway to unbreak builds. Was meaning to have a look today but got sidetracked onto other things, sorry. Thanks, Richard
On Fri, Jan 11, 2019 at 11:42:31AM -0700, Jeff Law wrote: > On 1/10/19 12:19 AM, Alan Modra wrote: > > bb-reorder is quite seriously broken if get_attr_min_length should > > return INT_MAX, which it does for hppa on branches with r267666. > Presumably you're referring to the overflows and such? Yes. Even get_uncond_jump_length would have been INT_MAX. All of the predicates deciding on whether to copy or reorder blocks were therefore broken. The following is fairly obvious and would stop some of the silliness, but I guess now is not the time to propose this sort of patch. * bb-reorder.c (copy_bb_p): Don't overflow size calculation. (get_uncond_jump_length): Assert length less than INT_MAX and non-negative. diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c index e4ae8b89c09..c21d204627e 100644 --- a/gcc/bb-reorder.c +++ b/gcc/bb-reorder.c @@ -1357,8 +1357,8 @@ connect_traces (int n_traces, struct trace *traces) static bool copy_bb_p (const_basic_block bb, int code_may_grow) { - int size = 0; - int max_size = uncond_jump_length; + unsigned int size = 0; + unsigned int max_size = uncond_jump_length; rtx_insn *insn; if (EDGE_COUNT (bb->preds) < 2) @@ -1376,7 +1376,11 @@ copy_bb_p (const_basic_block bb, int code_may_grow) FOR_BB_INSNS (bb, insn) { if (INSN_P (insn)) - size += get_attr_min_length (insn); + { + size += get_attr_min_length (insn); + if (size > max_size) + break; + } } if (size <= max_size) @@ -1385,7 +1389,7 @@ copy_bb_p (const_basic_block bb, int code_may_grow) if (dump_file) { fprintf (dump_file, - "Block %d can't be copied because its size = %d.\n", + "Block %d can't be copied because its size = %u.\n", bb->index, size); } @@ -1397,7 +1401,7 @@ copy_bb_p (const_basic_block bb, int code_may_grow) int get_uncond_jump_length (void) { - int length; + unsigned int length; start_sequence (); rtx_code_label *label = emit_label (gen_label_rtx ()); @@ -1405,6 +1409,7 @@ get_uncond_jump_length (void) length = get_attr_min_length (jump); end_sequence (); + gcc_assert (length < INT_MAX); return length; }
On Fri, Jan 11, 2019 at 07:49:54PM +0000, Richard Sandiford wrote:
> FWIW, I think this is papering over a deeper issue,
Most certainly. At the very least there's the fact that many places
in the compiler that call attr_min_value (via other functions) don't
bother checking for an INT_MAX return. I also didn't try to analyse
why bb-reorder.c making wrong decisions due to confused copy_bb_p and
similar predicates led to hppa not replacing short branches with
longer ones. So there are likely to be some final.c issues too.
On 1/11/19 4:44 PM, Alan Modra wrote: > On Fri, Jan 11, 2019 at 11:42:31AM -0700, Jeff Law wrote: >> On 1/10/19 12:19 AM, Alan Modra wrote: >>> bb-reorder is quite seriously broken if get_attr_min_length should >>> return INT_MAX, which it does for hppa on branches with r267666. >> Presumably you're referring to the overflows and such? > > Yes. Even get_uncond_jump_length would have been INT_MAX. All of > the predicates deciding on whether to copy or reorder blocks were > therefore broken. > > The following is fairly obvious and would stop some of the silliness, > but I guess now is not the time to propose this sort of patch. > > * bb-reorder.c (copy_bb_p): Don't overflow size calculation. > (get_uncond_jump_length): Assert length less than INT_MAX and > non-negative. Now seems like a good time to revisit. I ran this through the usual bootstrap and regression text on x86_64. It's also built and tested on a good variety of the embedded targets. Can't really test the PA right now due to what I believe is a qemu bug. David is still doing bootstraps on real hardware through, so if it causes a problem on the PA I'm sure he'll chime in at some point. I'm going to go ahead and install this on the trunk. jeff
diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c index 1dd4f142672..78816ba3179 100644 --- a/gcc/genattrtab.c +++ b/gcc/genattrtab.c @@ -1556,10 +1556,7 @@ max_fn (rtx exp) static rtx min_fn (rtx exp) { - int val = min_attr_value (exp); - if (val < 0) - val = INT_MAX; - return make_numeric_value (val); + return make_numeric_value (min_attr_value (exp)); } static void @@ -3786,11 +3783,10 @@ max_attr_value (rtx exp) current_max = max_attr_value (XEXP (exp, 0)); if (current_max != INT_MAX) { - n = min_attr_value (XEXP (exp, 1)); - if (n == INT_MIN) - current_max = INT_MAX; - else - current_max -= n; + n = current_max; + current_max = min_attr_value (XEXP (exp, 1)); + if (current_max != INT_MAX) + current_max = n - current_max; } break; @@ -3831,8 +3827,11 @@ max_attr_value (rtx exp) } /* Given an attribute value expression, return the minimum value that - might be evaluated. Return INT_MIN if the value can't be - calculated by this function. */ + might be evaluated. Return INT_MAX if the value can't be + calculated by this function. Note that when this function can + calculate one value inside IF_THEN_ELSE or some but not all values + inside COND, then it returns the minimum among those values it can + calculate. */ static int min_attr_value (rtx exp) @@ -3852,34 +3851,33 @@ min_attr_value (rtx exp) case PLUS: current_min = min_attr_value (XEXP (exp, 0)); - if (current_min != INT_MIN) + if (current_min != INT_MAX) { n = current_min; current_min = min_attr_value (XEXP (exp, 1)); - if (current_min != INT_MIN) + if (current_min != INT_MAX) current_min += n; } break; case MINUS: current_min = min_attr_value (XEXP (exp, 0)); - if (current_min != INT_MIN) + if (current_min != INT_MAX) { - n = max_attr_value (XEXP (exp, 1)); - if (n == INT_MAX) - current_min = INT_MIN; - else - current_min -= n; + n = current_min; + current_min = max_attr_value (XEXP (exp, 1)); + if (current_min != INT_MAX) + current_min = n - current_min; } break; case MULT: current_min = min_attr_value (XEXP (exp, 0)); - if (current_min != INT_MIN) + if (current_min != INT_MAX) { n = current_min; current_min = min_attr_value (XEXP (exp, 1)); - if (current_min != INT_MIN) + if (current_min != INT_MAX) current_min *= n; } break; @@ -3902,7 +3900,7 @@ min_attr_value (rtx exp) break; default: - current_min = INT_MIN; + current_min = INT_MAX; break; }