Re: [PATCH] restore -Wunused-variable on a typedef'd variable in a function template (PR 79548)

[Martin Sebor]

On 02/22/2017 05:43 PM, Jason Merrill wrote:

On Wed, Feb 22, 2017 at 3:44 PM, Martin Sebor <mse...@gmail.com> wrote:

On 02/22/2017 11:02 AM, Jason Merrill wrote:


On Tue, Feb 21, 2017 at 4:27 PM, Martin Sebor <mse...@gmail.com> wrote:


Ah, I see, your patch changes attribute unused handling for local
variables from tracking TREE_USED to lookup_attribute.  I'm not
opposed to this change, but I'd like to understand why the TREE_USED
handling wasn't working.




In the test case in the bug:

  template 
  void g ()
  {
T t;   // warning, ok

typedef T U;
U u;   // no warning, bug
  }

  template void g();

both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
so the function doesn't set already_used or TREE_USED(t) and we get
a warning as expected.

But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
(to implement -Wunused-local-typedefs),  initialize_local_var then
sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
the warning.


Hmm, I would expect maybe_record_typedef_use to set TREE_USED on the
TYPE_DECL, not on the *_TYPE which initialize_local_var checks.


That's what it does:

  void
  maybe_record_typedef_use (tree t)
  {
if (!is_typedef_decl (t))
  return;

TREE_USED (t) = true;
  }

Here, t is a TYPE_DECL of the typedef U.


Yes.  It is a TYPE_DECL, not a type.


It has the effect of TREE_USED (TREE_TYPE (decl)) being set in
initialize_local_var.  The TREE_USED bit on the type (i.e., on
TREE_TYPE(decl) where decl is the u in the test case above) is
set when the function template is instantiated, in
set_underlying_type called from tsubst_decl.


Aha!  That seems like the problem.  Does removing that copy of
TREE_USED from the decl to the type break anything?


As far as I can see it breaks just gcc.dg/unused-3.c which is:

  typedef short unused_type __attribute__ ((unused));

  void f (void)
  {
unused_type y;
  }

The reason is that the type is TREE_USED isn't set on the variable
(I didn't look where the bit from the type is copied into the var).,

This could be fixed by also looking at the type's attribute in this
case.  That would seem to me like the better approach because it
more faithfully represents what's going on in the code.  I.e., that
the variable is in fact unused, but that its type says not to complain
about it.

Let me know what you prefer.

While looking into this I noticed that regardless of this change,
the C++ front end warns on the following modified version of the
test case:

  static unused_type x;   // bogus -Wunused-variable

  void f (void)
  {
static unused_type y;   // bogus -Wunused-variable
  }

I opened bug 79695 for it.

Martin

Re: [PATCH, doc]: Mention that -mfpmath=sse is the default on 32bit x86 w/ SSE2 and -ffast-math

[Martin Sebor]

On 02/23/2017 12:01 PM, Uros Bizjak wrote:

On Thu, Feb 23, 2017 at 7:07 PM, Martin Sebor <mse...@gmail.com> wrote:


A minor grammatical nit:

  +This is the default choice for most of x86-32 targets.

"for most x86-32 targets" is correct unless the targets are some
specific subset, in which case "most of the [previously mentioned]
x86-32 targets" would work.


Maybe we can say "This is the default choice for non-Darwin x86-32
targets." here?

And further extend:
"This is the default choice for the x86-64 compiler, Darwin x86-32
targets, and the default choice for x86-32 targets with SSE2
instruction set when @option{-ffast-math} is enabled."


The phrasing looks good to me.  There should be an article before
SSE2, i.e., "targets with the SSE2 instruction set"

Martin

enable -Wformat-truncation with -Og (PR 79691)

[Martin Sebor]

Bug 79691 - -Wformat-truncation suppressed by (and only by) -Og
points out that the gimple-ssa-sprintf pass doesn't run when
this optimization option is used.  That's because I forgot to
add it to the set of optimization passes that run with that
option.  The attached trivial patch tested on x86_64 corrects
the oversight.

Is this okay for 7.0?

Martin
PR tree-optimization/79691 - -Wformat-truncation suppressed by (and only by) -Og

gcc/ChangeLog:

	PR c/79691
	* passes.def (pass_all_optimizations_g): Enable pass_sprintf_length.

gcc/testsuite/ChangeLog:

	PR c/79691
	* gcc.dg/tree-ssa/pr79691.c: New test.

diff --git a/gcc/passes.def b/gcc/passes.def
index c09ec22..6b0f05b 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -364,6 +364,7 @@ along with GCC; see the file COPYING3.  If not see
   NEXT_PASS (pass_object_sizes);
   /* Fold remaining builtins.  */
   NEXT_PASS (pass_fold_builtins);
+  NEXT_PASS (pass_sprintf_length, true);
   /* Copy propagation also copy-propagates constants, this is necessary
  to forward object-size and builtin folding results properly.  */
   NEXT_PASS (pass_copy_prop);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr79691.c b/gcc/testsuite/gcc.dg/tree-ssa/pr79691.c
new file mode 100644
index 000..badbfcd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79691.c
@@ -0,0 +1,27 @@
+/* PR tree-optimization/79691 - -Wformat-truncation suppressed by
+   (and only by) -Og
+
+   { dg-compile }
+   { dg-options "-Og -Wall" } */
+
+char d[2];
+
+/* Verify -Wformat-overflow works.  */
+void f (void)
+{
+  __builtin_sprintf (d, "%i", 123);   /* { dg-warning "directive writing 3 bytes" } */
+}
+
+/* Verify -Wformat-truncation works.  */
+void g (void)
+{
+  __builtin_snprintf (d, sizeof d, "%i", 1234);   /* { dg-warning "output truncated writing 4 bytes" } */
+}
+
+/* Verify -fprintf-return-value works.  */
+int h (void)
+{
+  return __builtin_snprintf (0, 0, "%i", 12345);
+}
+
+/* { dg-final { scan-tree-dump-not "snprintf" "optimized" } } */

Re: enable -Wformat-truncation with -Og (PR 79691)

[Martin Sebor]

On 02/24/2017 03:10 AM, Richard Biener wrote:

On Fri, Feb 24, 2017 at 1:35 AM, Martin Sebor <mse...@gmail.com> wrote:

Bug 79691 - -Wformat-truncation suppressed by (and only by) -Og
points out that the gimple-ssa-sprintf pass doesn't run when
this optimization option is used.  That's because I forgot to
add it to the set of optimization passes that run with that
option.  The attached trivial patch tested on x86_64 corrects
the oversight.

Is this okay for 7.0?


Any reason for the placement before copy-prop?  I'd have done it
after pass_late_warn_uninitialized for example.


I wanted to make sure that folded sprintf return values would be
eligible for further copy propagation.  E.g., that a + b would
be folded into a constant:

  int foo (void)
  {
int a = snprintf (0, 0, "%i", 123);
int b = snprintf (0, 0, "%i", 1234);
return a + b;
  }

But I could have easily missed some important use case where this
placement will compromise the warning.  I don't have any tests
for this one way or the other so I'm happy to go with your
recommendation.  Let me know which you think is more appropriate
(if you have a suggestion for a test case I'd be grateful).



Also doesn't pass_sprintf_length rely on get_range_info ()?  With -Og
nothing populates those so you'll always get effectively VARYING ranges.


It does when it's available but as Jakub noted, it works without
it as well (at -O0).

Thanks
Martin

[PATCH] avoid using upper bound of width and precision in -Wformat-overlow (PR 79692)

[Martin Sebor]

In an arithmetic directive with the width or precision specified
by an argument to the asterisk (e.g., "%*x") and where the argument
range is unknown, for the purposes of the return value optimization
the pass must assume it's potentially as large as INT_MAX.  But for
the purposes of issuing a warning, that assumption leads to false
positives because the value of the argument can and in reality
usually is much smaller.

The attached patch adjusts the checker to use the lower bound in
this case to avoid these false positives.  It does that for both
integer and floating directives (for the latter it uses the lesser
of 3 and the lower bound in this case).

In addition, the patch corrects the handling of the pound flag ('#')
in "%#.*g" directives which makes it keep as many trailing zeros after
the radix character as specified by the precision.  (Without the '#',
"%.*g" trims trailing zeros.)

Martin


PR middle-end/79692 - [7 Regression] -Wformat-overflow false positive

gcc/ChangeLog:

	PR c/79692
	* gimple-ssa-sprintf.c
	(directive::known_width_and_precision): New function.
	(format_integer): Use it.
	(get_mpfr_format_length): Consider the full range of precision
	when computing %g output with the # flag.  Set the likely byte
	count to 3 rather than 1 when precision is indeterminate.
	(format_floating): Correct the lower bound of precision.

gcc/testsuite/ChangeLog:

	PR c/79692
	* gcc.dg/tree-ssa/builtin-sprintf-2.c: Add test cases.
	* gcc.dg/tree-ssa/builtin-sprintf-warn-10.c: Correct %#g.
	* gcc.dg/tree-ssa/builtin-sprintf-warn-15.c: New test.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 7688439..6777092 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -692,6 +692,16 @@ struct directive
   {
 get_int_range (arg, integer_type_node, prec, prec + 1, false, -1);
   }
+
+  /* Return true if both width and precision are known to be
+ either constant or in some range, false otherwise.  */
+  bool known_width_and_precision () const
+  {
+return ((width[1] < 0
+	 || (unsigned HOST_WIDE_INT)width[1] <= target_int_max ())
+	&& (prec[1] < 0
+		|| (unsigned HOST_WIDE_INT)prec[1] < target_int_max ()));
+  }
 };
 
 /* Return the logarithm of X in BASE.  */
@@ -1180,10 +1190,10 @@ format_integer (const directive , tree arg)
 	  /* As a special case, a precision of zero with a zero argument
 	 results in zero bytes except in base 8 when the '#' flag is
 	 specified, and for signed conversions in base 8 and 10 when
-	 flags when either the space or '+' flag has been specified
-	 when it results in just one byte (with width having the normal
-	 effect).  This must extend to the case of a specified precision
-	 with an unknown value because it can be zero.  */
+	 either the space or '+' flag has been specified and it results
+	 in just one byte (with width having the normal effect).  This
+	 must extend to the case of a specified precision with
+	 an unknown value because it can be zero.  */
 	  res.range.min = ((base == 8 && dir.get_flag ('#')) || maybesign);
 	  if (res.range.min == 0 && dir.prec[0] != dir.prec[1])
 	{
@@ -1254,10 +1264,12 @@ format_integer (const directive , tree arg)
 	  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).  */
+	 of the directive's type and neither width nor precision
+	 is unknown.  (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));
+	= ((!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin)
+		|| !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax))
+	   && dir.known_width_and_precision ());
 
 	  res.argmin = argmin;
 	  res.argmax = argmax;
@@ -1421,12 +1433,12 @@ get_mpfr_format_length (mpfr_ptr x, const char *flags, HOST_WIDE_INT prec,
 
   HOST_WIDE_INT p = prec;
 
-  if (spec == 'G')
+  if (spec == 'G' && !strchr (flags, '#'))
 {
-  /* For G/g, precision gives the maximum number of significant
-	 digits which is bounded by LDBL_MAX_10_EXP, or, for a 128
-	 bit IEEE extended precision, 4932.  Using twice as much
-	 here should be more than sufficient for any real format.  */
+  /* For G/g without the pound flag, precision gives the maximum number
+	 of significant digits which is bounded by LDBL_MAX_10_EXP, or, for
+	 a 128 bit IEEE extended precision, 4932.  Using twice as much here
+	 should be more than sufficient for any real format.  */
   if ((IEEE_MAX_10_EXP * 2) < prec)
 	prec = IEEE_MAX_10_EXP * 2;
   p = prec;
@@ -1609,7 +1621,12 @@ format_floating (const directive )
 	/* Compute the upper bound for -TYPE_MAX.  */
 	res.range.max = format_floating_max (type, 'f', dir.prec[1]);
 
-	res.range.likely = res.range.min;
+	/* The minimum output with unknown precision is a 

Re: [PATCH] restore -Wunused-variable on a typedef'd variable in a function template (PR 79548)

[Martin Sebor]

On 02/22/2017 11:02 AM, Jason Merrill wrote:

On Tue, Feb 21, 2017 at 4:27 PM, Martin Sebor <mse...@gmail.com> wrote:

Ah, I see, your patch changes attribute unused handling for local
variables from tracking TREE_USED to lookup_attribute.  I'm not
opposed to this change, but I'd like to understand why the TREE_USED
handling wasn't working.



In the test case in the bug:

  template 
  void g ()
  {
T t;   // warning, ok

typedef T U;
U u;   // no warning, bug
  }

  template void g();

both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
so the function doesn't set already_used or TREE_USED(t) and we get
a warning as expected.

But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
(to implement -Wunused-local-typedefs),  initialize_local_var then
sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
the warning.


Hmm, I would expect maybe_record_typedef_use to set TREE_USED on the
TYPE_DECL, not on the *_TYPE which initialize_local_var checks.


That's what it does:

  void
  maybe_record_typedef_use (tree t)
  {
if (!is_typedef_decl (t))
  return;

TREE_USED (t) = true;
  }

Here, t is a TYPE_DECL of the typedef U.

It has the effect of TREE_USED (TREE_TYPE (decl)) being set in
initialize_local_var.  The TREE_USED bit on the type (i.e., on
TREE_TYPE(decl) where decl is the u in the test case above) is
set when the function template is instantiated, in
set_underlying_type called from tsubst_decl.

Martin

Re: [PATCH, doc]: Mention that -mfpmath=sse is the default on 32bit x86 w/ SSE2 and -ffast-math

[Martin Sebor]

On 02/23/2017 04:09 AM, Uros Bizjak wrote:

Hello!

This patch documents a little gcc secret...

2017-02-23  Uros Bizjak  

* doc/invoke.texi (x86 Options, -mfpmath=sse): Mention that
-mfpmath=sse is the default also for x86-32 targets with SSE2
instruction set when @option{-ffast-math} is enabled

Bootstrapped on x86_64-linux-gnu.

Uros.



A minor grammatical nit:

  +This is the default choice for most of x86-32 targets.

"for most x86-32 targets" is correct unless the targets are some
specific subset, in which case "most of the [previously mentioned]
x86-32 targets" would work.

Martin

Re: [PATCH] restore -Wunused-variable on a typedef'd variable in a function template (PR 79548)

[Martin Sebor]

On 02/21/2017 11:08 AM, Jason Merrill wrote:

On 02/17/2017 05:07 PM, Martin Sebor wrote:

* decl.c (poplevel): Avoid diagnosing entities declared with
attribute unused.


This change is OK.


(initialize_local_var): Do not consider the type of a variable
when determining whether or not it's used.


This is not; the documentation for attribute unused says,

When attached to a type (including a @code{union} or a @code{struct}),
this attribute means that variables of that type are meant to appear
possibly unused.  GCC does not produce a warning for any variables of
that type, even if the variable appears to do nothing.  This is often
the case with lock or thread classes, which are usually defined and then
not referenced, but contain constructors and destructors that have
nontrivial bookkeeping functions.

So a TREE_USED type should imply TREE_USED on variables of that type.


Yes, I'm familiar with the effect of the attribute on types but
in my testing this change doesn't affect how it works (i.e., it
passes a full bootstrap and regression tests and I haven't been
able to construct a failing test case.)  It looks like that's
because TREE_USED(decl) is already set here for a decl whose
type is declared attribute used.

While trying to come up with a test case to exercise the scenario
you describe I see that current trunk doesn't handle it correctly
but the patch just happens to fix that too.  For example:

template 
void f ()
{
  T t;   // trunk warns for f (good)

  typedef T U;
  U u;   // trunk doesn't warn for f (bug 79548)
}

template void f();

struct __attribute__ ((unused)) S { };

template void f();   // no warnings here (good)

void g ()
{
  S s;

  typedef S T;
  T t;   // trunk warns here (new bug), doesn't with the patch
}

In the test case above, TREE_USED (TREE_TYPE (decl)) is set for
t in g() so trunk sets it on t too and warbs.  The patch doesn't
and so it doesn't warn as expected.

But it's entirely possible I'm missing a case.  Do you have
a suggestion for a test that trunk handles correctly but that
fails with the patch?

Thanks
Martin

[PATCH] avoid eliminating snprintf(d, n, ...) whose zero size comes from a range (PR 79496)

[Martin Sebor]

When the size of the destination in a call to snprintf is in
a range, at level 1 -Wformat-truncation uses the upper bound
as the size while the stricter level 2 uses the lower bound.
However, when the lower bound is zero treating it the same as
a constant zero and optimizing the call into a constant isn't
correct because the actual argument need not be zero and the
output of the function is important.  The attached patch
avoids this unsafe transformation.

Is this okay for trunk?

Martin
PR middle-end/79496 - call to snprintf with zero size eliminated with -Wformat-truncation=2

gcc/ChangeLog:

	PR middle-end/79496
	* gimple-ssa-sprintf.c (pass_sprintf_length::handle_gimple_call): Avoid
	clearing info.nowrite flag when snprintf size argument is a range.

gcc/testsuite/ChangeLog:

	PR middle-end/79496
	* gcc.dg/tree-ssa/builtin-snprintf-2.c: New test.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index bf76162..1079a41 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -3452,6 +3452,10 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
 
   info.format = gimple_call_arg (info.callstmt, idx_format);
 
+  /* True when the destination size is constant as opposed to the lower
+ or upper bound of a range.  */
+  bool dstsize_cst_p = true;
+
   if (idx_dstsize == HOST_WIDE_INT_M1U)
 {
   /* For non-bounded functions like sprintf, determine the size
@@ -3492,8 +3496,8 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
   else if (TREE_CODE (size) == SSA_NAME)
 	{
 	  /* Try to determine the range of values of the argument
-	 and use the greater of the two at -Wformat-level 1 and
-	 the smaller of them at level 2.  */
+	 and use the greater of the two at level 1 and the smaller
+	 of them at level 2.  */
 	  wide_int min, max;
 	  enum value_range_type range_type
 	= get_range_info (size, , );
@@ -3504,6 +3508,11 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
 		   ? wi::fits_uhwi_p (max) ? max.to_uhwi () : max.to_shwi ()
 		   : wi::fits_uhwi_p (min) ? min.to_uhwi () : min.to_shwi ());
 	}
+
+	  /* The destination size is not constant.  If the function is
+	 bounded (e.g., snprintf) a lower bound of zero doesn't
+	 necessarily imply it can be eliminated.  */
+	  dstsize_cst_p = false;
 	}
 }
 
@@ -3520,7 +3529,7 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
 	 without actually producing any.  Pretend the size is
 	 unlimited in this case.  */
   info.objsize = HOST_WIDE_INT_MAX;
-  info.nowrite = true;
+  info.nowrite = dstsize_cst_p;
 }
   else
 {
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-2.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-2.c
new file mode 100644
index 000..a192aee
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-2.c
@@ -0,0 +1,24 @@
+/* PR middle-end/79496 - call to snprintf with non-zero size eliminated
+   with -Wformat-truncation=2
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wformat-truncation=2 -fprintf-return-value -fdump-tree-optimized" } */
+
+char d[2];
+
+int test_cst (unsigned n)
+{
+  if (1 < n)
+n = 0;
+
+  return __builtin_snprintf (d, n, "%d", 1);
+}
+
+int test_var (char *d, unsigned n)
+{
+  if (2 < n)
+n = 0;
+
+  return __builtin_snprintf (d, n, "%i", 1);
+}
+
+/* { dg-final { scan-tree-dump-times "snprintf" 2 "optimized"} } */

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 doc] update -dM to mention excluded macros (PR 41540)

[Martin Sebor]

On 02/12/2017 01:27 AM, Gerald Pfeifer wrote:

On Tue, 7 Feb 2017, Martin Sebor wrote:

The attached documentation-only patch clarifies the description
of the -dM option to mention that  __FILE__ (and other predefined
macros) do no appear on the list generated by the option.


+The predefined macros @code{__FILE__}, @code{__LINE__}, @code{__DATE__},
+and @code{__TIME__} are excluded from this list because their replacements
+may change from one line of output to the next.

Is this ("their replacements may change") truefor __FILE__ as well?

In any case, this may be too nit-picking, and the patch looks fine
to me.


Thanks.  __FILE__ too can change between different lines of output:
by means of a #line directive.

I actually think it would be preferable if GCC printed the value of
__FILE__ and all the other macros, including the four mentioned here.
But since in response to the bug report Andrew implied that not
printing them is intentional I figured I'd just update the docs
and resolve this old bug (raised in 2009).

That said, I find the argument that these four are special because
their value can change from line to line a weak one given that the
definition of any predefined macro can change (simply by undefining
and redefining it something else).

Is there a better argument for not printing __FILE__ et al?  Do we
want to document this or change GCC to print all macros?

Martin

[PATCH] suppress unhelpful -Wformat-truncation=2 INT_MAX warning (PR 79448)

[Martin Sebor]

The recent Fedora mass rebuild revealed that the Wformat-truncation=2
checker is still a bit too aggressive and complains about potentially
unbounded strings causing subsequent directives t exceed the INT_MAX
limit.  (It's unclear how the build ended up enabling level 2 of
the warning.)

This is because for the purposes of the return value optimization
the pass must assume that such strings really are potentially unbounded
and result in as many as INT_MAX bytes (or more).  That doesn't mean
that it should warn on such cases.

The attached patch relaxes the checker to avoid the warning in this
case.  Since there's no easy way for a user to suppress the warning,
is this change okay for trunk at this stage?

Martin
PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning

gcc/testsuite/ChangeLog:

	PR middle-end/79448
	* gcc.dg/tree-ssa/builtin-snprintf-warn-3.c: New test.
	* gcc.dg/tree-ssa/pr79448-2.c: New test.
	* gcc.dg/tree-ssa/pr79448.c: New test.

