Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/14/2017 01:32 PM, Jakub Jelinek wrote:

On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote:

That comment explains how the likely_adjust variable ("the adjustment")
is being used, or more precisely, how it was being used in the first
version of the patch.  The comment became somewhat out of date with
the committed version of the patch (this was my bad).

The variable is documented where it's defined and again where it's
assigned to.  With the removal of those comments it seems especially
important that the only remaining description of what's going on be
accurate.

The comment is outdated because it refers to "the adjustment" which
doesn't exist anymore.  (It was replaced by a flag in my commit).
To bring it up to date it should say something like:

  /* Set the LIKELY counter to MIN.  In base 8 and 16, when
 the argument is in range that includes zero, adjust it
 upward to include the length of the base prefix since
 in that case the MIN counter does include it.  */


So for a comment, what about following then?  With or without
the IMNSHO useless
&& (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)


On a separate note, while testing the patch I noticed that it's
not exactly equivalent to what's on trunk.  Trunk silently accepts
the call below but with the patch it complains.  That's great (it
should complain) but the change should be tested.  More to my point,
while in this case your change happened to fix a subtle bug (which
I'm certainly happy about), it could have just as easily introduced
one.


Yeah, indeed.  That should be a clear argument for why writing it in
so many places is bad, it is simply much more error-prone, there are
too many cases to get right.


  char d[2];

  void f (unsigned i)
  {
if (i < 1234 || 12345 < i)
  i = 1234;

__builtin_sprintf (d, "%#hhx", i);
  }


What happens is that because the original range doesn't contain zero
you set likely_adjust to false and then never update it again because
the implicit cast changed the range.

If some version of the patch is approved, I'll leave addition of this
testcase to you (incrementally).

2017-02-14  Jakub Jelinek  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
variable, its initialization and use.
This is fine.  And the addition of the test from Martin is pre-approved 
as well.


jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/14/2017 06:43 AM, Jakub Jelinek wrote:

On Tue, Feb 14, 2017 at 08:18:13AM +0100, Jakub Jelinek wrote:

On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote:

dirtype is one of the standard {un,}signed {char,short,int,long,long long}
types, all of them have 0 in their ranges.
For VR_RANGE we almost always set res.knownrange to true:
  /* Set KNOWNRANGE if the argument is in a known subrange
 of the directive's type (KNOWNRANGE may be reset below).  */
  res.knownrange
= (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
   || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
(the exception is in case that range clearly has to include zero),
and reset it only if adjust_range_for_overflow returned true, which means
it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
includes zero.
So IMNSHO likely_adjust in what you've committed is always true
when you use it and thus just a useless computation and something to make
the code harder to understand.

If KNOWNRANGE is false, then LIKELY_ADJUST will be true.  But I don't see
how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true.


We can't, but that doesn't matter, we only use it if KNOWNRANGE is false.
The only user of LIKELY_ADJUST is:

  if (res.knownrange)
res.range.likely = res.range.max;
  else
{
// -- Here we know res.knownrage is false
  res.range.likely = res.range.min;
  if (likely_adjust && maybebase && base != 10)
// -- and here is the only user of likely_adjust
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;
  else if (res.range.min == 2
   && base == 16
   && (dir.width[0] == 2 || dir.prec[0] == 2))
++res.range.likely;
}
}

Duh.  You're obviously right.  Still reading the rest of the thread.

jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/14/2017 01:32 PM, Jakub Jelinek wrote:

On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote:

That comment explains how the likely_adjust variable ("the adjustment")
is being used, or more precisely, how it was being used in the first
version of the patch.  The comment became somewhat out of date with
the committed version of the patch (this was my bad).

The variable is documented where it's defined and again where it's
assigned to.  With the removal of those comments it seems especially
important that the only remaining description of what's going on be
accurate.

The comment is outdated because it refers to "the adjustment" which
doesn't exist anymore.  (It was replaced by a flag in my commit).
To bring it up to date it should say something like:

  /* Set the LIKELY counter to MIN.  In base 8 and 16, when
 the argument is in range that includes zero, adjust it
 upward to include the length of the base prefix since
 in that case the MIN counter does include it.  */


So for a comment, what about following then?  With or without
the IMNSHO useless
&& (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)


If the condition is redundant (it seems like it could be) it
shouldn't be included in the patch.  It seems like an opportunity
for further simplification.  I'm sure it's not the only one, either.


On a separate note, while testing the patch I noticed that it's
not exactly equivalent to what's on trunk.  Trunk silently accepts
the call below but with the patch it complains.  That's great (it
should complain) but the change should be tested.  More to my point,
while in this case your change happened to fix a subtle bug (which
I'm certainly happy about), it could have just as easily introduced
one.


Yeah, indeed.  That should be a clear argument for why writing it in
so many places is bad, it is simply much more error-prone, there are
too many cases to get right.


No argument there.  There's always room for improvement, cleanup,
or refactoring.

Martin




  char d[2];

  void f (unsigned i)
  {
if (i < 1234 || 12345 < i)
  i = 1234;

__builtin_sprintf (d, "%#hhx", i);
  }


What happens is that because the original range doesn't contain zero
you set likely_adjust to false and then never update it again because
the implicit cast changed the range.

If some version of the patch is approved, I'll leave addition of this
testcase to you (incrementally).

2017-02-14  Jakub Jelinek  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
variable, its initialization and use.

--- gcc/gimple-ssa-sprintf.c.jj 2017-02-14 21:21:56.048745037 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-14 21:25:20.939033174 +0100
@@ -1232,10 +1232,6 @@ format_integer (const directive , tr
of the format string by returning [-1, -1].  */
 return fmtresult ();

-  /* True if the LIKELY counter should be adjusted upward from the MIN
- counter to account for arguments with unknown values.  */
-  bool likely_adjust = false;
-
   fmtresult res;

   /* Using either the range the non-constant argument is in, or its
@@ -1265,14 +1261,6 @@ format_integer (const directive , tr

  res.argmin = argmin;
  res.argmax = argmax;
-
- /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
- wide_int wzero = wi::zero (wi::get_precision (min));
- if (wi::le_p (min, wzero, SIGNED)
- && !wi::neg_p (max))
-   likely_adjust = true;
}
   else if (range_type == VR_ANTI_RANGE)
{
@@ -1307,11 +1295,6 @@ format_integer (const directive , tr

   if (!argmin)
 {
-  /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
-  likely_adjust = true;
-
   if (TREE_CODE (argtype) == POINTER_TYPE)
{
  argmin = build_int_cst (pointer_sized_int_node, 0);
@@ -1364,14 +1347,19 @@ format_integer (const directive , tr
   res.range.max = MAX (max1, max2);
 }

-  /* Add the adjustment for an argument whose range includes zero
- since it doesn't include the octal or hexadecimal base prefix.  */
+  /* If the range is known, use the maximum as the likely length.  */
   if (res.knownrange)
 res.range.likely = res.range.max;
   else
 {
+  /* Otherwise, use the minimum.  Except for the case where for %#x or
+ %#o the minimum is just for a single value in the range (0) and
+ for all other values it is something longer, like 0x1 or 01.
+ Use the length for value 1 in that case instead as the likely
+ length.  */
   res.range.likely = res.range.min;
-  if (likely_adjust && maybebase && base != 10)
+  if (maybebase && base != 10
+ && (tree_int_cst_sgn (argmin) < 0 || 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote:
> That comment explains how the likely_adjust variable ("the adjustment")
> is being used, or more precisely, how it was being used in the first
> version of the patch.  The comment became somewhat out of date with
> the committed version of the patch (this was my bad).
> 
> The variable is documented where it's defined and again where it's
> assigned to.  With the removal of those comments it seems especially
> important that the only remaining description of what's going on be
> accurate.
> 
> The comment is outdated because it refers to "the adjustment" which
> doesn't exist anymore.  (It was replaced by a flag in my commit).
> To bring it up to date it should say something like:
> 
>   /* Set the LIKELY counter to MIN.  In base 8 and 16, when
>  the argument is in range that includes zero, adjust it
>  upward to include the length of the base prefix since
>  in that case the MIN counter does include it.  */

So for a comment, what about following then?  With or without
the IMNSHO useless
&& (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)

> On a separate note, while testing the patch I noticed that it's
> not exactly equivalent to what's on trunk.  Trunk silently accepts
> the call below but with the patch it complains.  That's great (it
> should complain) but the change should be tested.  More to my point,
> while in this case your change happened to fix a subtle bug (which
> I'm certainly happy about), it could have just as easily introduced
> one.

Yeah, indeed.  That should be a clear argument for why writing it in
so many places is bad, it is simply much more error-prone, there are
too many cases to get right.

>   char d[2];
> 
>   void f (unsigned i)
>   {
> if (i < 1234 || 12345 < i)
>   i = 1234;
> 
> __builtin_sprintf (d, "%#hhx", i);
>   }

What happens is that because the original range doesn't contain zero
you set likely_adjust to false and then never update it again because
the implicit cast changed the range.

If some version of the patch is approved, I'll leave addition of this
testcase to you (incrementally).

2017-02-14  Jakub Jelinek  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
variable, its initialization and use.

--- gcc/gimple-ssa-sprintf.c.jj 2017-02-14 21:21:56.048745037 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-14 21:25:20.939033174 +0100
@@ -1232,10 +1232,6 @@ format_integer (const directive , tr
of the format string by returning [-1, -1].  */
 return fmtresult ();
 
-  /* True if the LIKELY counter should be adjusted upward from the MIN
- counter to account for arguments with unknown values.  */
-  bool likely_adjust = false;
-
   fmtresult res;
 
   /* Using either the range the non-constant argument is in, or its
@@ -1265,14 +1261,6 @@ format_integer (const directive , tr
 
  res.argmin = argmin;
  res.argmax = argmax;
-
- /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
- wide_int wzero = wi::zero (wi::get_precision (min));
- if (wi::le_p (min, wzero, SIGNED)
- && !wi::neg_p (max))
-   likely_adjust = true;
}
   else if (range_type == VR_ANTI_RANGE)
{
@@ -1307,11 +1295,6 @@ format_integer (const directive , tr
 
   if (!argmin)
 {
-  /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
-  likely_adjust = true;
-
   if (TREE_CODE (argtype) == POINTER_TYPE)
{
  argmin = build_int_cst (pointer_sized_int_node, 0);
@@ -1364,14 +1347,19 @@ format_integer (const directive , tr
   res.range.max = MAX (max1, max2);
 }
 
-  /* Add the adjustment for an argument whose range includes zero
- since it doesn't include the octal or hexadecimal base prefix.  */
+  /* If the range is known, use the maximum as the likely length.  */
   if (res.knownrange)
 res.range.likely = res.range.max;
   else
 {
+  /* Otherwise, use the minimum.  Except for the case where for %#x or
+ %#o the minimum is just for a single value in the range (0) and
+ for all other values it is something longer, like 0x1 or 01.
+ Use the length for value 1 in that case instead as the likely
+ length.  */
   res.range.likely = res.range.min;
-  if (likely_adjust && maybebase && base != 10)
+  if (maybebase && base != 10
+ && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0))
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;


Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/14/2017 09:39 AM, Jakub Jelinek wrote:

On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote:

@@ -1371,7 +1354,8 @@ format_integer (const directive , tr
   else
 {
   res.range.likely = res.range.min;
-  if (likely_adjust && maybebase && base != 10)
+  if (maybebase && base != 10
+ && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0))
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;


You've removed all the comments that explain what's going on.  If
you must make the change (I see no justification for it now) please
at least document it.


I thought that is the:
  /* Add the adjustment for an argument whose range includes zero
 since it doesn't include the octal or hexadecimal base prefix.  */
comment above the if.


That comment explains how the likely_adjust variable ("the adjustment")
is being used, or more precisely, how it was being used in the first
version of the patch.  The comment became somewhat out of date with
the committed version of the patch (this was my bad).

The variable is documented where it's defined and again where it's
assigned to.  With the removal of those comments it seems especially
important that the only remaining description of what's going on be
accurate.

The comment is outdated because it refers to "the adjustment" which
doesn't exist anymore.  (It was replaced by a flag in my commit).
To bring it up to date it should say something like:

  /* Set the LIKELY counter to MIN.  In base 8 and 16, when
 the argument is in range that includes zero, adjust it
 upward to include the length of the base prefix since
 in that case the MIN counter does include it.  */

On a separate note, while testing the patch I noticed that it's
not exactly equivalent to what's on trunk.  Trunk silently accepts
the call below but with the patch it complains.  That's great (it
should complain) but the change should be tested.  More to my point,
while in this case your change happened to fix a subtle bug (which
I'm certainly happy about), it could have just as easily introduced
one.

  char d[2];

  void f (unsigned i)
  {
if (i < 1234 || 12345 < i)
  i = 1234;

__builtin_sprintf (d, "%#hhx", i);
  }

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote:
> > @@ -1371,7 +1354,8 @@ format_integer (const directive , tr
> >else
> >  {
> >res.range.likely = res.range.min;
> > -  if (likely_adjust && maybebase && base != 10)
> > +  if (maybebase && base != 10
> > + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0))
> > {
> >   if (res.range.min == 1)
> > res.range.likely += base == 8 ? 1 : 2;
> 
> You've removed all the comments that explain what's going on.  If
> you must make the change (I see no justification for it now) please
> at least document it.

I thought that is the:
  /* Add the adjustment for an argument whose range includes zero
 since it doesn't include the octal or hexadecimal base prefix.  */
comment above the if.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/14/2017 12:18 AM, Jakub Jelinek wrote:

On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote:

dirtype is one of the standard {un,}signed {char,short,int,long,long long}
types, all of them have 0 in their ranges.
For VR_RANGE we almost always set res.knownrange to true:
  /* Set KNOWNRANGE if the argument is in a known subrange
 of the directive's type (KNOWNRANGE may be reset below).  */
  res.knownrange
= (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
   || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
(the exception is in case that range clearly has to include zero),
and reset it only if adjust_range_for_overflow returned true, which means
it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
includes zero.
So IMNSHO likely_adjust in what you've committed is always true
when you use it and thus just a useless computation and something to make
the code harder to understand.

If KNOWNRANGE is false, then LIKELY_ADJUST will be true.  But I don't see
how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true.


We can't, but that doesn't matter, we only use it if KNOWNRANGE is false.
The only user of LIKELY_ADJUST is:

  if (res.knownrange)
res.range.likely = res.range.max;
  else
{
// -- Here we know res.knownrage is false
  res.range.likely = res.range.min;
  if (likely_adjust && maybebase && base != 10)
// -- and here is the only user of likely_adjust
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;
  else if (res.range.min == 2
   && base == 16
   && (dir.width[0] == 2 || dir.prec[0] == 2))
++res.range.likely;
}
}


Even if you don't trust this, with the ranges in argmin/argmax, it is
IMHO undesirable to set it differently at the different code paths,
if you want to check whether the final range includes zero and at least
one another value, just do
-  if (likely_adjust && maybebase && base != 10)
+  if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
   && maybebase && base != 10)
Though, it is useless both for the above reason and for the reason that you
actually do something only:

I'm not convinced it's useless, but it does seem advisable to bring test
down to where it's actually used and to bse it strictly on argmin/argmax.
Can you test a patch which does that?


That would then be (the only difference compared to the previous patch is
the last hunk) following.  I can surely test that, I'm still convinced it
would work equally if that
(tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
is just gcc_checking_assert.

2017-02-14  Jakub Jelinek  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
variable, its initialization and use.

--- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.0 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-04 08:45:33.173709580 +0100
@@ -1232,10 +1232,6 @@ format_integer (const directive , tr
of the format string by returning [-1, -1].  */
 return fmtresult ();

-  /* True if the LIKELY counter should be adjusted upward from the MIN
- counter to account for arguments with unknown values.  */
-  bool likely_adjust = false;
-
   fmtresult res;

   /* Using either the range the non-constant argument is in, or its
@@ -1265,14 +1261,6 @@ format_integer (const directive , tr

  res.argmin = argmin;
  res.argmax = argmax;
-
- /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
- wide_int wzero = wi::zero (wi::get_precision (min));
- if (wi::le_p (min, wzero, SIGNED)
- && !wi::neg_p (max))
-   likely_adjust = true;
}
   else if (range_type == VR_ANTI_RANGE)
{
@@ -1307,11 +1295,6 @@ format_integer (const directive , tr

   if (!argmin)
 {
-  /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
-  likely_adjust = true;
-
   if (TREE_CODE (argtype) == POINTER_TYPE)
{
  argmin = build_int_cst (pointer_sized_int_node, 0);
@@ -1371,7 +1354,8 @@ format_integer (const directive , tr
   else
 {
   res.range.likely = res.range.min;
-  if (likely_adjust && maybebase && base != 10)
+  if (maybebase && base != 10
+ && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0))
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;


You've removed all the comments that explain what's going on.  If
you must make the change (I see no justification for it now) please
at least document it.

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Tue, Feb 14, 2017 at 08:18:13AM +0100, Jakub Jelinek wrote:
> On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote:
> > > dirtype is one of the standard {un,}signed {char,short,int,long,long long}
> > > types, all of them have 0 in their ranges.
> > > For VR_RANGE we almost always set res.knownrange to true:
> > >   /* Set KNOWNRANGE if the argument is in a known subrange
> > >  of the directive's type (KNOWNRANGE may be reset below).  */
> > >   res.knownrange
> > > = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
> > >|| !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
> > > (the exception is in case that range clearly has to include zero),
> > > and reset it only if adjust_range_for_overflow returned true, which means
> > > it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
> > > includes zero.
> > > So IMNSHO likely_adjust in what you've committed is always true
> > > when you use it and thus just a useless computation and something to make
> > > the code harder to understand.
> > If KNOWNRANGE is false, then LIKELY_ADJUST will be true.  But I don't see
> > how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true.
> 
> We can't, but that doesn't matter, we only use it if KNOWNRANGE is false.
> The only user of LIKELY_ADJUST is:
>  
>   if (res.knownrange)
> res.range.likely = res.range.max;
>   else
> {
> // -- Here we know res.knownrage is false
>   res.range.likely = res.range.min;
>   if (likely_adjust && maybebase && base != 10)
> // -- and here is the only user of likely_adjust
> {
>   if (res.range.min == 1)
> res.range.likely += base == 8 ? 1 : 2;
>   else if (res.range.min == 2
>&& base == 16
>&& (dir.width[0] == 2 || dir.prec[0] == 2))
> ++res.range.likely;
> }
> }

Another argument I had was that if maybebase and base != 10, then
if the range does not include zero (if it ever could be !res.knownrange
in that case), then res.range.min won't be 1 and for base 16 won't be
even 2, because all the values in the range will include the 0 or 0x prefixes.
The only controversion then would be if the range was [0, 0], then
bumping res.range.likely would not be appropriate.  But such range is
really res.knownrange and never anything else.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote:
> > dirtype is one of the standard {un,}signed {char,short,int,long,long long}
> > types, all of them have 0 in their ranges.
> > For VR_RANGE we almost always set res.knownrange to true:
> >   /* Set KNOWNRANGE if the argument is in a known subrange
> >  of the directive's type (KNOWNRANGE may be reset below).  */
> >   res.knownrange
> > = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
> >|| !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
> > (the exception is in case that range clearly has to include zero),
> > and reset it only if adjust_range_for_overflow returned true, which means
> > it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
> > includes zero.
> > So IMNSHO likely_adjust in what you've committed is always true
> > when you use it and thus just a useless computation and something to make
> > the code harder to understand.
> If KNOWNRANGE is false, then LIKELY_ADJUST will be true.  But I don't see
> how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true.

We can't, but that doesn't matter, we only use it if KNOWNRANGE is false.
The only user of LIKELY_ADJUST is:
 
  if (res.knownrange)
res.range.likely = res.range.max;
  else
{
// -- Here we know res.knownrage is false
  res.range.likely = res.range.min;
  if (likely_adjust && maybebase && base != 10)
// -- and here is the only user of likely_adjust
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;
  else if (res.range.min == 2
   && base == 16
   && (dir.width[0] == 2 || dir.prec[0] == 2))
++res.range.likely;
}
}

> > Even if you don't trust this, with the ranges in argmin/argmax, it is
> > IMHO undesirable to set it differently at the different code paths,
> > if you want to check whether the final range includes zero and at least
> > one another value, just do
> > -  if (likely_adjust && maybebase && base != 10)
> > +  if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
> >&& maybebase && base != 10)
> > Though, it is useless both for the above reason and for the reason that you
> > actually do something only:
> I'm not convinced it's useless, but it does seem advisable to bring test
> down to where it's actually used and to bse it strictly on argmin/argmax.
> Can you test a patch which does that?

That would then be (the only difference compared to the previous patch is
the last hunk) following.  I can surely test that, I'm still convinced it
would work equally if that
(tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
is just gcc_checking_assert.

2017-02-14  Jakub Jelinek  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
variable, its initialization and use.

--- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.0 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-04 08:45:33.173709580 +0100
@@ -1232,10 +1232,6 @@ format_integer (const directive , tr
of the format string by returning [-1, -1].  */
 return fmtresult ();
 
-  /* True if the LIKELY counter should be adjusted upward from the MIN
- counter to account for arguments with unknown values.  */
-  bool likely_adjust = false;
-
   fmtresult res;
 
   /* Using either the range the non-constant argument is in, or its
@@ -1265,14 +1261,6 @@ format_integer (const directive , tr
 
  res.argmin = argmin;
  res.argmax = argmax;
-
- /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
- wide_int wzero = wi::zero (wi::get_precision (min));
- if (wi::le_p (min, wzero, SIGNED)
- && !wi::neg_p (max))
-   likely_adjust = true;
}
   else if (range_type == VR_ANTI_RANGE)
{
@@ -1307,11 +1295,6 @@ format_integer (const directive , tr
 
   if (!argmin)
 {
-  /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
-  likely_adjust = true;
-
   if (TREE_CODE (argtype) == POINTER_TYPE)
{
  argmin = build_int_cst (pointer_sized_int_node, 0);
@@ -1371,7 +1354,8 @@ format_integer (const directive , tr
   else
 {
   res.range.likely = res.range.min;
-  if (likely_adjust && maybebase && base != 10)
+  if (maybebase && base != 10
+ && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0))
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;


Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/04/2017 01:07 AM, Jakub Jelinek wrote:

On Fri, Feb 03, 2017 at 05:39:21PM +0100, Jakub Jelinek wrote:

Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html
patch, you would with my patch need just the tree_digits part,
and then the very last hunk in gimple-ssa-sprintf.c (with
likely_adjust &&
removed).  Because you do the adjustments only if !res.knownrange
and in that case you know argmin/argmax are actually dirtype's min/max,
so 0 must be in the range.


You've committed the patch unnecessarily complicated, see above.
The following gives the same testsuite result.

As you know, just getting the same testsuite result is not sufficient ;-)



dirtype is one of the standard {un,}signed {char,short,int,long,long long}
types, all of them have 0 in their ranges.
For VR_RANGE we almost always set res.knownrange to true:
  /* Set KNOWNRANGE if the argument is in a known subrange
 of the directive's type (KNOWNRANGE may be reset below).  */
  res.knownrange
= (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
   || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
(the exception is in case that range clearly has to include zero),
and reset it only if adjust_range_for_overflow returned true, which means
it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
includes zero.
So IMNSHO likely_adjust in what you've committed is always true
when you use it and thus just a useless computation and something to make
the code harder to understand.
If KNOWNRANGE is false, then LIKELY_ADJUST will be true.  But I don't 
see how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true.





Even if you don't trust this, with the ranges in argmin/argmax, it is
IMHO undesirable to set it differently at the different code paths,
if you want to check whether the final range includes zero and at least
one another value, just do
-  if (likely_adjust && maybebase && base != 10)
+  if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
   && maybebase && base != 10)
Though, it is useless both for the above reason and for the reason that you
actually do something only:
I'm not convinced it's useless, but it does seem advisable to bring test 
down to where it's actually used and to bse it strictly on 
argmin/argmax.  Can you test a patch which does that?



Jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Sat, Feb 04, 2017 at 09:07:23AM +0100, Jakub Jelinek wrote:
> You've committed the patch unnecessarily complicated, see above.
> The following gives the same testsuite result.
> 
> dirtype is one of the standard {un,}signed {char,short,int,long,long long}
> types, all of them have 0 in their ranges.
> For VR_RANGE we almost always set res.knownrange to true:
>   /* Set KNOWNRANGE if the argument is in a known subrange
>  of the directive's type (KNOWNRANGE may be reset below).  */
>   res.knownrange
> = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
>|| !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
> (the exception is in case that range clearly has to include zero),
> and reset it only if adjust_range_for_overflow returned true, which means
> it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
> includes zero.
> So IMNSHO likely_adjust in what you've committed is always true
> when you use it and thus just a useless computation and something to make
> the code harder to understand.
> 
> Even if you don't trust this, with the ranges in argmin/argmax, it is
> IMHO undesirable to set it differently at the different code paths,
> if you want to check whether the final range includes zero and at least
> one another value, just do
> -  if (likely_adjust && maybebase && base != 10)
> +  if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
>  && maybebase && base != 10)
> Though, it is useless both for the above reason and for the reason that you
> actually do something only:
>   if (res.range.min == 1)
> res.range.likely += base == 8 ? 1 : 2;
>   else if (res.range.min == 2
>&& base == 16
>&& (dir.width[0] == 2 || dir.prec[0] == 2))
> ++res.range.likely;
> where if the range doesn't include zero, you would never get
> res.range.min of 1 and for base == 16 also not 2.
> 
> 2017-02-04  Jakub Jelinek  
> 
>   PR tree-optimization/79327
>   * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
>   variable, its initialization and use.

Now bootstrapped/regtested on x86_64-linux and i686-linux (ignore the
testsuite/ChangeLog part, I've committed that part in another commit already),
ok for trunk?

> --- gcc/gimple-ssa-sprintf.c.jj   2017-02-04 08:43:12.0 +0100
> +++ gcc/gimple-ssa-sprintf.c  2017-02-04 08:45:33.173709580 +0100
> @@ -1232,10 +1232,6 @@ format_integer (const directive , tr
> of the format string by returning [-1, -1].  */
>  return fmtresult ();
>  
> -  /* True if the LIKELY counter should be adjusted upward from the MIN
> - counter to account for arguments with unknown values.  */
> -  bool likely_adjust = false;
> -
>fmtresult res;
>  
>/* Using either the range the non-constant argument is in, or its
> @@ -1265,14 +1261,6 @@ format_integer (const directive , tr
>  
> res.argmin = argmin;
> res.argmax = argmax;
> -
> -   /* Set the adjustment for an argument whose range includes
> -  zero since that doesn't include the octal or hexadecimal
> -  base prefix.  */
> -   wide_int wzero = wi::zero (wi::get_precision (min));
> -   if (wi::le_p (min, wzero, SIGNED)
> -   && !wi::neg_p (max))
> - likely_adjust = true;
>   }
>else if (range_type == VR_ANTI_RANGE)
>   {
> @@ -1307,11 +1295,6 @@ format_integer (const directive , tr
>  
>if (!argmin)
>  {
> -  /* Set the adjustment for an argument whose range includes
> -  zero since that doesn't include the octal or hexadecimal
> -  base prefix.  */
> -  likely_adjust = true;
> -
>if (TREE_CODE (argtype) == POINTER_TYPE)
>   {
> argmin = build_int_cst (pointer_sized_int_node, 0);
> @@ -1371,7 +1354,7 @@ format_integer (const directive , tr
>else
>  {
>res.range.likely = res.range.min;
> -  if (likely_adjust && maybebase && base != 10)
> +  if (maybebase && base != 10)
>   {
> if (res.range.min == 1)
>   res.range.likely += base == 8 ? 1 : 2;

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Fri, Feb 03, 2017 at 05:39:21PM +0100, Jakub Jelinek wrote:
> Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html
> patch, you would with my patch need just the tree_digits part,
> and then the very last hunk in gimple-ssa-sprintf.c (with
> likely_adjust && 
> removed).  Because you do the adjustments only if !res.knownrange
> and in that case you know argmin/argmax are actually dirtype's min/max,
> so 0 must be in the range.

You've committed the patch unnecessarily complicated, see above.
The following gives the same testsuite result.

dirtype is one of the standard {un,}signed {char,short,int,long,long long}
types, all of them have 0 in their ranges.
For VR_RANGE we almost always set res.knownrange to true:
  /* Set KNOWNRANGE if the argument is in a known subrange
 of the directive's type (KNOWNRANGE may be reset below).  */
  res.knownrange
= (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
   || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax));
(the exception is in case that range clearly has to include zero),
and reset it only if adjust_range_for_overflow returned true, which means
it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again
includes zero.
So IMNSHO likely_adjust in what you've committed is always true
when you use it and thus just a useless computation and something to make
the code harder to understand.

Even if you don't trust this, with the ranges in argmin/argmax, it is
IMHO undesirable to set it differently at the different code paths,
if you want to check whether the final range includes zero and at least
one another value, just do
-  if (likely_adjust && maybebase && base != 10)
+  if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)
   && maybebase && base != 10)
Though, it is useless both for the above reason and for the reason that you
actually do something only:
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;
  else if (res.range.min == 2
   && base == 16
   && (dir.width[0] == 2 || dir.prec[0] == 2))
++res.range.likely;
where if the range doesn't include zero, you would never get
res.range.min of 1 and for base == 16 also not 2.

2017-02-04  Jakub Jelinek  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (format_integer): Remove likely_adjust
variable, its initialization and use.

--- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.0 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-04 08:45:33.173709580 +0100
@@ -1232,10 +1232,6 @@ format_integer (const directive , tr
of the format string by returning [-1, -1].  */
 return fmtresult ();
 
-  /* True if the LIKELY counter should be adjusted upward from the MIN
- counter to account for arguments with unknown values.  */
-  bool likely_adjust = false;
-
   fmtresult res;
 
   /* Using either the range the non-constant argument is in, or its
@@ -1265,14 +1261,6 @@ format_integer (const directive , tr
 
  res.argmin = argmin;
  res.argmax = argmax;
-
- /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
- wide_int wzero = wi::zero (wi::get_precision (min));
- if (wi::le_p (min, wzero, SIGNED)
- && !wi::neg_p (max))
-   likely_adjust = true;
}
   else if (range_type == VR_ANTI_RANGE)
{
@@ -1307,11 +1295,6 @@ format_integer (const directive , tr
 
   if (!argmin)
 {
-  /* Set the adjustment for an argument whose range includes
-zero since that doesn't include the octal or hexadecimal
-base prefix.  */
-  likely_adjust = true;
-
   if (TREE_CODE (argtype) == POINTER_TYPE)
{
  argmin = build_int_cst (pointer_sized_int_node, 0);
@@ -1371,7 +1354,7 @@ format_integer (const directive , tr
   else
 {
   res.range.likely = res.range.min;
-  if (likely_adjust && maybebase && base != 10)
+  if (maybebase && base != 10)
{
  if (res.range.min == 1)
res.range.likely += base == 8 ? 1 : 2;
--- gcc/testsuite/ChangeLog.jj  2017-02-04 08:43:14.0 +0100
+++ gcc/testsuite/ChangeLog 2017-02-04 08:48:14.930627979 +0100
@@ -20,8 +20,8 @@
 
PR tree-optimization/79327
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust.
-   * gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test.
-   * gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c: Ditto.
+   * gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test.
+   * gcc.dg/tree-ssa/pr79327-2.c: Ditto.
 
 2017-02-03  Jakub Jelinek  
Martin Sebor  
@@ -306,7 +306,7 @@
 2017-01-27  Vladimir Makarov  
 
PR tree-optimization/71374
-   * 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/03/2017 12:02 PM, Jeff Law wrote:

On 02/02/2017 05:31 PM, Martin Sebor wrote:

-  T (2, "%#hho",a); /* { dg-warning "nul past the end"
} */
-  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
writing between 3 and . bytes into a region of size 2" } */
+  T (2, "%#hho",a);
+  T (2, "%#hhx",a);


On reflection, this isn't quite the right fix.  We want to both set
the correct range and warn because the call will likely overflow.
This is an example of why the likely/unlikely counters have been
introduced.  By setting min = 1 and likely = 2 for the %#hho and
3 for the %#hhx we get the desired result.


Attached is a simple patch that removes the vestigial setting of
the minimum counter while preserving the warnings above by using
the likely counter.

I had overlooked this when I introduced the likely counter and so
in the corner cases of "%#o" and "%#x" with a non-constant argument
that could be zero, the minimum counter would be set to 2 and 3
respectively rather than 1 (because zero is formatted without
the '0' or '0x' base prefix).

This patch almost certainly conflicts with Jakub's.  But I think if
anything it may get simpler after Jakub applies his patch.

Jakub, if you want to do the updates and commit after your patch so they
can both get into any potential weekend gcc spin for Fedora, go right
ahead :-)

Otherwise it's good to go for Martin after making the minor updates.


Let's let Jakub go first so he can be done with his day/week.
I'll deal with the conflicts and retest everything.

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/02/2017 05:49 PM, Martin Sebor wrote:


That is unrelated to the patch, both in the current trunk, with your
patch as well as with my patch there is just
  res.range.likely = res.knownrange ? res.range.max : res.range.min;
  res.range.unlikely = res.range.max;
for these cases.

Do you want likely 2 because that the shortest length for more than
one value (only a single value has the shortest length)?
Something else?


For "%#o" the shortest output of one byte (for zero) is less likely
than the next shortest output of 2 bytes (0 vs 01).

For "%#x" it's one byte vs three bytes (0 vs 0x1).

Since specifying '#' clearly indicates the user wants the base
prefix it seems very likely that the argument will be non-zero.
Whether it's going to be greater than 7 or 15 is not so clear.

So I think setting the likely counter to 2 for octal and 3 for
hexadecimal makes the most sense.
We're trying to guess intent here, which I generally prefer to avoid. 
While we may get it right often, there's going to be cases where we 
don't -- and I see tuning that kind of decision as ultimately a losing 
battle.


To some degree it may be inevitable in this code, but let's not let it 
get out of hand.


Jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/02/2017 05:31 PM, Martin Sebor wrote:

-  T (2, "%#hho",a); /* { dg-warning "nul past the end"
} */
-  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
writing between 3 and . bytes into a region of size 2" } */
+  T (2, "%#hho",a);
+  T (2, "%#hhx",a);


On reflection, this isn't quite the right fix.  We want to both set
the correct range and warn because the call will likely overflow.
This is an example of why the likely/unlikely counters have been
introduced.  By setting min = 1 and likely = 2 for the %#hho and
3 for the %#hhx we get the desired result.


Attached is a simple patch that removes the vestigial setting of
the minimum counter while preserving the warnings above by using
the likely counter.

I had overlooked this when I introduced the likely counter and so
in the corner cases of "%#o" and "%#x" with a non-constant argument
that could be zero, the minimum counter would be set to 2 and 3
respectively rather than 1 (because zero is formatted without
the '0' or '0x' base prefix).
This patch almost certainly conflicts with Jakub's.  But I think if 
anything it may get simpler after Jakub applies his patch.


Jakub, if you want to do the updates and commit after your patch so they 
can both get into any potential weekend gcc spin for Fedora, go right 
ahead :-)


Otherwise it's good to go for Martin after making the minor updates.

jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Fri, Feb 03, 2017 at 11:43:53AM -0700, Jeff Law wrote:
> > +  if (TREE_CODE (argtype) == POINTER_TYPE)
> > {
> > - if (POINTER_TYPE_P (argtype))
> > -   argmax = build_all_ones_cst (argtype);
> > - else if (TYPE_UNSIGNED (argtype))
> > -   argmax = TYPE_MAX_VALUE (argtype);
> > - else
> > -   argmax = TYPE_MIN_VALUE (argtype);
> > + argmin = build_int_cst (pointer_sized_int_node, 0);
> > + argmax = build_all_ones_cst (pointer_sized_int_node);
> Curious why you didn't use the wide_int_to_tree API here.  I'm not objecting
> to using build_XXX, it's more to help guide me when I need to make a choice
> between the APIs for building INTEGER_CST nodes.

For wide_int_to_tree I need to have some wide_int, which I have from the
VR_RANGE query, but not here.  If I have an integer rather than wide-int,
then build_int_cst is the right API (I could use build_zero_cst, but
that in the end just checks if it is INTEGER_TYPE and calls build_int_cst
with 0 in that case).

> OK pending resolution of any conflicts with vla/flexible array changes from
> last night.

Thanks, I think there aren't any conflicts.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/02/2017 03:15 PM, Jakub Jelinek wrote:



Plus there are certain notes removed:
-builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for 
directive argument
  T (0, "%d",   a); /* { dg-warning "into a region" } */
without replacement, here a has [-2147483648, 2147483647] range, but as that
is equal to the type's range, I bet it omits it.


Could be.  As I said, there are opportunities for improvements in
what the notes print.  Someone pointed out, for example, that very
large numbers are hard to read.  It might be clearer to print some
of them using constants like LONG_MAX - 2, or in hex, etc.


The *_MAX or hex is a separate issue, that can be done or doesn't have to be
done, the values are simply the same.  But picking some random numbers in
the ranges is just wrong.
Agreed.  Further refinements in this area should be distinct patches 
from the ones currently under consideration.


One of the high level things that worries me is the level of patch 
dependencies I see as we work through the issues in this code.  That is 
part of what led to the mega patch that I asked Martin to break down 
into pieces, and Martin has indicated levels of pain around keeping 
patches up-to-date WRT the trunk sources.


This is often an indication we went forward with the original work too 
quickly.  I'll take the heat on gating this stuff in too quickly.  It 
happens once in a while (I did the same with the IPA ICF code a couple 
years back).


It's a manageable problem as long as we're aware of and work to avoid 
dependencies.


Jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/02/2017 07:12 AM, Jakub Jelinek wrote:

On Thu, Feb 02, 2017 at 10:52:18AM +0100, Jakub Jelinek wrote:

That said, as I've said in the PR and earlier in December on gcc-patches,
the way format_integer is structured is not really maintainable, has
very different code paths for arguments with known ranges and without them
intermixed together and I still ran into wrong-code issues or wrong warnings
with your patch.  See below.  Thus I'd like to propose to just very much
simplify the code:

 gimple-ssa-sprintf.c |  108 ---
 1 file changed, 27 insertions(+), 81 deletions(-)

So far I haven't done full bootstrap/regtest on this, just
make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp
which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk
below.  Here it is turned into a short wrong-code without the patch below:


Testing revealed a bug in my patch (POINTER_TYPE args really need special
handling, restored), and one further testcase glitch, plus I've added
another testcase for the other wrong-code.

Going to bootstrap/regtest this again:

2017-02-02  Jakub Jelinek  
Martin Sebor  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning
true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of
dirtype.
(format_integer): Use wide_int_to_tree instead of build_int_cst
+ to_?hwi.  If argmin is NULL, just set argmin and argmax to
TYPE_{MIN,MAX}_VALUE of argtype.  Simplify and fix computation
of shortest and longest sequence.

* gcc.dg/tree-ssa/pr79327.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
(test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings.
* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
(test_sprintf_chk_range_schar): Adjust dg-message.
* gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test.
* gcc.c-torture/execute/pr79327.c: New test.
After a lot of thinking about the situation, I think we're better off 
doing this bit of cleanup now.  Jakub and I both expect we're going to 
be in this code again during the gcc-7 cycle, so the cleanups have 
immediate benefit.  I don't see them as inherently destabilizing and 
they're a smaller than I had anticipated.


They may (or may not) need tweaks to adjust for the vla/flexible array 
support that went in last night.  I don't see an obvious conflict, but 
if there is use your best judgment about whether or not you need to go 
through another review/approval cycle.





--- gcc/gimple-ssa-sprintf.c.jj 2017-02-02 11:03:46.536526801 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-02 14:53:54.657592450 +0100
@@ -1260,10 +1248,8 @@ format_integer (const directive , tr
@@ -1307,47 +1293,16 @@ format_integer (const directive , tr

   if (!argmin)
 {
-  /* For an unknown argument (e.g., one passed to a vararg function)
-or one whose value range cannot be determined, create a T_MIN
-constant if the argument's type is signed and T_MAX otherwise,
-and use those to compute the range of bytes that the directive
-can output.  When precision may be zero, use zero as the minimum
-since it results in no bytes on output (unless width is specified
-to be greater than 0).  */
-  bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0;
-  argmin = build_int_cst (argtype, !zero);
-
-  int typeprec = TYPE_PRECISION (dirtype);
-  int argprec = TYPE_PRECISION (argtype);
-
-  if (argprec < typeprec)
+  if (TREE_CODE (argtype) == POINTER_TYPE)
{
- if (POINTER_TYPE_P (argtype))
-   argmax = build_all_ones_cst (argtype);
- else if (TYPE_UNSIGNED (argtype))
-   argmax = TYPE_MAX_VALUE (argtype);
- else
-   argmax = TYPE_MIN_VALUE (argtype);
+ argmin = build_int_cst (pointer_sized_int_node, 0);
+ argmax = build_all_ones_cst (pointer_sized_int_node);
Curious why you didn't use the wide_int_to_tree API here.  I'm not 
objecting to using build_XXX, it's more to help guide me when I need to 
make a choice between the APIs for building INTEGER_CST nodes.


OK pending resolution of any conflicts with vla/flexible array changes 
from last night.


jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/03/2017 09:39 AM, Jakub Jelinek wrote:

On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote:

I'm not opposed to the changes, certainly not to cleaning things up,
but I don't think now is the time to be making these tweaks.  Jakub's
patch is fine with me without those tweaks, and with the correction


What do you mean by my patch without those tweaks?
The intent of the patch is not just fix the diagnostics and
wrong-code issue, but primarily that any further needed fix will need
to tweak just a single spot, not many, otherwise e.g. you need to have
sufficient testsuite coverage for all those paths.
Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html
patch, you would with my patch need just the tree_digits part,
and then the very last hunk in gimple-ssa-sprintf.c (with
likely_adjust &&
removed).  Because you do the adjustments only if !res.knownrange
and in that case you know argmin/argmax are actually dirtype's min/max,
so 0 must be in the range.
Right.  It's a combination of 3 things in Jakub patch.  FIx the 
diagnostics, fix the codegen issue and refactoring to make the ongoing 
maintenance easier.



I'm not opposed to changing this to make it more intuitive or useful
but again, I would rather not spend time on it now.  Instead, I would
prefer to discuss what we want after the dust from the release has
settled and implement a consistent solution in GCC 8.


I fear this isn't the last fix needed, the code is very complex and not
sufficiently covered by testcases, so if we don't want to make the code
more maintainable now, I'd be strongly for just turning
-fprintf-return-value off by default for the release.  Bogus warnings
can be lived with or worked around, silent wrong-code is much worse.
I also expect we're going to be in this code more through the gcc-7 
process and that's the primary reason why I'm considering the 
refactoring aspects of Jakub's patch.   Normally I would have nixed 
those changes as inappropriate at this stage.


Jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Fri, Feb 03, 2017 at 10:02:48AM -0700, Martin Sebor wrote:
> On 02/03/2017 09:39 AM, Jakub Jelinek wrote:
> > On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote:
> > > I'm not opposed to the changes, certainly not to cleaning things up,
> > > but I don't think now is the time to be making these tweaks.  Jakub's
> > > patch is fine with me without those tweaks, and with the correction
> > 
> > What do you mean by my patch without those tweaks?
> ...
> > I fear this isn't the last fix needed, the code is very complex and not
> > sufficiently covered by testcases, so if we don't want to make the code
> > more maintainable now, I'd be strongly for just turning
> > -fprintf-return-value off by default for the release.  Bogus warnings
> > can be lived with or worked around, silent wrong-code is much worse.
> 
> I mean without changes to the content of the notes.  If you feel
> it important to simplify the code now that's okay with me.

I haven't changed anything in order to change the notes, that is just the
side-effect of arg{min,max} to always mean the range boundaries, and only
res.  If you want to print what values the pass considered as shortest or
longest output, then it would need to remember (in other named fields)
those values that it picked and use suitable wording to tell those
values to the user.  I think the ranges are very desirable to be printed
in any cases, and if further info is added, if it doesn't clutter the
message too much, why not.

> I'm not empowered to either approve or reject any changes so what
> say is obviously just my input into Jeff's decision.  I also care

I'm not a maintainer of that part of GCC, so I can only wait for some
reviewer or maintainer too.

> a lot about this work so I'm not going to resist when faced with
> a threat of having it disabled.  If leaving the optimization
> enabled is conditional on it then feel free to make whatever
> changes you see fit.

Leaving the optimization enabled by default should be dependent on
1) what value it brings 2) what are the risks (how much we can trust
it not to have other silent wrong-code issues lurking in).
That decision is of course again something some reviewer would need
to ack if I were to propose it, but with Fedora gcc package maintainer
hat on, that is an option I'm seriously considering at least for the
upcoming non-test mass rebuild, because we have at most days to resolve
stuff.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/03/2017 09:39 AM, Jakub Jelinek wrote:

On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote:

I'm not opposed to the changes, certainly not to cleaning things up,
but I don't think now is the time to be making these tweaks.  Jakub's
patch is fine with me without those tweaks, and with the correction


What do you mean by my patch without those tweaks?

...

I fear this isn't the last fix needed, the code is very complex and not
sufficiently covered by testcases, so if we don't want to make the code
more maintainable now, I'd be strongly for just turning
-fprintf-return-value off by default for the release.  Bogus warnings
can be lived with or worked around, silent wrong-code is much worse.


I mean without changes to the content of the notes.  If you feel
it important to simplify the code now that's okay with me.

I'm not empowered to either approve or reject any changes so what
say is obviously just my input into Jeff's decision.  I also care
a lot about this work so I'm not going to resist when faced with
a threat of having it disabled.  If leaving the optimization
enabled is conditional on it then feel free to make whatever
changes you see fit.

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote:
> I'm not opposed to the changes, certainly not to cleaning things up,
> but I don't think now is the time to be making these tweaks.  Jakub's
> patch is fine with me without those tweaks, and with the correction

What do you mean by my patch without those tweaks?
The intent of the patch is not just fix the diagnostics and
wrong-code issue, but primarily that any further needed fix will need
to tweak just a single spot, not many, otherwise e.g. you need to have
sufficient testsuite coverage for all those paths.
Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html
patch, you would with my patch need just the tree_digits part,
and then the very last hunk in gimple-ssa-sprintf.c (with
likely_adjust && 
removed).  Because you do the adjustments only if !res.knownrange
and in that case you know argmin/argmax are actually dirtype's min/max,
so 0 must be in the range.

> to keep the warning (and fix the octal base prefix) that I posted
> in the followup patch.  (The followup patch is also necessary to
> avoid incorrect optimization.)
> 
> Regarding the printed ranges: for integer arguments the pass prints
> one of two sets:
> 
> 1) either the range the argument supplied by the caller is known to
>be in, or
> 2) the range synthesized by the pass for an argument in an unknown
>range, or after conversion to the type of the directive
> 
> The notes distinguish between these two by using the two phrases:
> 
> 1) directive argument in the range [MIN, MAX]
> 
> 2) using the range [MIN, MAX] for directive argument
> 
> I suspect this isn't entirely consistent with all the recent changes
> but that's the idea behind it.  It's subtle and I'm not surprised
> Jakub missed it.

You still print as range [MIN, MAX] something that is in no way a range,
just some values picked from the range.

> I'm not opposed to changing this to make it more intuitive or useful
> but again, I would rather not spend time on it now.  Instead, I would
> prefer to discuss what we want after the dust from the release has
> settled and implement a consistent solution in GCC 8.

I fear this isn't the last fix needed, the code is very complex and not
sufficiently covered by testcases, so if we don't want to make the code
more maintainable now, I'd be strongly for just turning
-fprintf-return-value off by default for the release.  Bogus warnings
can be lived with or worked around, silent wrong-code is much worse.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/03/2017 12:35 AM, Jeff Law wrote:

On 02/02/2017 09:58 AM, Martin Sebor wrote:


Otherwise all the tests succeeded, though looking e.g. at the diff
between builtin-sprintf-warn-1.c diagnostics with your patch and with
the patch below instead, there are also differences like:
-builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for
directive argument
+builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127]
for directive argument
where [1, -128] looks clearly wrong, that isn't a valid range,


The range in the note is the artificial one the pass uses to compute
the range of output.  I went back and forth on this as I think it's
debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]).
The informative notes aren't completely consistent in what ranges
they print.  It would be nice to nail down what we think is the most
useful output and make them all consistent.

I don't think there's any argument against moving towards consistency,
but I would well see folks not agreeing on which consistent style is
preferable.

I do think that we should strive to print ranges in the same format that
we use to describe ranges internally.  So a range like [1, -128] is not
a valid range.  That may argue against using the artificial range
representation in the output.






As for the rest of the patch, while I appreciate your help and
acknowledge it's not my call to make I'm not entirely comfortable
with this much churn at this stage.  My preference would to just
fix the bugs and do the restructuring in stage 1.

I'm really torn here.  I'm keen to raise the bar in terms of what we're
willing to do for gcc-7.  But I'm also keen to have the codebase in a
reasonable state that will allow for the ongoing maintenance of the
gcc-7 branch.

The sprintf stuff is fairly dense and there's almost certainly more
dusty corner case issues we're going to have to work through.  Thus we
stand to be in a better state to maintain the code if we can do some
refactoring.

The fact that Jakub, one of the release managers, is proposing the
change carries a lot of weight in terms of trying to make a decision
here.  Similarly your weight as the author of this code carries a lot of
weight.  Leaving us without a clearly preferable path.


I'm not opposed to the changes, certainly not to cleaning things up,
but I don't think now is the time to be making these tweaks.  Jakub's
patch is fine with me without those tweaks, and with the correction
to keep the warning (and fix the octal base prefix) that I posted
in the followup patch.  (The followup patch is also necessary to
avoid incorrect optimization.)

Regarding the printed ranges: for integer arguments the pass prints
one of two sets:

1) either the range the argument supplied by the caller is known to
   be in, or
2) the range synthesized by the pass for an argument in an unknown
   range, or after conversion to the type of the directive

The notes distinguish between these two by using the two phrases:

1) directive argument in the range [MIN, MAX]

2) using the range [MIN, MAX] for directive argument

I suspect this isn't entirely consistent with all the recent changes
but that's the idea behind it.  It's subtle and I'm not surprised
Jakub missed it.

I'm not opposed to changing this to make it more intuitive or useful
but again, I would rather not spend time on it now.  Instead, I would
prefer to discuss what we want after the dust from the release has
settled and implement a consistent solution in GCC 8.

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jeff Law]

On 02/02/2017 09:58 AM, Martin Sebor wrote:


Otherwise all the tests succeeded, though looking e.g. at the diff
between builtin-sprintf-warn-1.c diagnostics with your patch and with
the patch below instead, there are also differences like:
-builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for
directive argument
+builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127]
for directive argument
where [1, -128] looks clearly wrong, that isn't a valid range,


The range in the note is the artificial one the pass uses to compute
the range of output.  I went back and forth on this as I think it's
debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]).
The informative notes aren't completely consistent in what ranges
they print.  It would be nice to nail down what we think is the most
useful output and make them all consistent.
I don't think there's any argument against moving towards consistency, 
but I would well see folks not agreeing on which consistent style is 
preferable.


I do think that we should strive to print ranges in the same format that 
we use to describe ranges internally.  So a range like [1, -128] is not 
a valid range.  That may argue against using the artificial range 
representation in the output.







As for the rest of the patch, while I appreciate your help and
acknowledge it's not my call to make I'm not entirely comfortable
with this much churn at this stage.  My preference would to just
fix the bugs and do the restructuring in stage 1.
I'm really torn here.  I'm keen to raise the bar in terms of what we're 
willing to do for gcc-7.  But I'm also keen to have the codebase in a 
reasonable state that will allow for the ongoing maintenance of the 
gcc-7 branch.


The sprintf stuff is fairly dense and there's almost certainly more 
dusty corner case issues we're going to have to work through.  Thus we 
stand to be in a better state to maintain the code if we can do some 
refactoring.


The fact that Jakub, one of the release managers, is proposing the 
change carries a lot of weight in terms of trying to make a decision 
here.  Similarly your weight as the author of this code carries a lot of 
weight.  Leaving us without a clearly preferable path.



Jeff

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/02/2017 05:31 PM, Martin Sebor wrote:

-  T (2, "%#hho",a); /* { dg-warning "nul past the end"
} */
-  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
writing between 3 and . bytes into a region of size 2" } */
+  T (2, "%#hho",a);
+  T (2, "%#hhx",a);


On reflection, this isn't quite the right fix.  We want to both set
the correct range and warn because the call will likely overflow.
This is an example of why the likely/unlikely counters have been
introduced.  By setting min = 1 and likely = 2 for the %#hho and
3 for the %#hhx we get the desired result.


Attached is a simple patch that removes the vestigial setting of
the minimum counter while preserving the warnings above by using
the likely counter.

I had overlooked this when I introduced the likely counter and so
in the corner cases of "%#o" and "%#x" with a non-constant argument
that could be zero, the minimum counter would be set to 2 and 3
respectively rather than 1 (because zero is formatted without
the '0' or '0x' base prefix).


The attached is an updated version that addresses Jakub's comments
in a separate post.

Martin
gcc/ChangeLog:

	* gimple-ssa-sprintf.c (tree_digits): Avoid adding octal prefix
	when precision has resulted in leading zeros.
	(format_integer): Adjust the likely counter to assume an unknown
	argument that may be zero is non-zero.

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test.

Index: gcc/gimple-ssa-sprintf.c
===
--- gcc/gimple-ssa-sprintf.c	(revision 245142)
+++ gcc/gimple-ssa-sprintf.c	(working copy)
@@ -762,7 +762,9 @@ tree_digits (tree x, int base, HOST_WIDE_INT prec,
 
   res += prec < ndigs ? ndigs : prec;
 
-  if (prefix && absval)
+  /* Adjust a non-zero value for the base prefix, either hexadecimal,
+ or, unless precision has resulted in a leading zero, also octal.  */
+  if (prefix && absval && (base == 16 || prec <= ndigs))
 {
   if (base == 8)
 	res += 1;
@@ -1242,6 +1244,10 @@ format_integer (const directive , tree arg)
of the format string by returning [-1, -1].  */
 return fmtresult ();
 
+  /* True if the LIKELY counter should be adjusted upward from the MIN
+ counter to account for arguments with unknown values.  */
+  bool likely_adjust = false;
+
   fmtresult res;
 
   /* Using either the range the non-constant argument is in, or its
@@ -1273,6 +1279,14 @@ format_integer (const directive , tree arg)
 
 	  res.argmin = argmin;
 	  res.argmax = argmax;
+
+	  /* Set the adjustment for an argument whose range includes
+	 zero since that doesn't include the octal or hexadecimal
+	 base prefix.  */
+	  wide_int wzero = wi::zero (wi::get_precision (min));
+	  if (wi::le_p (min, wzero, SIGNED)
+	  && !wi::neg_p (max))
+	likely_adjust = true;
 	}
   else if (range_type == VR_ANTI_RANGE)
 	{
@@ -1311,12 +1325,14 @@ format_integer (const directive , tree arg)
 	 or one whose value range cannot be determined, create a T_MIN
 	 constant if the argument's type is signed and T_MAX otherwise,
 	 and use those to compute the range of bytes that the directive
-	 can output.  When precision may be zero, use zero as the minimum
-	 since it results in no bytes on output (unless width is specified
-	 to be greater than 0).  */
-  bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0;
-  argmin = build_int_cst (argtype, !zero);
+	 can output.  */
+  argmin = integer_zero_node;
 
+  /* Set the adjustment for an argument whose range includes
+	 zero since that doesn't include the octal or hexadecimal
+	 base prefix.  */
+  likely_adjust = true;
+
   int typeprec = TYPE_PRECISION (dirtype);
   int argprec = TYPE_PRECISION (argtype);
 
@@ -1391,7 +1407,24 @@ format_integer (const directive , tree arg)
   res.range.min = tmp;
 }
 
-  res.range.likely = res.knownrange ? res.range.max : res.range.min;
+  /* Add the adjustment for an argument whose range includes zero
+ since it doesn't include the octal or hexadecimal base prefix.  */
+  if (res.knownrange)
+res.range.likely = res.range.max;
+  else
+{
+  res.range.likely = res.range.min;
+  if (likely_adjust && maybebase && base != 10)
+	{
+	  if (res.range.min == 1)
+	res.range.likely += base == 8 ? 1 : 2;
+	  else if (res.range.min == 2
+		   && base == 16
+		   && (dir.width[0] == 2 || dir.prec[0] == 2))
+	++res.range.likely;
+	}
+}
+
   res.range.unlikely = res.range.max;
   res.adjust_for_width_or_precision (dir.width, dirtype, base,
  (sign | maybebase) + (base == 16));
Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
===
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c	(revision 245142)
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c	(working copy)
@@ -1152,7 +1152,7 @@ void test_sprintf_chk_hh_nonconst (int w, 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/02/2017 04:23 PM, Jakub Jelinek wrote:

On Thu, Feb 02, 2017 at 12:59:11PM -0700, Martin Sebor wrote:

-  T (2, "%#hho",a); /* { dg-warning "nul past the end" } */
-  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
writing between 3 and . bytes into a region of size 2" } */
+  T (2, "%#hho",a);
+  T (2, "%#hhx",a);


On reflection, this isn't quite the right fix.  We want to both set
the correct range and warn because the call will likely overflow.
This is an example of why the likely/unlikely counters have been
introduced.  By setting min = 1 and likely = 2 for the %#hho and
3 for the %#hhx we get the desired result.


Then please first define what should likely mean and document that.


That's a fair request.   I implemented the "likely counter" approach
based on your suggestion (in comment #3 on bug 78703) so I'd expect
the concept to feel intuitive to you, but documenting it is a good
idea (and in fact on my list of things to do).



That is unrelated to the patch, both in the current trunk, with your
patch as well as with my patch there is just
  res.range.likely = res.knownrange ? res.range.max : res.range.min;
  res.range.unlikely = res.range.max;
for these cases.

Do you want likely 2 because that the shortest length for more than
one value (only a single value has the shortest length)?
Something else?


For "%#o" the shortest output of one byte (for zero) is less likely
than the next shortest output of 2 bytes (0 vs 01).

For "%#x" it's one byte vs three bytes (0 vs 0x1).

Since specifying '#' clearly indicates the user wants the base
prefix it seems very likely that the argument will be non-zero.
Whether it's going to be greater than 7 or 15 is not so clear.

So I think setting the likely counter to 2 for octal and 3 for
hexadecimal makes the most sense.

That's what I did in the patch I just posted:
  https://gcc.gnu.org/ml/gcc-patches/2017-02/msg00223.html

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Thu, Feb 02, 2017 at 05:31:19PM -0700, Martin Sebor wrote:
> index 9e099f0..84dd3671 100644
> --- a/gcc/gimple-ssa-sprintf.c
> +++ b/gcc/gimple-ssa-sprintf.c
> @@ -1242,6 +1242,10 @@ format_integer (const directive , tree arg)
> of the format string by returning [-1, -1].  */
>  return fmtresult ();
>  
> +  /* The adjustment to add to the MIN counter when computing the LIKELY
> + counter for arguments with unknown values.  */
> +  unsigned likely_adjust = 0;
> +
>fmtresult res;
>  
>/* Using either the range the non-constant argument is in, or its
> @@ -1273,6 +1277,15 @@ format_integer (const directive , tree arg)
>  
> res.argmin = argmin;
> res.argmax = argmax;
> +
> +   /* Set the adjustment for an argument whose range includes
> +  zero since that doesn't include the octal or hexadecimal
> +  base prefix.  */
> +   wide_int wzero = wi::zero (wi::get_precision (min));
> +   if (maybebase
> +   && wi::le_p (min, wzero, SIGNED)
> +   && wi::le_p (wzero, max, SIGNED))
> + likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0;
>   }
>else if (range_type == VR_ANTI_RANGE)
>   {
> @@ -1311,11 +1324,14 @@ format_integer (const directive , tree arg)
>or one whose value range cannot be determined, create a T_MIN
>constant if the argument's type is signed and T_MAX otherwise,
>and use those to compute the range of bytes that the directive
> -  can output.  When precision may be zero, use zero as the minimum
> -  since it results in no bytes on output (unless width is specified
> -  to be greater than 0).  */
> -  bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0;
> -  argmin = build_int_cst (argtype, !zero);
> +  can output.  */
> +  argmin = integer_zero_node;
> +
> +  /* Set the adjustment for an argument whose range includes
> +  zero since that doesn't include the octal or hexadecimal
> +  base prefix.  */
> +  if (maybebase)
> + likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0;

This is another sign why the patch I've posted is desirable.  Now you have
to tweak several places, with the patch just one, you have a final range
and you can check whether it includes zero or not.

wi::le_p (wzero, max, SIGNED) is !wi::neg_p (max) I think.

That said, adding something to res.range.min looks wrong to me, you don't
know without more analysis whether actually the length for 0 and length for
say 1 or -1 isn't the same.
Consider %#x where indeed 0 is 2 byte shorter than 1, but say %#3x where
it is the same length, so if you still add your likely_adjust of 2 to that,
the res.range.likely value will be suddenly 5 instead of correct 3.

Easier would be to see if 0 is in the range, see if 1 is also in the range
and set res.range.likely to the length of 1, or, if 0 is in the range, 1
is not but -1 is, to that of -1 or something like that.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

-  T (2, "%#hho",a); /* { dg-warning "nul past the end" } */
-  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
writing between 3 and . bytes into a region of size 2" } */
+  T (2, "%#hho",a);
+  T (2, "%#hhx",a);


On reflection, this isn't quite the right fix.  We want to both set
the correct range and warn because the call will likely overflow.
This is an example of why the likely/unlikely counters have been
introduced.  By setting min = 1 and likely = 2 for the %#hho and
3 for the %#hhx we get the desired result.


Attached is a simple patch that removes the vestigial setting of
the minimum counter while preserving the warnings above by using
the likely counter.

I had overlooked this when I introduced the likely counter and so
in the corner cases of "%#o" and "%#x" with a non-constant argument
that could be zero, the minimum counter would be set to 2 and 3
respectively rather than 1 (because zero is formatted without
the '0' or '0x' base prefix).

Martin
gcc/ChangeLog:

	* gimple-ssa-sprintf.c (format_integer): Adjust the likely counter
	to assume an unknown argument that may be zero is non-zero.

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 9e099f0..84dd3671 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -1242,6 +1242,10 @@ format_integer (const directive , tree arg)
of the format string by returning [-1, -1].  */
 return fmtresult ();
 
+  /* The adjustment to add to the MIN counter when computing the LIKELY
+ counter for arguments with unknown values.  */
+  unsigned likely_adjust = 0;
+
   fmtresult res;
 
   /* Using either the range the non-constant argument is in, or its
@@ -1273,6 +1277,15 @@ format_integer (const directive , tree arg)
 
 	  res.argmin = argmin;
 	  res.argmax = argmax;
+
+	  /* Set the adjustment for an argument whose range includes
+	 zero since that doesn't include the octal or hexadecimal
+	 base prefix.  */
+	  wide_int wzero = wi::zero (wi::get_precision (min));
+	  if (maybebase
+	  && wi::le_p (min, wzero, SIGNED)
+	  && wi::le_p (wzero, max, SIGNED))
+	likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0;
 	}
   else if (range_type == VR_ANTI_RANGE)
 	{
@@ -1311,11 +1324,14 @@ format_integer (const directive , tree arg)
 	 or one whose value range cannot be determined, create a T_MIN
 	 constant if the argument's type is signed and T_MAX otherwise,
 	 and use those to compute the range of bytes that the directive
-	 can output.  When precision may be zero, use zero as the minimum
-	 since it results in no bytes on output (unless width is specified
-	 to be greater than 0).  */
-  bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0;
-  argmin = build_int_cst (argtype, !zero);
+	 can output.  */
+  argmin = integer_zero_node;
+
+  /* Set the adjustment for an argument whose range includes
+	 zero since that doesn't include the octal or hexadecimal
+	 base prefix.  */
+  if (maybebase)
+	likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0;
 
   int typeprec = TYPE_PRECISION (dirtype);
   int argprec = TYPE_PRECISION (argtype);
@@ -1391,7 +1407,11 @@ format_integer (const directive , tree arg)
   res.range.min = tmp;
 }
 
-  res.range.likely = res.knownrange ? res.range.max : res.range.min;
+  /* Add the adjustment for an argument whose range includes zero
+ since it doesn't include the octal or hexadecimal base prefix.  */
+  res.range.likely
+= res.knownrange ? res.range.max : res.range.min + likely_adjust;
+
   res.range.unlikely = res.range.max;
   res.adjust_for_width_or_precision (dir.width, dirtype, base,
  (sign | maybebase) + (base == 16));
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c
new file mode 100644
index 000..deba705
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c
@@ -0,0 +1,180 @@
+/* { dg-do compile }
+   { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" }
+   { dg-require-effective-target int32plus } */
+
+typedef __SIZE_TYPE__  size_t;
+typedef __WCHAR_TYPE__ wchar_t;
+
+#define INT_MAX __INT_MAX__
+#define INT_MIN (-INT_MAX - 1)
+
+/* When debugging, define LINE to the line number of the test case to exercise
+   and avoid exercising any of the others.  The buffer and objsize macros
+   below make use of LINE to avoid warnings for other lines.  */
+#ifndef LINE
+# define LINE 0
+#endif
+
+void sink (char*, char*);
+
+int dummy_sprintf (char*, const char*, ...);
+
+char buffer [256];
+extern char *ptr;
+
+int int_range (int min, int max)
+{
+  extern int int_value (void);
+  int n = int_value ();
+  return n < min || max < n ? min : n;
+}
+
+unsigned uint_range (unsigned min, unsigned max)
+{
+  extern unsigned uint_value (void);
+  unsigned n = uint_value 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Thu, Feb 02, 2017 at 12:59:11PM -0700, Martin Sebor wrote:
> > > -  T (2, "%#hho",a); /* { dg-warning "nul past the end" } */
> > > -  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
> > > writing between 3 and . bytes into a region of size 2" } */
> > > +  T (2, "%#hho",a);
> > > +  T (2, "%#hhx",a);
> 
> On reflection, this isn't quite the right fix.  We want to both set
> the correct range and warn because the call will likely overflow.
> This is an example of why the likely/unlikely counters have been
> introduced.  By setting min = 1 and likely = 2 for the %#hho and
> 3 for the %#hhx we get the desired result.

Then please first define what should likely mean and document that.

That is unrelated to the patch, both in the current trunk, with your
patch as well as with my patch there is just
  res.range.likely = res.knownrange ? res.range.max : res.range.min;
  res.range.unlikely = res.range.max;
for these cases.

Do you want likely 2 because that the shortest length for more than
one value (only a single value has the shortest length)?
Something else?

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

Hi!

Note, the second patch I've posted passed bootstrap/regtest on both
x86_64-linux and i686-linux.

On Thu, Feb 02, 2017 at 09:58:06AM -0700, Martin Sebor wrote:
> > int
> > main (void)
> > {
> >   int i;
> >   char buf[64];
> >   if (__builtin_sprintf (buf, "%#hho", a) > 1)
> > __builtin_abort ();
> >   if (__builtin_sprintf (buf, "%#hhx", a) > 1)
> > __builtin_abort ();
> >   return 0;
> > }
> > Current trunk as well as trunk + your patch computes ranges [2, 4] and
> > [3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0
> > (or say -131072 etc.) it sets buf to "0" in both cases.
> 
> That's right.  This is a good find.  This case is being tested in
> builtin-sprintf-warn-1.c but the assertions (on lines 1154 and
> 1155) seem to expect the wrong numbers.  I would expect your fix
> to cause failures here. (And now that I've read the rest of the
> patch I see it does.)

That testcase is derived from the builtin-sprintf-warn-1.c:115{4,5}
FAILs I got with the patch + analysis (and included in the second patch
in the testsuite as executable testcase too).

> The range in the note is the artificial one the pass uses to compute
> the range of output.  I went back and forth on this as I think it's
> debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]).
> The informative notes aren't completely consistent in what ranges
> they print.  It would be nice to nail down what we think is the most
> useful output and make them all consistent.

You say in the wording range and print it as a range, then it really should
be a range, and not an artificial one, but one reflecting actual possible
values.  If the user knows that the variable can have values -123 and 126
at that point and you still print [1, -128] or [-128, 1] as range, the
users will just think the compiler is buggy and disable the warning.

> > Plus there are certain notes removed:
> > -builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] 
> > for directive argument
> >   T (0, "%d",   a); /* { dg-warning "into a region" } */
> > without replacement, here a has [-2147483648, 2147483647] range, but as that
> > is equal to the type's range, I bet it omits it.
> 
> Could be.  As I said, there are opportunities for improvements in
> what the notes print.  Someone pointed out, for example, that very
> large numbers are hard to read.  It might be clearer to print some
> of them using constants like LONG_MAX - 2, or in hex, etc.

The *_MAX or hex is a separate issue, that can be done or doesn't have to be
done, the values are simply the same.  But picking some random numbers in
the ranges is just wrong.

> > -  tree dirmin = TYPE_MIN_VALUE (dirtype);
> > -  tree dirmax = TYPE_MAX_VALUE (dirtype);
> > -
> > -  if (TYPE_UNSIGNED (dirtype))
> > -{
> > -  *argmin = dirmin;
> > -  *argmax = dirmax;
> > -}
> > -  else
> > -{
> > -  *argmin = integer_zero_node;
> > -  *argmax = dirmin;
> > -}
> > -
> > +  *argmin = TYPE_MIN_VALUE (dirtype);
> > +  *argmax = TYPE_MAX_VALUE (dirtype);
> 
> This is likely behind the changes in the notes you pointed out above.
> The code here is intentional to reflect the range synthesized by
> the pass to compute the output.  I don't have a big issue with

The change I've done was completely intentional, it is again mixing of
value ranges with already premature guessing on what values are shorter or
longer.

> As for the rest of the patch, while I appreciate your help and
> acknowledge it's not my call to make I'm not entirely comfortable
> with this much churn at this stage.  My preference would to just
> fix the bugs and do the restructuring in stage 1.

The thing is, right now we have 3 independent but intermixed code paths,
one for arguments with VR_RANGE that doesn't need conversion for dirtype
(then argmin/argmax until very lately is actual range and the
shortest/longest computation is done at the latest point), then
VR_RANGE that does need conversion for dirtype (that one is prematurely
partly converted to the shortest/longest above), and then
another one that handles the non-VR_RANGE, which commits to the
shortest/longest immediately.  What my patch does is that it unifies all
the 3 paths on essentially what the first path does.  If we don't apply that
patch, we are going to have 3 times as many possibilities for bugs; you will
need to add some hack to get the above mentioned wrong-code fixed, and
IMNSHO another hack to get the bogus printed ranges fixed.
It is always better to remove code than to add it.

Jakub

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

So far I haven't done full bootstrap/regtest on this, just
make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp
which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk
below.  Here it is turned into a short wrong-code without the patch
below:
volatile int a;

int
main (void)
{
  int i;
  char buf[64];
  if (__builtin_sprintf (buf, "%#hho", a) > 1)
__builtin_abort ();
  if (__builtin_sprintf (buf, "%#hhx", a) > 1)
__builtin_abort ();
  return 0;
}
Current trunk as well as trunk + your patch computes ranges [2, 4] and
[3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0
(or say -131072 etc.) it sets buf to "0" in both cases.


That's right.  This is a good find.  This case is being tested in
builtin-sprintf-warn-1.c but the assertions (on lines 1154 and
1155) seem to expect the wrong numbers.  I would expect your fix
to cause failures here. (And now that I've read the rest of the
patch I see it does.)

...

-  T (2, "%#hho",a); /* { dg-warning "nul past the end" } */
-  T (2, "%#hhx",a); /* { dg-warning ".%#hhx. directive
writing between 3 and . bytes into a region of size 2" } */
+  T (2, "%#hho",a);
+  T (2, "%#hhx",a);


On reflection, this isn't quite the right fix.  We want to both set
the correct range and warn because the call will likely overflow.
This is an example of why the likely/unlikely counters have been
introduced.  By setting min = 1 and likely = 2 for the %#hho and
3 for the %#hhx we get the desired result.

Martin

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

On 02/02/2017 02:52 AM, Jakub Jelinek wrote:

On Thu, Feb 02, 2017 at 08:37:07AM +0100, Jakub Jelinek wrote:

+  if (res.range.max < res.range.min)
+   {
+ unsigned HOST_WIDE_INT tmp = res.range.max;
+ res.range.max = res.range.min;
+ res.range.min = tmp;


These 3 lines are
std::swap (res.range.max, res.range.min);
also note the spaces instead of tabs and indentation by 7/9 spaces
instead of 1 tab and 1 tab + 2 spaces.


+   }
+
+  if (!TYPE_UNSIGNED (argtype)
+ && tree_int_cst_lt (integer_zero_node, argmax)
+ && tree_int_cst_lt (argmin, integer_zero_node))


And these 2 lines
  && tree_int_cst_sgn (argmax) > 0
  && tree_int_cst_sgn (argmin) < 0


That said, as I've said in the PR and earlier in December on gcc-patches,
the way format_integer is structured is not really maintainable, has
very different code paths for arguments with known ranges and without them
intermixed together and I still ran into wrong-code issues or wrong warnings
with your patch.  See below.  Thus I'd like to propose to just very much
simplify the code:

 gimple-ssa-sprintf.c |  108 ---
 1 file changed, 27 insertions(+), 81 deletions(-)

So far I haven't done full bootstrap/regtest on this, just
make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp
which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk
below.  Here it is turned into a short wrong-code without the patch below:
volatile int a;

int
main (void)
{
  int i;
  char buf[64];
  if (__builtin_sprintf (buf, "%#hho", a) > 1)
__builtin_abort ();
  if (__builtin_sprintf (buf, "%#hhx", a) > 1)
__builtin_abort ();
  return 0;
}
Current trunk as well as trunk + your patch computes ranges [2, 4] and
[3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0
(or say -131072 etc.) it sets buf to "0" in both cases.


That's right.  This is a good find.  This case is being tested in
builtin-sprintf-warn-1.c but the assertions (on lines 1154 and
1155) seem to expect the wrong numbers.  I would expect your fix
to cause failures here. (And now that I've read the rest of the
patch I see it does.)


Otherwise all the tests succeeded, though looking e.g. at the diff
between builtin-sprintf-warn-1.c diagnostics with your patch and with
the patch below instead, there are also differences like:
-builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for directive 
argument
+builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127] for 
directive argument
where [1, -128] looks clearly wrong, that isn't a valid range,


The range in the note is the artificial one the pass uses to compute
the range of output.  I went back and forth on this as I think it's
debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]).
The informative notes aren't completely consistent in what ranges
they print.  It would be nice to nail down what we think is the most
useful output and make them all consistent.


and
this test is -O0 anyway, plus a doesn't have a known range:
  T (0, "%hhd", a); /* { dg-warning ".%hhd. directive writing between 1 
and . bytes into a region of size 0" } */
(so to some extent even the note with the patch isn't accurate, the
printed value range is the range of the value after an implicit conversion
from int to signed char).
Or
-builtin-sprintf-warn-1.c:1122:3: note: using the range [1, 255] for directive 
argument
+builtin-sprintf-warn-1.c:1122:3: note: using the range [0, 255] for directive 
argument


Same as above.


  T (0, "%hhu", a); /* { dg-warning "into a region" } */
similarly, a has [INT_MIN, INT_MAX] range, but after implicit conversion to 
unsigned
char it is [0, 255], certainly not [1, 255].


Ditto.


Plus there are certain notes removed:
-builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for 
directive argument
  T (0, "%d",   a); /* { dg-warning "into a region" } */
without replacement, here a has [-2147483648, 2147483647] range, but as that
is equal to the type's range, I bet it omits it.


Could be.  As I said, there are opportunities for improvements in
what the notes print.  Someone pointed out, for example, that very
large numbers are hard to read.  It might be clearer to print some
of them using constants like LONG_MAX - 2, or in hex, etc.



2017-02-02  Jakub Jelinek  
Martin Sebor  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning
true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of
dirtype.
(format_integer): Use wide_int_to_tree instead of build_int_cst
+ to_?hwi.  If argmin is NULL, just set argmin and argmax to
TYPE_{MIN,MAX}_VALUE of argtype.  Simplify and fix computation
of shortest and longest sequence.

* 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Thu, Feb 02, 2017 at 10:52:18AM +0100, Jakub Jelinek wrote:
> That said, as I've said in the PR and earlier in December on gcc-patches,
> the way format_integer is structured is not really maintainable, has
> very different code paths for arguments with known ranges and without them
> intermixed together and I still ran into wrong-code issues or wrong warnings
> with your patch.  See below.  Thus I'd like to propose to just very much
> simplify the code:
> 
>  gimple-ssa-sprintf.c |  108 
> ---
>  1 file changed, 27 insertions(+), 81 deletions(-)
> 
> So far I haven't done full bootstrap/regtest on this, just
> make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp
> which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk
> below.  Here it is turned into a short wrong-code without the patch below:

Testing revealed a bug in my patch (POINTER_TYPE args really need special
handling, restored), and one further testcase glitch, plus I've added
another testcase for the other wrong-code.

Going to bootstrap/regtest this again:

2017-02-02  Jakub Jelinek  
Martin Sebor  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning
true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of
dirtype.
(format_integer): Use wide_int_to_tree instead of build_int_cst
+ to_?hwi.  If argmin is NULL, just set argmin and argmax to
TYPE_{MIN,MAX}_VALUE of argtype.  Simplify and fix computation
of shortest and longest sequence.

* gcc.dg/tree-ssa/pr79327.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
(test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings.
* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
(test_sprintf_chk_range_schar): Adjust dg-message.
* gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test.
* gcc.c-torture/execute/pr79327.c: New test.

--- gcc/gimple-ssa-sprintf.c.jj 2017-02-02 11:03:46.536526801 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-02 14:53:54.657592450 +0100
@@ -1014,8 +1014,8 @@ get_int_range (tree arg, tree type, HOST
determined by checking for the actual argument being in the range
of the type of the directive.  If it isn't it must be assumed to
take on the full range of the directive's type.
-   Return true when the range has been adjusted to the full unsigned
-   range of DIRTYPE, or [0, DIRTYPE_MAX], and false otherwise.  */
+   Return true when the range has been adjusted to the full range
+   of DIRTYPE, and false otherwise.  */
 
 static bool
 adjust_range_for_overflow (tree dirtype, tree *argmin, tree *argmax)
@@ -1051,20 +1051,8 @@ adjust_range_for_overflow (tree dirtype,
return false;
 }
 
-  tree dirmin = TYPE_MIN_VALUE (dirtype);
-  tree dirmax = TYPE_MAX_VALUE (dirtype);
-
-  if (TYPE_UNSIGNED (dirtype))
-{
-  *argmin = dirmin;
-  *argmax = dirmax;
-}
-  else
-{
-  *argmin = integer_zero_node;
-  *argmax = dirmin;
-}
-
+  *argmin = TYPE_MIN_VALUE (dirtype);
+  *argmax = TYPE_MAX_VALUE (dirtype);
   return true;
 }
 
@@ -1260,10 +1248,8 @@ format_integer (const directive , tr
   enum value_range_type range_type = get_range_info (arg, , );
   if (range_type == VR_RANGE)
{
- argmin = build_int_cst (argtype, wi::fits_uhwi_p (min)
- ? min.to_uhwi () : min.to_shwi ());
- argmax = build_int_cst (argtype, wi::fits_uhwi_p (max)
- ? max.to_uhwi () : max.to_shwi ());
+ argmin = wide_int_to_tree (argtype, min);
+ argmax = wide_int_to_tree (argtype, max);
 
  /* Set KNOWNRANGE if the argument is in a known subrange
 of the directive's type (KNOWNRANGE may be reset below).  */
@@ -1307,47 +1293,16 @@ format_integer (const directive , tr
 
   if (!argmin)
 {
-  /* For an unknown argument (e.g., one passed to a vararg function)
-or one whose value range cannot be determined, create a T_MIN
-constant if the argument's type is signed and T_MAX otherwise,
-and use those to compute the range of bytes that the directive
-can output.  When precision may be zero, use zero as the minimum
-since it results in no bytes on output (unless width is specified
-to be greater than 0).  */
-  bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0;
-  argmin = build_int_cst (argtype, !zero);
-
-  int typeprec = TYPE_PRECISION (dirtype);
-  int argprec = TYPE_PRECISION (argtype);
-
-  if (argprec < typeprec)
+  if (TREE_CODE (argtype) == POINTER_TYPE)
{
- if (POINTER_TYPE_P (argtype))
-   argmax = build_all_ones_cst (argtype);
- else if (TYPE_UNSIGNED (argtype))
-   argmax = TYPE_MAX_VALUE (argtype);
- else
-   argmax = 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Thu, Feb 02, 2017 at 08:37:07AM +0100, Jakub Jelinek wrote:
> > +  if (res.range.max < res.range.min)
> > +   {
> > + unsigned HOST_WIDE_INT tmp = res.range.max;
> > + res.range.max = res.range.min;
> > + res.range.min = tmp;
> 
> These 3 lines are
>   std::swap (res.range.max, res.range.min);
> also note the spaces instead of tabs and indentation by 7/9 spaces
> instead of 1 tab and 1 tab + 2 spaces.
> 
> > +   }
> > +
> > +  if (!TYPE_UNSIGNED (argtype)
> > + && tree_int_cst_lt (integer_zero_node, argmax)
> > + && tree_int_cst_lt (argmin, integer_zero_node))
> 
> And these 2 lines
> && tree_int_cst_sgn (argmax) > 0
> && tree_int_cst_sgn (argmin) < 0

That said, as I've said in the PR and earlier in December on gcc-patches,
the way format_integer is structured is not really maintainable, has
very different code paths for arguments with known ranges and without them
intermixed together and I still ran into wrong-code issues or wrong warnings
with your patch.  See below.  Thus I'd like to propose to just very much
simplify the code:

 gimple-ssa-sprintf.c |  108 ---
 1 file changed, 27 insertions(+), 81 deletions(-)

So far I haven't done full bootstrap/regtest on this, just
make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp
which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk
below.  Here it is turned into a short wrong-code without the patch below:
volatile int a;

int
main (void)
{
  int i;
  char buf[64];
  if (__builtin_sprintf (buf, "%#hho", a) > 1)
__builtin_abort ();
  if (__builtin_sprintf (buf, "%#hhx", a) > 1)
__builtin_abort ();
  return 0;
}
Current trunk as well as trunk + your patch computes ranges [2, 4] and
[3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0
(or say -131072 etc.) it sets buf to "0" in both cases.
Otherwise all the tests succeeded, though looking e.g. at the diff
between builtin-sprintf-warn-1.c diagnostics with your patch and with
the patch below instead, there are also differences like:
-builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for directive 
argument
+builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127] for 
directive argument
where [1, -128] looks clearly wrong, that isn't a valid range, and
this test is -O0 anyway, plus a doesn't have a known range:
  T (0, "%hhd", a); /* { dg-warning ".%hhd. directive writing 
between 1 and . bytes into a region of size 0" } */
(so to some extent even the note with the patch isn't accurate, the
printed value range is the range of the value after an implicit conversion
from int to signed char).
Or
-builtin-sprintf-warn-1.c:1122:3: note: using the range [1, 255] for directive 
argument
+builtin-sprintf-warn-1.c:1122:3: note: using the range [0, 255] for directive 
argument
  T (0, "%hhu", a); /* { dg-warning "into a region" } */
similarly, a has [INT_MIN, INT_MAX] range, but after implicit conversion to 
unsigned
char it is [0, 255], certainly not [1, 255].
Plus there are certain notes removed:
-builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for 
directive argument
  T (0, "%d",   a); /* { dg-warning "into a region" } */
without replacement, here a has [-2147483648, 2147483647] range, but as that
is equal to the type's range, I bet it omits it.

2017-02-02  Jakub Jelinek  
Martin Sebor  

PR tree-optimization/79327
* gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning
true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of
dirtype.
(format_integer): Use wide_int_to_tree instead of build_int_cst
+ to_?hwi.  If argmin is NULL, just set argmin and argmax to
TYPE_{MIN,MAX}_VALUE of argtype.  Simplify and fix computation
of shortest and longest sequence.

* gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test.
* gcc.dg/tree-ssa/pr79327.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
(test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings.

--- gcc/gimple-ssa-sprintf.c.jj 2017-01-31 09:26:00.692873226 +0100
+++ gcc/gimple-ssa-sprintf.c2017-02-02 10:20:06.965884495 +0100
@@ -1014,8 +1014,8 @@ get_int_range (tree arg, tree type, HOST
determined by checking for the actual argument being in the range
of the type of the directive.  If it isn't it must be assumed to
take on the full range of the directive's type.
-   Return true when the range has been adjusted to the full unsigned
-   range of DIRTYPE, or [0, DIRTYPE_MAX], and false otherwise.  */
+   Return true when the range has been adjusted to the full range
+   of DIRTYPE, and false otherwise.  */
 
 static bool
 adjust_range_for_overflow (tree dirtype, tree *argmin, tree *argmax)
@@ -1051,20 +1051,8 @@ adjust_range_for_overflow (tree 

Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek]

On Wed, Feb 01, 2017 at 04:52:35PM -0700, Martin Sebor wrote:
> --- a/gcc/gimple-ssa-sprintf.c
> +++ b/gcc/gimple-ssa-sprintf.c
> @@ -1382,13 +1382,26 @@ format_integer (const directive , tree arg)
>  would reflect the largest possible precision (i.e., INT_MAX).  */
>res.range.min = format_integer (dir, argmax).range.min;
>res.range.max = format_integer (dir, argmin).range.max;
> -}
>  
> -  if (res.range.max < res.range.min)
> -{
> -  unsigned HOST_WIDE_INT tmp = res.range.max;
> -  res.range.max = res.range.min;
> -  res.range.min = tmp;
> +  if (res.range.max < res.range.min)
> +   {
> + unsigned HOST_WIDE_INT tmp = res.range.max;
> + res.range.max = res.range.min;
> + res.range.min = tmp;

These 3 lines are
std::swap (res.range.max, res.range.min);
also note the spaces instead of tabs and indentation by 7/9 spaces
instead of 1 tab and 1 tab + 2 spaces.

> +   }
> +
> +  if (!TYPE_UNSIGNED (argtype)
> + && tree_int_cst_lt (integer_zero_node, argmax)
> + && tree_int_cst_lt (argmin, integer_zero_node))

And these 2 lines
  && tree_int_cst_sgn (argmax) > 0
  && tree_int_cst_sgn (argmin) < 0

> +   {
> + /* The minimum output for a signed argument in a negative-positive
> +range is that of zero.  */
> + unsigned HOST_WIDE_INT
> +   nzero = format_integer (dir, integer_zero_node).range.min;
> +
> + if (nzero < res.range.min)
> +   res.range.min = nzero;
> +   }
>  }
>  
>res.range.likely = res.knownrange ? res.range.max : res.range.min;

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79327.c
> @@ -0,0 +1,31 @@
> +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value
> +   { dg-do run }
> +   { dg-options "-O2 -Wall" }  */
> +
> +volatile int a, b = -1;
> +char buf[64];
> +
> +#define FMT "%+03d%02d"
> +const char* fmt = FMT;

Perhaps make it even
const char *volatile fmt = FMT;
to make sure we don't see through it even with -fwhole-program or LTO?
Don't you need -Wno-format-security or is that not on in -Wall?

> +
> +int main ()
> +{
> +  int c = a;
> +  int d = b;
> +  if (c >= -35791395 && c < 35791394 && d >= -1 && d < __INT_MAX__)
> +{
> +  /* In the following the range of return values can be computed
> +  by GCC. */
> +  int n1 = __builtin_sprintf (buf, FMT, c + 1, d + 1);
> +  if (n1 > 7)
> + __builtin_abort ();
> +
> +  /* Here GCC can't see the format string so the return value
> +  must be computed by a libc call.  */
> +  int n2 = __builtin_sprintf (buf, fmt, c + 1, d + 1);
> +
> +  if (n1 != n2)
> +__builtin_abort ();
> +}
> +  return 0;
> +}


Jakub

[PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Martin Sebor]

PR tree-optimization/79327 exposes a bug in the gimple-ssa-sprintf
pass in the computation of the expected size of output of an integer
format directive whose argument is in a signed-unsigned range (such
as [-12, 345]).  The current algorithm on trunk uses the bounds of
the range to compute the range of output when it should be using
zero to obtain the minimum output and whichever of the two bounds
results in more output to compute the maximum.  This can lead to
both wrong code (as pointed out in the referenced bug) to and to
the wrong range printed in the warnings.

The attached patch fixes that bug.

Martin
PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value

gcc/ChangeLog:

	PR tree-optimization/79327
	* gimple-ssa-sprintf.c (format_integer): Replace with zero
	the lower bound of an argument in a signed-unsigned range.

gcc/testsuite/ChangeLog:

	PR tree-optimization/79327
	* gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test.
	* gcc.dg/tree-ssa/pr79327.c: New test.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 11f4174..4f0670e 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -1382,13 +1382,26 @@ format_integer (const directive , tree arg)
 would reflect the largest possible precision (i.e., INT_MAX).  */
   res.range.min = format_integer (dir, argmax).range.min;
   res.range.max = format_integer (dir, argmin).range.max;
-}
 
-  if (res.range.max < res.range.min)
-{
-  unsigned HOST_WIDE_INT tmp = res.range.max;
-  res.range.max = res.range.min;
-  res.range.min = tmp;
+  if (res.range.max < res.range.min)
+   {
+ unsigned HOST_WIDE_INT tmp = res.range.max;
+ res.range.max = res.range.min;
+ res.range.min = tmp;
+   }
+
+  if (!TYPE_UNSIGNED (argtype)
+ && tree_int_cst_lt (integer_zero_node, argmax)
+ && tree_int_cst_lt (argmin, integer_zero_node))
+   {
+ /* The minimum output for a signed argument in a negative-positive
+range is that of zero.  */
+ unsigned HOST_WIDE_INT
+   nzero = format_integer (dir, integer_zero_node).range.min;
+
+ if (nzero < res.range.min)
+   res.range.min = nzero;
+   }
 }
 
   res.range.likely = res.knownrange ? res.range.max : res.range.min;
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c
new file mode 100644
index 000..35db400
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c
@@ -0,0 +1,228 @@
+/* PR tree-optimization79/327 - wrong code at -O2 and -fprintf-return-value
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" }
+   { dg-require-effective-target int32plus } */
+
+typedef __SIZE_TYPE__  size_t;
+typedef __WCHAR_TYPE__ wchar_t;
+
+#define INT_MAX __INT_MAX__
+#define INT_MIN (-INT_MAX - 1)
+
+/* When debugging, define LINE to the line number of the test case to exercise
+   and avoid exercising any of the others.  The buffer and objsize macros
+   below make use of LINE to avoid warnings for other lines.  */
+#ifndef LINE
+# define LINE 0
+#endif
+
+void sink (char*, char*);
+
+int dummy_sprintf (char*, const char*, ...);
+
+char buffer [256];
+extern char *ptr;
+
+int int_range (int min, int max)
+{
+  extern int int_value (void);
+  int n = int_value ();
+  return n < min || max < n ? min : n;
+}
+
+unsigned uint_range (unsigned min, unsigned max)
+{
+  extern unsigned uint_value (void);
+  unsigned n = uint_value ();
+  return n < min || max < n ? min : n;
+}
+
+/* Evaluate to an array of SIZE characters when non-negative, or to
+   a pointer to an unknown object otherwise.  */
+#define buffer(size)	\
+  ((0 <= size) ? buffer + sizeof buffer - (size) : ptr)
+
+/* Helper to expand function to either __builtin_f or dummy_f to
+   make debugging GCC easy.  */
+#define FUNC(f)			\
+  ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f)
+
+/* Macro to verify that calls to __builtin_sprintf (i.e., with no size
+   argument) issue diagnostics by correctly determining the size of
+   the destination buffer.  */
+#define T(size, ...)		\
+  (FUNC (sprintf) (buffer (size),  __VA_ARGS__),		\
+   sink (buffer, ptr))
+
+/* Return a signed integer in the range [MIN, MAX].  */
+#define R(min, max)  int_range (min, max)
+
+/* Return a unsigned integer in the range [MIN, MAX].  */
+#define U(min, max)  uint_range (min, max)
+
+/* Exercise the hh length modifier with ranges.  */
+void test_hh (void)
+{
+  T (0, "%hhi", R (  -1,0));/* { dg-warning "between 1 and 2 bytes" } */
+  T (0, "%hhi", R (  -1,1));/* { dg-warning "between 1 and 2 bytes" } */
+  T (0, "%hhi", R (  -1,   12));/* { dg-warning "between 1 and 2 bytes" } */
+  T (0, "%hhi", R (  -1,  123));/* { dg-warning "between 1 and 3 bytes" } */
+  T (0, "%hhi", R (  -1,  128));/* { dg-warning