[PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[Alan Modra]

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

Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[Jeff Law]

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

Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[Richard Sandiford]

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

Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[Alan Modra]

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

Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[Alan Modra]

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

Re: [PATCH] PR88777, Out-of-range offsets building glibc test-tgmath2.c

[Jeff Law]

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