gcc/ChangeLog:

	PR middle-end/79448
	* gimple-ssa-sprintf.c (format_directive): Avoid issuing INT_MAX
	  warning for strings of unknown length.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index e6cc31d..bf76162 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2561,11 +2561,15 @@ format_directive (const pass_sprintf_length::call_info ,
   /* Raise the total unlikely maximum by the larger of the maximum
  and the unlikely maximum.  It doesn't matter if the unlikely
  maximum overflows.  */
+  unsigned HOST_WIDE_INT save = res->range.unlikely;
   if (fmtres.range.max < fmtres.range.unlikely)
 res->range.unlikely += fmtres.range.unlikely;
   else
 res->range.unlikely += fmtres.range.max;
 
+  if (res->range.unlikely < save)
+res->range.unlikely = HOST_WIDE_INT_M1U;
+
   res->range.min += fmtres.range.min;
   res->range.likely += fmtres.range.likely;
 
@@ -2616,7 +2620,12 @@ format_directive (const pass_sprintf_length::call_info ,
 
   /* Has the likely and maximum directive output exceeded INT_MAX?  */
   bool likelyximax = *dir.beg && res->range.likely > target_int_max ();
-  bool maxximax = *dir.beg && res->range.max > target_int_max ();
+  /* Don't consider the maximum to be in excess when it's the result
+ of a string of unknown length (i.e., whose maximum has been set
+ to HOST_WIDE_INT_M1U.  */
+  bool maxximax = (*dir.beg
+		   && res->range.max > target_int_max ()
+		   && res->range.max < HOST_WIDE_INT_MAX);
 
   if (!warned
   /* Warn for the likely output size at level 1.  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c
new file mode 100644
index 000..81c1d89
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c
@@ -0,0 +1,193 @@
+/* PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
+   { dg-do compile }
+   { dg-options "-O2 -Wformat -Wformat-truncation=2 -ftrack-macro-expansion=0" } */
+
+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
+
+extern int int_value (void);
+extern size_t size_value (void);
+
+int int_range (int min, int max)
+{
+  int n = int_value ();
+  return n < min || max < n ? min : n;
+}
+
+void sink (int, char*, char*);
+
+int dummy_snprintf (char*, size_t, const char*, ...);
+
+char fixed_buffer [256];
+extern char *unknown_buffer;
+extern size_t unknown_size;
+
+/* 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)
+
+/* Helper test macro.  */
+#define T(size, ...)	\
+  do {			\
+size_t n = size < 0 ? unknown_size : size;		\
+char *buf = size < 0 ? unknown_buffer		\
+  : n < sizeof fixed_buffer\
+  ? fixed_buffer + sizeof fixed_buffer - size	\
+  : unknown_buffer;	\
+FUNC (snprintf) (buf, n, __VA_ARGS__);		\
+sink (0, fixed_buffer, unknown_buffer);		\
+  } while (0)
+
+/* Return a value in the range [MIN, MAX].  */
+#define IR(min, max)  int_range (min, max)
+
+struct Arrays
+{
+  char a1[1];
+  char a4k[4096];
+  char a4kp1[4097];
+#if INT_MAX < LONG_MAX
+  char amax[INT_MAX];
+#else
+  char amax[32767];
+#endif
+  char ax[];
+};
+
+void test_string_unchecked (const char *s, const struct Arrays *ar)
+{
+  /* Verify there is no warning with strings of unknown length.  */
+  T (-1, "%-s", s);
+  T (-1, "%-s", ar->ax);
+
+  T (-1, "%s%s", s, s);
+  T (-1, "%s%s", "", s);
+  T (-1, "%s%s", s, "1");
+  T (-1, "%s%s", "1", s);
+
+  /* Verify there is no warning with strings of length that cannot
+ exceed 4k (because of 

Re: [PATCH] avoid ICE when attempting to init a flexible array member (PR c++/79363)

[Martin Sebor]

Ping:
  https://gcc.gnu.org/ml/gcc-patches/2017-02/msg00489.html

On 02/06/2017 07:04 PM, Martin Sebor wrote:

The attached patch avoids another ICE (in addition to the already
fixed bug 72775) in flexible array member NSDMI.  To avoid code
duplication and for consistency I factored the diagnostic code
out of perform_member_init and into a new helper.

Martin

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] suppress unhelpful -Wformat-truncation=2 INT_MAX warning (PR 79448)

[Martin Sebor]

On 02/13/2017 04:33 PM, Jeff Law wrote:

On 02/10/2017 10:55 AM, Martin Sebor wrote:

The recent Fedora mass rebuild revealed that the Wformat-truncation=2
checker is still a bit too aggressive and complains about potentially
unbounded strings causing subsequent directives t exceed the INT_MAX
limit.  (It's unclear how the build ended up enabling level 2 of
the warning.)

This is because for the purposes of the return value optimization
the pass must assume that such strings really are potentially unbounded
and result in as many as INT_MAX bytes (or more).  That doesn't mean
that it should warn on such cases.

The attached patch relaxes the checker to avoid the warning in this
case.  Since there's no easy way for a user to suppress the warning,
is this change okay for trunk at this stage?

Martin

gcc-79448.diff


PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning

gcc/testsuite/ChangeLog:

PR middle-end/79448
* gcc.dg/tree-ssa/builtin-snprintf-warn-3.c: New test.
* gcc.dg/tree-ssa/pr79448-2.c: New test.
* gcc.dg/tree-ssa/pr79448.c: New test.

gcc/ChangeLog:

PR middle-end/79448
* gimple-ssa-sprintf.c (format_directive): Avoid issuing INT_MAX
  warning for strings of unknown length.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index e6cc31d..bf76162 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2561,11 +2561,15 @@ format_directive (const
pass_sprintf_length::call_info ,
   /* Raise the total unlikely maximum by the larger of the maximum
  and the unlikely maximum.  It doesn't matter if the unlikely
  maximum overflows.  */
+  unsigned HOST_WIDE_INT save = res->range.unlikely;
   if (fmtres.range.max < fmtres.range.unlikely)
 res->range.unlikely += fmtres.range.unlikely;
   else
 res->range.unlikely += fmtres.range.max;

+  if (res->range.unlikely < save)
+res->range.unlikely = HOST_WIDE_INT_M1U;
+

So this looks like you're doing an overflow check -- yet earlier your
comment says "It doesnt' matter if the unlikely maximum overflows". ISTM
that comment needs updating -- if it doesn't matter, then why check for
it and clamp the value?



   res->range.min += fmtres.range.min;
   res->range.likely += fmtres.range.likely;

@@ -2616,7 +2620,12 @@ format_directive (const
pass_sprintf_length::call_info ,

   /* Has the likely and maximum directive output exceeded INT_MAX?  */
   bool likelyximax = *dir.beg && res->range.likely > target_int_max ();
-  bool maxximax = *dir.beg && res->range.max > target_int_max ();
+  /* Don't consider the maximum to be in excess when it's the result
+ of a string of unknown length (i.e., whose maximum has been set
+ to HOST_WIDE_INT_M1U.  */
+  bool maxximax = (*dir.beg
+   && res->range.max > target_int_max ()
+   && res->range.max < HOST_WIDE_INT_MAX);

So your comment mentions HOST_WIDE_INT_M1U as the key for a string of
unknown length.  But that doesn't obviously correspond to what the code
checks.

Can you please fix up the two comments.  With the comments fixed, this
is OK.


Sure, I updated the comments.

The code alternately uses HOST_WIDE_INT_M1U and HOST_WIDE_INT_MAX as
a stand-in for either a "can't happen" or "unbounded/unknown" size.
It's not fully consistent and should be cleaned up and the uses of
HOST_WIDE_INT should be replaced by a class like wide_int as someone
suggested in the past.  If I get to some of the enhancements I'd like
to make in stage 1 (e.g., integrating the pass with tree-ssa-strlen)
I'll see about cleaning this up.

Martin

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  <ja...@redhat.com>

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

Re: [PATCH doc] clean up -fdump-tree- options (PR 32003)

[Martin Sebor]

On 02/15/2017 05:51 AM, Thomas Schwinge wrote:

Hi!

On Wed, 1 Feb 2017 20:26:24 -0700, Martin Sebor <mse...@gmail.com> wrote:

On 02/01/2017 08:06 PM, Sandra Loosemore wrote:

On 02/01/2017 06:57 PM, Martin Sebor wrote:

PR middle-end/32003
* doc/invoke.texi (-fdump-rtl-): Remove pass-specific options from
index.


"rtl" vs. "tree" typo.  ;-)


--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -544,29 +544,9 @@ Objective-C and Objective-C++ Dialects}.
 -fdump-rtl-@var{pass}  -fdump-rtl-@var{pass}=@var{filename} @gol
 -fdump-statistics @gol
 -fdump-tree-all @gol
--fdump-tree-original@r{[}-@var{n}@r{]}  @gol
-[...]
--fdump-tree-storeccp@r{[}-@var{n}@r{]} @gol
--fdump-final-insns=@var{file} @gol


Is it intentional that you've also removed "-fdump-final-insns" here?
(It remains documented further down the file.)


No, I meant to only remove the -fdump-tree-xxx options in this pass.
I committed r245493 to restore it and fix the ChangeLog.  Thanks for
the review and for pointing it out!

Martin

Re: [PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)

[Martin Sebor]

On 01/17/2017 08:26 AM, Jeff Law wrote:

On 01/16/2017 05:06 PM, Martin Sebor wrote:

The test case submitted in bug 79095 - [7 regression] spurious
stringop-overflow warning shows that GCC optimizes some loops
into calls to memset with size arguments in excess of the object
size limit.  Since such calls will unavoidably lead to a buffer
overflow and memory corruption the attached patch detects them
and replaces them with a trap.  That both prevents the buffer
overflow and eliminates the warning.

But doesn't the creation of the bogus memset signal an invalid
transformation in the loop optimizer?  ie, if we're going to convert a
loop into a memset, then we'd damn well better be sure the loop bounds
are reasonable.


I'm not sure that emitting the memset call is necessarily a bug in
the loop optimizer (which in all likelihood wasn't written with
the goal of preventing or detecting possible buffer overflows).
The loop with the excessive bound is in the source code and can
be reached given the right inputs (calling v.resize(v.size() - 1)
on an empty vector.  It's a lurking bug in the program that, if
triggered, will overflow the vector and crash the program (or worse)
with or without the optimization.

What else could the loop optimizer could do in this instance?
I suppose it could just leave the loop alone and avoid emitting
the memset call.  That would avoid the warning but mask the
problem with the overflow.  In my mind, preventing the overflow
given that we have the opportunity is the right thing to do.
That is, after all, the goal of the warning.

As I mentioned privately yesterday, I'm actually pleasantly
surprised that it's helped identify this opportunity in GCC itself.
My hope was to eventually go and find the places where GCC emits
potentially out of bounds calls (based on user inputs) and fix them
to emit better code on the assumption that they can't be valid or
replace them with traps if they could happen in a running program.
It didn't occur to me that the warning itself would help find them.

Martin

Re: [PATCH] adding missing LTO to some warning options (PR 78606)

[Martin Sebor]

On 01/17/2017 05:04 AM, Kyrill Tkachov wrote:

Hi Martin,

On 10/01/17 22:16, Martin Sebor wrote:

The -Walloca-larger-than, -Wformat-length, and -Wformat-truncation
options do not mention LTO among the supported languages and so are
disabled when -flto is used, causing false negatives.

The attached patch adds the missing LTO to the three options. This
makes -Walloca-larger-than work with LTO but not the other two
options, implying that something else is preventing the gimple-ssa-
sprintf pass from running when -flto is enabled.  I haven't had
the cycles to look into what that might be yet.  Since the root
causes are independent I'd like to commit this patch first and
deal with the  -Wformat-{length,truncation} problem separately,
under a new bug (or give someone with a better understanding of
LTO the opportunity to do it).



I see the new test FAILing on arm and aarch64 targets.
FAIL: gcc.dg/pr78768.c execution test


Thanks.  The test doesn't need to run.  It just needs to link.
I changed it in r244537.

Martin

Re: [PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)

[Martin Sebor]

On 01/17/2017 12:38 AM, Jakub Jelinek wrote:

On Mon, Jan 16, 2017 at 05:06:40PM -0700, Martin Sebor wrote:

The test case submitted in bug 79095 - [7 regression] spurious
stringop-overflow warning shows that GCC optimizes some loops
into calls to memset with size arguments in excess of the object
size limit.  Since such calls will unavoidably lead to a buffer
overflow and memory corruption the attached patch detects them
and replaces them with a trap.  That both prevents the buffer
overflow and eliminates the warning.


I fear this is going to break various 32-bit database programs and similar
that mmap say 3GB of RAM and then work on that memory chunk as contiguous.
Some things don't work too well in that case (pointer differences), but it
is unlikely they would be using those, while your patch actively breaks it
even for loops that can be transformed into memset (memcpy of course isn't a
problem, because you need some virtual address space to copy it from).


I agree that breaking those applications would be bad.  It could
be dealt with by adding an option to let them disable the insertion
of the trap.  With the warning, programmers would get a heads up
that their (already dubious) code won't work otherwise.  I don't
think it's a necessary or wise to have the default mode be the most
permissive (and most dangerous) and expect users to tweak options
to make it safe.  Rather, I would argue that it should be the other
way around.  Make the default safe and strict and let the advanced
users who know how deal with the risks tweak those options.

Martin

Re: [PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)

[Martin Sebor]

On 01/17/2017 10:57 AM, Jeff Law wrote:

On 01/17/2017 09:12 AM, Martin Sebor wrote:

On 01/17/2017 08:26 AM, Jeff Law wrote:

On 01/16/2017 05:06 PM, Martin Sebor wrote:

The test case submitted in bug 79095 - [7 regression] spurious
stringop-overflow warning shows that GCC optimizes some loops
into calls to memset with size arguments in excess of the object
size limit.  Since such calls will unavoidably lead to a buffer
overflow and memory corruption the attached patch detects them
and replaces them with a trap.  That both prevents the buffer
overflow and eliminates the warning.

But doesn't the creation of the bogus memset signal an invalid
transformation in the loop optimizer?  ie, if we're going to convert a
loop into a memset, then we'd damn well better be sure the loop bounds
are reasonable.


I'm not sure that emitting the memset call is necessarily a bug in
the loop optimizer (which in all likelihood wasn't written with
the goal of preventing or detecting possible buffer overflows).
The loop with the excessive bound is in the source code and can
be reached given the right inputs (calling v.resize(v.size() - 1)
on an empty vector.  It's a lurking bug in the program that, if
triggered, will overflow the vector and crash the program (or worse)
with or without the optimization.

Right, but that doesn't mean that the loop optimizer can turn it into a
memset.  If the bounds are such that we're going to invoke undefined
behaviour from memset, then the loop optimizer must leave the loop alone.



What else could the loop optimizer could do in this instance?
I suppose it could just leave the loop alone and avoid emitting
the memset call.  That would avoid the warning but mask the
problem with the overflow.  In my mind, preventing the overflow
given that we have the opportunity is the right thing to do.
That is, after all, the goal of the warning.

The right warning in this case is WRT the loop iteration space
independent of mem*.


I agree that warning for the user code would be appropriate if
the loop with the excessive bound were unavoidable or at least
reachable.  But in the submitted test case it isn't because
the call to vector::resize() is guarded.  In fact, the out of-
bounds memset is also emitted with this modified test case:

  void f (std::vector )
  {
size_t n = v.size ();

if (n > 1 && n < 5)
  v.resize (n - 1);
  }

I believe the root cause of the the out-of-bounds memset is the
lack of support for pointer ranges or at least some notion of
their relationships and constraints.  A std::vector is defined
by three pointers that satisfy the following relation:

  start <= finish <= end_of_storage

Queries about the capacity and size of a vector are done in terms
of expressions involving these three pointers:

  size_type capacity () {
return end_of_storage - start;
  }

  size_type size () {
return finish - start;
  }

Space remaining is hand-coded as

  end_of_storage - finish

The trouble is that GCC has no idea about the constraints on
the three pointers and so it treats them essentially as unrelated
integers with no ranges (except for their scale that depends on
the type the pointer points to).

Absent support for pointer ranges, GCC would be able to generate
much better code with just some help from annotations telling it
about at least some of the constraints.  In this case, for example,
adding the following optimistic invariant:

   size_type space_left = end_of_storage - finish;

   if (space_left > SIZE_MAX / 2 / sizeof (T))
 __builtin_unreachable ();

to vector::_M_default_append() lets GCC avoid the memset and emit
significantly more efficient code.  On x86_64 it reduces the number
instructions for the test case by 40%.

Martin

Re: [PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)

[Martin Sebor]

On 01/18/2017 01:10 AM, Jakub Jelinek wrote:

On Tue, Jan 17, 2017 at 10:59:43PM -0700, Jeff Law wrote:

I agree that breaking those applications would be bad.  It could
be dealt with by adding an option to let them disable the insertion
of the trap.  With the warning, programmers would get a heads up
that their (already dubious) code won't work otherwise.  I don't
think it's a necessary or wise to have the default mode be the most
permissive (and most dangerous) and expect users to tweak options
to make it safe.  Rather, I would argue that it should be the other
way around.  Make the default safe and strict and let the advanced
users who know how deal with the risks tweak those options.

I still come back to the assertion that changing this loop to a mem* is
fundamentally the wrong thing to do as it changes something that has well
defined semantics to something that is invalid.

Thus the transformation into a mem* call is invalid.


The mem* call is as valid as the loop, it will work exactly the same.


The problem here isn't the loop itself or the memset call but the
bounds computed from the inputs.

In the test case submitted with the bug the inputs supplied by
the program are well within a valid range, yet the bounds of
the loop that are computed by GCC from those inputs are excessive
because they are based on impossible ranges (or an incomplete view
of the program).

There is no unbounded loop in

  void f(std::vector )
  {
size_t n = v.size ();

if (n > 1 && n < 5)
  v.resize (n - 1);
  }

yet GCC synthesizes one:

  void f(std::vector&) (struct vector & v)
  {
...
 [100.00%]:
_7 = MEM[(int * *)v_5(D)]; // v._M_impl._M_start
_8 = MEM[(int * *)v_5(D) + 8B];// v._M_impl._M_finish
_9 = (long int) _8;
_10 = (long int) _7;
_11 = _9 - _10;
_12 = _11 /[ex] 4;
_13 = (long unsigned int) _12; // v.size()
_1 = _13 + 18446744073709551614;   // v.size() - 4
if (_1 <= 2)   // if (v.size() <= 6)
  goto ; [36.64%]
else
  goto ; [63.36%]

 [36.64%]:
_2 = _13 + 18446744073709551615;   // v.size() - 1
if (_2 > _13)  // if (v.size() == 0)
  goto ; [29.56%]//   this can't happen
else
  goto ; [70.44%]

 [5.85%]:
_24 = v_5(D)->D.15828._M_impl._M_end_of_storage;
_25 = (long int) _24;
_28 = _25 - _9;// bytes left
if (_28 == -4) // this can't happen
  goto ; [67.00%]
else
  goto ; [33.00%]

 [3.92%]:
__builtin_memset (_8, 0, 18446744073709551612);   // (size_t)-4
...

 [1.93%]:
std::__throw_length_error ("vector::_M_default_append");


If you have say on 32-bit target
#include 

int
main ()
{
  char *p = malloc (3U * 1024 * 1024 * 1024);
  if (p == NULL)
return 0;
  size_t i;
  for (i = 0; i < 3U * 1024 * 1024 * 1024; i++)
p[i] = 6;
  use (p);
  return 0;
}


Unlike in the test case submitted with the bug, here the loop is in
the source and warning on it would be justified for most programs.

Here's a test case that's closer to the one from the bug.  It also
ends up with the out of bounds memset even at -O1, during PRE.

typedef __SIZE_TYPE__ size_t;

struct S
  int *p0, *p1, *p2;

  size_t size () const { return p1 - p0; }

  void f (size_t n) {
if (n > size ())   // can't happen because
  foo (n - size ());   //   n is in [1, MIN(size() - 1, 3)]
else if (n < size ())
  bar (p0 + n);
  }

  void foo (size_t n)
  {
size_t left = (size_t)(p2 - p1);
if (left >= n)
  __builtin_memset (p2, 0, n * sizeof *p2);
  }

  void bar (int*);
};

void f (S )
{
  size_t n = s.size ();
  if (n > 1 && n < 5)
s.f (n - 1);
}

Martin

Re: [PR middle-end/79123] cast false positive in -Walloca-larger-than=

[Martin Sebor]

On 01/20/2017 01:17 AM, Richard Biener wrote:

On Thu, Jan 19, 2017 at 5:53 PM, Martin Sebor <mse...@gmail.com> wrote:

On 01/19/2017 05:45 AM, Richard Biener wrote:


On Thu, Jan 19, 2017 at 1:17 PM, Aldy Hernandez <al...@redhat.com> wrote:


In the attached testcase, we have a clearly bounded case of alloca which
is
being incorrectly reported:

void g (int *p, int *q)
{
   size_t n = (size_t)(p - q);

   if (n < 10)
 f (__builtin_alloca (n));
}

The problem is that VRP gives us an anti-range for `n' which may be out
of
range:

  # RANGE ~[2305843009213693952, 16140901064495857663]
   n_9 = (long unsigned int) _4;

We do a less than stellar job with casts and VR_ANTI_RANGE's, mostly
because
we're trying various heuristics to make up for the fact that we have
crappy
range info from VRP.  More specifically, we're basically punting on an
VR_ANTI_RANGE and ignoring that the casted result (n_9) has a bound later
on.

Luckily, we already have code to check simple ranges coming into the
alloca
by looking into all the basic blocks feeding it.  The attached patch
delays
the final decision on anti ranges until we have examined the basic blocks
and determined for that we are definitely out of range.

I expect all this to disappear with Andrew's upcoming range info
overhaul.

OK for trunk?



I _really_ wonder why all the range consuming warnings are not emitted
from VRP itself (like we do for -Warray-bounds).  There we'd still see
a range for the argument derived from the if () rather than needing to
do our own mini-VRP from the needessly "incomplete" range-info on
SSA vars.



Can you explain why the range info is only available in VRP and
not outside, via the get_range_info() API?  It sounds as though
the API shouldn't be relied on (it was virtually unused before
GCC 7).


It's very simple.  Look at the testcase from above


Thanks for the detailed answer.  A few more questions below.



void g (int *p, int *q)
{
   size_t n = (size_t)(p - q);

   if (n < 10)
 f (__builtin_alloca (n));
}

The IL outside of VRP is

   [100.00%]:
  p.0_1 = (long int) p_7(D);
  q.1_2 = (long int) q_8(D);
  _3 = p.0_1 - q.1_2;
  _4 = _3 /[ex] 4;
  n_9 = (size_t) _4;
  if (n_9 <= 9)
goto ; [36.64%]
  else
goto ; [63.36%]

   [36.64%]:
  _5 = __builtin_alloca (n_9);
  f (_5);

so there is no SSA name we can tack a range to covering the n_9 <= 9
condition that dominates __builtin_alloca.


This may be a naive question but why is it not possible to create
such an SSA name?

Having the get_range_info() API depend on this subtlety makes its
clients quite unreliable.  It may be acceptable for optimization
but when it's visible to users in the form of false positives or
negatives it's a source of bug reports.


Inside VRP we have

   [100.00%]:
  p.0_1 = (long int) p_7(D);
  q.1_2 = (long int) q_8(D);
  _3 = p.0_1 - q.1_2;
  _4 = _3 /[ex] 4;
  n_9 = (size_t) _4;
  if (n_9 <= 9)
goto ; [36.64%]
  else
goto ; [63.36%]

   [36.64%]:
  n_13 = ASSERT_EXPR <n_9, n_9 <= 9>;
  _5 = __builtin_alloca (n_13);
  f (_5);

and viola - now the alloca call uses n_13 which is defined at a point
dominated by if (n_9 <= 9) and thus it has an improved range:

n_13: [0, 9]  EQUIVALENCES: { n_9 } (1 elements)


Yes, I see that.  But when I change size_t to unsigned int (in LP64)
like so:

  void g (int *p, int *q)
  {
unsigned n = (unsigned)(p - q);

if (n < 10)
  f (__builtin_alloca (n));
  }

-Walloca-larger-than=100 still complains:

  warning: argument to ‘alloca’ may be too large
  note: limit is 100 bytes, but argument may be as large as 4294967295

and the VRP dump shows

  _5: [0, 4294967295]
  _14: [0, 4294967295]
  ...
_3 = p.0_1 - q.1_2;
_4 = _3 /[ex] 4;
n_10 = (unsigned int) _4;
if (n_10 <= 9)
  goto ; [36.64%]
else
  goto ; [63.36%]

 [36.64%]:
_14 = _4 & 4294967295;
_5 = (long unsigned int) _14;
_6 = __builtin_alloca (_5);

so it seems that even in VRP itself the range information isn't
quite good enough to avoid false positives.  (Perhaps that's one
the missed cases you mention below?)


When in EVRP you get similar behavior (well, there are some missed
cases I have patches queued for for GCC 8), but instead of modifying
the IL EVRP just temporarily sets the above range when it processes
BBs dominated by the condition.

So while for VRP you can put the warning code after propagation for
EVRP you'd have to do warning processing during propagation itself
(and EVRP doesn't iterate).


To answer your question, the gimple-ssa-sprintf pass that depends
heavily on ranges would also benefit from having access to the data
computed in the strlen pass that's not available outside it.

The -Wstringop-overflow and -Walloc-size-larger-than warnings depend
on both VRP and tree-object-size.

I have been thinking about how to integrate these passes in GCC 8
to improve the overall quality.  (By integrating I don't necessar

[PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)

[Martin Sebor]

The test case submitted in bug 79095 - [7 regression] spurious
stringop-overflow warning shows that GCC optimizes some loops
into calls to memset with size arguments in excess of the object
size limit.  Since such calls will unavoidably lead to a buffer
overflow and memory corruption the attached patch detects them
and replaces them with a trap.  That both prevents the buffer
overflow and eliminates the warning.

Martin
PR c++/79095 - [7 regression] spurious stringop-overflow warning

gcc/ChangeLog:

	PR c++/79095
	* tree-loop-distribution.c (maybe_emit_trap): New function.
	(generate_memset_builtin): Call it.
	(generate_memcpy_builtin): Same.

gcc/testsuite/ChangeLog:

	PR c++/79095
	* g++.dg/pr79095.C: New test.

Index: gcc/testsuite/g++.dg/pr79095.C
===
--- gcc/testsuite/g++.dg/pr79095.C	(revision 0)
+++ gcc/testsuite/g++.dg/pr79095.C	(working copy)
@@ -0,0 +1,41 @@
+/* PR c++/79095 - spurious stringop-overflow warning
+   { dg-do compile }
+   { dg-options "-O3 -Wall -fdump-tree-optimized" } */
+
+typedef long unsigned int size_t;
+
+inline void
+fill (int *p, size_t n, int)
+{
+  while (n--)
+*p++ = 0;
+}
+
+struct B
+{
+  int* p0, *p1, *p2;
+
+  size_t size () const {
+return size_t (p1 - p0);
+  }
+
+  void resize (size_t n) {
+if (n > size())
+  append (n - size());
+  }
+
+  void append (size_t n)
+  {
+if (size_t (p2 - p1) >= n) {
+  fill (p1, n, 0);
+}
+  }
+};
+
+void foo (B )
+{
+  b.resize (b.size () - 1);
+}
+
+
+/* { dg-final { scan-tree-dump-not "memset" "optimized" } } */
Index: gcc/tree-loop-distribution.c
===
--- gcc/tree-loop-distribution.c	(revision 244508)
+++ gcc/tree-loop-distribution.c	(working copy)
@@ -796,6 +796,49 @@ const_with_all_bytes_same (tree val)
   return buf[0];
 }
 
+/* If NBYTES is greater than SSIZE_MAX emit a trap and return true.
+   Otherwise return false.  FNCODE identifies the built-in function
+   being generated.  */
+
+static bool
+maybe_emit_trap (gimple_stmt_iterator gsi, tree nbytes,
+		 built_in_function fncode)
+{
+  if (TREE_CODE (nbytes) != INTEGER_CST
+  || tree_int_cst_le (nbytes, TYPE_MAX_VALUE (ssizetype)))
+return false;
+
+  tree fn = builtin_decl_implicit (BUILT_IN_TRAP);
+  gimple *fn_call = gimple_build_call (fn, 0);
+  gsi_insert_after (, fn_call, GSI_CONTINUE_LINKING);
+  split_block (gimple_bb (fn_call), fn_call);
+
+  if (dump_file && (dump_flags & TDF_DETAILS))
+{
+  const char *fname = 0;
+
+  switch (fncode)
+	{
+	case BUILT_IN_MEMCPY:
+	  fname = "memcpy";
+	  break;
+	case BUILT_IN_MEMMOVE:
+	  fname = "memove";
+	  break;
+	case BUILT_IN_MEMSET:
+	  fname = "memset";
+	  break;
+	default:
+	  gcc_unreachable ();
+	}
+
+  fprintf (dump_file, "generated trap for an out-of-bounds %s "
+	   "with %wu size", fname, tree_to_uhwi (nbytes));
+}
+
+  return true;
+}
+
 /* Generate a call to memset for PARTITION in LOOP.  */
 
 static void
@@ -817,6 +860,13 @@ generate_memset_builtin (struct loop *loop, partit
  partition->plus_one);
   nb_bytes = force_gimple_operand_gsi (, nb_bytes, true, NULL_TREE,
    false, GSI_CONTINUE_LINKING);
+
+  /* If the number of bytes is in excess of SSIZE_MAX avoid generating
+ the memset call that would certainly overflow and emit a trap
+ instead.  */
+  if (maybe_emit_trap (gsi, nb_bytes, BUILT_IN_MEMSET))
+return;
+
   mem = build_addr_arg_loc (loc, partition->main_dr, nb_bytes);
   mem = force_gimple_operand_gsi (, mem, true, NULL_TREE,
   false, GSI_CONTINUE_LINKING);
@@ -873,6 +923,7 @@ generate_memcpy_builtin (struct loop *loop, partit
  partition->plus_one);
   nb_bytes = force_gimple_operand_gsi (, nb_bytes, true, NULL_TREE,
    false, GSI_CONTINUE_LINKING);
+
   dest = build_addr_arg_loc (loc, partition->main_dr, nb_bytes);
   src = build_addr_arg_loc (loc, partition->secondary_dr, nb_bytes);
   if (partition->kind == PKIND_MEMCPY
@@ -881,6 +932,12 @@ generate_memcpy_builtin (struct loop *loop, partit
   else
 kind = BUILT_IN_MEMMOVE;
 
+  /* If the number of bytes is in excess of SSIZE_MAX avoid generating
+ the mem(cpy|move) call that would certainly overflow and instead
+ emit a trap.  */
+  if (maybe_emit_trap (gsi, nb_bytes, kind))
+return;
+
   dest = force_gimple_operand_gsi (, dest, true, NULL_TREE,
    false, GSI_CONTINUE_LINKING);
   src = force_gimple_operand_gsi (, src, true, NULL_TREE,

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Martin Sebor]

If the FIXME was a future thing, then this is OK with the nits fixed. If
the FIXME was a marker for something you intended to address now and
just forgot, then we either need another iteration or a follow-up patch
depending on the severity of the FIXME in your  mind.


As we discussed privately, I went ahead and committed just a minimal
patch to resolve the bug in r244511.  Most of the rest of the changes
in the patch have either already been made to resolve other bugs or
are a part of the bigger patch for bug 78703 that's under review.

I wanted to resolve the bug without making merging the bigger patch
harder than it needs to be.

Thanks
Martin

Re: [PR middle-end/79123] cast false positive in -Walloca-larger-than=

[Martin Sebor]

On 01/19/2017 05:45 AM, Richard Biener wrote:

On Thu, Jan 19, 2017 at 1:17 PM, Aldy Hernandez  wrote:

In the attached testcase, we have a clearly bounded case of alloca which is
being incorrectly reported:

void g (int *p, int *q)
{
   size_t n = (size_t)(p - q);

   if (n < 10)
 f (__builtin_alloca (n));
}

The problem is that VRP gives us an anti-range for `n' which may be out of
range:

  # RANGE ~[2305843009213693952, 16140901064495857663]
   n_9 = (long unsigned int) _4;

We do a less than stellar job with casts and VR_ANTI_RANGE's, mostly because
we're trying various heuristics to make up for the fact that we have crappy
range info from VRP.  More specifically, we're basically punting on an
VR_ANTI_RANGE and ignoring that the casted result (n_9) has a bound later
on.

Luckily, we already have code to check simple ranges coming into the alloca
by looking into all the basic blocks feeding it.  The attached patch delays
the final decision on anti ranges until we have examined the basic blocks
and determined for that we are definitely out of range.

I expect all this to disappear with Andrew's upcoming range info overhaul.

OK for trunk?


I _really_ wonder why all the range consuming warnings are not emitted
from VRP itself (like we do for -Warray-bounds).  There we'd still see
a range for the argument derived from the if () rather than needing to
do our own mini-VRP from the needessly "incomplete" range-info on
SSA vars.


Can you explain why the range info is only available in VRP and
not outside, via the get_range_info() API?  It sounds as though
the API shouldn't be relied on (it was virtually unused before
GCC 7).

To answer your question, the gimple-ssa-sprintf pass that depends
heavily on ranges would also benefit from having access to the data
computed in the strlen pass that's not available outside it.

The -Wstringop-overflow and -Walloc-size-larger-than warnings depend
on both VRP and tree-object-size.

I have been thinking about how to integrate these passes in GCC 8
to improve the overall quality.  (By integrating I don't necessarily
mean merging the code but rather having them share data or be able
to make calls into one another.)

I'd be very interested in your thoughts on this.

Thanks
Martin

Re: [PATCH] tree-optimization/71831 - __builtin_object_size poor results with no optimization

[Martin Sebor]

On 08/20/2016 01:26 AM, Jakub Jelinek wrote:

On Fri, Aug 19, 2016 at 04:30:47PM -0600, Martin Sebor wrote:

The patch looks bigger than it actually is because:

1) It modifies the return type of the function to bool rather than
unsigned HOST_WIDE_INT representing the object size (this was
necessary to avoid having its callers misinterpret zero as
unknown when it means zero bytes).


Can you explain why do you need this?  I don't understand why do you need to
differentiate between unknown and maximum (or minimum for modes 2 and 3 that
nobody actually uses in real-world), the builtin after all returns the same
value for both.  If you want to know if the compiler knows the size
precisely, you can request both mode 0 (or 1) and 2 (or 3) and compare, if
the values are the same, it is the exact size, if there is a range, then you
have minimum and maximum (and, if minimum is 0, but maximum non-zero, you
really don't know minimum, if maximum is -1, then you really don't know the
maximum (no object should be better that big).  For the return value, I
don't see how you could reliably differentiate between the two even if it
made for whatever strange reason sense - for SSA_NAMEs etc. you have just
recorded the sizes, not also a flag whether it is unknown or known.


The change makes it possible to fold into constants even at -O0 type
2 and 3 calls to the built-in with zero-sized objects.

fold_builtin_object_size repeatedly calls compute_builtin_object_size
to compute the same result (zero) only to interpret it as a failure
and try over and over, never succeeding.  The built-in call is
ultimately expanded into a zero but that's too late to eliminate code
that depends on it. For example, the following emits the call to abort
that's never executed.

  char a[2];

  void f (void)
  {
if (__builtin_object_size (a + 2, 2) != 0)
  __builtin_abort ();
  }

With the change, compute_builtin_object_size is called just once and
the call to abort is not emitted.

The initial version of the test verified this folding but adding code
to work around the built-ins various idiosyncrasies inadvertently wound
up removing it.  The attached patch adds a new test to exercise this
folding.




2) As a result of a small change to the conditional that controls
the main algorithm of the compute_builtin_object_size function
it changes the depth of its indentation (without actually
changing any of the code there).


If you've done lots of redindentation, then additionally diff -upb would be
appreciated.


Sure.  The attached diff was created without regard to whitespace.

Martin


PR tree-optimization/71831 - __builtin_object_size poor results with no
	optimization

gcc/testsuite/ChangeLog:
2016-08-21  Martin Sebor  <mse...@redhat.com>

	PR tree-optimization/71831
	* gcc.dg/builtin-object-size-16.c: New test.
	* gcc.dg/builtin-object-size-17.c: New test.

gcc/ChangeLog:
2016-08-21  Martin Sebor  <mse...@redhat.com>

	PR tree-optimization/71831
	* tree-object-size.h: Return bool instead of the size and add
	argument for the size.
	* tree-object-size.c (compute_object_offset): Update signature.
	(addr_object_size): Same.
	(compute_builtin_object_size): Return bool instead of the size
	and add argument for the size.  Handle POINTER_PLUS_EXPR when
	optimization is disabled.
	(expr_object_size): Adjust.
	(plus_stmt_object_size): Adjust.
	(pass_object_sizes::execute): Adjust.
	* builtins.c (fold_builtin_object_size): Adjust.
	* doc/extend.texi (Object Size Checking): Update.
	* ubsan.c (instrument_object_size): Adjust.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 03a0dc8..5d0c1af 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -9610,7 +9610,7 @@ fold_builtin_object_size (tree ptr, tree ost)
 
   if (TREE_CODE (ptr) == ADDR_EXPR)
 {
-  bytes = compute_builtin_object_size (ptr, object_size_type);
+  compute_builtin_object_size (ptr, object_size_type, );
   if (wi::fits_to_tree_p (bytes, size_type_node))
 	return build_int_cstu (size_type_node, bytes);
 }
@@ -9619,8 +9619,7 @@ fold_builtin_object_size (tree ptr, tree ost)
   /* If object size is not known yet, delay folding until
later.  Maybe subsequent passes will help determining
it.  */
-  bytes = compute_builtin_object_size (ptr, object_size_type);
-  if (bytes != (unsigned HOST_WIDE_INT) (object_size_type < 2 ? -1 : 0)
+  if (compute_builtin_object_size (ptr, object_size_type, )
 	  && wi::fits_to_tree_p (bytes, size_type_node))
 	return build_int_cstu (size_type_node, bytes);
 }
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 5285e00..d9dc137 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -10009,8 +10009,15 @@ __atomic_store_n(, 0, __ATOMIC_RELEASE|__ATOMIC_HLE_RELEASE);
 @findex __builtin___fprintf_chk
 @findex __builtin___vfprintf_chk
 
-GCC implements a limited buffer overflow protection mechanism
-that can prevent some buffer overflow a

Re: [PATCH] tree-optimization/71831 - __builtin_object_size poor results with no optimization

[Martin Sebor]

On 08/20/2016 01:02 AM, Florian Weimer wrote:

* Martin Sebor:


As requested in the review of the following patch

   https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01363.html

attached is the small enhancement to compute_builtin_object_size to
make the function usable even without optimization without the full
overhead of the tree-object-size pass.


Is its overhead that significant?


That's a good question.  I haven't measured it but having limited
access to object sizes beyond what my simple patch adds would make
it possible to improve the quality of other warnings as well (e.g.,
-Warray-bounds and -Wplacement-new) and improve buffer overflow
detection in libc function calls without optimization (if enabled
as suggested below).



Does this mean that with this patch, glibc should remove its
_FORTIFY_SOURCE warning for non-optimized builds when compiling under
GCC >= 7?


No, but I see no reason why GCC couldn't provide the same (limited)
overflow checking for other libc built-ins that the -Wformat-length
patch adds to __builtin_sprintf et al.  The proof of concept patch
below shows that it would be nearly trivial to do (the warning
mentions memcpy and strcpy because that's what GCC has transformed
the strcpy call into by the time it's expanded but presumably that
could be fixed).

Martin

$ cat z.c && /build/gcc-71831/gcc/xgcc -B /build/gcc-71831/gcc -O0 -S 
-Wall -Wextra z.c

char a[2];

extern char* strcpy (char*, const char*);

void f (void)
{
  strcpy (a, "abc");
}
z.c: In function ‘f’:
z.c:7:3: warning: call to __builtin_memcpy writing 4 bytes into 
destination of size 2

   strcpy (a, "abc");
   ^


--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -2985,6 +2985,19 @@ expand_builtin_memcpy (tree exp, rtx target)
   tree dest = CALL_EXPR_ARG (exp, 0);
   tree src = CALL_EXPR_ARG (exp, 1);
   tree len = CALL_EXPR_ARG (exp, 2);
+
+  unsigned HOST_WIDE_INT dstsize;
+  compute_builtin_object_size (dest, 0, );
+
+  unsigned HOST_WIDE_INT srclen
+   = tree_fits_uhwi_p (len) ? tree_to_uhwi (len) : HOST_WIDE_INT_M1U;
+
+  if (dstsize <= srclen)
+   warning_at (tree_nonartificial_location (exp),
+   0, "%Kcall to %D writing %wu bytes into destination "
+   "of size %wu",
+   exp, get_callee_fndecl (exp), srclen, dstsize);
+
   return expand_builtin_memcpy_args (dest, src, len, target, exp);
 }
 }

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

My biggest concern with this iteration is the tight integration between
the optimization and warning.  We generally avoid that kind of tight
integration such that enabling the warning does not affect the
optimization and vice-versa.

So ISTM you have to do the analysis if the optimization or warning has
been requested.  Then you conditionalize whether or not the warnings are
emitted by their flag and the optimization based on its flag.


As we discussed in IRC yesterday, the warning and the optimization
are independent of one another, and each controlled by its own option
(-Wformat-length and -fprintf-return-value).  In light of that we've
agreed that submitting both as part of the same patch is sufficient.



I understand you're going to have some further work to do because of
conflicts with David's patches.  With that in mind, I'd suggest a bit of
carving things up so things can start moving forward.


Patch #1.  All the fixes to static buffer sizes that were inspired by
your warning.  These are all approved and can go in immediately.


Attached is this patch.



Patch #2. Improvement to __builtin_object_size to handle
POINTER_PLUS_EXPR on arrays.  This is something that stands on it own
and ought to be reviewable quickly and doesn't really belong in the
bowels of the warning/optimization patch you're developing.


Sure.  I'll submit this patch next.



Patch #3. Core infrastructure and possibly the warning.  The reason I
say possibly the warning is they may be intertwined enough that
separating them makes more work than it saves.  I think the warning bits
are largely ready to go and may just need twiddling due to conflicts
with David's work.

Patch #4. The optimizations you've got now which I'll want to take
another look at.  Other than the overly tight integration with the
warning, I don't see anything inherently wrong, but I would like to take
another look at those once #1-#3 are done and dusted.


As we agreed, these will be submitted as one patch (probably
next week).



Patch #5 and beyond: Further optimization work.


As one of the next steps I'd like to make this feature available
to user-defined sprintf-like functions decorated with attribute
format.  To do that, I'm thinking of adding either a fourth
(optional) argument to attribute format printf indicating which
of the function arguments is the destination buffer (to compute
its size), or perhaps a new attribute under its own name.  I'm
actually leaning toward latter since I think it could be used
in other contexts as well.  I welcome comments and suggestions
on this idea.

Thanks also for the rest of the detailed comments (snipped). I'll
also take care of those requests before I submit the next patch.

Martin
gcc/c-family/ChangeLog:
2016-08-18  Martin Sebor  <mse...@redhat.com>

	* c-ada-spec.c (dump_ada_function_declaration): Increase buffer
	size to guarantee it fits the output of the formatted function
	regardless of its arguments.

gcc/cp/ChangeLog:
2016-08-18  Martin Sebor  <mse...@redhat.com>

	* mangle.c: Increase buffer size to guarantee it fits the output
	of the formatted function regardless of its arguments.

gcc/go/ChangeLog:
2016-08-18  Martin Sebor  <mse...@redhat.com>

	* gofrontend/expressions.cc: Increase buffer size to guarantee
	it fits the output of the formatted function regardless of its
	arguments.

gcc/java/ChangeLog:
2016-08-18  Martin Sebor  <mse...@redhat.com>

	* decl.c (give_name_to_locals): Increase buffer size to guarantee
	it fits the output of the formatted function regardless of its
	arguments.
	* mangle_name.c (append_unicode_mangled_name): Same.

gcc/ChangeLog:
2016-08-18  Martin Sebor  <mse...@redhat.com>

	* genmatch.c (parser::parse_expr): Increase buffer size to guarantee
	it fits the output of the formatted function regardless of its
	arguments.
	* gcc/genmodes.c (parser::parse_expr): Same.
	* gimplify.c (gimplify_asm_expr): Same.
	* passes.c (pass_manager::register_one_dump_file): Same.
	* print-tree.c (print_node): Same.

diff --git a/gcc/c-family/c-ada-spec.c b/gcc/c-family/c-ada-spec.c
index a4e0c38..6a8e04b 100644
--- a/gcc/c-family/c-ada-spec.c
+++ b/gcc/c-family/c-ada-spec.c
@@ -1603,7 +1603,7 @@ dump_ada_function_declaration (pretty_printer *buffer, tree func,
 {
   tree arg;
   const tree node = TREE_TYPE (func);
-  char buf[16];
+  char buf[17];
   int num = 0, num_args = 0, have_args = true, have_ellipsis = false;
 
   /* Compute number of arguments.  */
diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index d8b5c45..5859d62 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -1740,7 +1740,9 @@ static void
 write_real_cst (const tree value)
 {
   long target_real[4];  /* largest supported float */
-  char buffer[9];   /* eight hex digits in a 32-bit number */
+  /* Buffer for eight hex digits in a 32-bit number but big enough
+ even for 64-bit long to avoid warnings.  */
+  char buffer[17];
   int i, limit, dir;
 
   tree type = TREE_TYPE (value);
dif

[PATCH] tree-optimization/71831 - __builtin_object_size poor results with no optimization

[Martin Sebor]

As requested in the review of the following patch

  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01363.html

attached is the small enhancement to compute_builtin_object_size to
make the function usable even without optimization without the full
overhead of the tree-object-size pass.  The enhancement (disabled
when optimization is enabled so as not to change the results there)
is relied on by the -Wformat-length patch.

The patch looks bigger than it actually is because:

1) It modifies the return type of the function to bool rather than
   unsigned HOST_WIDE_INT representing the object size (this was
   necessary to avoid having its callers misinterpret zero as
   unknown when it means zero bytes).

2) As a result of a small change to the conditional that controls
   the main algorithm of the compute_builtin_object_size function
   it changes the depth of its indentation (without actually
   changing any of the code there).

Martin
PR tree-optimization/71831 - __builtin_object_size poor results with no
	optimization

gcc/testsuite/ChangeLog:
2016-08-19  Martin Sebor  <mse...@redhat.com>

	PR tree-optimization/71831
	* gcc.dg/builtin-object-size-16.c: New test.

gcc/ChangeLog:
2016-08-19  Martin Sebor  <mse...@redhat.com>

	PR tree-optimization/71831
	* tree-object-size.h: Return bool instead of the size and add
	argument for the size.
	* tree-object-size.c (compute_object_offset): Update signature.
	(addr_object_size): Same.
	(compute_builtin_object_size): Return bool instead of the size
	and add argument for the size.  Handle POINTER_PLUS_EXPR when
	optimization is disabled.
	(expr_object_size): Adjust.
	(plus_stmt_object_size): Adjust.
	(pass_object_sizes::execute): Adjust.
	* builtins.c (fold_builtin_object_size): Adjust.
	* doc/extend.texi (Object Size Checking): Update.
	* ubsan.c (instrument_object_size): Adjust.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 03a0dc8..5d0c1af 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -9610,7 +9610,7 @@ fold_builtin_object_size (tree ptr, tree ost)
 
   if (TREE_CODE (ptr) == ADDR_EXPR)
 {
-  bytes = compute_builtin_object_size (ptr, object_size_type);
+  compute_builtin_object_size (ptr, object_size_type, );
   if (wi::fits_to_tree_p (bytes, size_type_node))
 	return build_int_cstu (size_type_node, bytes);
 }
@@ -9619,9 +9619,8 @@ fold_builtin_object_size (tree ptr, tree ost)
   /* If object size is not known yet, delay folding until
later.  Maybe subsequent passes will help determining
it.  */
-  bytes = compute_builtin_object_size (ptr, object_size_type);
-  if (bytes != (unsigned HOST_WIDE_INT) (object_size_type < 2 ? -1 : 0)
-  && wi::fits_to_tree_p (bytes, size_type_node))
+  if (compute_builtin_object_size (ptr, object_size_type, )
+	  && wi::fits_to_tree_p (bytes, size_type_node))
 	return build_int_cstu (size_type_node, bytes);
 }
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 5285e00..6cf64f5 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -10009,8 +10009,15 @@ __atomic_store_n(, 0, __ATOMIC_RELEASE|__ATOMIC_HLE_RELEASE);
 @findex __builtin___fprintf_chk
 @findex __builtin___vfprintf_chk
 
-GCC implements a limited buffer overflow protection mechanism
-that can prevent some buffer overflow attacks.
+GCC implements a limited buffer overflow protection mechanism that can
+prevent some buffer overflow attacks by determining the sizes of objects
+into which data is about to be written and preventing the writes when
+the size isn't sufficient.  The built-in functions described below yield
+the best results when used together and when optimization is enabled.
+For example, to detect object sizes across function boundaries or to
+follow pointer assignments through non-trivial control flow they rely
+on various optimization passes enabled with @option{-O2}.  However, to
+a limited extent, they can be used without optimization as well.
 
 @deftypefn {Built-in Function} {size_t} __builtin_object_size (void * @var{ptr}, int @var{type})
 is a built-in construct that returns a constant number of bytes from
diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-16.c b/gcc/testsuite/gcc.dg/builtin-object-size-16.c
new file mode 100644
index 000..c2dbe76
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-object-size-16.c
@@ -0,0 +1,195 @@
+/* PR 71831 - __builtin_object_size poor results with no optimization
+   Verify that even without optimization __builtin_object_size returns
+   a meaningful result for a subset of simple expressins.  In cases
+   where the result could not easily be made to match the one obtained
+   with optimization the built-in was made to fail instead.  */
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+static int nfails;
+
+#define TEST_FAILURE(line, obj, type, expect, result)		\
+  __builtin_printf ("FAIL: line %i: __builtin_object_size("	\
+		#obj ", %i) == %zu, got

Re: Implement -Wimplicit-fallthrough (version 4)

[Martin Sebor]

Just a few minor nits based on a quick reading of the patch:


@@ -24114,6 +24164,16 @@ cp_parser_std_attribute (cp_parser *parser)
 " use %");
  TREE_PURPOSE (TREE_PURPOSE (attribute)) = get_identifier ("gnu");
}
+  /* C++17 fallthrough attribute is equivalent to GNU's.  */
+  else if (cxx_dialect >= cxx11
+  && is_attribute_p ("fallthrough", attr_id))
+   {
+ if (cxx_dialect < cxx1z)
+   pedwarn (token->location, OPT_Wpedantic,
+"% is a C++17 feature;"


The text of the message above is missing the word "attribute" that's
normally used in all other diagnostics about attributes.

But I wonder about issuing a pedantic warning for this C++ 17 attribute
when it isn't issued for others such as nodiscard.  I would expect them
to be diagnosed consistently (i.e., __attribute__((fallthrough)) to be
accepted in all modes, and [[fallthrough]] diagnosed in C++ 14 mode,
and all [[...]] attributes diagnosed in C++ 98 as they are now).  It
looks to me like your patch is close to what I expect but different
from other C++ 17 attributes.



+" use %");
+ TREE_PURPOSE (TREE_PURPOSE (attribute)) = get_identifier ("gnu");
+   }
/* Transactional Memory TS optimize_for_synchronized attribute is
 equivalent to GNU transaction_callable.  */
