* [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c
@ 2019-01-10 7:19 Alan Modra
2019-01-11 18:42 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2019-01-10 7:19 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Sandiford, Jeff Law
bb-reorder is quite seriously broken if get_attr_min_length should
return INT_MAX, which it does for hppa on branches with r267666.
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.
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;
}
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c
2019-01-10 7:19 [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c Alan Modra
@ 2019-01-11 18:42 ` Jeff Law
2019-01-11 19:50 ` Richard Sandiford
2019-01-11 23:45 ` Alan Modra
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Law @ 2019-01-11 18:42 UTC (permalink / raw)
To: Alan Modra, gcc-patches; +Cc: Richard Sandiford
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c
2019-01-11 18:42 ` Jeff Law
@ 2019-01-11 19:50 ` Richard Sandiford
2019-01-12 2:40 ` Alan Modra
2019-01-11 23:45 ` Alan Modra
1 sibling, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2019-01-11 19:50 UTC (permalink / raw)
To: Jeff Law; +Cc: Alan Modra, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c
2019-01-11 19:50 ` Richard Sandiford
@ 2019-01-12 2:40 ` Alan Modra
0 siblings, 0 replies; 6+ messages in thread
From: Alan Modra @ 2019-01-12 2:40 UTC (permalink / raw)
To: Jeff Law, gcc-patches, richard.sandiford
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.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c
2019-01-11 18:42 ` Jeff Law
2019-01-11 19:50 ` Richard Sandiford
@ 2019-01-11 23:45 ` Alan Modra
2019-06-03 17:35 ` Jeff Law
1 sibling, 1 reply; 6+ messages in thread
From: Alan Modra @ 2019-01-11 23:45 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, Richard Sandiford
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;
}
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c
2019-01-11 23:45 ` Alan Modra
@ 2019-06-03 17:35 ` Jeff Law
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Law @ 2019-06-03 17:35 UTC (permalink / raw)
To: Alan Modra; +Cc: gcc-patches, Richard Sandiford
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-03 17:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-10 7:19 [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c Alan Modra
2019-01-11 18:42 ` Jeff Law
2019-01-11 19:50 ` Richard Sandiford
2019-01-12 2:40 ` Alan Modra
2019-01-11 23:45 ` Alan Modra
2019-06-03 17:35 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).