else if (is_attribute_p ("optimize_for_synchronized", attr_id))
@@ -24178,6 +24238,10 @@ cp_parser_check_std_attribute (tree attributes, tree 
attribute)
   && lookup_attribute ("deprecated", attributes))
error ("attribute deprecated can appear at most once "
   "in an attribute-list")
+  else if (is_attribute_p ("fallthrough", name)
+  && lookup_attribute ("fallthrough", attributes))
+   error ("attribute fallthrough can appear at most once "
+  "in an attribute-list");


Here "fallthrough" isn't quoted when it is everywhere else as far
as I noticed (looks like the same applies to "deprecated" above).


+C++17 provides a standard way to suppress the @option{-Wimplicit-fallthrough}
+warning using @code{[[fallthrough]];} instead of the GNU attribute.  In C++11
+or C++14 users can use @code{[[gnu::fallthrough]];}, which is a GNU extension.
+Instead of the these attributes, it is also possible to add a "falls through"
+comment to silence the warning.  GCC accepts wide range of such comments, so
+e.g. all of "Falls through.", "fallthru", "FALLS-THROUGH" work.


The last sentence is missing an article:

  GCC accepts _a_ wide range of such comments...

(Personally, I also find using "e.g." in the middle of a sentence
a bit too informal and would like it better spelled out, but that
could just be me.)


+/* Find LABEL in vector of label entries VEC.  */
+
+static struct label_entry *
+find_label_entry (const auto_vec *vec, tree label)
+{
+  unsigned int i;
+  struct label_entry *l;
+
+  FOR_EACH_VEC_ELT (*vec, i, l)
+if (l->label == label)
+  return l;
+  return NULL;
+}
+
+/* Return true if LABEL, a LABEL_DECL, represents a case label
+   in a vector of labels CASES.  */
+
+static bool
+case_label_p (const vec , tree label)


I'm curious why this function takes the first argument by const
reference when the function above by const pointer.  Is there
a subtle difference between the functions that I'm not seeing?
For that matter, is there a convention in GCC?  (FWIW, my own
rule of thumb is to have a function take a large argument by
const reference when it must be non-null and by pointer
otherwise.)


+/* Collect interesting labels to LABELS and return the statement preceding
+   another case label, or a user-defined label.  */


Suggest either "Collect interesting labels in LABELS" or "Append
interesting labels to LABELS..."


+
+static gimple *
+collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
+   auto_vec  *const labels)


The const above seems superfluous (it says the labels pointer
doesn't change, which could be helpful to the reader of the code,
but then it suggests that gsi_p does change, which isn't the case).


+   /* Suggestion one: add "__attribute__ ((fallthrough));".  */
+   rich_location rloc_attr (line_table, gimple_location (next));
+   rloc_attr.add_fixit_insert (gimple_location (next),
+   "__attribute__ ((fallthrough));");


The warnings above use "attribute %" yet the notes here
use "__attribute__ ((fallthrough));"  It seems that using consistent
spelling in all messages would be better (and avoid emphasizing one
spelling of the attribute over others, especially the C++ 17
[[fallthrough]]).



+static void
+expand_FALLTHROUGH (internal_fn, gcall *call)
+{
+  error_at (gimple_location (call),
+   "invalid use of %<__attribute__((fallthrough));%>");


Similar to my comment above, 

Re: PR79697: Delete calls to strdup, strndup, realloc if there is no lhs

[Martin Sebor]

On 02/25/2017 11:54 AM, Prathamesh Kulkarni wrote:

On 25 February 2017 at 14:43, Marc Glisse  wrote:

On Sat, 25 Feb 2017, Prathamesh Kulkarni wrote:


Hi,
The attached patch deletes calls to strdup, strndup if it's
return-value is unused,
and same for realloc if the first arg is NULL.
Bootstrap+tested on x86_64-unknown-linux-gnu.
OK for GCC 8 ?



Instead of specializing realloc(0,*) wherever we can perform the same
optimization as with malloc, wouldn't it be better to optimize:
realloc(0,n) -> malloc(n)
and let the malloc optimizations happen?

Thanks for the suggestions. In the attached patch, realloc (0, n) is
folded to malloc (n).
Bootstrap+test in progress on x86_64-unknown-linux-gnu.
Does the patch look OK ?


Although it's not part of the bug report, I wonder if a complete
patch should also extend the malloc/free DCE to str{,n}dup/free
calls and eliminate pairs like these:

  void f (const char *s)
  {
char *p = strdup (src);
free (p);
  }

(That seems to be just a matter of adding a couple of conditionals
for BUILT_IN_STR{,N}DUP in propagate_necessity).

Martin

PS Another optimization, though one that's most likely outside
the scope of this patch, is to eliminate all of the following:

  void f (const char *s, char *s2)
  {
char *p = strdup (s);
strcpy (p, s2);
free (p);
  }

as is done in:

  void f (unsigned n, const char *s)
  {
char *p = malloc (n);
memcpy (p, s, n);
free (p);
  }

Re: [PATCH docs] remove Java from GCC 7 release criteria

[Martin Sebor]

On 02/28/2017 11:00 AM, Jeff Law wrote:

On 02/28/2017 10:54 AM, Martin Sebor wrote:

The GCC 7 release criteria page mentions Java even though
the front end has been removed.  The attached patch removes Java
from the criteria page.  While reviewing the rest of the text I
noticed a few minor typos that I corrected in the patch as well.

Btw., as an aside, I read the page to see if I could find out more
about the "magic" bug counts that are being aimed for to decide when
to cut the release.  Can someone say what those are and where to
find them?  I understand from the document that they're not exact
but even ballpark numbers would be useful.


OK.

WRT the bug counts.  0 P1 regressions, < 100 P1-P3 regressions.  I'm not
sure if that's documented anywhere though.


Thanks.  Would it make sense to mention these numbers in the criteria?
E.g., like so (assuming those apply to primary platforms):

-Our release criteria for primary platforms is:
+Our release criteria for primary platforms are:
 

 
+No open P1 regressions in Bugzilla and no more than 100 open P2
+and P3 regressions.
+
+
+

Martin

[PATCH docs] remove Java from GCC 7 release criteria

[Martin Sebor]

The GCC 7 release criteria page mentions Java even though
the front end has been removed.  The attached patch removes Java
from the criteria page.  While reviewing the rest of the text I
noticed a few minor typos that I corrected in the patch as well.

Btw., as an aside, I read the page to see if I could find out more
about the "magic" bug counts that are being aimed for to decide when
to cut the release.  Can someone say what those are and where to
find them?  I understand from the document that they're not exact
but even ballpark numbers would be useful.

Thanks
Martin
Index: criteria.html
===
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-7/criteria.html,v
retrieving revision 1.1
diff -u -r1.1 criteria.html
--- criteria.html	23 May 2016 09:16:41 -	1.1
+++ criteria.html	28 Feb 2017 17:41:00 -
@@ -7,7 +7,7 @@
 
 GCC 7 Release Criteria
 
-This page provides the release criteria for GCC 7.  
+This page provides the release criteria for GCC 7.
 
 The GCC team (and, in particular, the Release Managers) will attempt
 to meet these criteria before the release of GCC 7.
@@ -18,7 +18,7 @@
 candidate will probably not become the eventual release.  However, a
 release candidate that does meet these criteria may not necessarily
 become the official release; there may be other unforeseen issues that
-prevent release.  For example, if support for the Intel Pentium II is
+prevent the release.  For example, if support for the Intel Pentium II is
 required by the release criteria, it is nevertheless unlikely that GCC
 would be released even though it did not support the Intel Pentium.
 
@@ -32,10 +32,10 @@
 Languages
 
 GCC supports several programming languages, including Ada, C, C++,
-Fortran, Objective-C, Objective-C++, Go and Java.
+Fortran, Objective-C, Objective-C++, and Go.
 For the purposes of making releases,
 however, we will consider primarily C and C++, as those are the
-languages used by the vast majority of users.  Therefore, if, below,
+languages used by the vast majority of users.  Therefore, if below
 the criteria indicate, for example, that there should be no DejaGNU
 regressions on a particular platform, that criteria should be read as
 applying only to DejaGNU regressions within the C, C++, and C++
@@ -53,12 +53,12 @@
 All platforms that are neither primary nor secondary are tertiary
 platforms.
 
-Our release criteria for primary platforms is:
+Our release criteria for primary platforms are:
 
 
 
 All regressions open in Bugzilla have been analyzed, and all are
-deemed as either unlikely to affect most users, or are determined to
+deemed either unlikely to affect most users, or are determined to
 have a minimal impact on affected users.  For example, a
 typographical error in a diagnostic might be relatively common, but
 also has minimal impact on users.
@@ -67,14 +67,14 @@
 code, or refuses to compile a valid program, will be considered to
 be sufficiently severe to block the release, unless there are
 substantial mitigating factors.
-  
+
 
 The DejaGNU testsuite has been run, and compared with a run of
 the testsuite on the previous release of GCC, and no regressions are
 observed.
 
 
-Our release criteria for the secondary platforms is:
+Our release criteria for the secondary platforms are:
 
 The compiler bootstraps successfully, and the C++ runtime library
 builds.
@@ -92,7 +92,7 @@
 explicit application testing.  It is our experience that, with the
 resources available, it is very difficult to methodically carry out
 such testing. However, we expect that interested users will submit
-bug reports for problems encountered building and using popular
+bug reports for problems encountered while building and using popular
 packages.  Therefore, we do not intend the elimination of application
 testing from our criteria to imply that we will not pay attention to
 application testing.
@@ -142,7 +142,7 @@
 superior code on other test cases.  Therefore, the Release Manager, or
 parties to whom he or she delegates responsibility, will make
 determinations on a case-by-case basis as to whether or not a code
-quality or compilation time regression is sufficiently severe as to
+quality or compilation time regression is sufficiently severe to
 merit blocking the release.
 
 

Re: [PATCH] avoid using upper bound of width and precision in -Wformat-overlow (PR 79692)

[Martin Sebor]

So in some cases you use

+  /* For an indeterminate precision the lower bound must be assumed
+ to be zero.  */
+  if (prec[0] < 0 && prec[1] >= 0)

Note prec[1] >= 0

In other cases you have:

+/* The minimum output with unknown precision is a single byte
+   (e.g., "0") but the more likely output is 3 bytes ("0.0").  */
+if (dir.prec[0] < 0 && dir.prec[1] > 0)

Note  dir.prec[1] > 0

Shouldn't one of those be changed to be consistent with the other?


Thanks for the careful review!  The two tests determine two different
things so I'm not sure they need to be consistent. But considering
your question made me realize that the first conditional isn't
completely correct:

+  /* For an indeterminate precision the lower bound must be assumed
+ to be zero.  */
+  if (prec[0] < 0 && prec[1] >= 0)
+prec[0] = 0;

Precisions in a negative-positive range with an upper bound of less
than 6 must be assumed to have an upper bound of 6 because that's
the default when precision is negative.  E.g., given
snprintf (0, 0, "%*f", p, 1.23456789) where p is in [-1, 0] results
either in:

  1.234568

when (p == -1) holds, or in:

  1

when (p == 0) holds.  So while the lower bound in the if statement
above must be set to zero, the upper bound may need to be adjusted
as well.

The patch I just committed in r245822 fixes that (and also changes
the conditional so that consistency is no longer an issue).

However, while reviewing the rest of the floating point handling
code it became clear that this is a bug that affects both floating
point formatting functions (i.e., the one that handles constants
as well as the non-constant one).  I also noticed some other minor
glitches in this area that should be fixed.  I'm testing another
patch that resolves those problem as well.


Similarly in known_width_and_precision.  Please review the patch to
ensure that we're as consistent as possible for these tests.


In known_width_and_precision the different inequalities are
deliberate.  Because the actual width is an absolute value
of the specified argument the range of unknown width is
[0, INT_MAX + 1] (printf("%*i", INT_MIN, i) specifies a width
of -INT_MIN, or INT_MAX + 1 (without overflow).  But because
negative precisions are ignored, the largest upper bound is
INT_MAX.

Martin

[PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Martin Sebor]

The uses of MPFR in gimple-ssa-sprintf.c apparently cause
the library to allocates some internal caches that it then leaks
on program exit, causing Valgrind memory leak errors.  The MPFR
manual "strongly advises to [call mpfr_free_cache] before
terminating a thread, or before exiting when using tools like
'valgrind' (to avoid memory leaks being reported)) so the attached
patch does just that.

It' seems like an obvious fix that could presumably be committed
without a review or approval but I'd like to give others a chance
to comment on the placement of the call and whether it should be
guarded by ENABLE_VALGRIND_ANNOTATIONS.

Joseph, since you commented on the bug, do you have a suggestion
for a different site for it or a guard?  The only other call to
the function is in the Fortran FE and it's neither guarded nor
does it appear to ever be called.

Thanks
Martin
PR tree-optimization/79699 - small memory leak in MPFR

gcc/ChangeLog:

	PR tree-optimization/79699
	* gimple-ssa-sprintf.c (pass_sprintf_length::execute): Free MPFR
	caches to avoid a memory leak on program exit.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 7688439..0c00fa0 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -3612,6 +3612,8 @@ pass_sprintf_length::execute (function *fun)
   /* Clean up object size info.  */
   fini_object_sizes ();
 
+  /* Clean up MPFR caches (see bug 79699).  */
+  mpfr_free_cache ();
   return 0;
 }
 

Re: [PATCH docs] remove Java from GCC 7 release criteria

[Martin Sebor]

On 03/01/2017 08:08 AM, Gerald Pfeifer wrote:

On Tue, 28 Feb 2017, Martin Sebor wrote:

The GCC 7 release criteria page mentions Java even though
the front end has been removed.  The attached patch removes Java
from the criteria page.  While reviewing the rest of the text I
noticed a few minor typos that I corrected in the patch as well.


Thanks, Martin!

To minor comments:

-bug reports for problems encountered building and using popular
+bug reports for problems encountered while building and using popular

I believe the original version was fine (an is shorter), so personally
would have left it as is.  Your proposed one is correct, too, of course.

-quality or compilation time regression is sufficiently severe as to
+quality or compilation time regression is sufficiently severe to
merit blocking the release.

Same here, though here I like yours edit better. :-)


Thanks for the review!

I committed the first patch for now (without the P1/P2 numbers).
If there's consensus to add something more about those than what's
already there I'll post a new patch to add that.

Martin

Re: [PATCH docs] remove Java from GCC 7 release criteria

[Martin Sebor]

On 02/28/2017 11:41 PM, Richard Biener wrote:

On March 1, 2017 3:34:46 AM GMT+01:00, Martin Sebor <mse...@gmail.com> wrote:

On 02/28/2017 01:41 PM, Richard Biener wrote:

On February 28, 2017 7:00:39 PM GMT+01:00, Jeff Law <l...@redhat.com>

wrote:

On 02/28/2017 10:54 AM, Martin Sebor wrote:

The GCC 7 release criteria page mentions Java even though
the front end has been removed.  The attached patch removes Java
from the criteria page.  While reviewing the rest of the text I
noticed a few minor typos that I corrected in the patch as well.

Btw., as an aside, I read the page to see if I could find out more
about the "magic" bug counts that are being aimed for to decide

when

to cut the release.  Can someone say what those are and where to
find them?  I understand from the document that they're not exact
but even ballpark numbers would be useful.


OK.

WRT the bug counts.  0 P1 regressions, < 100 P1-P3 regressions.  I'm
not
sure if that's documented anywhere though.


Actually the only criteria is zero P1 regressions.  Those are

documented to block a release.

Yes, that is mentioned in the document.  Would it be fair to say
that the number of P2 bugs (or regressions) or their nature plays
into the decision in some way as well?  If so, what can the release
criteria say about it?


Ultimatively P2 bugs do not play a role and 'time' will trump them.  OTOH we 
never were in an uncomfortable situation with P2s at the desired point of 
release.

Also note that important P2 bugs can be promoted to P1 and not important P1 to 
P2.


I'm trying to get a better idea which bugs to work on and where
my help might have the biggest impact.  I think having better
visibility into the bug triage process (such as bug priorities
and how they impact the release schedule) might help others
focus too.


In order of importance:
- P1
- wrong-code, rejects-valid, ice-on-valid (even if not regressions, regressions 
more important)
- P2 regressions, more recent ones first (newest working version)


I see.  This is helpful, thanks.

The kinds of problems you mention are discussed in the document
so just to make the importance clear, would adding the following
after this sentence

  In general bugs blocking the release are marked with priority P1
  (Maintaining the GCC Bugzilla database).

accurately reflect what you described?

  As a general rule of thumb, within each priority level, bugs that
  result in incorrect code are considered more urgent than those
  that lead to rejecting valid code, which in turn are viewed as
  more severe than ice-on-valid code (compiler crashes).  More
  recently reported bugs are also prioritized over very old ones.

Martin

Re: [wwwdocs] Document in changes.html -fcode-hoisting, -fipa-bit-cp, -fipa-vrp, -fsplit-loops, GCJ removal, x86 ISA additions, -fshrink-wrap-separate etc.

[Martin Sebor]

On 02/26/2017 07:21 AM, Gerald Pfeifer wrote:

On Sun, 19 Feb 2017, Gerald Pfeifer wrote:

That was quite a bit; thanks for doing that, Jakub!

In the patch below I try to streamline language a bit, document options
being implied by -Os (in addition to -O2 or higher), fix grammar in a few
places, and change a textual link to being a real one.

I have _not_ applied this yet, and would appreciate your feedback.


I did another two passes through this, making a few more simplifications
and changes, and committed it.

If you see anything beyond it, or changes you disagree with, let me know!

@@ -525,8 +524,8 @@
 New __builtin_add_overflow_p,
   __builtin_sub_overflow_p,
   __builtin_mul_overflow_p built-in functions have been added.
-  These work similarly to earlier added built-in functions without the
-  _p suffix, but don't actually store the result of the
+  These work similarly to their siblings without the
+  _p suffix, but do not actually store the result of the
   arithmetics anywhere, just return whether the operation would overflow.
   These builtins allow easy checking for overflows e.g. in C++
   constexpr contexts.


Sorry to be jumping in so late. I only noticed this bit now.

I would suggest to say that these new built-ins evaluate to integer
constant expressions when their arguments do.  Not all C programmers
are familiar with C++ constexpr so they may not understand the use
use case.  The request for these built-ins also came from a C user
and was specifically motivated by avoiding -Woverflow warnings so
adding an example demonstrating that might be helpful as well.
Something like this:

  ... Calls to these built-ins with integer constant arguments
  evaluate to integer constants expressions.
  For example, in the following, c is assigned
  the result of a * b only if the multiplication
  does not overflow, otherwise it is assigned the value zero.
  The multiplication is performed at compile-time and without
  triggering a -Woverflow warning.
  
enum {
  a = 12345678,
  b = 87654321,
  c = __builtin_mul_overflow_p (a, b, a) ? 0 : a * b
};
  

Martin

Re: [PATCH docs] remove Java from GCC 7 release criteria

[Martin Sebor]

On 02/28/2017 01:41 PM, Richard Biener wrote:

On February 28, 2017 7:00:39 PM GMT+01:00, Jeff Law <l...@redhat.com> wrote:

On 02/28/2017 10:54 AM, Martin Sebor wrote:

The GCC 7 release criteria page mentions Java even though
the front end has been removed.  The attached patch removes Java
from the criteria page.  While reviewing the rest of the text I
noticed a few minor typos that I corrected in the patch as well.

Btw., as an aside, I read the page to see if I could find out more
about the "magic" bug counts that are being aimed for to decide when
to cut the release.  Can someone say what those are and where to
find them?  I understand from the document that they're not exact
but even ballpark numbers would be useful.


OK.

WRT the bug counts.  0 P1 regressions, < 100 P1-P3 regressions.  I'm
not
sure if that's documented anywhere though.


Actually the only criteria is zero P1 regressions.  Those are documented to 
block a release.


Yes, that is mentioned in the document.  Would it be fair to say
that the number of P2 bugs (or regressions) or their nature plays
into the decision in some way as well?  If so, what can the release
criteria say about it?

I'm trying to get a better idea which bugs to work on and where
my help might have the biggest impact.  I think having better
visibility into the bug triage process (such as bug priorities
and how they impact the release schedule) might help others
focus too.

Martin

Re: [PATCH] RFC: On-demand locations within string-literals

[Martin Sebor]

On 08/23/2016 07:59 AM, David Malcolm wrote:

On Mon, 2016-08-22 at 21:25 -0600, Martin Sebor wrote:

Beyond that, the range normally works fine, except when macros
are involved like they are in my tests.  You can see the effect
in the range.out file.  (This works without your patch but it
could very well be because I didn't set it up right.)


Sadly I can't figure out what's going wrong - but the code's
changed a
lot at my end since then.  Sorry.


I have integrated the latest (already committed) version of your
patch into my -Wformat-length patch.  Everything (well almost)
works and I get nice ranges for the format string and for (some)
arguments.

I was surprised at how long it took me to switch from the previous
implementation (also copied from c-format.c) to this new API.  As
before, I had to copy bits and pieces of code from other parts of
the compiler to get it to work.  I was also surprised at how complex
making use of it is.  It added 130 lines of code to the pass, which
is 40 more than what I had before.  It seems that the
format_warning_at_substring function from c-format.c (perhaps
generalized and with some reasonable defaults hardcoded) should
be defined where other parts of GCC (including the middle end)
can reuse it.


I'm guessing that it was difficult because the most useful parts are
currently in c-format.c, whereas your code is in the middle-end.

Is the latest version of your patch posted somewhere where I can see
it?


I'm planning/hoping to post it this week.  It was only difficult
in that I had to gather bits from different parts of the compiler
and figure out how to make them work together.  I.e., it wasn't
a simple matter of replacing one function call with another.  In
the end, it boiled down to replacing the get_location function
with this:

const char *
substring_loc::get_location (location_t *out_loc) const
{
  gcc_assert (out_loc);

  if (!g_string_concat_db)
g_string_concat_db
  = new (ggc_alloc  ()) string_concat_db ();

  static struct cpp_reader* parse_in;
  if (!parse_in)
{
  /* Create and initialize a preprocessing reader.  */
  parse_in = cpp_create_reader (CLK_GNUC99, ident_hash, line_table);
  cpp_init_iconv (parse_in);
}

  return get_source_location_for_substring (parse_in, g_string_concat_db,
m_fmt_string_loc, CPP_STRING,
m_caret_idx, m_start_idx, m_end_idx,
out_loc);
}



The substring_loc class should probably be moved from c-family to gcc
also.   We might need a langhook to support that though (not sure yet).

I'd be up for doing these moves (maybe moving
  format_warning_at_substring to diagnostic.h/c), but I'd prefer to see
your patch first.



Sure.


So would you like the output to look like this:


Option (a): underline whole string, with caret at close-quote

t.c: In function ‘f’:
t.c:5:25: warning: writing a terminating nul past the end of the
destination [-Wformat-length=]
  __builtin_sprintf (d, "%sX", "1");
^

or like this:

Option (b): just the close-quote

t.c: In function ‘f’:
t.c:5:25: warning: writing a terminating nul past
the end of the
destination [-Wformat-length=]
  __builtin_sprintf (d,
"%sX", "1");
^
?



Either one would work for me.  If this were to become a general
purpose interface then I think it would be nice to let the caller
decide which end/quote (if any) to point the caret at.


(do you also emit a note/inform showing the size of d?)


Yes.  The full diagnostic looks like this (with the care at
the wrong end as we're discussing):

t.c:5:25: warning: writing a terminating nul past the end of the 
destination [-Wformat-length=]

   __builtin_sprintf (d, "%sX", "A");
 ^
t.c:5:3: note: format output 3 bytes into a destination of size 2
   __builtin_sprintf (d, "%sX", "A");
   ^



What API are you using to emit the warning?


I copied the format_warning_va and format_warning_at subscript
functions from c-format.c and I'm calling the latter.  I'm not
using the hint for anything yet (not sure if there's
an opportunity to make use of it).


Given the location of the
string as a whole expressed as a location_t, you can probably do
something like this:

location_t
option_a (location_t string_as_a_whole_loc)
{
   source_range src_range
 = get_range_from_loc (line_table, string_as_a_whole_loc);

   return make_location (src_range.m_finish, /* caret */
 src_range.m_start, src_range.m_finish);
}

location_t
option_b (location_t string_as_a_whole_loc)
{
   source_range
src_range
 = get_range_from_loc (line_table, string_as_a_whole_loc);

   return src_range.m_finish;
}

(these could be added to libcpp)

but I get the impression you want someth

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 08/24/2016 04:03 PM, Joseph Myers wrote:

On Wed, 24 Aug 2016, Martin Sebor wrote:


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.


It's a general ISO C principle that there may be implementation limits,
including in areas where no specific minimum limit is given in the
standard, and even where there is a mimimum, that doesn't mean that every
possible program that does not exceed that minimum does not exceed the
limit in that area.  In this case, a specific minimum limit is given,
7.21.6.1#15 "The number of characters that can be produced by any single
conversion shall be at least 4095.".  That is, libc can't reject all cases
of larger results, but it's possible that if memory is short then some
cases with shorter results could still fail.


It sounds like using the 4095 limit should be safe even with Glibc.
I'll use it then, thanks.


It sounds like the concern is that for the following call (when
UCHAR_MAX is 255):

   sprintf (d, "%hhu", 1000)

some implementation (an old version of Glibc?) may have actually
produced four digits and returned 4 on the basis of C saying that
the %hhu argument must be an unsigned char (promoted to int) and
thus the behavior of the call being undefined.


Yes.


Since it's presumably a Glibc bug (bug 2509?) is this something
you believe the optimization needs to worry about?  If so, can
you confirm that only Glibc versions 2.4 and prior are affected
and 2.5 is not?

Martin

Re: [PATCH] Move class substring_loc from c-family into gcc

[Martin Sebor]

I've successfully tested the patch below by incorporating it into
the -Wformat-length pass I've been working on.  I'd like to submit
the latest and hopefully close to final version of my work for
review without duplicating this code and it might be helpful if
it was possible to build my latest patch without having to find
and install this one first.  Could someone review and approve
David's patch?

Thanks
Martin

On 08/25/2016 10:09 AM, David Malcolm wrote:

This patch is intended to help Martin's new middle-end sprintf format
warning.

It moves most of the on-demand locations-within-strings
code in c-family into gcc, into a new substring-locations.c file to
go with substring-locations.h: class substring_loc, representing
a source caret and source range within a STRING_CST, along with
format_warning for handling emitting a warning relating to it.

The actual work of reconstructing the source locations within
a string seems inherently frontend-specific, so the patch introduces a
langhook to do this, implementing it for C using the existing code
(and thus hiding the details of string-concatenation as being
specific to the c-family).  Attempts to get substring location
from other frontends will fail, and the format_warning_* calls
handle such failures gracefully by simply using the location of
the string as a whole for the warning.

I've tested it with Martin's gimple-ssa-sprintf patch, and I'm able to
emit carets and underlines in the correct places in C code from the
middle end with this approach (patch to follow).

Successfully bootstrapped on x86_64-pc-linux-gnu.

OK for trunk?

gcc/ChangeLog:
* Makefile.in (OBJS): Add substring-locations.o.
* langhooks-def.h (class substring_loc): New forward decl.
(lhd_get_substring_location): New decl.
(LANG_HOOKS_GET_SUBSTRING_LOCATION): New macro.
(LANG_HOOKS_INITIALIZER): Add LANG_HOOKS_GET_SUBSTRING_LOCATION.
* langhooks.c (lhd_get_substring_location): New function.
* langhooks.h (class substring_loc): New forward decl.
(struct lang_hooks): Add field get_substring_location.
* substring-locations.c: New file, taking definition of
format_warning_va and format_warning_at_substring from
c-family/c-format.c, making them non-static.
* substring-locations.h: Include .
(class substring_loc): Move class here from c-family/c-common.h.
Add comments.
(format_warning_va): New decl.
(format_warning_at_substring): New decl.
(get_source_location_for_substring): Add comment.

gcc/c-family/ChangeLog:
* c-common.c (get_cpp_ttype_from_string_type): Handle being passed
a POINTER_TYPE.
(substring_loc::get_location): Move to substring-locations.c,
keeping implementation as...
(c_get_substring_location): New function, from the above, reworked
to use accessors rather than member lookup.
* c-common.h (class substring_loc): Move to substring-locations.h,
replacing with a forward decl.
(c_get_substring_location): New decl.
* c-format.c: Include "substring-locations.h".
(format_warning_va): Move to substring-locations.c.
(format_warning_at_substring): Likewise.

gcc/c/ChangeLog:
* c-lang.c (LANG_HOOKS_GET_SUBSTRING_LOCATION): Use
c_get_substring_location for this new langhook.

gcc/testsuite/ChangeLog:
* gcc.dg/plugin/diagnostic_plugin_test_string_literals.c: Include
"substring-locations.h".
---
  gcc/Makefile.in|   1 +
  gcc/c-family/c-common.c|  23 +--
  gcc/c-family/c-common.h|  32 +---
  gcc/c-family/c-format.c| 157 +
  gcc/c/c-lang.c |   3 +
  gcc/langhooks-def.h|   8 +-
  gcc/langhooks.c|   8 +
  gcc/langhooks.h|   9 +
  gcc/substring-locations.c  | 195 +
  gcc/substring-locations.h  |  67 +++
  .../diagnostic_plugin_test_string_literals.c   |   1 +
  11 files changed, 308 insertions(+), 196 deletions(-)
  create mode 100644 gcc/substring-locations.c

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 1b7464b..769efcf 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1444,6 +1444,7 @@ OBJS = \
store-motion.o \
streamer-hooks.o \
stringpool.o \
+   substring-locations.o \
target-globals.o \
targhooks.o \
timevar.o \
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..f3e44c2 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -1122,6 +1122,9 @@ static enum cpp_ttype
  get_cpp_ttype_from_string_type (tree string_type)
  {
gcc_assert (string_type);
+  if (TREE_CODE (string_type) 

Re: [PATCH] Move class substring_loc from c-family into gcc

[Martin Sebor]

On 09/03/2016 03:13 AM, Manuel López-Ibáñez wrote:

On 02/09/16 23:55, Martin Sebor wrote:

diff --git a/gcc/substring-locations.h b/gcc/substring-locations.h
index f839c74..bb0de4f 100644
--- a/gcc/substring-locations.h
+++ b/gcc/substring-locations.h
@@ -20,6 +20,73 @@ along with GCC; see the file COPYING3.  If not see
  #ifndef GCC_SUBSTRING_LOCATIONS_H
  #define GCC_SUBSTRING_LOCATIONS_H

+#include 
+


Is this header file going to be used in the middle-end? If so, then it
is suspicious that it includes cpplib.h. Otherwise, perhaps it should
live in c-family/


I'm not sure if you meant this question for me or for David but
I believe the main (only?) reason for this patch is let to make
use, in the -Wformat-length middle-end pass, of the same range
location API as in c-family/c-format in the C/C++ front ends.
David, please correct me if I mischaracterized it.

Martin



I'm not complaining about substring-locations.c because libcpp is
already linked with everything else even for other non-C languages, like
Ada, but the above is leaking all cpplib.h into the rest of the
compiler, which defeats the purpose of this in coretypes.h

/* Provide forward struct declaration so that we don't have to include
all of cpplib.h whenever a random prototype includes a pointer.
Note that the cpp_reader and cpp_token typedefs remain part of
cpplib.h.  */

struct cpp_reader;
struct cpp_token;


Cheers,
 Manuel.

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

The attached version passes bootstrap+test on ppc64le-linux-gnu.
Given that it only looks if parameters are restrict qualified and not
how they're used inside the callee,
this can have false positives as in above test-cases.
Should the warning be put in Wextra rather than Wall (I have left it
in Wall in the patch)  or only enabled with -Wrestrict ?


Awesome!  I've wished for years for a warning like this!

I'm curious if you've tested other examples from 6.7.3.1 of C11
besides Example 3.  Example 4 seems like something GCC should be
able to detect but I didn't get a warning with the patch.

I would expect the warning to be especially valuable with string
manipulation functions like memcpy that have undefined behavior
for overlapping regions, such as in:

  char a[4];

  void g (void)
  {
__builtin_memcpy (a, a + 1, 3);
  }

But here, too, I didn't get a warning.  I understand that this
case cannot be handled for arbitrary functions whose semantics
aren't known but with standard functions for which GCC provides
intrinsics the effects are known and aliasing violations can in
common cases be detected.

Martin

Re: [PATCH] Fixups for Martin's gimple-ssa-sprintf.c patch

[Martin Sebor]

On 08/25/2016 10:30 AM, Martin Sebor wrote:

On 08/25/2016 10:23 AM, David Malcolm wrote:

Martin: here are the fixups for your patch I needed to apply to make
it work with mine.  I couldn't actually get any of your existing test
cases to emit locations within the string literals, due to them all
being embedded in macro expansions (possibly relating to PR c/77328),
so I added a simple testcase using -fdiagnostics-show-caret, which
does successfully show a range within the string.

Posting in the hope that it's helpful; I haven't attempted a bootstrap
with it.


I've tried the patch but the changes don't compile because
substring_loc is not declared.  I see the class defined in
c-family/c-common.h which I can't include here in the middle
end.  Am I missing some another patch?

Thanks
Martin

Re: Fix bogus warning with -Wlogical-not-parentheses (PR c/77292)

[Martin Sebor]

On 08/26/2016 07:44 AM, Marek Polacek wrote:

On Thu, Aug 25, 2016 at 11:35:53AM -0600, Martin Sebor wrote:

+foo (int a, int b)
+{
+  int r = 0;
+  r += !a == (a < b);
+  r += !a == (a > b);
+  r += !a == (a >= b);
+  r += !a == (a <= b);
+  r += !a == (a != b);
+  r += !a == (a == b);
+  r += !a == (a || b);
+  r += !a == (a && b);
+  r += !a == (!b);
+
+  r += !a == (a ^ b); /* { dg-warning "logical not is only applied to the left hand 
side of comparison" } */
+  r += !a == (a | b); /* { dg-warning "logical not is only applied to the left hand 
side of comparison" } */
+  r += !a == (a & b); /* { dg-warning "logical not is only applied to the left hand 
side of comparison" } */


A question more than a comment: warning on the last three expressions
above makes sense to me when the operands are integers but I'm less
sure that the same logic applies when the operands are Boolean.  Does
it make sense to issue the warning for those given that (a | b) and
(a & b) are the same as (a || b) and (a && b) for which the warning
is suppressed?

In other words, is warning on the latter of the two below but not on
the former a good idea or should they be treated the same way?


I gave this a shot but it seems to be quite complicated, and I'm not
sure if it's worth the effort.  If you want, open a BZ and I'll look
into this later.


I opened bug 77423 for this.

Martin

Re: [PATCH] Fixups for Martin's gimple-ssa-sprintf.c patch

[Martin Sebor]

On 08/31/2016 10:26 AM, David Malcolm wrote:

On Wed, 2016-08-31 at 10:23 -0600, Martin Sebor wrote:

On 08/25/2016 10:30 AM, Martin Sebor wrote:

On 08/25/2016 10:23 AM, David Malcolm wrote:

Martin: here are the fixups for your patch I needed to apply to
make
it work with mine.  I couldn't actually get any of your existing
test
cases to emit locations within the string literals, due to them
all
being embedded in macro expansions (possibly relating to PR
c/77328),
so I added a simple testcase using -fdiagnostics-show-caret,
which
does successfully show a range within the string.

Posting in the hope that it's helpful; I haven't attempted a
bootstrap
with it.


I've tried the patch but the changes don't compile because
substring_loc is not declared.  I see the class defined in
c-family/c-common.h which I can't include here in the middle
end.  Am I missing some another patch?


The fixup patch is on top of
   https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01811.html
which moves class substring_loc to gcc/substring-locations.h.


Great!  With that patch my pass builds fine, all my tests pass,
and based on a quick comparison the output of the diagnostics
looks unchanged.

Thanks
Martin

Re: C++ PATCH to forbid use of bool with the ++ operator

[Martin Sebor]

-   /* Forbid using -- on `bool'.  */
+   /* Forbid using -- or ++ in C++17 on `bool'.  */
if (TREE_CODE (declared_type) == BOOLEAN_TYPE)
  {
if (code == POSTDECREMENT_EXPR || code == PREDECREMENT_EXPR)
@@ -6040,6 +6040,20 @@ cp_build_unary_op (enum tree_code code, tree xarg, int 
noconvert,
   "to %");
return error_mark_node;
  }
+   else
+ {
+   if (cxx_dialect >= cxx1z)
+ {
+   if (complain & tf_error)
+ error ("use of Boolean expression as operand "
+"to % is forbidden in C++1z");


The capitalization of Boolean here caught my eye because it's
inconsistent with the recent spelling adopted in the documentation.
(It's also missing an article "a Boolean expression," although
dropping those is common in diagnostics. Still, it would be nice
to have a guideline/convention and use it consistently.)

Back to Boolean, I was actually going to comment on the Boolean
-> boolean change and suggest going in the opposite direction but
in the end decided not to (as Sandra's links showed, there's support
for both).

But having seen Boolean capitalized here I have changed my mind
again. I'd like to (belatedly) speak up in support of Boolean
(though I feel less strongly about it than I do about consistency).

FWIW, I've always found the capitalized spelling more appropriate
than the lowercase form. There are many examples of it, including
the Wikipedia entries on Boolean algebra, Boolean expression, and
Boolean data type, compiler manuals including those of HP C and
aCC (though HP is inconsistent and uses both), the IBM XLC/C++
Language Reference, the Microsoft Visual Basic documentation of
the type, the Wolfram MathWorld article on Boolean Algebra, and
so on.

Having said all that, since this is C++ the message could and
arguably should refer to a bool expression (or type) instead
and avoid having to deal with this altogether. In fact, it
would be simpler to rephrase the message as:

  "use of an operand of type %qT in ... is deprecated",
  boolean_type_node

As a separate issue, the message hardcodes operator++ but
the comment farther above says:

   Forbid using -- or ++ in C++17 on `bool'

Should it be parameterized on the kind expression and both
expressions tested?

Martin

Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Martin Sebor]

Attached is an updated patch that does away with the "overlapping"
warning and instead issues the same warnings as the C front end
(though with more context).

In struct flexmems_t I've retained the NEXT array even though only
the first element is used to diagnose problems.  The second element
is used by the find_flexarrays function to differentiate structs
with flexible array members in unions (which are valid) from those
in structs (which are not).

FWIW, I think this C restriction on structs is unnecessary and will
propose to remove it so that's valid to define a struct that contains
another struct (possibly recursively) with a flexible array member.
I.e., I think this should be made valid in C (and accepted without
the pedantic warning that GCC, and with this patch also G++, issues):

  struct X { int i, a[]; };
  struct S1 { struct X x; };
  struct S2 { struct S1 s1; };

z.C:2:22: warning: invalid use of ‘struct X’ with a flexible array 
member in ‘struct S1’ [-Wpedantic]

 struct S1 { struct X x; };
  ^
z.C:1:21: note: array member ‘int X::a []’ declared here
 struct X { int i, a[]; };
 ^
z.C:3:23: warning: invalid use of ‘struct X’ with a flexible array 
member in ‘struct S2’ [-Wpedantic]

 struct S2 { struct S1 s1; };
   ^~
z.C:1:21: note: array member ‘int X::a []’ declared here
 struct X { int i, a[]; };
 ^

Martin
PR c++/71912 - [6/7 regression] flexible array in struct in union rejected

gcc/cp/ChangeLog:
	PR c++/71912
	* class.c (struct flexmems_t):  Add members.
	(find_flexarrays): Add arguments.  Correct handling of anonymous
	structs.
	(diagnose_flexarrays): Adjust to issue warnings in addition to errors.
	(check_flexarrays): Add argument.
	(anon_context, diagnose_invalid_flexarray): New functions.

gcc/testsuite/ChangeLog:
	PR c++/71912
	* g++.dg/ext/flexary4.C: Adjust.
	* g++.dg/ext/flexary5.C: Same.
	* g++.dg/ext/flexary9.C: Same.
	* g++.dg/ext/flexary19.C: New test.
	* g++.dg/ext/flexary18.C: New test.
	* g++.dg/torture/pr64312.C: Add a dg-error directive to an ill-formed
	regression test.
* g++.dg/compat/struct-layout-1_generate.c (subfield): Add argument.
Avoid generating a flexible array member in an array.

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index f7147e6..6ae4fe2 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -147,11 +147,12 @@ static void check_methods (tree);
 static void remove_zero_width_bit_fields (tree);
 static bool accessible_nvdtor_p (tree);
 
-/* Used by find_flexarrays and related.  */
+/* Used by find_flexarrays and related functions.  */
 struct flexmems_t;
-static void find_flexarrays (tree, flexmems_t *);
 static void diagnose_flexarrays (tree, const flexmems_t *);
-static void check_flexarrays (tree, flexmems_t * = NULL);
+static void find_flexarrays (tree, flexmems_t *, bool = false,
+			 tree = NULL_TREE, tree = NULL_TREE);
+static void check_flexarrays (tree, flexmems_t * = NULL, bool = false);
 static void check_bases (tree, int *, int *);
 static void check_bases_and_members (tree);
 static tree create_vtable_ptr (tree, tree *);
@@ -6711,53 +6712,155 @@ field_nonempty_p (const_tree fld)
   return false;
 }
 
-/* Used by find_flexarrays and related.  */
-struct flexmems_t {
+/* Used by find_flexarrays and related functions.  */
+
+struct flexmems_t
+{
   /* The first flexible array member or non-zero array member found
- in order of layout.  */
+ in the order of layout.  */
   tree array;
   /* First non-static non-empty data member in the class or its bases.  */
   tree first;
-  /* First non-static non-empty data member following either the flexible
- array member, if found, or the zero-length array member.  */
-  tree after;
+  /* The first non-static non-empty data member following either
+ the flexible array member, if found, or the zero-length array member
+ otherwise.  AFTER[1] refers to the first such data member of a union
+ of which the struct containing the flexible array member or zero-length
+ array is a member, or NULL when no such union exists.  This element is
+ only used during searching, not for diagnosing problems.  AFTER[0]
+ refers to the first such data member that is not a member of such
+ a union.  */
+  tree after[2];
+
+  /* Refers to a struct (not union) in which the struct of which the flexible
+ array is member is defined.  Used to diagnose strictly (according to C)
+ invalid uses of the latter structs.  */
+  tree enclosing;
 };
 
 /* Find either the first flexible array member or the first zero-length
-   array, in that order or preference, among members of class T (but not
-   its base classes), and set members of FMEM accordingly.  */
+   array, in that order of preference, among members of class T (but not
+   its base classes), and set members of FMEM accordingly.
+   BASE_P is true if T is a base class of another class.
+   PUN is set to the outermost union in 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/08/2016 01:45 PM, David Malcolm wrote:

On Thu, 2016-09-08 at 13:03 -0600, Martin Sebor wrote:

Attached is another update to the patch to address the last round
of comments and suggestions, most notably to:

   *  implement the checking of the implementation limit of 4,095 on
  the output of a single directive to allow for the Glibc failure
  due to ENOMEM (the patch issues a warning and disables the
  optimization when this happens)
   *  implement checking for exceeding INT_MAX bytes (warn and disable
  optimization)
   *  call set_range_info when the return value optimization is not
  possible
   *  remove code to work around tree-optimization/71831 (now on
  trunk)

The -fprintf-return value optimization is still disabled.  GCC
successfully bootstraps with it and most tests pass but there's
a failure in the Fortran libgomp tests that I am yet to figure
out.

I'm hoping to get the patch reviewed and hopefully approved while
I debug the libgomp failure.

Martin


I see that you also integrated the substring_loc and format_warning API
into this revision - thanks.


Yes, I forgot to mention it among the highlights.  Thanks for making
the API available to the middle-end!



The patch has a lot of macro-based testcases, presumably for exercising
all of the format codes and boundary conditions, but it seems to be
lacking what I'd call a "usability test case" - a test case that shows
a representative example of idiomatic but buggy code, along with the
full output of the warning, with carets and underlining, so that we can
easily see what the user experience is.  (sorry if there is one and I
didn't see it).


There is a simple test (in the test_sprintf_note() function in
builtin-sprintf-warn-1.c) that exercises the notes but there's always
room for more and more robust test cases :)  I'll see about adding
a few.

FWIW, the challenge here is not in adding them but rather in knowing
when to stop and what level of detail to go to.  With too many test
cases (exercising this level of detail) it can get very tedious and
time consuming to then change the diagnostics or add more detail.
This is not an excuse for not having enough tests.  But striking
the right balance between the level of detail in them is something
I had to grapple with on this project.


 From a marketing point-of-view, I think any new diagnostics like this
deserve a screenshot on the website's gcc-7/changes.html page, showing
a simple example of the above that makes a casual reader think "gcc 7
looks neat; I've definitely made that mistake; I wonder if that's going
to find bugs in my code; I'd better try it at some point".

So please can you add a test case that demonstrates such a screenshot
-worthy example, using:

   /* { dg-options "-fdiagnostics-show-caret" } */

and you can use:

   /* { dg-begin-multiline-output "" }
copy the source, underlines and carets here, omitting trailing dg
directives.
  { dg-end-multiline-output "" } */

(we'd strip away all the dg- directives when making the screenshots for
the website).

The act of creating such an example sometimes suggests tweaks e.g. to
the exact wording of the warning.

Sorry if this seems like I'm picking on you Martin; I just wanted to
share some thoughts that I'm trying to crystallize into general
guidelines on writing diagnostics.


Not at all!  Thanks for the review and for the suggestion to make
use of the multiline DejaGnu directives.  I'll add more tests in
the next revision of the patch (as I have been in each iteration).

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/08/2016 01:21 PM, Jeff Law wrote:

On 08/24/2016 10:40 AM, Martin Sebor wrote:

On 08/23/2016 05:00 PM, Joseph Myers wrote:

Some observations:

* Does -fprintf-return-value allow for the possibility of snprintf
failing
because of a memory allocation failure and so returning -1 when GCC
computed bounds on what it could return if successful?


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.

At the same time, the cause of the failure doesn't really matter.
If the function could fail, unless GCC could determine the failure
at compile time and avoid the optimization, the transformation
wouldn't be guaranteed to be safe.  It might be something to
consider and possibly accommodate in the implementation.  It's
one of the reasons why I want to expose the optimization to
more code before enabling it.

I also haven't yet thought about how to deal with it but if it
is a possibility we want to allow for then maybe a target hook
for libc implementers to set to indicate whether sprintf can fail
and when would work.  Libc implementations that can fail under
any conditions (whether allowed by the standard or not) would
need to disable (or not enable, depending on the default) the
optimization.  I'm certainly open to other ideas.

So what are the implications for the optimization part of this patch?


By coincidence I've just posted an updated patch that handles this
situation by avoiding the optimization (and issuing a warning pointing
out that a directive produced more that 4,095 bytes worth of output).
Ditto for INT_MAX.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/12/2016 04:08 PM, Joseph Myers wrote:

On Sun, 11 Sep 2016, Martin Sebor wrote:


Attached is revision 8 of the patch (hopefully) adjusted as
requested above, and with a simple test with of the multiline


The ChangeLog appears not to have been updated (at least, it doesn't
mention the changes to target.def).


I did update the ChangeLog but missed this file.  Are there any
substantive issues with the patch that you noticed or may I take
this as your approval (modulo the ChangeLog)?

Martin

[PATCH] have __builtin_object_size handle POINTER_PLUS with non-const offset

[Martin Sebor]

__builtin_object_size fails for POINTER_PLUS expressions involving
non-constant offsets into objects of known size, causing GCC to fail
to detect (and add instrumentation to prevent) buffer overflow in
some basic cases such as the following:

  void f (unsigned i)
  {
char d [3];
memcpy (d + i, "abcdef", 5);
  }

Since the size of the destination object is known, the call to memcpy
is guaranteed to write past the end of it regardless of the value of
the offset.

The attached patch enhances __builtin_object_size to handle this case
by returning the size of the whole object as the maximum and the size
of the object minus T_MAX for the type of the offset T as the minimum.

The patch also adds handling of ranges even though only very few cases
benefit from it because the VRP pass runs after the object size pass.
The one case that does appear to profit is when the value of the offset
is constrained by its type, as in

  char a [1000];

  unsigned g (unsigned char i)
  {
char *p = a + i;
return __builtin_object_size (p, 2);
  }

Here get_range_info () lets __builtin_object_size determine that
the minimum number of bytes between (a + i) and (a + sizeof s) is
sizeof a - UCHAR_MAX.

The patch results in 64 more checking calls in a Binutils build than
before.

Martin

PS What would be a good way to arrange for the VRP pass to run before
the object size pass so that the latter can benefit more from range
information?  As an experiment I added another instance of the VRP
pass before the object size pass in passes.def and that worked, but
I suspect that running VRP a third time isn't optimal.
PR middle-end/77608 - missing protection on trivially detectable runtime
	buffer overflow

gcc/testsuite/ChangeLog:
	PR middle-end/77608
	* gcc.dg/builtin-object-size-18.c: New test.

gcc/ChangeLog:
	PR middle-end/77608
	* tree-object-size.c (plus_stmt_object_size): Handle non-constant
	offsets.

diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-18.c b/gcc/testsuite/gcc.dg/builtin-object-size-18.c
new file mode 100644
index 000..e8e2269
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-object-size-18.c
@@ -0,0 +1,76 @@
+/* PR 77608 - missing protection on trivially detectable runtime buffer
+   overflow */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+#define concat(a, b)   a ## b
+#define CAT(a, b)  concat (a, b)
+
+/* Create a symbol name unique to each tes and object size type.  */
+#define SYM(type)  CAT (CAT (CAT (failure_on_line_, __LINE__), _type_), type)
+
+/* References to the following undefined symbol which is unique for each
+   test case are expected to be eliminated.  */
+#define TEST_FAILURE(type)			\
+  do {		\
+extern void SYM (type)(void);		\
+SYM (type)();\
+  } while (0)
+
+#define bos(obj, type) __builtin_object_size (obj, type)
+#define size(obj, n) ((size_t)n == X ? sizeof *obj : (size_t)n)
+
+/* For convenience.  Substitute for 'sizeof object' in test cases where
+   the size can vary from target to target.  */
+#define X  (size_t)0xdeadbeef
+
+#define test(expect, type, obj)			\
+  do {		\
+if (bos (obj, type)	!= size (obj, expect))	\
+  TEST_FAILURE (type);			\
+  } while (0)
+
+/* Verify that each of the expressions __builtin_object_size(OBJ, n)
+   is folded into Rn.  */
+#define fold(r0, r1, r2, r3, obj)		\
+  do {		\
+test (r0, 0, (obj));			\
+test (r1, 1, (obj));			\
+test (r2, 2, (obj));			\
+test (r3, 3, (obj));			\
+  } while (0)
+
+/* Verify that __builtin_object_size(xBUF + OFF, n) is folded into Rn
+   for the specified OFF and each of the Rn values.  */
+#define FOLD(off, r0, r1, r2, r3)		\
+  do {		\
+fold (r0, r1, r2, r3, gbuf + off);		\
+sink (gbuf);\
+fold (r0, r1, r2, r3, lbuf + off);		\
+sink (lbuf);\
+fold (r0, r1, r2, r3, abuf + off);		\
+sink (abuf);\
+  } while (0)
+
+#define UCHAR_MAX (__SCHAR_MAX__  * 2 + 1)
+#define BUFSIZE   (UCHAR_MAX * 2)
+
+typedef __SIZE_TYPE__ size_t;
+
+void sink (void*, ...);
+
+char gbuf[BUFSIZE];
+
+void test_pointer_plus (char ci, short si, int i, long li)
+{
+  char lbuf[BUFSIZE];
+
+  char *abuf = __builtin_malloc (BUFSIZE);
+
+  FOLD (ci, BUFSIZE, BUFSIZE, BUFSIZE - UCHAR_MAX, BUFSIZE - UCHAR_MAX);
+  FOLD (si, BUFSIZE, BUFSIZE, 0, 0);
+  FOLD ( i, BUFSIZE, BUFSIZE, 0, 0);
+  FOLD (li, BUFSIZE, BUFSIZE, 0, 0);
+}
+
+/* { dg-final { scan-tree-dump-not "failue_on_line" "optimized" } } */
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index 1317ad7..9e84156 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -837,21 +839,49 @@ plus_stmt_object_size (struct object_size_info *osi, tree var, gimple *stmt)
   if (object_sizes[object_size_type][varno] == unknown[object_size_type])
 return false;
 
+  bool estimated = false;
+
+  /* If the OFFSET isn't constant try to determine its range and use
+ either the lower or the upper bound of the range in its place.  */
+  if 

Re: [PATCH] avoid non-printable characters in diagnostics (c/77620, c/77521)

[Martin Sebor]

On 09/09/2016 07:59 AM, Joseph Myers wrote:

On Thu, 8 Sep 2016, Martin Sebor wrote:


PS I used hexadecimal based on what c-format.c does but now that
I checked more carefully how %qE formats string literals I see it
uses octal.  I think hexadecimal is preferable because it avoids
ambiguity but I'm open to changing it to octal if there's a strong


I'm not clear what you mean about ambiguity.  In C strings, an octal
escape sequence has up to three characters, so if it has three characters
it's unambiguous, whereas a hex escape sequence can have any number of
characters, so if the unprintable character is followed by a valid hex
digit then in C you need to represent that as an escape (or use string
constant concatenation, etc.).  The patch doesn't try to do that as far as
I can see.

Now, presumably the output isn't intended to be interpreted as C strings
anyway (if it was, you'd need to escape " and \ as well), so the patch is
OK, but I don't think it avoids ambiguity (and there's a clear case that
it shouldn't - that if the string passed to %qs is printable, it should be
printed as-is even if it contains escape sequences that could also result
from a non-printable string passed to %qs).


Thank you.

I tried to be clear about it in the description of the changes
but I see the PS caused some confusion.  Let me clarify that
the patch has nothing to do with with ambiguity (perceived or
real) in the representation of the escape sequences.  The only
purpose of the change is to avoid printing non-printable
characters or excessively large escape sequences in GCC
diagnostics.

I mentioned the hex vs octal notation to invite input into which
of the two of them people would prefer to see used by the %qc and
qs directives, and whether it's worth considering changing the %qE
directive to use the same notation as well, for consistency (and
to help with readability if there is consensus that one is clearer
than the other).

What I meant by ambiguity is for example a string like "\1234"
where it's not obvious where the octal sequence ends.  Is it '\1'
followed  by "234" or '\12' followed by "34" or '\123' followed
by "4"?  (It's only possible to tell if one knows that GCC always
uses three digits for the octal character, but not everyone knows
that.)  To be clear: I'm talking about the GCC output and not
necessarily about what the standard has to say about it.

In contrast to the octal notation, I find the string "\x1234"
clearer.  It can only mean '\x1' followed by "234" or '\x12'
followed by "34" and I think more people will expect it to be
the latter because representing characters using two hex digits
is more common.  But this is just my own perception and YMMV.

Martin

[PATCH] avoid non-printable characters in diagnostics (c/77620, c/77521)

[Martin Sebor]

While working on the -Wformat-length pass I noticed that in some
diagnostics that make use of the %qc and %qs directives GCC prints
non-printable characters raw.  For example, it might print a newline,
corrupting the diagnostic stream (bug 77521).

Some other diagnostics that try to avoid this problem by using
a directive such as %x when the character is not printable might
use the sign-extended value of the character, printing a very
large hexadecimal value (bug 77520).

The attached patch changes the pretty printer to detect non-printable
characters in %qc and %qs directives (but not %c or %s) and print
those in hexadecimal (via "\\x%02x").

Martin

PS I used hexadecimal based on what c-format.c does but now that
I checked more carefully how %qE formats string literals I see it
uses octal.  I think hexadecimal is preferable because it avoids
ambiguity but I'm open to changing it to octal if there's a strong
preference for it.  Incidentally, %qE too suffers from bug 77520
(see below).  The patch doesn't try to fix that.

$ cat z.C && gcc z.C
constexpr int i = "ABC\x7f_\x80XYZ";
z.C:1:19: error: invalid conversion from ‘const char*’ to ‘int’ 
[-fpermissive]

 constexpr int i = "ABC\x7f_\x80XYZ";
   ^
z.C:1:19: error: ‘(int)((const char*)"ABC\177_\3777600XYZ")’ is not 
a constant expression
PR c/77520 - wrong value for extended ASCII characters in -Wformat message
PR c/77521 - %qc format directive should quote non-printable characters

gcc/c-family/ChangeLog:
2016-09-08  Martin Sebor  <mse...@redhat.com>

	PR c/77520
	PR c/77521
	* c-format.c (argument_parser::find_format_char_info): Use %qc
	format directive unconditionally.

gcc/ChangeLog:
2016-09-08  Martin Sebor  <mse...@redhat.com>

	PR c/77520
	PR c/77521
	* pretty-print.c (pp_quoted_string): New function.
	(pp_format): Call it for %c and %s directives.

gcc/testsuite/ChangeLog:
2016-09-08  Martin Sebor  <mse...@redhat.com>

	PR c/77520
	PR c/77521
	* gcc.dg/pr77520.c: New test.
	* gcc.dg/pr77521.c: New test.

diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
index 09d514e..0c17340 100644
--- a/gcc/c-family/c-format.c
+++ b/gcc/c-family/c-format.c
@@ -2355,20 +2355,12 @@ argument_parser::find_format_char_info (char format_char)
 ++fci;
   if (fci->format_chars == 0)
 {
-  if (ISGRAPH (format_char))
-	format_warning_at_char (format_string_loc, format_string_cst,
-format_chars - orig_format_chars,
-OPT_Wformat_,
-"unknown conversion type character"
-" %qc in format",
-format_char);
-  else
-	format_warning_at_char (format_string_loc, format_string_cst,
-format_chars - orig_format_chars,
-OPT_Wformat_,
-"unknown conversion type character"
-" 0x%x in format",
-format_char);
+  format_warning_at_char (format_string_loc, format_string_cst,
+			  format_chars - orig_format_chars,
+			  OPT_Wformat_,
+			  "unknown conversion type character"
+			  " %qc in format",
+			  format_char);
   return NULL;
 }
 
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index 325263e..a39815e 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
 #include 
 #endif
 
+static void pp_quoted_string (pretty_printer *, const char *, size_t = -1);
+
 /* Overwrite the given location/range within this text_info's rich_location.
For use e.g. when implementing "+" in client format decoders.  */
 
@@ -555,8 +557,20 @@ pp_format (pretty_printer *pp, text_info *text)
 	  break;
 
 	case 'c':
-	  pp_character (pp, va_arg (*text->args_ptr, int));
-	  break;
+	  {
+	/* When quoting, print alphanumeric, punctuation, and the space
+	   character unchanged, and all others in hexadecimal with the
+	   "\x" prefix.  Otherwise print them all unchanged.  */
+	int chr = va_arg (*text->args_ptr, int);
+	if (ISPRINT (chr) || !quote)
+	  pp_character (pp, chr);
+	else
+	  {
+		const char str [2] = { chr, '\0' };
+		pp_quoted_string (pp, str, 1);
+	  }
+	break;
+	  }
 
 	case 'd':
 	case 'i':
@@ -577,7 +591,10 @@ pp_format (pretty_printer *pp, text_info *text)
 	  break;
 
 	case 's':
-	  pp_string (pp, va_arg (*text->args_ptr, const char *));
+	  if (quote)
+	pp_quoted_string (pp, va_arg (*text->args_ptr, const char *));
+	  else
+	pp_string (pp, va_arg (*text->args_ptr, const char *));
 	  break;
 
 	case 'p':
@@ -939,6 +956,41 @@ pp_string (pretty_printer *pp, const char *str)
   pp_maybe_wrap_text (pp, str, str + strlen (str));
 }
 
+/* Append the leading N characters of STRING to the output area of
+   PRETTY-PRINTER, quoting in hexadecimal non-printable characters.
+   Setting N = -1 is as if N were set to strlen (STRING).  The STRING
+   may be line-wrapped if in appropri

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

Patch #1.  All the fixes to static buffer sizes that were inspired by
your warning.  These are all approved and can go in immediately.



Attached is this patch.


Hi, this patch changed the file gcc/go/gofrontend/expressions.cc.  As
described in gcc/go/gofrontend/README, the files in the directory
gcc/go/gofrontend should not be changed in the GCC repository.
Instead, they are mirrored into GCC from a separate repository,
https://go.googlesource.com/gofrontend .  When you need to make
changes in the gofrontend directory, you can either follow the
contribution process described as
https://golang.org/doc/gccgo_contribute.html, or simply ask me to make
the change.  Thanks.


Sorry if this caused an inconvenience. I'll keep it in mind in
the future.

Thanks
Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

The problem is e.g. code calling getwchar and not checking for WEOF.
0xu does not fit in a 32-bit signed variable unless your code is
confusing signed and unsigned freely.  And even if you consider it OK to
check after conversion to wchar_t rather than before, the lack of a wint_t
variable for the result seems likely to accompany a missing check.


Calling printf("%lc", x) with a wchar_t argument hardly suggests
anything about where the argument originated or what checking it
may have been subjected to.  Issuing a warning for a safe piece
of code only on the basis that there might be some other piece
of code in the program that does something wrong makes no sense.
Suggesting to the user to change the safe piece of code is
misleading and counterproductive.


There are plenty of architectures where the ABI does require the high part
to be extended in a particular way;


There is no high part when the wchar_t and wint_t types have
the same size, which they do not only in the case of interest but
also in all the other cases we know of.  Again, unless you know
of a counterexample this point isn't relevant to the discussion.


I think the warning is the correct thing for anyone trying to write C as a
high-level language and be type-correct and properly check before doing
conversions that might change values unless those changes are actually
intended, and that warning rather than lesser warnings for "C as portable
assembler" is the appropriate default for format checking.


I certainly agree with this general philosophy.  Unfortunately,
the wchar_t and wint_t types on the target of interest (i386
or ILP32 on x86_64) are defined in an unhelpful way that the
-Wformat checker normally uses to diagnose real (or potential)
problems that can do do come up with their underlying types.
They just can't come up with wchar_t and wint_t.  Useful
warnings shouldn't pedantically enforce type rules in cases
where the rules serve no practical purpose or are loose enough
to allow for nonsensical implementations.  This is a basic
principle that's been followed by the vast majority of GCC
warnings and that users have come to appreciate and rely on.

The underlying problem here (and I claim a bug) is simply that
the -Wformat code doesn't distinguish the wint_t typedef (and
in C, the wchar_t typedef as well) from their underlying types
and treats the typedefs the same.  It should not, at least not
at the default warning level.  I wouldn't object to retaining
the warning with -Wpedantic but otherwise it's unhelpful.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 05:11 PM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


may have been subjected to.  Issuing a warning for a safe piece
of code only on the basis that there might be some other piece
of code in the program that does something wrong makes no sense.
Suggesting to the user to change the safe piece of code is
misleading and counterproductive.


It's warning because it's *bad code*.


Why?  What can go wrong and on what system?  Please name at least
one specific example.


Whether it's safe or not is
peripheral.  Lots of warnings are for things that are dubious even if they
may happen to do what the user wants on the platforms they care about.
I'd consider even cases of different signedness (e.g. passing one of int
and unsigned int to a format expecting the other) to be dubious without a
reason why values not representable in the other type can't reach that
code - and I'd consider cases where the types are different not just in
signedness to be clearly worse than the cases that we only warn for with
-Wformat-signedness.


There are plenty of architectures where the ABI does require the high part
to be extended in a particular way;


There is no high part when the wchar_t and wint_t types have
the same size, which they do not only in the case of interest but


I mean high parts if the ABI is passing those arguments in 64-bit
registers / stack slots and has requirements on how 32-bit arguments are
extended for passing as 64-bit.


I don't understand what you mean.  We're talking about passing
an integer (wchar_t) via the ellipsis.  That's just as well
defined as passing any other integer of that size, as is (for
all practical purposes) extracting it via an integer of the
opposite signedness.  printf then stuffs the extracted wint_t
value (which is of the same size as the original wchar_t) into
a wchar_t, appends a nul, and formats the result via %ls (at
least that's how it's specified).

Please explain what can go wrong on i386 when someone calls
printf("%lc", L'a'), or on any other target GCC supports.
I will be happy to agree that the warning is justified if
you show me how this breaks and where.

Martin

Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Martin Sebor]

On 09/23/2016 12:00 PM, Jason Merrill wrote:

On Thu, Sep 22, 2016 at 7:39 PM, Martin Sebor <mse...@gmail.com> wrote:

On 09/21/2016 02:43 PM, Jason Merrill wrote:

On Tue, Sep 20, 2016 at 1:01 PM, Martin Sebor <mse...@gmail.com> wrote:

On 09/16/2016 12:19 PM, Jason Merrill wrote:

On 09/14/2016 01:03 PM, Martin Sebor wrote:


+  /* Type of the member.  */
+  tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld;


Why set "fldtype" to be a TYPE_DECL rather than its type?


I'm not sure I understand the question but (IIRC) the purpose of
this code is to detect invalid uses of flexible array members in
typedefs such as this:

   struct S { typedef struct { int i, a[]; } X2 [2]; };

Sadly, it doesn't do anything for

   struct X { int i, a[]; };
   struct S { typedef struct X X2 [2]; };


The issue is I don't see anything that uses fldtype as a decl, and
it's strange to have a variable called "*type" that can either be a
type or a decl, which later code still assumes will be a type.


I suspect I'm simply looking at it from a different point of view.
The definition

  tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld

denotes the type of the field if fld is a data member.  Otherwise,
it denotes a type (like a typedef).


FLD is always a _DECL.  The result of this assignment is that fldtype
refers to either a _TYPE or a _DECL.  This creates a strange
asymmetry, and none of the following code seems to depend on it.
I would prefer

tree fldtype = TREE_TYPE (fld);

so that it's always a _TYPE.


I mentioned that this code is important to avoid diagnosing typedefs, 
but the example I showed wasn't the right one.  The examples that do

depend on it are these two:

  struct S1 {
typedef int A [0];
A a;
  };

  struct S2 {
typedef struct { int i, a[]; } A1;
typedef struct { int i, a[]; } A2;
  };

The expected (pedantic) warning is:

  warning: zero-size array member ‘S1::a’ in an otherwise empty ‘struct 
S1’


With the change you're asking we end up with:

  warning: zero-size array member ‘S1::a’ not at end of ‘struct S1’

followed by a bogus

  error: flexible array member ‘S2::A1::a’ not at end of ‘struct S2’

So I still don't understand what you're looking for.  It sounds to
me like you're asking me to rewrite the subsequent code that uses
fldtype to use the expression above because you don't like that
fldtype can be either a _DECL or _TYPE.  But repeating that check
four times doesn't make sense, so what am I missing?


My point is that for

struct X { int n, a[]; };
struct X2 { struct { int n, a[]; }; };
struct Y { int n, b[]; };

struct D: X, Y { };
struct D2: X2, Y { };

We say

wa3.C:1:21: error: flexible array member ‘X::a’ not at end of ‘struct D’
wa3.C:2:31: error: flexible array member ‘X2a’ not
at end of ‘struct X2’

If we want to say "struct D", why don't we also want to say "struct
D2"?  It seems that you could unconditionally set fmemctx to t and not
bother calling anon_context.


I see.  It looks like it doesn't matter anymore.  I suspect the
whole anon_context business became unnecessary with the removal
of the overlapping union member code. I've removed the code and
added a test case to verify that the error references the same
parent class in both cases.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 10:27 AM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


The warning would only helpful if there was a target where wchar_t
didn't promote to a type with the same precision as wint_t, or in


Or it could show up a logical error where the code should have had a
wint_t variable and checked for WEOF somewhere but just assumed things
would fit in wchar_t


WEOF that fits in a wint_t also fits in a wchar_t when the two types
have the same precision, the case where GCC issues the warning (and
where WEOF expands to 0xu).  So this isn't an example of
a problem that could arise on the targets that brought this up, or
any other targets that I'm aware of.


(and so could be passing a negative value where the
ABI means the callee expects wint_t values to be zero-extended, with
consequent undefined behavior there).


Sign extension is also not an issue on the target where the warning
is issued (x86_64 ILP32) and where wchar_t is int and wint_t is
unsigned long.


The correct fix involves thinking
about where you know something is a wide character, and where something
might be a wide character or WEOF.  It might then involve a cast to
wint_t, or changing the type of a variable, or more changes than that.


There is nothing to fix when wchar_t and wint_t have the same
precision and  printf("%lc", L'x') does the same thing as
printf("%lc", (wint_t) L'x').  The cast is entirely superfluous.

Even so, the warning could have merit if there were a different
target to which the program might need to be ported where this
property didn't hold.  But as I said, I don't know of one and,
as it seems, neither do you.

I'm very much in favor of strict warnings that help point out
either real or potential bugs.  But I see no value in warning
about constructs that are perfectly safe.  To the contrary,
I think such warnings are harmful because they lead to
unnecessary churn and to bug injection.

It might be worthwhile to mention that Clang used to issue the
same warning as GCC until 2011 when they fixed it.  The bug is
here:

  https://llvm.org/bugs/show_bug.cgi?id=7981

(Although I think there is a flaw in the logic they used in
comment #2 it's only hypothetical because there is no target
that Clang supports where the submitted test case doesn't do
what it's expected to do either.)

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 09:40 AM, Joseph Myers wrote:

On Tue, 4 Oct 2016, Martin Sebor wrote:


Well, typically cases where one of long and int is passed and the other is
expected, but they have the same size, are bugs waiting to happen when the
code is built on a 64-bit system.  That is, they *should* warn.


Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
think it's helpful.  Consider this case from my comment #5 on bug
72858.  I don't think there is any point in issuing a warning here.
On the majority of targets they either all are or promote to a type
of the same size, don't they?


I'm unconvinced by that "majority of targets" of argument (which once
would have been true for int and long, and you could say it is true for
long and size_t).  There's a clear correct type here, and it's wint_t, and
it's always easy to fix the code to use the correct type.


The warning would only helpful if there was a target where wchar_t
didn't promote to a type with the same precision as wint_t, or in
other words where printf("%lc", L'a') did something different than
printf("%lc", (wint_t) L'a'). I don't know of such a target.  Can
you give an example of one?

It isn't any of the ILP32 targets where wint_t is int and wchar_t
is long and where the warning was found to cause test suite failures,
and it isn't any other other targets where the warning didn't trigger
because the two types have the same precision.

Otherwise, when there is no such target there is nothing to fix, and
changing working code just for the sake of pedantry only runs the
unnecessary risk of introducing bugs.  IMO, this is especially true
in subtle cases like this where the people who make the changes often
don't fully appreciate the subtleties of the code.  Suggesting the
wrong directive (replacing %lc with %ld) compounds the problem
further(*).

Martin

[*] This is not meant as a dig at David but rather to underscore
that both the "problem" is subtle and the expected solution not
obvious.

Re: [PATCH] add uClibc target hook (PR bootstrap/77819)

[Martin Sebor]

On 10/04/2016 08:54 AM, Bernd Schmidt wrote:

On 10/04/2016 04:34 PM, Martin Sebor wrote:


I copied the conditional from config/linux.h but I admit I don't
fully understand when the macro is defined.


AFAICT it's done in config.gcc, for a limited set of targets.


Should I still remove it from targhooks.c?


That is compiled for all targets, not just for those which define the
macro, so yes.


Yes, the glibc and uclibc hooks are the same.  I don't know what
the convention is for these target hooks (i.e., whether they are
expected to be duplicated across targets even if they are the
same to reduce the risk of breaking one target as a result of
changing another, or whether duplication should be avoided even
at this risk).  From your comment it sounds like it should be
the latter and I'm okay with that.


There's arguments for both. In this particular case I don't see a strong
reason not to have a general hook available.


Sounds good.  Attached is an updated patch reflecting these changes.
Restested by building the powerpc64-linux and tic6x-uclinux cross
toolchains.  (Sharing the Glibc and uClibc implementation of the
target hook and defining it in targhooks.c also obviates the patch
I sent in for bug 77837 last night so it seems like a win-win.)

Martin
PR bootstrap/77819 - undefined reference to gnu_libc_printf_pointer_format with uClibc

gcc/ChangeLog:

	PR bootstrap/77819
	* config/linux.h (TARGET_PRINTF_POINTER_FORMAT): Define macro.
	* config/linux.c (gnu_libc_printf_pointer_format): Remove.
	* targhooks.c [DEFAULT_LIBC == LIBC_UCLIBC) && SINGLE_LIBC]
	(default_printf_pointer_format): Define function.
	* targhooks.c (linux_printf_pointer_format): Define new function.
	* targhooks.h (linux_printf_pointer_format): Declare.
	(gnu_libc_printf_pointer_format): Remove declaration.

diff --git a/gcc/config/linux.c b/gcc/config/linux.c
index 9aac38b..a393d3b 100644
--- a/gcc/config/linux.c
+++ b/gcc/config/linux.c
@@ -24,9 +24,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree.h"
 #include "linux-protos.h"
 
-#undef TARGET_PRINTF_POINTER_FORMAT
-#define TARGET_PRINTF_POINTER_FORMAT gnu_libc_printf_pointer_format
-
 bool
 linux_libc_has_function (enum function_class fn_class)
 {
@@ -40,16 +37,3 @@ linux_libc_has_function (enum function_class fn_class)
 
   return false;
 }
-
-/* Glibc formats pointers as if by "%zx" except for the null pointer
-   which outputs "(nil)".  It ignores the pound ('#') format flag but
-   interprets the space and plus flags the same as in the integer
-   directive.  */
-
-const char*
-gnu_libc_printf_pointer_format (tree arg, const char **flags)
-{
-  *flags = " +";
-
-  return arg && integer_zerop (arg) ? "(nil)" : "%#zx";
-}
diff --git a/gcc/config/linux.h b/gcc/config/linux.h
index 3ff005b..7211da2 100644
--- a/gcc/config/linux.h
+++ b/gcc/config/linux.h
@@ -209,6 +209,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #endif
 
-/* The format string to which "%p" corresponds.  */
+/* The format string to which "%p" corresponds (same in Glibc and
+   uClibc.  */
 #undef TARGET_PRINTF_POINTER_FORMAT
-#define TARGET_PRINTF_POINTER_FORMAT gnu_libc_printf_pointer_format
+#define TARGET_PRINTF_POINTER_FORMAT linux_printf_pointer_format
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index d75650f..c7977be 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1523,6 +1523,22 @@ default_printf_pointer_format (tree, const char **flags)
   return "%zx";
 }
 
+/* For Glibc and uClibc targets also define the hook here because
+   otherwise it would have to be duplicated in each target's .c file
+   (such as in bfin/bfin.c and c6x/c6x.c, etc.)
+   Glibc and uClibc format pointers as if by "%zx" except for the null
+   pointer which outputs "(nil)".  It ignores the pound ('#') format
+   flag but interprets the space and plus flags the same as in the integer
+   directive.  */
+
+const char*
+linux_printf_pointer_format (tree arg, const char **flags)
+{
+  *flags = " +";
+
+  return arg && integer_zerop (arg) ? "(nil)" : "%#zx";
+}
+
 tree
 default_builtin_tm_load_store (tree ARG_UNUSED (type))
 {
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 3356f0a..afb1c00 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -192,7 +192,7 @@ extern bool no_c99_libc_has_function (enum function_class);
 extern bool gnu_libc_has_function (enum function_class);
 
 extern const char* default_printf_pointer_format (tree, const char **);
-extern const char* gnu_libc_printf_pointer_format (tree, const char **);
+extern const char* linux_printf_pointer_format (tree, const char **);
 extern const char* solaris_printf_pointer_format (tree, const char **);
 
 extern tree default_builtin_tm_load_store (tree);

[PATCH] backport dejagnu relative numbers to 6-branch?

[Martin Sebor]

While backporting a patch for 77804 to the gcc-6-branch I noticed
that the DejaGnu relative number patch below is not available
there (the new test failed).  Is it worth backporting it to it?

  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01617.html

Martin

Re: [PATCH] backport dejagnu relative numbers to 6-branch?

[Martin Sebor]

On 10/04/2016 12:35 PM, Jakub Jelinek wrote:

On Tue, Oct 04, 2016 at 12:05:50PM -0600, Martin Sebor wrote:

While backporting a patch for 77804 to the gcc-6-branch I noticed
that the DejaGnu relative number patch below is not available
there (the new test failed).  Is it worth backporting it to it?

  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01617.html


Dunno, some people tend to backport testsuite infrastructure improvements,
but one still has to test stuff on the release branches anyway, so adjusting
to older infrastructure isn't that hard either.

So, I'd leave such a decision to the testsuite maintainers if somebody is
willing to test the backport of the patch.


I ran the C++ test suite with the patch with no unexpected failures
before proposing it.  I'm willing to run the rest of it if Mike and
Rainer (CC'd) think it's worth the effort.  I don't know how many
more 6.x releases to expect and don't have a sense of how many bug
fixes tend to get backported into each so I fully defer to others
on this decision.

Martin

Re: [PATCH] add uClibc target hook (PR bootstrap/77819)

[Martin Sebor]

On 10/04/2016 02:42 AM, Bernd Schmidt wrote:

On 10/03/2016 12:02 AM, Martin Sebor wrote:

I couldn't find a good uclibc-only file where to put the new
definition of the hook so I conditionally added it to
targethooks.c.



diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index d75650f..77b4a18 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1523,6 +1523,26 @@ default_printf_pointer_format (tree, const char
**flags)
   return "%zx";
 }

+#if (DEFAULT_LIBC == LIBC_UCLIBC) && defined (SINGLE_LIBC) /* uClinux */


I think DEFAULT_LIBC is defined only for linux targets, so this won't
do. Just define unconditionally, with a declaration in targhooks.h?


I copied the conditional from config/linux.h but I admit I don't
fully understand when the macro is defined.  Should I still remove
it from targhooks.c?




+const char*
+uclibc_printf_pointer_format (tree arg, const char **flags)
+{
+  *flags = " +";
+
+  return arg && integer_zerop (arg) ? "(nil)" : "%#zx";
+}


Then again, maybe also just move the hook from linux.c here, it appears
identical.


Yes, the glibc and uclibc hooks are the same.  I don't know what
the convention is for these target hooks (i.e., whether they are
expected to be duplicated across targets even if they are the
same to reduce the risk of breaking one target as a result of
changing another, or whether duplication should be avoided even
at this risk).  From your comment it sounds like it should be
the latter and I'm okay with that.

Thanks
Martin

Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Martin Sebor]

ot;, t);
+		  fmem->after[0]);
+	  inform (location_of (t), "in the definition of %q#T", t);
+	}
 	}
 }
+
+  if (!diagd && fmem->array && fmem->enclosing)
+diagnose_invalid_flexarray (fmem);
 }
 
 
@@ -6854,7 +6997,8 @@ diagnose_flexarrays (tree t, const flexmems_t *fmem)
that fails the checks.  */
 
 static void
-check_flexarrays (tree t, flexmems_t *fmem /* = NULL */)
+check_flexarrays (tree t, flexmems_t *fmem /* = NULL */,
+		  bool base_p /* = false */)
 {
   /* Initialize the result of a search for flexible array and zero-length
  array members.  Avoid doing any work if the most interesting FMEM data
@@ -6862,18 +7006,20 @@ check_flexarrays (tree t, flexmems_t *fmem /* = NULL */)
   flexmems_t flexmems = flexmems_t ();
   if (!fmem)
 fmem = 
-  else if (fmem->array && fmem->first && fmem->after)
+  else if (fmem->array && fmem->first && fmem->after[0])
 return;
 
+  tree fam = fmem->array;
+
   /* Recursively check the primary base class first.  */
   if (CLASSTYPE_HAS_PRIMARY_BASE_P (t))
 {
   tree basetype = BINFO_TYPE (CLASSTYPE_PRIMARY_BINFO (t));
-  check_flexarrays (basetype, fmem);
+  check_flexarrays (basetype, fmem, true);
 }
 
   /* Recursively check the base classes.  */
-  int nbases = BINFO_N_BASE_BINFOS (TYPE_BINFO (t));
+  int nbases = TYPE_BINFO (t) ? BINFO_N_BASE_BINFOS (TYPE_BINFO (t)) : 0;
   for (int i = 0; i < nbases; ++i)
 {
   tree base_binfo = BINFO_BASE_BINFO (TYPE_BINFO (t), i);
@@ -6887,7 +7033,7 @@ check_flexarrays (tree t, flexmems_t *fmem /* = NULL */)
 	continue;
 
   /* Check the base class.  */
-  check_flexarrays (BINFO_TYPE (base_binfo), fmem);
+  check_flexarrays (BINFO_TYPE (base_binfo), fmem, /*base_p=*/true);
 }
 
   if (fmem == )
@@ -6904,17 +7050,26 @@ check_flexarrays (tree t, flexmems_t *fmem /* = NULL */)
 	  /* Check the virtual base class.  */
 	  tree basetype = TREE_TYPE (base_binfo);
 
-	  check_flexarrays (basetype, fmem);
+	  check_flexarrays (basetype, fmem, /*base_p=*/true);
 	}
 }
 
-  /* Search the members of the current (derived) class.  */
-  find_flexarrays (t, fmem);
+  /* Is the type unnamed (and therefore a member of it potentially
+ an anonymous struct or union)?  */
+  bool maybe_anon_p = TYPE_UNNAMED_P (t);
 
-  if (fmem == )
+  /* Search the members of the current (possibly derived) class, skipping
+ unnamed structs and unions since those could be anonymous.  */
+  if (fmem !=  || !maybe_anon_p)
+find_flexarrays (t, fmem, base_p || fam != fmem->array);
+
+  if (fmem ==  && !maybe_anon_p)
 {
-  /* Issue diagnostics for invalid flexible and zero-length array members
-	 found in base classes or among the members of the current class.  */
+  /* Issue diagnostics for invalid flexible and zero-length array
+	 members found in base classes or among the members of the current
+	 class.  Ignore anonymous structs and unions whose members are
+	 considered to be members of the enclosing class and thus will
+	 be diagnosed when checking it.  */
   diagnose_flexarrays (t, fmem);
 }
 }
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f2e83f8..bd674b0 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1324,6 +1324,12 @@
 	* gcc.target/msp430/function-attributes-2.c: New test.
 	* gcc.target/msp430/function-attributes-3.c: New test.
 
+2015-04-18  Martin Sebor  <mse...@redhat.com>
+
+	* gfortran.dg/pr32627.f03 (strptr): Change size to match the number
+	of non-nul characters.
+	* gfortran.dg/substr_6.f90: Make the NUL character visible on stdout
+
 2016-09-13  Jakub Jelinek  <ja...@redhat.com>
 
 	* g++.dg/cpp0x/gen-attrs-61.C: New test.
diff --git a/gcc/testsuite/g++.dg/compat/struct-layout-1_generate.c b/gcc/testsuite/g++.dg/compat/struct-layout-1_generate.c
index 71d4af9..8b620e4 100644
--- a/gcc/testsuite/g++.dg/compat/struct-layout-1_generate.c
+++ b/gcc/testsuite/g++.dg/compat/struct-layout-1_generate.c
@@ -495,7 +495,16 @@ struct types attrib_array_types[] = {
 #define HASH_SIZE 32749
 static struct entry *hash_table[HASH_SIZE];
 
-static int idx, limidx, output_one, short_enums;
+/* The index of the current type being output.  */
+static int idx;
+
+/* The maximum index of the type(s) to output.  */
+static int limidx;
+
+/* Set to non-zero to output a single type in response to the -i option
+   (which sets LIMIDX to the index of the type to output.  */
+static int output_one;
+static int short_enums;
 static const char *destdir;
 static const char *srcdir;
 static const char *srcdir_safe;
@@ -535,6 +544,7 @@ switchfiles (int fields)
   fputs ("failed to create test files\n", stderr);
   exit (1);
 }
+
   for (i = 0; i < NDG_OPTIONS; i++)
 fprintf (outfile, dg_options[i], "", srcdir_safe);
   fprintf (outfile, "\n\
@@ -607,9 +617,14 @@ getra

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 05:54 PM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


On 10/05/2016 05:11 PM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


may have been subjected to.  Issuing a warning for a safe piece
of code only on the basis that there might be some other piece
of code in the program that does something wrong makes no sense.
Suggesting to the user to change the safe piece of code is
misleading and counterproductive.


It's warning because it's *bad code*.


Why?  What can go wrong and on what system?  Please name at least
one specific example.


It's bad because getting types wrong (more generally than just in this
case) is a careless coding practice.  It's a code smell.


There are lots of other "smells" that GCC doesn't warn about by
default even though it easily could, including -Wformat-signedness.


The problem is that extracting via an integer of the opposite signedness
is *not* defined unless the argument has a value representable in both
types - both as a matter of ISO C rules on variadic functions, and as a
practical matter of what various ABIs say about how sub-word arguments are
extended when passed as function arguments.


No, sign mismatch is not the problem we're discussing.  GCC doesn't
warn for %lc when wchar_t and wint_t differ in signedness except
with -Wformat-signedness.  The cause of the warning is that wchar_t
is a distinct type from wint_t such as long from int (irrespective
of their sign), even if the two have the same size, and the checker
doesn't distinguish the typedefs from the underlying types.

My position is that when int and long have the same size, although
warning for objects of those types is appropriate because their
sizes often do differ from one target to the next, the %lc warning
is unhelpful because, unlike the underlying types, wchar_t and
wint_t are specifically designed to be interoperable and the same
propensity for a difference in size doesn't apply to them.

I did say, however, that the warning might have some merit even
in this case if there also were a target to which the code could
be ported where wchar_t and wint_t had different sizes (i.e., as
a portability warning).  But for such a portability warning to
be useful it would need to be issued by all GCC compilers, not
just when compiling for the target with that property.  (But
since neither of us knows(*) of such a target this is a moot
point.)


But my basic point is: if something can lead to analysis in this level of
detail of whether the code is or is not safe in a particular context,
while there is a trivial fix to the code that would short-circuit all that
analysis, that by itself is enough evidence that the code deserves a
warning and should be cleaned up to make it more obviously safe.


The premise that "it's trivial to fix" is wrong and at the core
of my objection to the warning (by default; I would have no issue
with it being issued under a separate option or under -Wpedantic).

The text of the warning itself indicates that even suggesting how
to fix it isn't trivial.  GCC warns for just for the %lc directive
(not for the sign mismatch) and suggests to fix it by replacing
the %lc directive with %ld.  That's clearly wrong.  Printing
a hint with fix you're looking for (i.e., casting the argument
to wint_t) may be difficult because of the lack of location
information for local variables.

  $ cat a.c && gcc -S -Wall -m32 a.c
  typedef __WCHAR_TYPE__ wchar_t;

  void f (int x, unsigned y)
  {
extern wchar_t w;

__builtin_printf ("%lc", w);
__builtin_printf ("%lc", x);
__builtin_printf ("%lc", y);
  }
  a.c: In function ‘f’:
  a.c:7:24: warning: format ‘%lc’ expects argument of type ‘wint_t’, 
but argument 2 has type ‘wchar_t {aka long int}’ [-Wformat=]

 __builtin_printf ("%lc", w);
~~^
%ld

But I think we're both starting to repeat ourselves without making
any headway.  I spent more time on this discussion (and the research
I did for it) than I should have an I don't view this as serious
enough of a problem to be worth spending more time on, just a minor
wart in GCC.  There are plenty others with a much better ROI.

Martin

[*] Since you've persistently dodged the question about the target
where this might be a problem I surveyed the GCC config files to
see if I could find one.  All but two of the operating systems GCC
apparently knows about define wint_t to be an alias for signed int.
The two exceptions are Ming32 and VxWorks both of which define both
wint_t and wchar_t to be unsigned short, and so those two targets
can be ruled out.

Besides these, the are a bunch of back ends that define wchar_t
to be long where, if long were wider than int, calling
printf("%lc", L'x') would be a problem.  But based on a closer
review of some of these targets it looks like they define both
int and long as 32-bit types an

Re: [PATCH] define TARGET_PRINTF_POINTER_FORMAT for powerpc-linux (77837)

[Martin Sebor]

On 10/03/2016 08:11 PM, Martin Sebor wrote:

On 10/03/2016 07:10 PM, Segher Boessenkool wrote:

On Mon, Oct 03, 2016 at 05:30:35PM -0600, Martin Sebor wrote:

The attached patch adds definitions of TARGET_PRINTF_POINTER_FORMAT
to the rs6000 pair of linux.h and linux64.h headers, analogous to
the config/linux.h header.  This appears to be necessary since
unlike most other back ends, the rs6000 back end doesn't include
the latter linux.h.

The patch fixes bug 77837 - missing -Wformat-length warning for %p
with null argument on powerpc64.

Thanks
Martin



PR target/77837 - missing -Wformat-length warning for %p with null
argument on powerpc64

gcc/ChangeLog:
2016-10-03  Martin Sebor  <mse...@redhat.com>

PR target/77837
* config/rs6000/linux.h (TARGET_PRINTF_POINTER_FORMAT): Define.
* config/rs6000/linux64.h (TARGET_PRINTF_POINTER_FORMAT): Likewise.


Okay for trunk, thanks!


I was able to fix this in r240793 without changing the rs6000 back
end so this patch is no longer necessary.

Thanks
Martin

Re: [PATCH] backport dejagnu relative numbers to 6-branch?

[Martin Sebor]

On 10/04/2016 01:01 PM, Mike Stump wrote:

On Oct 4, 2016, at 11:05 AM, Martin Sebor <mse...@gmail.com> wrote:


While backporting a patch for 77804 to the gcc-6-branch I noticed
that the DejaGnu relative number patch below is not available
there (the new test failed).  Is it worth backporting it to it?

 https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01617.html


Backport Ok.



Thank you.  I saw no unexpected failures in a full bootstrap and
regression test run on pwerpc64le so I committed Jakub's change
in r240762.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

as it happens, I'd already started bootstraps with your patch before
your mail arrived :-)


Thanks for your help getting to the bottom of this!



We're left with

FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)

for 32 bit and

FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)

for 64 bit on both i386-pc-solaris2.12 and sparc-sun-solaris2.12.

In the 32-bit builtin-sprintf-warn-1.c case, there are many instances of

Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:224:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]


I've built the sparc-sun-solaris2.12 toolchain and reproduced these
warnings.  They are vestiges of those I saw and some of which I fixed
before.  The problem is that %lc expects a wint_t argument which on
this target is an alias for long in but the argument of 0 has type
int.  The warning is coming out of the -Wformat checker which doesn't
seem to care that int and long have the same size.  I've committed
r240758 that should fix the remaining warnings of this kind but long
term I think GCC should change to avoid warning in this case (Clang
doesn't).



while the second is

Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:15:23:
 warning: writing a terminating nul past the end of the destination 
[-Wformat-length=]/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:30:21:
 warning: writing format character '4' at offset 3 past the end of the 
destination [-Wformat-length=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:46:21:
 warning: writing format character '4' at offset 3 past the end of the 
destination [-Wformat-length=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:61:25:
 warning: writing a terminating nul past the end of the destination 
[-Wformat-length=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:74:22:
 warning: '%-s' directive writing 4 bytes into a region of size 1 
[-Wformat-length=]

I've no idea yet why in the first error message two different messages
are joined into one line.  Probably something with DejaGnu mangling the
output...


I've reproduced this as well and it took me a while to see the
problem.  It turns out that the target specifier I used in the
test (*-*-*-*) happened to match my native target
x86_64-pc-linux-gnu but not sparc-sun-solaris2.12.  Let me fix
that in the next patch.  Hopefully with that all the remaining
failures should clear up.

Thanks again for your help and patience!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/04/2016 06:21 PM, Joseph Myers wrote:

On Tue, 4 Oct 2016, Martin Sebor wrote:


I've built the sparc-sun-solaris2.12 toolchain and reproduced these
warnings.  They are vestiges of those I saw and some of which I fixed
before.  The problem is that %lc expects a wint_t argument which on
this target is an alias for long in but the argument of 0 has type
int.  The warning is coming out of the -Wformat checker which doesn't
seem to care that int and long have the same size.  I've committed
r240758 that should fix the remaining warnings of this kind but long
term I think GCC should change to avoid warning in this case (Clang
doesn't).


Well, typically cases where one of long and int is passed and the other is
expected, but they have the same size, are bugs waiting to happen when the
code is built on a 64-bit system.  That is, they *should* warn.


Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
think it's helpful.  Consider this case from my comment #5 on bug
72858.  I don't think there is any point in issuing a warning here.
On the majority of targets they either all are or promote to a type
of the same size, don't they?

$ cat t.c && gcc -S -Wall -m32 t.c
typedef __WCHAR_TYPE__ wchar_t;

void f (const wchar_t *ws)
{
  __builtin_printf ("%lc", *ws);
}
t.c: In function ‘f’:
t.c:5:24: warning: format ‘%lc’ expects argument of type ‘wint_t’, but 
argument 2 has type ‘wchar_t {aka const long int}’ [-Wformat=]

   __builtin_printf ("%lc", *ws);
  ~~^   ~~~
  %ld


There have been arguments that we should go further and warn for e.g. %zu
with a type that happens to be the same as size_t but doesn't use the
size_t typedef (or sizeof etc.), %td for something that happens to be the
same as ptrdiff_t but doesn't use the typedef (or pointer difference
etc.), etc. - which would get many similar cases of bugs waiting to happen
on a different system, but is also tricker because you need to decide
whether a given type is logically size_t etc. or not - code could validly
use autoconf to identify the underlying type, or use __SIZE_TYPE__, or use
an expression mixing size_t with other types, or in the case of %j*
(intmax_t) use the INTMAX_C macro to construct constants.  That probably
*would* need an option to disable just those format warnings (whereas I
don't see the need for such an option for the case of mixing int and
long).


I would view it useful if GCC warned on %zu with an argument that's
not size_t, and similarly for other directives and typedefs, for the
reason you mention.  But I don't think the same rationale applies
to warning on %lc with wchar_t or int arguments.

Martin

Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Martin Sebor]

On 09/16/2016 12:19 PM, Jason Merrill wrote:

On 09/14/2016 01:03 PM, Martin Sebor wrote:

Attached is an updated patch that does away with the "overlapping"
warning and instead issues the same warnings as the C front end
(though with more context).

In struct flexmems_t I've retained the NEXT array even though only
the first element is used to diagnose problems.  The second element
is used by the find_flexarrays function to differentiate structs
with flexible array members in unions (which are valid) from those
in structs (which are not).

FWIW, I think this C restriction on structs is unnecessary and will
propose to remove it so that's valid to define a struct that contains
another struct (possibly recursively) with a flexible array member.
I.e., I think this should be made valid in C (and accepted without
the pedantic warning that GCC, and with this patch also G++, issues):


Agreed.


This is now N2083:
  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2083.htm




+  /* Is FLD a typedef for an anonymous struct?  */
+  bool anon_type_p
+   = (TREE_CODE (fld) == TYPE_DECL
+  && DECL_IMPLICIT_TYPEDEF_P (fld)
+  && anon_aggrname_p (DECL_NAME (fld)));


We talked earlier about handling typedefs in
cp_parser_{simple,single}_declaration, so that we catch

typedef struct { int a[]; } B;

or at least adding a FIXME comment here explaining that looking at
typedefs is a temporary hack.


I've added a FIXME comment.




+  /* Type of the member.  */
+  tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld)
: fld;


Why set "fldtype" to be a TYPE_DECL rather than its type?


I'm not sure I understand the question but (IIRC) the purpose of
this code is to detect invalid uses of flexible array members in
typedefs such as this:

   struct S { typedef struct { int i, a[]; } X2 [2]; };

Sadly, it doesn't do anything for

   struct X { int i, a[]; };
   struct S { typedef struct X X2 [2]; };


+  /* Determine the type of the array element or object referenced
+by the member so that it can be checked for flexible array
+members if it hasn't been yet.  */
+  tree eltype = TREE_CODE (fld) == FIELD_DECL ? fldtype :
TREE_TYPE (fld);


Given the above, this seems equivalent to

tree eltype = TREE_TYPE (fld);


Yes.




+  if (RECORD_OR_UNION_TYPE_P (eltype))
+   {

...

+ if (fmem->array && !fmem->after[bool (pun)])
+   {
+ /* Once the member after the flexible array has been found
+we're done.  */
+ fmem->after[bool (pun)] = fld;
+ break;
+   }

...

  if (field_nonempty_p (fld))
{

...

  /* Remember the first non-static data member after the flexible
 array member, if one has been found, or the zero-length
array
 if it has been found.  */
  if (fmem->array && !fmem->after[bool (pun)])
fmem->after[bool (pun)] = fld;
}


Can we do this in one place rather than two?


You mean merge the two assignments to fmem->after[bool (pun)] into
one?  I'm not sure.  There's quite a bit of conditional logic in
between them, including a recursive call that might set fmem.  What
would be gained by rewriting it all to merge the two assignments?
If it could be done I worry that I'd just end up duplicating the
conditions instead.




+ if (eltype == fldtype || TYPE_UNNAMED_P (eltype))


This is excluding arrays, so we don't diagnose, say,


struct A
{
  int i;
  int ar[];
};

struct B {
  A a[2];
};


Should we complain elsewhere about an array of a class with a flexible
array member?


Yes, there are outstanding gaps/bugs that remain to be fixed.  I tried
to limit this fix to not much more than the reported regression.  I did
end up tightening other things up as I found more gaps during testing
(though I probably should have resisted).  Sometimes I find it hard to
know where to stop but I feel like I need to draw the line somewhere.
I of course agree that the outstanding bugs should be fixed as well
but I'd prefer to do it in a separate change.


+ /* Does the field represent an anonymous struct?  */
+ bool anon_p = !DECL_NAME (fld) && ANON_AGGR_TYPE_P
(eltype);


You don't need to check DECL_NAME; ANON_AGGR_TYPE_P by itself indicates
that we're dealing with an anonymous struct/union.


Thanks, I've removed the variable.




   Similarly, PSTR is set to the outermost struct of which T is a member
   if one such struct exists, otherwise to NULL.  */

...

  find_flexarrays (eltype, fmem, false, pun,
   !pstr && TREE_CODE (t) == RECORD_TYPE ?
fld : pstr);

...

+diagnose_invalid_flexarray (const flexmems_t *fmem)
+{
+  if (fmem->array && fmem->enclosing
+  && pedwarn (location_of (fmem->enclosing), OPT_Wpedantic,
+   

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:

On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:

On 09/08/2016 04:10 PM, Joseph Myers wrote:

On Thu, 8 Sep 2016, Martin Sebor wrote:



PR middle-end/49905 - Better sanity checking on sprintf src & dest to
produce warning for dodgy code


The patch uses "nul" instead of "null" throughout.


Yes, that's intentional.  NUL and null are alternate spellings for
the same character. I went with nul to distinguish it from the null
pointer and used all lowercase as per the GCC convention.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 04:36 AM, Christophe Lyon wrote:

Hi Martin,

On 21 September 2016 at 09:11, Markus Trippelsdorf
<mar...@trippelsdorf.de> wrote:

On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:

On 09/08/2016 04:10 PM, Joseph Myers wrote:

On Thu, 8 Sep 2016, Martin Sebor wrote:



PR middle-end/49905 - Better sanity checking on sprintf src & dest to
  produce warning for dodgy code


The patch uses "nul" instead of "null" throughout.

--
Markus


I've noticed that some of the new tests fail on arm linux:
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1220)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1270)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1381)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 815)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 816)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 817)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 820)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 821)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 824)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 825)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 828)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 829)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 832)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 833)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c  (test for warnings, line 46)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c  (test for warnings, line 47)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf.c (test for excess errors)

on aarch64 and arm bare-metal targets (using newlib), I've noticed
these failures:
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into
strcpy (test for warnings, line 83)
  gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into
strcpy (test for warnings, line 84)
  gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf.c execution test

You may download the results from
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/240298/report-build-info.html


Thanks, I'll look into these today.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 08:37 AM, Alexander Monakov wrote:

On Wed, 21 Sep 2016, Martin Sebor wrote:

On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:

The patch uses "nul" instead of "null" throughout.


Yes, that's intentional.  NUL and null are alternate spellings for
the same character. I went with nul to distinguish it from the null
pointer and used all lowercase as per the GCC convention.


Can you elaborate which guideline suggests spelling that in lowercase?
It seems quite strange to me, especially given that the documentation
added with the patch uses "NUL character" (which I believe to be a more
common form), but then warnings use "nul" (without the "character" iiuc).


I don't know if there is a specific guideline that applies here,
but it's my impression that conventionally GCC messages use
lowercase letters.  The only guideline I know about is in the GNU
Coding Standard that calls for messages not to begin with capital
letters (or end in a period;  though now that I've checked more
carefully there are a fair number of messages that ignore that
suggestion).

That said, if the lowercase nul is offensive it can certainly be
changed to some other spelling (as can any of the other messages
emitted by the pass).

Martin

PS Yes, the C and C++ standards refer to NUL as the null character.
NUL is an abbreviation, used extensively (for example) by POSIX.

Re: Add FALLTHRU to gimple-ssa-sprintf.c

[Martin Sebor]

On 09/21/2016 05:07 AM, Marek Polacek wrote:

On Wed, Sep 21, 2016 at 12:57:23PM +0200, Jakub Jelinek wrote:

On Wed, Sep 21, 2016 at 12:52:33PM +0200, Marek Polacek wrote:

Pointed out by Tobias.  This looks like a missing fallthru marker.

Ok?

2016-09-21  Marek Polacek  

* gimple-ssa-sprintf.c (pass_sprintf_length::compute_format_length):
Add falls through comment.


This is obvious.


I wanted to give Martin a chance to comment, maybe it should've been
a break, but I'll commit this now.  Thanks.


Yes, falling through is intentional here and should have probably
been better documented.  Thanks for taking care of it!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 01:40 PM, Gerald Pfeifer wrote:

I noticed the following bootstrap failure on i?86-unknown-freebsd
that started in the last 24 hours:

/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c: In member function ‘void 
vec_usage::dump(mem_location*, mem_usage&) const’:
/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:79:3: error: ‘%s’ directive writing 
between 0 and 4294967295 bytes into a region of size 4096 
[-Werror=format-length=]
   dump (mem_location *loc, mem_usage ) const
   ^~~~
/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:83:36: note: format output between 6 
and4294967311 bytes into a destination of size 4096
   loc->m_line, loc->m_function);
^
cc1plus: all warnings being treated as errors
gmake[3]: *** [Makefile:2557: build/vec.o] Error 1
gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705/gcc'
gmake[2]: *** [Makefile:4612: all-stage2-gcc] Error 2
gmake[2]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705'
gmake[1]: *** [Makefile:24365: stage2-bubble] Error 2

Is it possible that is related to your warning patches?


Yes, this is likely the same bug as mentioned in comment #6 on
pr77676.  The bug in the comment ILP32-specific and only tangentially
related to the PR itself.  I'm testing the patch that's attached to
the PR that should fix both of these problems.  I don't have access
to i?86-unknown-freebsd so if you could help validate it there I'd
be grateful.  (The patch just successfully bootstrapped on
i386-pc-linux-gnu.)

Thanks and sorry for the breakage.
Martin

[PATCH] fix ICE on %lf directive in format_floating in gimple-ssa-sprintf.c (middle-end/77683)

[Martin Sebor]

The attached patch corrects the handling of the 'l' length modifier
for floating point directives (such as %lf) where it's specified to
have no effect.  It also replaces the gcc_unreachable() macro with
a return statement telling the pass to give up on unknown length
modifiers (rather than abort).

Martin
PR middle-end/77683 - ICE on %lf directive in format_floating in
  gimple-ssa-sprintf.c:1163

gcc/testsuite/ChangeLog:
2016-09-21  Martin Sebor  <mse...@redhat.com>

	PR middle-end/77683
	* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Add test cases.

gcc/ChangeLog:
2016-09-21  Martin Sebor  <mse...@redhat.com>

	PR middle-end/77683
	* gimple-ssa-sprintf.c (format_integer): Fail gracefully when
	length modifier is not expected.
	(format_floating): Ignore l length modifier and fail gracefuly
	when it isn't one of the other expected ones.


diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 2e4a89f..34595a5 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -867,7 +867,14 @@ format_integer (const conversion_spec , tree arg)
   break;
 
 default:
-  gcc_unreachable ();
+  {
+	fmtresult res = fmtresult ();
+	res.range.min = HOST_WIDE_INT_MAX;
+	res.range.max = HOST_WIDE_INT_MAX;
+	res.bounded = false;
+	res.constant = false;
+	return res;
+  }
 }
 
   /* The type of the argument to the directive, either deduced from
@@ -1145,6 +1152,7 @@ format_floating (const conversion_spec , int width, int prec)
 
   switch (spec.modifier)
 {
+case FMT_LEN_l:
 case FMT_LEN_none:
   type = double_type_node;
   break;
@@ -1160,7 +1168,14 @@ format_floating (const conversion_spec , int width, int prec)
   break;
 
 default:
-  gcc_unreachable ();
+  {
+	fmtresult res = fmtresult ();
+	res.range.min = HOST_WIDE_INT_MAX;
+	res.range.max = HOST_WIDE_INT_MAX;
+	res.bounded = false;
+	res.constant = false;
+	return res;
+  }
 }
 
   /* The minimum and maximum number of bytes produced by the directive.  */
@@ -1246,7 +1261,14 @@ format_floating (const conversion_spec , int width, int prec)
   }
 
 default:
-  gcc_unreachable ();
+  {
+	fmtresult res = fmtresult ();
+	res.range.min = HOST_WIDE_INT_MAX;
+	res.range.max = HOST_WIDE_INT_MAX;
+	res.bounded = false;
+	res.constant = false;
+	return res;
+  }
 }
 
   if (0 < width)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
index 7261dbd..214dbdc 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
@@ -863,6 +863,8 @@ void test_sprintf_chk_z_const (void)
 void test_sprintf_chk_e_const (void)
 {
   T (-1, "%E",   0.0);
+  T (-1, "%lE",  0.0);
+
   T ( 0, "%E",   0.0);  /* { dg-warning "into a region" } */
   T ( 0, "%e",   0.0);  /* { dg-warning "into a region" } */
   T ( 1, "%E",   1.0);  /* { dg-warning "into a region" } */
@@ -1073,6 +1075,8 @@ void test_sprintf_chk_int_nonconst (int a)
 void test_sprintf_chk_e_nonconst (double d)
 {
   T (-1, "%E",  d);
+  T (-1, "%lE", d);
+
   T ( 0, "%E",  d);   /* { dg-warning "writing between 12 and 14 bytes into a region of size 0" } */
   T ( 0, "%e",  d);   /* { dg-warning "into a region" } */
   T ( 1, "%E",  d);   /* { dg-warning "into a region" } */
@@ -1104,6 +1108,8 @@ void test_sprintf_chk_e_nonconst (double d)
 void test_sprintf_chk_f_nonconst (double d)
 {
   T (-1, "%F",  d);
+  T (-1, "%lF", d);
+
   T ( 0, "%F",  d);   /* { dg-warning "into a region" } */
   T ( 0, "%f",  d);   /* { dg-warning "into a region" } */
   T ( 1, "%F",  d);   /* { dg-warning "into a region" } */

Re: [PATCH] have __builtin_object_size handle POINTER_PLUS with non-const offset

[Martin Sebor]

On 09/16/2016 04:29 AM, Richard Biener wrote:

On Fri, Sep 16, 2016 at 5:29 AM, Martin Sebor <mse...@gmail.com> wrote:

__builtin_object_size fails for POINTER_PLUS expressions involving
non-constant offsets into objects of known size, causing GCC to fail
to detect (and add instrumentation to prevent) buffer overflow in
some basic cases such as the following:

  void f (unsigned i)
  {
char d [3];
memcpy (d + i, "abcdef", 5);
  }

Since the size of the destination object is known, the call to memcpy
is guaranteed to write past the end of it regardless of the value of
the offset.

The attached patch enhances __builtin_object_size to handle this case
by returning the size of the whole object as the maximum and the size
of the object minus T_MAX for the type of the offset T as the minimum.

The patch also adds handling of ranges even though only very few cases
benefit from it because the VRP pass runs after the object size pass.
The one case that does appear to profit is when the value of the offset
is constrained by its type, as in

  char a [1000];

  unsigned g (unsigned char i)
  {
char *p = a + i;
return __builtin_object_size (p, 2);
  }

Here get_range_info () lets __builtin_object_size determine that
the minimum number of bytes between (a + i) and (a + sizeof s) is
sizeof a - UCHAR_MAX.

The patch results in 64 more checking calls in a Binutils build than
before.


I believe that you can't simply use the min/max of the ranges the way you
do nor can you assume zero for the minimum size as op1 might be
negative (for POINTER_PLUS_EXPR you need to interpret the offset
as signed).

Unless I completely misremember how tree-object-size.c works, of course.

Say,

char a[1000];

unsigned g (signed char i)
{
  char *p = [10] + i;
  return __builtin_object_size (p, 1);
}

range for i will be [-128, 127] but we'd like to return 1000 here I think.


Thanks for the example.  I agree that 1000 is the correct result
in type < 2.  Curiously, the range reported for this case is
actually the anti-range ~[128, -129], and because the patch
didn't handle those the result was 990 (i.e., the same as
__builtin_object_size([10], type) for type < 2).

I think the way to handle cases like this in (type < 2) might be
by computing the size of the whole object first (i.e., ignoring
the array subscript) and then adjusting the result down according
to the bounds of the range.  Let me work on that.

Thanks
Martin

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

and used
pp_format for formatting arg_positions by adding specifier %I (name
chosen arbitrarily).
Does that look OK ?


diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index a39815e..e8bd1ef 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -610,6 +610,23 @@ pp_format (pretty_printer *pp, text_info *text)
  (pp, *text->args_ptr, precision, unsigned, "u");
  break;

+   case 'I':

The comment just above pp_format that lists the format specifiers
understood by the function should be updated.  There probably (I
hope) is more formal documentation of the format specifiers
somewhere else that should be updated as well.

That said, since this specifier formats a vec, it seems that
it might be useful to be able to format vectors of other elements,
such as short and long.  With that in mind, would adding a new V
length modifier instead be a more regular way to extend the pretty
printer to these types?  The V length modifier would have to be
accepted in conjunction with other length modifiers (h, l, etc
and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept
a vec*  as an argument and format it as a sequence
of decimal numbers, and "%lVx" would do the same for vec*
formatting them in hex.

Martin

Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Martin Sebor]

On 09/21/2016 02:43 PM, Jason Merrill wrote:

On Tue, Sep 20, 2016 at 1:01 PM, Martin Sebor <mse...@gmail.com> wrote:

On 09/16/2016 12:19 PM, Jason Merrill wrote:

On 09/14/2016 01:03 PM, Martin Sebor wrote:


+  /* Type of the member.  */
+  tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld)
: fld;


Why set "fldtype" to be a TYPE_DECL rather than its type?


I'm not sure I understand the question but (IIRC) the purpose of
this code is to detect invalid uses of flexible array members in
typedefs such as this:

   struct S { typedef struct { int i, a[]; } X2 [2]; };

Sadly, it doesn't do anything for

   struct X { int i, a[]; };
   struct S { typedef struct X X2 [2]; };


The issue is I don't see anything that uses fldtype as a decl, and
it's strange to have a variable called "*type" that can either be a
type or a decl, which later code still assumes will be a type.


I suspect I'm simply looking at it from a different point of view.
The definition

  tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld

denotes the type of the field if fld is a data member.  Otherwise,
it denotes a type (like a typedef).  Fldtype seems as a good a name
as any but I'll gladly rename it to something else if that lets us
move forward.  What would you prefer?


+  /* The context of the flexible array member.  Either the struct
+ in which it's declared or, for anonymous structs and unions,
+ the struct/union of which the array is effectively a member.  */
+  tree fmemctx = anon_context (fmem->array);
+  bool anon_p = fmemctx != DECL_CONTEXT (fmem->array);
+  if (!anon_p)
+fmemctx = t;


Why do you need to do something different here based on anon_p?  I don't
see why that would affect whether we want to look through intermediate
non-anonymous classes.


In code like this:

   struct X { int n, a[]; };
   struct Y { int n, b[]; };

   struct D: X, Y { };

The test above make the diagnostic point to the context of the invalid
flexible array member, or Y::b, rather than that of X. Without it, we
end up with:

   error: flexible array member ‘X::a’ not at end of ‘struct X’

rather than with

   error: flexible array member ‘X::a’ not at end of ‘struct D’


Yes, but why does whether we want to talk about X or D change if a is
wrapped in struct { }?


Sorry, I don't understand the question.  A flexible array is always
"wrapped in struct { }" so I'm not sure what you mean by that.  And
"we want to talk about D" because that's where the next member after
a is defined (we could also say it's defined in struct Y and I think
I may have been down that path at one point and decided this was fine
or perhaps even better or simpler but I don't recall which for sure.)

Btw., I used quotes above only to explain how I read your question,
not to mock what you said in any way.

Going back and looking at some of the older patches, this hunk above
was changed from the original patch like so:

@@ -6923,7 +6930,10 @@ diagnose_flexarrays (tree t, const flexmems_t *fmem)
 /* The context of the flexible array member.  Either the struct
in which it's declared or, for anonymous structs and unions,
the struct/union of which the array is effectively a member.  */
-tree fmemctx = fmem->anonctx ? fmem->anonctx : t;
+tree fmemctx = anon_context (fmem->array);
+bool anon_p = fmemctx != DECL_CONTEXT (fmem->array);
+if (!anon_p)
+  fmemctx = t;

I made this change because you preferred deriving the fmemctx value
in a new function rather than storing it the fmem->anonctx member.
I don't know if this helps clarify it.

  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00153.html

(FWIW, it's been a stressful couple of days with the regressions
that my recent commit caused, so I hope you can bear with me if
I seem slow or dense on these two points.)




+  check_flexarrays (basetype, fmem, true);


Please decorate boolean literal arguments like this with the name of the
parameter, e.g. /*base_p*/true.


Sure.  I should mention that Jeff recently advised against it in
another review, so it would be nice to adopt the same convention
throughout and document it (I'm happy to add it to the Wiki once
it has been agreed on):

   https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00354.html


Interesting.  It's pretty common in the C++ front end.


FWIW, I haven't found mentioning the formal argument in a comment
a useful or consistent convention.  Other arguments' names aren't
mentioned, and the bool argument's name cannot be mentioned when
it has a default value.


True, but other arguments usually have more implied meaning and so I
think they are less of a barrier to reading.


On the other hand, a convention I do find useful (though not one
that seems to be used in GCC) is indicating in a comment in the
definition of a function the value of the default argument in
functions declared to take one.


I agree that wou

Re: [PATCH, GCC] Deal with singular sentences in builtin-sprintf-warn-1.c regex

[Martin Sebor]

On 09/23/2016 09:42 AM, Thomas Preudhomme wrote:

Sorry, forgot the patch. Please find it attached.

Best regards,

Thomas

On 23/09/16 16:40, Thomas Preudhomme wrote:

Hi,

New builtin-sprintf-warn-1.c testcase contains a few regex of the form
"\[0-9\]+
bytes" or ". bytes". This does not account for the case where the
number of byte
is 0 in which case byte would be in the singular form. This caused a
FAIL on
arm-none-eabi targets. This patch makes the s optional in all cases
where the
number of bytes is unknown.

I did not commit this fix as obvious as people might want to only do
the changes
for lines where the number of bytes *could* be 0 so I prefer to get
review.


Thanks for the patch.  The %p fixes look correct to me (someone
else needs to approve the final patch).

For the INT_MAX tests, I think it's a sign of a bug in the pass if
for the following call

  sprintf (buf, "X%*c", INT_MAX, '1');

GCC prints

  warning: directive output of 0 byte causes result to exceed 'INT_MAX'

My guess is that the bug is specific to a 32-bit compiler (I can't
reproduce it in a 64-bit one with -m32).  Let me build one and look
into fixing it.

Martin



ChangeLog entry is as follows:

*** gcc/testsuite/ChangeLog ***

2016-09-23  Thomas Preud'homme  

* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust regex to
accept
singular form of byte when quantity is unknown.


Is this ok for trunk?

Best regards,

Thomas

Re: [PATCH] Fix various minor gimple-ssa-sprintf.c issues

[Martin Sebor]

On 09/22/2016 01:46 AM, Marek Polacek wrote:

On Thu, Sep 22, 2016 at 09:24:11AM +0200, Jakub Jelinek wrote:

On Wed, Sep 21, 2016 at 06:38:54PM -0600, Martin Sebor wrote:

On 09/21/2016 09:09 AM, Jakub Jelinek wrote:

When looking at PR77676, I've noticed various small formatting etc.
issues, like not using is_gimple_* APIs where we have them, not using
gimple_call_builtin_p/gimple_call_fndecl (this one actually can show up,
if e.g. uses the builtin with incorrect arguments (fewer, different
types etc.)), one pasto, 2 spaces in comments instead of 1 in the middle
of sentences.  And, lastly 0 < var is very unusual ordering of the
comparison operands, while we have a couple of such cases in the sources,
usually it is when using 0 < var && var <= someotherconst, while
var > 0 is used hundred times more often.


Thanks for correcting the uses of the gimple APIs!  I appreciate
your fixing the various typos as well, but I see no value in
changing the order of operands in inequality expressions or in
moving code around for no apparent reason.  However, I won't


The moving of code around is in just one spot, and it has a reason -
consistency.  After the move, each non-_chk builtin is followed by its _chk
counterpart, before that the order has been random.
Another possible ordering that makes sense is putting all the non-_chk
builtins first and then in the same order all their _chk counterparts.

The reason why I wrote the patch has been that when skimming the code I've
noticed the missing is_* calls, then when looking for that issue discovered
something different etc.  The var > 0 vs. 0 < var is just something that
caught my eye when looking around, I don't feel too strongly about it, it
just looked weird and unexpected.  There are > 50 optimize > 0 preexisting
checks elsewhere, and even far more just optimize, but none 0 < optimize.


I find those 0 < var confusing and hard to read.  While I know that some
people prefer 0 == var (0 is not an lvalue so it catches mistakes like
var = 0 instead of var == 0), I don't see why 0 < optimized would ever be
preferred.


I don't have a preference for one or the other.  It's just how
I wrote the code.  Over the years of working on the C++ standard
library I trained myself to use the less than expression in
preference to any of the others because C++ algorithms had to
(or we wanted them to, I don't remember which anymore) work with 
user-defined types that only defined that one relational operator

and not any of the other forms.

Martin

Re: [PATCH] fix ILP32 bootstrap failures due to -Wformat-length

[Martin Sebor]

On 09/21/2016 05:09 PM, Jakub Jelinek wrote:

On Wed, Sep 21, 2016 at 04:39:42PM -0600, Martin Sebor wrote:

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index dddb026..652d3fb 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -210,8 +210,8 @@ struct format_result
 static HOST_WIDE_INT
 target_int_min ()
 {
-  static const unsigned HOST_WIDE_INT int_min
-= 1LLU << (sizeof int_min * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_min
+= 1LLU << (HOST_BITS_PER_WIDE_INT


1LLU should be really HOST_WIDE_INT_1U


   - TYPE_PRECISION (integer_type_node) + 1);


Is the shift amount really what you mean?
HOST_BITS_PER_WIDE_INT - TYPE_PRECISION (integer_type_node) + 1
is usually 33 (or 17 or 9 in much rarer cases), so that is
0x2ULL.  Don't you want instead
  = HOST_WIDE_INT_1U << (TYPE_PRECISION (ingeger_type_node) - 1);
so that it will be
0x8000ULL?


Great catch, thank you!  That must have been a copy and paste mistake.




@@ -221,8 +221,8 @@ target_int_min ()
 static unsigned HOST_WIDE_INT
 target_int_max ()
 {
-  static const unsigned HOST_WIDE_INT int_max
-= HOST_WIDE_INT_M1U >> (sizeof int_max * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_max
+= HOST_WIDE_INT_M1U >> (HOST_BITS_PER_WIDE_INT
- TYPE_PRECISION (integer_type_node) + 1);
   return int_max;
 }


This is expectedly -1ULL >> 33, i.e. 0x7fffULL, which looks ok.


Thanks for double checking that.  There is a test case for this
in the test suite for the optimization but there wasn't one for
the INT_MIN case.  I've added it in the attached patch.  I'll
beef up the return range testing some more before re-enabling
the optimization.

Martin
PR target/77676 - powerpc64 and powerpc64le stage2 bootstrap fail

gcc/testsuite/ChangeLog:
2016-09-21  Martin Sebor  <mse...@redhat.com>

	PR target/77676
	* gcc.dg/tree-ssa/builtin-sprintf-2.c: Fix typo.
	* gcc.dg/tree-ssa/builtin-sprintf-3.c: New test.
	* gcc.dg/tree-ssa/builtin-sprintf-warn-5.c: New test.

gcc/ChangeLog:
2016-09-21  Martin Sebor  <mse...@redhat.com>

	PR target/77676
	* gimple-ssa-sprintf.c (target_int_min, target_int_max): Use
	HOST_BITS_PER_WIDE_INT, make a static local variable auto.
	(target_int_min): Correct computation.
	(format_integer): Use long long as the argument for the ll length
	modifier.
	(format_floating): Use target_int_max().
	(get_string_length): Same.
	(format_string): Avoid setting the bounded flag for strings
	of unknown length.
	(try_substitute_return_value): Avoid setting range info when
	the result isn't bounded.
	* varasm.c (assemble_name): Increase buffer size.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index dddb026..d1d20ca 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -210,9 +210,9 @@ struct format_result
 static HOST_WIDE_INT
 target_int_min ()
 {
-  static const unsigned HOST_WIDE_INT int_min
-= 1LLU << (sizeof int_min * CHAR_BIT
-	   - TYPE_PRECISION (integer_type_node) + 1);
+  const unsigned HOST_WIDE_INT int_min
+= HOST_WIDE_INT_M1U << (TYPE_PRECISION (integer_type_node) - 1);
+
   return int_min;
 }
 
@@ -221,8 +221,8 @@ target_int_min ()
 static unsigned HOST_WIDE_INT
 target_int_max ()
 {
-  static const unsigned HOST_WIDE_INT int_max
-= HOST_WIDE_INT_M1U >> (sizeof int_max * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_max
+= HOST_WIDE_INT_M1U >> (HOST_BITS_PER_WIDE_INT
 			- TYPE_PRECISION (integer_type_node) + 1);
   return int_max;
 }
@@ -851,7 +851,9 @@ format_integer (const conversion_spec , tree arg)
 
 case FMT_LEN_L:
 case FMT_LEN_ll:
-  dirtype = sign ? long_integer_type_node : long_unsigned_type_node;
+  dirtype = (sign
+		 ? long_long_integer_type_node
+		 : long_long_unsigned_type_node);
   break;
 
 case FMT_LEN_z:
@@ -1366,7 +1368,7 @@ format_floating (const conversion_spec , tree arg)
 	  *minmax[i] = mpfr_snprintf (NULL, 0, fmtstr, mpfrval);
 	}
 
-  res.bounded = res.range.min < HOST_WIDE_INT_MAX;
+  res.bounded = res.range.min < target_int_max ();
   return res;
 }
 
@@ -1420,7 +1422,7 @@ get_string_length (tree str)
   /* Set RES.BOUNDED to true if and only if all strings referenced
 	 by STR are known to be bounded (though not necessarily by their
 	 actual length but perhaps by their maximum possible length).  */
-  res.bounded = res.range.max < HOST_WIDE_INT_MAX;
+  res.bounded = res.range.max < target_int_max ();
 
   /* Set RES.CONSTANT to false even though that may be overly
 	 conservative in rare cases like: 'x ? a : b' where a and
@@ -1471,6 +1473,10 @@ format_string (const conversion_spec , tree arg)
: 2 == warn_format_length ? 0 <= prec ? prec : 1
: HOST_WIDE_INT_MAX);
 
+  /* The result is bounded unless overriddden for a non-constant string
+ of an unknown length.  */
+  bool bound

Re: [PATCH] Fix various minor gimple-ssa-sprintf.c issues

[Martin Sebor]

On 09/21/2016 09:09 AM, Jakub Jelinek wrote:

Hi!

When looking at PR77676, I've noticed various small formatting etc.
issues, like not using is_gimple_* APIs where we have them, not using
gimple_call_builtin_p/gimple_call_fndecl (this one actually can show up,
if e.g. uses the builtin with incorrect arguments (fewer, different
types etc.)), one pasto, 2 spaces in comments instead of 1 in the middle
of sentences.  And, lastly 0 < var is very unusual ordering of the
comparison operands, while we have a couple of such cases in the sources,
usually it is when using 0 < var && var <= someotherconst, while
var > 0 is used hundred times more often.


Thanks for correcting the uses of the gimple APIs!  I appreciate
your fixing the various typos as well, but I see no value in
changing the order of operands in inequality expressions or in
moving code around for no apparent reason.  However, I won't
object if you feel strongly about committing those changes(*).

What I would be even more grateful for is a review of the error
prone parts like those that caused the bootstrap failure.  I.e.,
any lingering assumptions about integer sizes between the host
and the target, and the code that controls the return value
range optimization.  (Similar to what you just did for the
PR77676 patch -- that was very helpful, thanks again.)

I'll look into the outstanding issues you noted below next.

Thanks
Martin

PS [*] I think it's unproductive to dwell on these details when
there are much bigger problems to worry about.  IME, insisting
on perfect formatting or on a particular order of operands only
takes time and energy away from focusing on the parts that could
actually cause serious issues.



I see various other issues, which I'll happily defer to Martin, e.g.
  /* These are available as macros in the C and C++ front ends but,
 sadly, not here.  */
  static tree intmax_type_node;
  static tree uintmax_type_node;

  /* Initialize the intmax nodes above the first time through here.  */
  if (!intmax_type_node)
build_intmax_type_nodes (_type_node, _type_node);
This is a big no no, the trees aren't registered with GC this way, so
they could be garbage collected (you'd need to move those to file-scope to
be able to GC register them).
Or
/* Compute the maximum just once.  */
static const int a_max[] = {
  format_floating_max (double_type_node, 'a'),
  format_floating_max (long_double_type_node, 'a')
};
etc. (used several time), do we really need to mess up with the
(thread-safe) local static locking/guards etc.?  Can't you initialize
the cache say to -1 and compute first if you need it and it is still -1?
PR77676 shows various other issues in the pass.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-09-21  Jakub Jelinek  

* gimple-ssa-sprintf.c: Fix comment formatting.
(pass_sprintf_length::gate): Use x > 0 instead of 0 < x.
(format_integer): Use is_gimple_assign.
(format_floating, format_string, format_directive,
get_destination_size): Use x > 0 instead of 0 < x.
(pass_sprintf_length::handle_gimple_call): Likewise.  Use
gimple_call_builtin_p and gimple_call_fndecl.  Reorder
case BUILT_IN_SPRINTF_CHK.  Fix up BUILT_IN_SNPRINTF_CHK comment.
Replace "to to" with "to" in comment.
(pass_sprintf_length::execute): Use is_gimple_call.

--- gcc/gimple-ssa-sprintf.c.jj 2016-09-21 08:54:15.0 +0200
+++ gcc/gimple-ssa-sprintf.c2016-09-21 15:09:02.209261013 +0200
@@ -37,7 +37,7 @@ along with GCC; see the file COPYING3.

The pass handles all forms standard sprintf format directives,
including character, integer, floating point, pointer, and strings,
-   with  the standard C flags, widths, and precisions.  For integers
+   with the standard C flags, widths, and precisions.  For integers
and strings it computes the length of output itself.  For floating
point it uses MPFR to fornmat known constants with up and down
rounding and uses the resulting range of output lengths.  For
@@ -130,8 +130,8 @@ pass_sprintf_length::gate (function *)
  not optimizing and the pass is being invoked early, or when
  optimizing and the pass is being invoked during optimization
  (i.e., "late").  */
-  return ((0 < warn_format_length || flag_printf_return_value)
- && (0 < optimize) == fold_return_value);
+  return ((warn_format_length > 0 || flag_printf_return_value)
+ && (optimize > 0) == fold_return_value);
 }

 /* The result of a call to a formatted function.  */
@@ -464,7 +464,7 @@ struct conversion_spec

   /* Format conversion function that given a conversion specification
  and an argument returns the formatting result.  */
-  fmtresult  (*fmtfunc) (const conversion_spec &, tree);
+  fmtresult (*fmtfunc) (const conversion_spec &, tree);

   /* Return True when a the format flag CHR has been used.  */
   bool get_flag (char chr) const
@@ 

[PATCH] fix ILP32 bootstrap failures due to -Wformat-length

[Martin Sebor]

As mentioned in PR77676. the gimple-ssa-sprintf.c pass committed
yesterday caused a number of bootstrap failures, including on ILP32
targets.  All of them were due to differences between integer sizes
between the host and the target.  To unblock the powerpc64 bootstrap
failure reported in the PR I temporarily disabled the
-fprintf-return-value optimization this morning.

The attached patch fixes the underlying problems.  I've retested
it on i386 (ILP32), x86_64, and powerpc64le, the latter with the
optimization enabled (even though the patch doesn't enable it).

I'd like to commit this patch first to unblock the ILP32 targets,
then re-enable the optimization in a subsequent commit, and then
look into fixing the outstanding minor issues in the test suite
on ILP32 (there's just one failure in my i368 test run) and some
of those pointed out by Jakub this morning in

  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01436.html

Thanks
Martin

PR target/77676 - powerpc64 and powerpc64le stage2 bootstrap fail

gcc/testsuite/ChangeLog:
2016-09-21  Martin Sebor  <mse...@redhat.com>

	PR target/77676
	* gcc.dg/tree-ssa/builtin-sprintf-2.c: Fix typo.
	* gcc.dg/tree-ssa/builtin-sprintf-3.c: New test.
	* gcc.dg/tree-ssa/builtin-sprintf-warn-5.c: New test.

gcc/ChangeLog:
2016-09-21  Martin Sebor  <mse...@redhat.com>

	PR target/77676
	* gimple-ssa-sprintf.c (target_int_min, target_int_max): Use
	HOST_BITS_PER_WIDE_INT, make a static local variable auto.
	(format_integer): Use long long as the argument for the ll length
	modifier.
	(format_floating): Use target_int_max().
	(get_string_length): Same.
	(format_string): Avoid setting the bounded flag for strings
	of unknown length.
	(try_substitute_return_value): Avoid setting range info when
	the result isn't bounded.
	* varasm.c (assemble_name): Increase buffer size.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index dddb026..652d3fb 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -210,8 +210,8 @@ struct format_result
 static HOST_WIDE_INT
 target_int_min ()
 {
-  static const unsigned HOST_WIDE_INT int_min
-= 1LLU << (sizeof int_min * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_min
+= 1LLU << (HOST_BITS_PER_WIDE_INT
 	   - TYPE_PRECISION (integer_type_node) + 1);
   return int_min;
 }
@@ -221,8 +221,8 @@ target_int_min ()
 static unsigned HOST_WIDE_INT
 target_int_max ()
 {
-  static const unsigned HOST_WIDE_INT int_max
-= HOST_WIDE_INT_M1U >> (sizeof int_max * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_max
+= HOST_WIDE_INT_M1U >> (HOST_BITS_PER_WIDE_INT
 			- TYPE_PRECISION (integer_type_node) + 1);
   return int_max;
 }
@@ -851,7 +851,9 @@ format_integer (const conversion_spec , tree arg)
 
 case FMT_LEN_L:
 case FMT_LEN_ll:
-  dirtype = sign ? long_integer_type_node : long_unsigned_type_node;
+  dirtype = (sign
+		 ? long_long_integer_type_node
+		 : long_long_unsigned_type_node);
   break;
 
 case FMT_LEN_z:
@@ -1366,7 +1368,7 @@ format_floating (const conversion_spec , tree arg)
 	  *minmax[i] = mpfr_snprintf (NULL, 0, fmtstr, mpfrval);
 	}
 
-  res.bounded = res.range.min < HOST_WIDE_INT_MAX;
+  res.bounded = res.range.min < target_int_max ();
   return res;
 }
 
@@ -1420,7 +1422,7 @@ get_string_length (tree str)
   /* Set RES.BOUNDED to true if and only if all strings referenced
 	 by STR are known to be bounded (though not necessarily by their
 	 actual length but perhaps by their maximum possible length).  */
-  res.bounded = res.range.max < HOST_WIDE_INT_MAX;
+  res.bounded = res.range.max < target_int_max ();
 
   /* Set RES.CONSTANT to false even though that may be overly
 	 conservative in rare cases like: 'x ? a : b' where a and
@@ -1471,6 +1473,10 @@ format_string (const conversion_spec , tree arg)
: 2 == warn_format_length ? 0 <= prec ? prec : 1
: HOST_WIDE_INT_MAX);
 
+  /* The result is bounded unless overriddden for a non-constant string
+ of an unknown length.  */
+  bool bounded = true;
+
   if (spec.specifier == 'c')
 {
   if (spec.modifier == FMT_LEN_l)
@@ -1550,16 +1556,17 @@ format_string (const conversion_spec , tree arg)
 	  if (0 <= prec)
 	{
 	  if ((unsigned)prec < slen.range.min
-		  || slen.range.min >= HOST_WIDE_INT_MAX)
+		  || slen.range.min >= target_int_max ())
 		slen.range.min = prec;
 	  if ((unsigned)prec < slen.range.max
-		  || slen.range.max >= HOST_WIDE_INT_MAX)
+		  || slen.range.max >= target_int_max ())
 		slen.range.max = prec;
 	}
-	  else if (slen.range.min >= HOST_WIDE_INT_MAX)
+	  else if (slen.range.min >= target_int_max ())
 	{
 	  slen.range.min = max_bytes_for_unknown_str;
 	  slen.range.max = max_bytes_for_unknown_str;
+	  bounded = false;
 	}
 
 	  res.range = slen.range;
@@ -1580,7 +1587,8 @@ format_string (const conversion_spec , tree arg)
  

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

your patch broke bootstrap with MPFR 2.4.2, which is still the
recommended (or perhaps minimal) version according to install.texi:

/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 'int 
{anonymous}::format_floating_max(tree, char)':
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:1128:27: error: 
'MPFR_RNDN' was not declared in this scope
   mpfr_from_real (x, , MPFR_RNDN);
   ^
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 
'{anonymous}::fmtresult {anonymous}::format_floating(const 
{anonymous}::conversion_spec&, tree)':
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:1328:37: error: 
'MPFR_RNDN' was not declared in this scope
   mpfr_from_real (mpfrval, rvp, MPFR_RNDN);
 ^

MPFR_RNDN was only introduced in mpfr 3.0.0, and everywhere else in gcc
GMP_RNDN is used instead.  mpfr 3.0.0  has

/* kept for compatibility with MPFR 2.4.x and before */
#define GMP_RNDN MPFR_RNDN

The following patch (together with your other one to fix ILP32 targets)
allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
commit it as obvious.


Thanks.  I didn't realize the older MPFR was still recommended.
FWIW, I was using MPFR 2.4 until recently but after running into
some (unexplained) runtime issues I reran the download_prerequisites
script to update them and it installed 3.1.4.

It seems that either the recommended MPFR version should be bumped
up (or the script downgraded to 2.4 to avoid this risk, though I
suspect there are reasons why it was updated to the latest).

Based on the log the script was updated from MPFR 2.4 to 3.0 in
r235763.  The change also updated install.texi but not the MPFR
version mentioned there.  It might be worth checking with the
author, Bernd Edlinger, to see how to resolve this.  Let me ping
him in a separate thread.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/22/2016 06:14 AM, Rainer Orth wrote:

Hi Martin,


your patch broke bootstrap with MPFR 2.4.2, which is still the
recommended (or perhaps minimal) version according to install.texi:

[...]

The following patch (together with your other one to fix ILP32 targets)
allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
commit it as obvious.


done now.  Once the bootstrap had finished, I see quite a number of
testsuite failures (i386-pc-solaris2.12 still running), both 32 and
64-bit:

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1220)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1270)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1381)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 356)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)


I have a patch for (hopefully) most of these failures that I will
commit along with the one for pr77676 as soon as it's approved.


Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:209:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:210:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:211:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:211:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 6 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:212:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:213:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:213:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 6 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1238:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1239:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1240:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1287:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 6 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1288:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 6 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1289:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 6 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:99:3:
 warning: '%p' directive writing 1 byte into a region of size 0 
[-Wformat-length=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1406:3:
 warning: specified size 4294967295 exceeds the size 2 of the destination 
object [-Wformat-length=]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c  (test for warnings, line 50)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into strcpy
(test for warnings, line 83)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into strcpy
(test for warnings, line 84)

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)

Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:10:21:
 warning: writing a terminating nul past the end of the destination 
[-Wformat-length=]/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:22:22:
 warning: '%-s' directive writing 4 bytes into a region of size 1 
[-Wformat-length=]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf.c execution test

FAIL: test_a_double:364: "%a" expected result for "0x0.0p+0" 
doesn't match function call return value: 20 != 6
FAIL: test_a_double:365: "%a" expected result for "0x1.0p+0" 
doesn't match function 

Re: [PATCH] fix ILP32 bootstrap failures due to -Wformat-length

[Martin Sebor]

Jeff or Richard,

As the two middle end maintainers familiar with my work, can one
of you approve the updated patch?  It's necessary to restore ILP32
bootstrap on a number of targets (pr77676).

  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01477.html

Thanks
Martin

On 09/21/2016 06:28 PM, Martin Sebor wrote:

On 09/21/2016 05:09 PM, Jakub Jelinek wrote:

On Wed, Sep 21, 2016 at 04:39:42PM -0600, Martin Sebor wrote:

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index dddb026..652d3fb 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -210,8 +210,8 @@ struct format_result
 static HOST_WIDE_INT
 target_int_min ()
 {
-  static const unsigned HOST_WIDE_INT int_min
-= 1LLU << (sizeof int_min * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_min
+= 1LLU << (HOST_BITS_PER_WIDE_INT


1LLU should be really HOST_WIDE_INT_1U


- TYPE_PRECISION (integer_type_node) + 1);


Is the shift amount really what you mean?
HOST_BITS_PER_WIDE_INT - TYPE_PRECISION (integer_type_node) + 1
is usually 33 (or 17 or 9 in much rarer cases), so that is
0x2ULL.  Don't you want instead
  = HOST_WIDE_INT_1U << (TYPE_PRECISION (ingeger_type_node) - 1);
so that it will be
0x8000ULL?


Great catch, thank you!  That must have been a copy and paste mistake.




@@ -221,8 +221,8 @@ target_int_min ()
 static unsigned HOST_WIDE_INT
 target_int_max ()
 {
-  static const unsigned HOST_WIDE_INT int_max
-= HOST_WIDE_INT_M1U >> (sizeof int_max * CHAR_BIT
+  const unsigned HOST_WIDE_INT int_max
+= HOST_WIDE_INT_M1U >> (HOST_BITS_PER_WIDE_INT
 - TYPE_PRECISION (integer_type_node) + 1);
   return int_max;
 }


This is expectedly -1ULL >> 33, i.e. 0x7fffULL, which looks ok.


Thanks for double checking that.  There is a test case for this
in the test suite for the optimization but there wasn't one for
the INT_MIN case.  I've added it in the attached patch.  I'll
beef up the return range testing some more before re-enabling
the optimization.

Martin

Re: [PATCH] Fix various minor gimple-ssa-sprintf.c issues

[Martin Sebor]

What I would be even more grateful for is a review of the error
prone parts like those that caused the bootstrap failure.  I.e.,
any lingering assumptions about integer sizes between the host


I must say I'm surprised you do all your computations in HOST_WIDE_INT,
rather than say in wide_int, then you could just compare against
TYPE_{MIN,MAX}_VALUE (TYPE_DOMAIN (integer_type_node)) instead of inventing
a function for that.  As for the warn_* vs. flag_* stuff, it just looked
that the warn_* is tested in lots of functions invoked even for the return
length folding, so it is much harder to prove whether it works properly or
not.


Well, chalk that up to my ignorance of a better/more appropriate
API.  I'll be happy to switch to using it if it simplifies things.
Thanks for the tip!

Martin

Re: [PATCHv3][ARM] -mpure-code option for ARM

[Martin Sebor]

FYI (to help avoid raising duplicate bugs), I opened bug 77695
for the bootstrap failure.

Martin

On 09/22/2016 01:33 PM, Bill Seurer wrote:

This patch breaks compilation on power:

g++ -fno-PIE -c   -g -O2 -DIN_GCC -fno-exceptions -fno-rtti
-fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings
-Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic
-Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common
-DHAVE_CONFIG_H -I. -I. -I/home/seurer/gcc/gcc-test2/gcc
-I/home/seurer/gcc/gcc-test2/gcc/.
-I/home/seurer/gcc/gcc-test2/gcc/../include
-I/home/seurer/gcc/gcc-test2/gcc/../libcpp/include
-I/home/seurer/gcc/gcc-test2/gcc/../libdecnumber
-I/home/seurer/gcc/gcc-test2/gcc/../libdecnumber/dpd -I../libdecnumber
-I/home/seurer/gcc/gcc-test2/gcc/../libbacktrace   -o rs6000.o -MT
rs6000.o -MMD -MP -MF ./.deps/rs6000.TPo
/home/seurer/gcc/gcc-test2/gcc/config/rs6000/rs6000.c
In file included from /home/seurer/gcc/gcc-test2/gcc/target-def.h:106:0,
 from
/home/seurer/gcc/gcc-test2/gcc/config/rs6000/rs6000.c:77:
./target-hooks-def.h:92:38: error: 'hook_uint_uintp_false' was not
declared in this scope
 #define TARGET_ASM_ELF_FLAGS_NUMERIC hook_uint_uintp_false
  ^
./target-hooks-def.h:2205:5: note: in expansion of macro
'TARGET_ASM_ELF_FLAGS_NUMERIC'
 TARGET_ASM_ELF_FLAGS_NUMERIC, \
 ^~~~
./target-hooks-def.h:1792:5: note: in expansion of macro 'TARGET_ASM_OUT'
 TARGET_ASM_OUT, \
 ^~
/home/seurer/gcc/gcc-test2/gcc/config/rs6000/rs6000.c:40709:29: note: in
expansion of macro 'TARGET_INITIALIZER'
 struct gcc_target targetm = TARGET_INITIALIZER;
 ^~
make[2]: *** [rs6000.o] Error 1


On 09/22/16 12:04, Andre Vieira (lists) wrote:

On 22/09/16 16:28, Richard Earnshaw (lists) wrote:

On 22/09/16 16:04, Andre Vieira (lists) wrote:


I reworked the patch according to the comments above.

Is this OK?

gcc/ChangeLog:
2016-09-22  Andre Vieira  
Terry Guo  

* target.def (elf_flags_numeric): New target hook.
* targhooks.h (default_asm_elf_flags_numeric): New.
* varasm.c (default_asm_elf_flags_numeric): New.
  (default_elf_asm_named_section): Use new target hook.
* config/arm/arm.opt (mpure-code): New.
* config/arm/arm.h (SECTION_ARM_PURECODE): New.
* config/arm/arm.c (arm_asm_init_sections): Add section
  attribute to default text section if -mpure-code.
  (arm_option_check_internal): Diagnose use of option with
  non supported targets and/or options.
  (arm_asm_elf_flags_numeric): New.
  (arm_function_section): New.
  (arm_elf_section_type_flags): New.
* config/arm/elf.h (JUMP_TABLES_IN_TEXT_SECTION): Disable
  for -mpure-code.
* gcc/doc/texi (TARGET_ASM_ELF_FLAGS_NUMERIC): New.
* gcc/doc/texi.in (TARGET_ASM_ELF_FLAGS_NUMERIC): Likewise.



gcc/testsuite/ChangeLog:
2016-09-22  Andre Vieira  
Terry Guo  

* gcc.target/arm/pure-code/ffunction-sections.c: New.
* gcc.target/arm/pure-code/no-literal-pool.c: New.
* gcc.target/arm/pure-code/pure-code.exp: New.




I missed this last time around, but please can you wrap references to
SHF_ARM_PURECODE in the documentation with @code{...}.

OK with that change.

R.


Done. Committed as revision r240379.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

David, in the way of feedback, I spent hours debugging the simple
multiline test I added.  It seems that DejaGnu is exquisitely
sensitive to whitespaces in the multiline output.  I appreciate
that spacing is essential to lining up the caret and the tildes
with the text of the program but tests fail even when all the
expected output is lined up right in the multiline directive but
off by a space (or tab) with respect to the actual output.  That
DejaGnu output doesn't make this very clear at all makes debugging
these types of issues a pain.  I wonder if there's a better to do
this.


Gah; I'm sorry this was such a pain to do.

I tend to just copy the stuff I want to quote directly from
Emacs's compilation buffer.


Yes, I did start with that.  The problem is that I have a script
that I run to help massage my patches to the format required by
the GCC Coding Conventions.  The script mainly replaces blocks
of spaces with tabs. So while my initial patch worked, the final
one didn't because of the tabs, and it took me hours to figure
out why.  Because I couldn't see the difference I thought it was
a bug in DejaGnu or some other tool that was different between
the two machines I was using for testing the two patches.  Until
it occurred to me to diff the test itself... (FWIW, I think this
style convention is bad and should be abolished in favor of using
only plain spaces.)



However a nit, which I think is why you had problems...


...


You have two pairs of dg-begin/end-multiline-output, but I think it's
better to have four; use a single begin/end pair for each call to
diagnostic_show_locus.  Hopefully doing that ought to make things a bit
less sensitive to whitespace, and easier to debug: each begin/end can
be handled by itself, whereas by combining them it's harder for
multiline.exp to detect them.  If combined, they can only be detected
if all of the dg-warning/dg-messages work (and are thus pruned by
prune.exp), if any aren't pruned, the multiline outputs will also fail.
 Maybe this exacerbated the problem?


Maybe.  I remember experimenting with the multiline directives when
I first added the test and was struggling to get it to work.  I think
the problems I was having were unrelated to tabs but probably had
something to do with the exact number of leading spaces.

FWIW, to make the multiline directives less prone to this type of
a problem it seems that instead of counting each leading space and
tab character and treating them as ordinary they could verify that
the first non-whitespace character on each line of the actual output
lines up with the first non-whitespace character of the expected
output with some constant offset.  That way spaces vs tabs shouldn't
matter as long as they were used consistently (or they could be made
not to matter at all if a tab was treated as a single space).  What
do you think about that?



So something like this, grouping the 4 diagnostics together with their
diagnostic_show_locus output by opening and closing comments (line
numbers will need adjusting):


Thanks.  Let me apply it and see how it works.



Another nit, if I may: FWIW I'm not in love with the wording of the messages.  
Sorry to bikeshed, but how about:
  warning: buffer overflow will occur when writing terminating NUL
and:
  note: formatted output of 2 bytes into a destination of size 1
or somesuch.


I won't claim the text of the messages is perfect but I did spend
a lot of time tweaking them.  That's not to say they can't be
improved but changing them means quite a bit of work adjusting
the tests.  At this point, I'd like to focus on getting the patch
committed.  After that, if there's still time, I'm happy to take
feedback and tweak the diagnostics based on it.

Thanks again for your help and the suggestions above!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

+  /* The minimum and maximum length.  The MAXLEN pointer stays unchanged
+ but MINLEN may be cleared during the execution of the function.  */
+  tree *minlen = length;
+  tree* const maxlen = length + 1;

Check formatting here.  I'm not sure if the formatting standards
explicitly state how to handle the "*" when there's a qualifier between
the type and the object.  But please check.


If my regular expressions were right, most code puts the star right
before the const (379 occurrences in .c and .h files under the gcc/
directory not counting the test suite).  For example:

  gcc/cp/decl.c:  const char *const name;

There's quite a bit of code that also puts a space after it (219
occurrences).  For example:

  gcc/cgraph.h:extern const char * const tls_model_names[];

There are 28 occurrences of the style I used, mostly in function
declarations.  For instance:

gcc/cp/mangle.c:decl_is_template_id (const tree decl, tree* const 
template_info)


I changed my code to follow with the dominant style.




+inform (info.fmtloc,
+"using the range [%qE, %qE] for directive argument",
+fmtres.argmin, fmtres.argmax);

Don't you need G_ for these messages?

Can you please check the calls to fmtwarn which pass in the string as an
argument and add G_ as needed?  I think the cases where the string is
computed into a variable are all handled correctly, it's jut those where
the string appears as a call argument that need double checking.  I
think there's ~3 of them.


I found an error() call in c-family/c-format.c with the string split
across two lines like here (see below) and the error message appears
correctly concatenated in the gcc.pot file so I take that to mean
that the G_ isn't necessary here.

  error ("found a %<%s%> reference but the format argument should"
 " be a string", format_name (gcc_objc_string_format_type));

Other than that, though, since it's an easy mistake to make, it
would be helpful if problems like the one you were worried about
could be detected by some tool run during a build.  I wonder how
hard it would be to write an awk scirpt...


+#if 0

...

+#else

...

+#endif

Please remove the #if 0'd code.


Sorry about that.  This was left over from my effort to persuade
the substring_loc class to underline just the last quote of the
format string.  I thought David had made some changes to make it
possible but I may have misremembered.  Let me raise a separate
bug for it.


I think with the nits above fixed, this is OK for the trunk.

Thanks for your patience.


Great, thank you!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/20/2016 01:38 PM, David Malcolm wrote:

On Tue, 2016-09-20 at 13:35 -0600, Martin Sebor wrote:

[...]


+#if 0

...

+#else

...

+#endif

Please remove the #if 0'd code.


Sorry about that.  This was left over from my effort to persuade
the substring_loc class to underline just the last quote of the
format string.  I thought David had made some changes to make it
possible but I may have misremembered.  Let me raise a separate
bug for it.


I don't think the substring_loc code supports such a feature yet;
please open a bug for it and CC me on it.


Done in bug 77672 - wrong rich location in warning: writing
a terminating nul past the end.

Thanks
Martin

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

On 09/20/2016 07:00 AM, Joseph Myers wrote:

On Tue, 20 Sep 2016, Martin Sebor wrote:


That said, since this specifier formats a vec, it seems that
it might be useful to be able to format vectors of other elements,
such as short and long.  With that in mind, would adding a new V
length modifier instead be a more regular way to extend the pretty
printer to these types?  The V length modifier would have to be
accepted in conjunction with other length modifiers (h, l, etc
and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept


That's much harder to support in format checking (which expects one length
modifier, not a combination like that).


I haven't looked into it detail but since the format checker supports
one-to-two character sequences of length modifiers (h or hh, etc) it
should be possible to extend it to handle one-to-three character
sequences (h, hV, or hhV, etc.)  The V character would not be a new
length modifier on its own but instead be recognized as part of
a longer length modifier sequence.  Do you see a problem with that?

Martin

Re: [RFC] Update gmp/mpfr/mpc minimum versions

[Martin Sebor]

On 04/27/2016 09:47 AM, Bernd Edlinger wrote:



Am 27.04.2016 um 17:37 schrieb Rainer Orth:

Bernd Edlinger  writes:


On 26.04.2016 22:14, Joseph Myers wrote:

On Tue, 26 Apr 2016, Bernd Edlinger wrote:


Hi,

as we all know, it's high time now to adjust the minimum supported
gmp/mpfr/mpc versions for gcc-7.


I think updating the minimum versions (when using previously built
libraries, not in-tree) is only appropriate when it allows some cleanup in
GCC, such as removing conditionals on whether a more recently added
function is available, adding functionality that depends on a newer
interface, or using newer interfaces instead of older ones that are now
deprecated.

For example, you could justify a move to requiring MPFR 3.0.0 or later
with cleanups to use MPFR_RND* instead of the older GMP_RND*, and
similarly mpfr_rnd_t instead of the older mp_rnd_t and likewise mpfr_exp_t
and mpfr_prec_t in fortran/.  You could justify a move to requiring MPC
1.0.0 (or 1.0.2) by optimizing clog10 using mpc_log10.  I don't know what
if any newer GMP interfaces would be beneficial in GCC.  And as always in
such cases, it's a good idea to look at e.g. how widespread the newer
versions are in GNU/Linux distributions, which indicates how many people
might be affected by an increase in the version requirement.



Yes I see.

I would justify it this way: gmp-6.0.0 is the first version that does
not invoke undefined behavior in gmp.h, once we update to gmp-6.0.0
we could emit at least a warning in cstddef for this invalid code.

Once we have gmp-6.0.0, the earliest mpfr version that compiles at all
is mpfr-3.1.1 and the earliest mpc version that compiles at all is
mpc-0.9.  This would be the supported installed versions.

In-tree gmp-6.0.0 does _not_ work for ARM.  But gmp-6.1.0 does (with a
little quirk).  All supported mpfr and mpc versions are working in-tree
too, even for the ARM target.

When we have at least mpfr-3.1.1, it is straight forward to remove the
pre-3.1.0 compatibility code from gcc/fortran/simplify.c for instance.

So I would propose this updated patch for gcc-7.


would this version combo (gmp 6.0.0, mpfr 3.1.1, mpc 0.9) also work on
the active release branches (gcc-5 and gcc-6, gcc-4.9 is on it's way
out)?  Having to install two different sets of the libraries for trunk
and branch work would be extremely tedious.

Rainer



Yes, when they are pre-installed there should be no problem.
Also newer versions than these seem to work.

In-tree only the versions that download_prerequisite picks are
tested and guaranteed to work.


I was made aware today that my recent patch for pr49905 broke
bootstrap with MPFR 2.4:

  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01507.html

In light of this risk and given that the recommended MPFR version
is still 2.4 I wonder if the download_prerequisites script shouldn't
instead download the minimum supported version.  That way those of
us working with MPFR but not intimately familiar with its version
specific details would have an easier way of avoiding such breakage.

Alternatively, perhaps the script could be extended to make it
possible to choose between the most recent and the recommended
versions of the prerequisites that GCC is intended to work with,
and people who make use of either in their code encouraged to
test with both.

Martin

Re: [PATCH] Fixups for Martin's gimple-ssa-sprintf.c patch

[Martin Sebor]

On 08/25/2016 10:23 AM, David Malcolm wrote:

Martin: here are the fixups for your patch I needed to apply to make
it work with mine.  I couldn't actually get any of your existing test
cases to emit locations within the string literals, due to them all
being embedded in macro expansions (possibly relating to PR c/77328),
so I added a simple testcase using -fdiagnostics-show-caret, which
does successfully show a range within the string.

Posting in the hope that it's helpful; I haven't attempted a bootstrap
with it.


Thanks!  Let me go through it, test it and get back to you when I'm
done (sometime next week after I get back from PTO).

Martin

Re: Fix bogus warning with -Wlogical-not-parentheses (PR c/77292)

[Martin Sebor]

+foo (int a, int b)
+{
+  int r = 0;
+  r += !a == (a < b);
+  r += !a == (a > b);
+  r += !a == (a >= b);
+  r += !a == (a <= b);
+  r += !a == (a != b);
+  r += !a == (a == b);
+  r += !a == (a || b);
+  r += !a == (a && b);
+  r += !a == (!b);
+
+  r += !a == (a ^ b); /* { dg-warning "logical not is only applied to the left hand 
side of comparison" } */
+  r += !a == (a | b); /* { dg-warning "logical not is only applied to the left hand 
side of comparison" } */
+  r += !a == (a & b); /* { dg-warning "logical not is only applied to the left hand 
side of comparison" } */


A question more than a comment: warning on the last three expressions
above makes sense to me when the operands are integers but I'm less
sure that the same logic applies when the operands are Boolean.  Does
it make sense to issue the warning for those given that (a | b) and
(a & b) are the same as (a || b) and (a && b) for which the warning
is suppressed?

In other words, is warning on the latter of the two below but not on
the former a good idea or should they be treated the same way?

$ cat t.c && gcc -S -Wlogical-not-parentheses t.c
_Bool f (int a, _Bool b, _Bool c)
{
  _Bool r;

  r = !a == (b || c);
  r = !a == (b | c);

  return r;
}
t.c: In function ‘f’:
t.c:6:10: warning: logical not is only applied to the left hand side of 
comparison [-Wlogical-not-parentheses]

   r = !a == (b | c);
  ^~
t.c:6:7: note: add parentheses around left hand side expression to 
silence this warning

   r = !a == (b | c);
   ^~
   ( )

Also, having hardly any experience with the fixit hints, I'm not
sure how helpful this one is.  It seems really hard to tell what
it suggests as the fix (I had to try it both ways to see).  I
think would be a lot clearer if it showed the full expression
rather than just the operators.  Would printing this instead be
doable?

   r = !a == (b | c);
   ^~
   (!a)

Martin

Re: [PATCH] Fix suggestions for non-trivial Wformat type cases (PR c/72858)

[Martin Sebor]

On 08/12/2016 01:27 PM, David Malcolm wrote:

In r239260 I attempted to add fix-it hints for -Wformat type
warnings.

Unfortunately, my implementation was too simplistic, and only
worked correctly for the most simple format strings: the fix-it
hint would suggest replacement of an entire conversion specification,
but the replacement would only include the length modifier (if
any) and the conversion character, thus effectively discarding
any user-supplied flags, width, or precision.

Additionally, the replacement failed to take into account the
user-specified conversion character, so that e.g. an "x" could
be replaced by a "d".

The following patch fixes these issues by:
(a) making the suggestion retain any user-supplied part of the
conversion specification before the length modifier, and
(b) attempt to preserve the user-specified conversion character, by
first considering replacements that only change the length
modifier.

It also expands the function comments in c-format.c, showing how
the functions work through a non-trivial example.

This patch is a followup to:
   "[PATCH] Fix caret locations in format_type_warning (PR c/72857)"
  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00867.html

Successfully bootstrapped on x86_64-pc-linux-gnu (on top
of the other patch).


A couple of things I noticed when getting the -Wformat-length code
to use the new infrastructure from c-format.

Hints that suggest alternate directives that don't accept all
the same flags as those used in the original should filter out
those flags. For example, when passing a pointer to a "%#lx"
directive, the hint should suggest "%s" for character pointers
and "%p" for others rather than "%#s" or "%#p".  Similarly,
when passing a string to a "%+i" or "% i" directive, the '+'
and space flags should be removed.

Also, with -Wformat-signedness (but I'd say without it as well),
the hints should respect the signedness of the arguments.  For
example, the hint for "%u" with a long int argument should be
"%lu" and not "%li".

Martin