PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

I'm testing this patch as a response to
    http://www.kb.cert.org/vuls/id/162289

This patch treats undefined pointer wraparound optimizations as an
instance of undefined signed overflow optimizations (they are of
course different, but they seem similar to users not educated in
standardese).  You will get a warning with -Wstrict-overflow, and you
can disable the optimization with -fno-strict-overflow.

My plan is to backport this patch to the gcc 4.2 and 4.3 branches.

Any comments or concerns?

Ian


gcc/ChangeLog:
2008-04-07  Ian Lance Taylor  <iant@google.com>

	* flags.h (POINTER_TYPE_OVERFLOW_UNDEFINED): Define.
	* fold-const.c (fold_comparison): If appropriate, test
	POINTER_TYPE_OVERFLOW_UNDEFINED, and issue an overflow warning.
	(fold_binary): Test POINTER_TYPE_OVERFLOW_UNDEFINED when
	reassociating a pointer type.
	* doc/invoke.texi (Optimize Options): Document that
	-fstrict-overflow applies to pointer wraparound.

gcc/testsuite/ChangeLog:
2008-04-07  Ian Lance Taylor  <iant@google.com>

	* gcc.dg/strict-overflow-6.c: New.
	* gcc.dg/no-strict-overflow-7.c: New.
	* gcc.dg/Wstrict-overflow-22.c: New.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Warn about pointer wraparound with -Wstrict-overflow --]
[-- Type: text/x-patch, Size: 5108 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 133985)
+++ gcc/doc/invoke.texi	(working copy)
@@ -6161,6 +6161,14 @@ using twos complement arithmetic.  When 
 attempt to determine whether an operation on signed numbers will
 overflow must be written carefully to not actually involve overflow.
 
+This option also allows the compiler to assume strict pointer
+semantics: given a pointer to an object, if adding an offset to that
+pointer does not produce a pointer to the same object, the addition is
+undefined.  This permits the compiler to conclude that @code{p + i >
+p} is always true for a pointer @code{p} and integer @code{i}.  This
+assumption is only valid if pointer wraparound is undefined, as the
+expression is false if @code{p + i} overflows.
+
 See also the @option{-fwrapv} option.  Using @option{-fwrapv} means
 that signed overflow is fully defined: it wraps.  When
 @option{-fwrapv} is used, there is no difference between
Index: gcc/flags.h
===================================================================
--- gcc/flags.h	(revision 133985)
+++ gcc/flags.h	(working copy)
@@ -332,6 +332,10 @@ extern bool flag_instrument_functions_ex
 #define TYPE_OVERFLOW_TRAPS(TYPE) \
   (!TYPE_UNSIGNED (TYPE) && flag_trapv)
 
+/* True if pointer types have undefined overflow.  */
+#define POINTER_TYPE_OVERFLOW_UNDEFINED \
+  (!flag_wrapv && !flag_trapv && flag_strict_overflow)
+
 /* Names for the different levels of -Wstrict-overflow=N.  The numeric
    values here correspond to N.  */
 
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 133985)
+++ gcc/fold-const.c	(working copy)
@@ -8564,7 +8564,9 @@ fold_comparison (enum tree_code code, tr
 	     because pointer arithmetic is restricted to retain within an
 	     object and overflow on pointer differences is undefined as of
 	     6.5.6/8 and /9 with respect to the signed ptrdiff_t.  */
-	  else if (bitpos0 == bitpos1)
+	  else if (bitpos0 == bitpos1
+		   && ((code == EQ_EXPR || code == NE_EXPR)
+		       || POINTER_TYPE_OVERFLOW_UNDEFINED))
 	    {
 	      tree signed_size_type_node;
 	      signed_size_type_node = signed_type_for (size_type_node);
@@ -8583,6 +8585,12 @@ fold_comparison (enum tree_code code, tr
 	      else
 		offset1 = fold_convert (signed_size_type_node, offset1);
 
+	      if (code != EQ_EXPR && code != NE_EXPR)
+		fold_overflow_warning (("assuming pointer wraparound does not "
+					"occur when comparing P +- C1 with "
+					"P +- C2"),
+				       WARN_STRICT_OVERFLOW_COMPARISON);
+
 	      return fold_build2 (code, type, offset0, offset1);
 	    }
 	}
@@ -9707,7 +9715,7 @@ fold_binary (enum tree_code code, tree t
 
 	  /* With undefined overflow we can only associate constants
 	     with one variable.  */
-	  if ((POINTER_TYPE_P (type)
+	  if (((POINTER_TYPE_P (type) && POINTER_TYPE_OVERFLOW_UNDEFINED)
 	       || (INTEGRAL_TYPE_P (type) && !TYPE_OVERFLOW_WRAPS (type)))
 	      && var0 && var1)
 	    {
Index: gcc/testsuite/gcc.dg/strict-overflow-6.c
===================================================================
--- gcc/testsuite/gcc.dg/strict-overflow-6.c	(revision 0)
+++ gcc/testsuite/gcc.dg/strict-overflow-6.c	(revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-fstrict-overflow -O2 -fdump-tree-final_cleanup" } */
+
+/* Source: Ian Lance Taylor.  Dual of no-strict-overflow-7.c.  */
+
+/* We can only simplify the conditional when using strict overflow
+   semantics.  */
+
+int
+foo (char* p)
+{
+  return p + 1000 < p;
+}
+
+/* { dg-final { scan-tree-dump-not "\[+\]\[ \]*1000" "final_cleanup" } } */
+/* { dg-final { cleanup-tree-dump "final_cleanup" } } */
Index: gcc/testsuite/gcc.dg/no-strict-overflow-7.c
===================================================================
--- gcc/testsuite/gcc.dg/no-strict-overflow-7.c	(revision 0)
+++ gcc/testsuite/gcc.dg/no-strict-overflow-7.c	(revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-strict-overflow -O2 -fdump-tree-final_cleanup" } */
+
+/* Source: Ian Lance Taylor.  Dual of strict-overflow-6.c.  */
+
+/* We can only simplify the conditional when using strict overflow
+   semantics.  */
+
+int
+foo (char* p)
+{
+  return p + 1000 < p;
+}
+
+/* { dg-final { scan-tree-dump "\[+\]\[ \]*1000" "final_cleanup" } } */
+/* { dg-final { cleanup-tree-dump "final_cleanup" } } */
Index: gcc/testsuite/gcc.dg/Wstrict-overflow-22.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstrict-overflow-22.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Wstrict-overflow-22.c	(revision 0)
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-fstrict-overflow -O2 -Wstrict-overflow=3" } */
+
+/* Source: Ian Lance Taylor.  Based on strict-overflow-6.c.  */
+
+/* We can only simplify the conditional when using strict overflow
+   semantics.  */
+
+int
+foo (char* p)
+{
+  return p + 1000 < p; /* { dg-warning "assuming pointer wraparound does not occur" "correct warning" } */
+}

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Richard Guenther]

On Mon, Apr 7, 2008 at 9:35 PM, Ian Lance Taylor <iant@google.com> wrote:
> I'm testing this patch as a response to
>     http://www.kb.cert.org/vuls/id/162289
>
>  This patch treats undefined pointer wraparound optimizations as an
>  instance of undefined signed overflow optimizations (they are of
>  course different, but they seem similar to users not educated in
>  standardese).  You will get a warning with -Wstrict-overflow, and you
>  can disable the optimization with -fno-strict-overflow.
>
>  My plan is to backport this patch to the gcc 4.2 and 4.3 branches.

Please leave at least the 4.2 branch alone.

>  Any comments or concerns?

+/* True if pointer types have undefined overflow.  */
+#define POINTER_TYPE_OVERFLOW_UNDEFINED \
+  (!flag_wrapv && !flag_trapv && flag_strict_overflow)

don't add flag_trapv here, it doesn't make sense.

In general I don't think we should do this.  The tests in this stupid
CERT are bogus and I have never seen such.  Also this test
will cause many false positives I belive, almost any loop with a
pointer induction variable should be affected.

But of course my complaints about -Wstrict-overflow were unheard
in the past as well ;)

Richard.

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

"Richard Guenther" <richard.guenther@gmail.com> writes:

> On Mon, Apr 7, 2008 at 9:35 PM, Ian Lance Taylor <iant@google.com> wrote:
>> I'm testing this patch as a response to
>>     http://www.kb.cert.org/vuls/id/162289
>>
>>  This patch treats undefined pointer wraparound optimizations as an
>>  instance of undefined signed overflow optimizations (they are of
>>  course different, but they seem similar to users not educated in
>>  standardese).  You will get a warning with -Wstrict-overflow, and you
>>  can disable the optimization with -fno-strict-overflow.
>>
>>  My plan is to backport this patch to the gcc 4.2 and 4.3 branches.
>
> Please leave at least the 4.2 branch alone.

Why?  The CERT complaint is specifically directed at gcc 4.2.
Skipping 4.2 makes no sense.


>>  Any comments or concerns?
>
> +/* True if pointer types have undefined overflow.  */
> +#define POINTER_TYPE_OVERFLOW_UNDEFINED \
> +  (!flag_wrapv && !flag_trapv && flag_strict_overflow)
>
> don't add flag_trapv here, it doesn't make sense.

If we view pointer wraparound as an overflow, then I think it does.
We should be trapping on the overflow.  Of course, -ftrapv doesn't
work in general.  But that doesn't permit us to elide the potential
overflow here.


> In general I don't think we should do this.  The tests in this stupid
> CERT are bogus and I have never seen such.  Also this test
> will cause many false positives I belive, almost any loop with a
> pointer induction variable should be affected.
> 
> But of course my complaints about -Wstrict-overflow were unheard
> in the past as well ;)

I completely agree with you that the CERT issue is bogus, and that
this test will have plenty of false positives.  But that's really not
the point.  We are defending gcc against an unwarranted attack.  The
way to defend against an unwarranted attack is to add an optional
warning.

Nobody has to use the new warning.  It's coming in at
-Wstrict-overflow=3--using plain -Wstrict-overflow is equivalent to
-Wstrict-overflow=2.

I don't want to have this same fight over and over again.  We can not
ignore our user base.  If you have a counter-proposal, tell us.  Valid
objections to my proposed patch are that it is too hard to maintain or
doesn't do the job.  I'm not interested in hearing objections that
this patch is inappropriate without a counter-proposal beyond ignoring
the issue.

I would never recommend that anybody actually use -Wstrict-overflow,
except maybe once to see what their code base looks like.  That's not
why it exists.

But I will look into making the warning not trigger on compiler
introduced induction variables.

Ian

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Richard Guenther]

On Tue, Apr 8, 2008 at 4:58 PM, Ian Lance Taylor <iant@google.com> wrote:
> "Richard Guenther" <richard.guenther@gmail.com> writes:
>
>  > On Mon, Apr 7, 2008 at 9:35 PM, Ian Lance Taylor <iant@google.com> wrote:
>  >> I'm testing this patch as a response to
>  >>     http://www.kb.cert.org/vuls/id/162289
>  >>
>  >>  This patch treats undefined pointer wraparound optimizations as an
>  >>  instance of undefined signed overflow optimizations (they are of
>  >>  course different, but they seem similar to users not educated in
>  >>  standardese).  You will get a warning with -Wstrict-overflow, and you
>  >>  can disable the optimization with -fno-strict-overflow.
>  >>
>  >>  My plan is to backport this patch to the gcc 4.2 and 4.3 branches.
>  >
>  > Please leave at least the 4.2 branch alone.
>
>  Why?  The CERT complaint is specifically directed at gcc 4.2.
>  Skipping 4.2 makes no sense.

4.2 is a mature release, this adds a new feature.  Also in the past
the overflow warning patches have introduced regressions, let's make
sure to not do that on the 4.2 branch.

>
>
>  >>  Any comments or concerns?
>  >
>  > +/* True if pointer types have undefined overflow.  */
>  > +#define POINTER_TYPE_OVERFLOW_UNDEFINED \
>  > +  (!flag_wrapv && !flag_trapv && flag_strict_overflow)
>  >
>  > don't add flag_trapv here, it doesn't make sense.
>
>  If we view pointer wraparound as an overflow, then I think it does.
>  We should be trapping on the overflow.  Of course, -ftrapv doesn't
>  work in general.  But that doesn't permit us to elide the potential
>  overflow here.

-ftrapv is documented to trigger on integer overflow, not pointer overflow.
(Likewise -fwrapv, whose documentation you should adjust for the
pointer case, or invent a new switch).

Richard.

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

"Richard Guenther" <richard.guenther@gmail.com> writes:

>>  >>  Any comments or concerns?
>>  >
>>  > +/* True if pointer types have undefined overflow.  */
>>  > +#define POINTER_TYPE_OVERFLOW_UNDEFINED \
>>  > +  (!flag_wrapv && !flag_trapv && flag_strict_overflow)
>>  >
>>  > don't add flag_trapv here, it doesn't make sense.
>>
>>  If we view pointer wraparound as an overflow, then I think it does.
>>  We should be trapping on the overflow.  Of course, -ftrapv doesn't
>>  work in general.  But that doesn't permit us to elide the potential
>>  overflow here.
>
> -ftrapv is documented to trigger on integer overflow, not pointer overflow.
> (Likewise -fwrapv, whose documentation you should adjust for the
> pointer case, or invent a new switch).

Any other opinions on this?  The question is whether -fwrapv and
-ftrapv should affect pointer wraparound.

My sense is that distinctions like integer overflow vs. pointer
overflow are lost on the kind of people who write code like that in
the CERT advisory.  And I don't see people crying out for one option
to control integer overflow and a separate option for pointer
overflow.  But I'm happy to hear what other people have to say.

Note that in gcc 4.2 -fwrapv *does* affect the pointer optimization in
question:

  if (POINTER_TYPE_P (TREE_TYPE (arg0))
      && !flag_wrapv
      && !TYPE_OVERFLOW_TRAPS (TREE_TYPE (arg0)))

In gcc 4.3 those tests were removed, though not as an explicit
change--new code was added for this optimization, I think as part of
POINTER_PLUS, and then the old code was removed later.  The new code
didn't test flag_wrapv or TYPE_OVERFLOW_TRAPS.

Ian

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Mark Mitchell]

Ian Lance Taylor wrote:
> I'm testing this patch as a response to
>     http://www.kb.cert.org/vuls/id/162289

Thank you for working on this; comments below.

First, I agree with you that this is a feature we want.  There is a 
class of users that do really need these kinds of warnings.  They have 
lots of code, used in environments where reliability requirements are 
high, and they want every warning they can get.  They are willing to 
modify their code base if necessary to avoid warnings -- even spurious 
ones.  I've worked with people who ran code through multiple compilers 
-- just to see if they could get more warnings.

And, clearly, from a PR point of view, this helps with the CERT issue.

> +This option also allows the compiler to assume strict pointer
> +semantics: given a pointer to an object, if adding an offset to that
> +pointer does not produce a pointer to the same object, the addition is
> +undefined.  This permits the compiler to conclude that @code{p + i >
> +p} is always true for a pointer @code{p} and integer @code{i}.  This
> +assumption is only valid if pointer wraparound is undefined, as the
> +expression is false if @code{p + i} overflows.

This is only true for non-negative integers "i".  I'm sure that's what 
the compiler does, but I think the documentation should say that.  I 
also think you say when pointer wraparound is undefined; otherwise, the 
reader doesn't know whether the condition in the last sentence applies.

As to the other issues:

* I think -fwrapv/-ftrapv should affect pointers as well as integers. 
Many people think of them the same, and I see no downside here.  Using 
-ftrapv says "break my code loudly if I do something weird with 
overflow".  Using -fwrapv says "make my code work if I do something 
weird with overflow, even at expense of speed".  I think treating these 
as applying to both pointers and integers makes sense.  If someone wants 
to create -fwrapv=int and -fwrapv=pointer options, that's fine with me, 
but I don't think we need to do it now.

* I think Richard's right that this is a new feature for 4.2, which 
means we would have to bend the rules to put it in.  But, the patch 
seems very safe by its construction, since flag_strict_overflow is 
usually false.  So, I think it should be in 4.2.  Jakub, Joseph, what 
say you?

In any case, this is OK for mainline and 4.3.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

Mark Mitchell <mark@codesourcery.com> writes:

> * I think Richard's right that this is a new feature for 4.2, which
> means we would have to bend the rules to put it in.  But, the patch
> seems very safe by its construction, since flag_strict_overflow is
> usually false.  So, I think it should be in 4.2.  Jakub, Joseph, what
> say you?

Note that flag_strict_overflow defaults to true when using -O2.

Ian

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Mark Mitchell]

Ian Lance Taylor wrote:
> Mark Mitchell <mark@codesourcery.com> writes:
> 
>> * I think Richard's right that this is a new feature for 4.2, which
>> means we would have to bend the rules to put it in.  But, the patch
>> seems very safe by its construction, since flag_strict_overflow is
>> usually false.  So, I think it should be in 4.2.  Jakub, Joseph, what
>> say you?
> 
> Note that flag_strict_overflow defaults to true when using -O2.

Excellent point.

That does up the risk a bit, since it means we are more likely to take 
the new code paths.  You're making optimization more aggressive, if I 
understand correctly, in that we'll now optimize based on pointers not 
overflowing.  I agree with Richard that in this case we should avoid 
this for 4.2; it seems like we might break existing programs in the 
field and it would be better to do that in 4.3.1, than on the 4.2 branch.

I still think putting it on the 4.3 branch is a good idea.  I can't 
justify that algorithmically; that's a stable branch too.  But, it's a 
lot newer, so I think it makes more sense there.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Jakub Jelinek]

On Tue, Apr 08, 2008 at 11:46:22AM -0700, Mark Mitchell wrote:
> As to the other issues:
> 
> * I think -fwrapv/-ftrapv should affect pointers as well as integers. 
> Many people think of them the same, and I see no downside here.  Using 
> -ftrapv says "break my code loudly if I do something weird with 
> overflow".  Using -fwrapv says "make my code work if I do something 
> weird with overflow, even at expense of speed".  I think treating these 
> as applying to both pointers and integers makes sense.  If someone wants 
> to create -fwrapv=int and -fwrapv=pointer options, that's fine with me, 
> but I don't think we need to do it now.
> 
> * I think Richard's right that this is a new feature for 4.2, which 
> means we would have to bend the rules to put it in.  But, the patch 
> seems very safe by its construction, since flag_strict_overflow is 
> usually false.  So, I think it should be in 4.2.  Jakub, Joseph, what 
> say you?

I agree, both that it is a good idea to make -fwrapv/-ftrapv affect
even pointer arithmetics and that we want this in 4.3 and maybe even
in 4.2.  I'd just be interested to know how many warnings it will generate
on a few popular packages to see whether the warnings might be actually
useful for security investigators or whether there will be just too many
to make them useless.

	Jakub

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Richard Guenther]

On Tue, Apr 8, 2008 at 8:46 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Ian Lance Taylor wrote:
>
> > I'm testing this patch as a response to
> >    http://www.kb.cert.org/vuls/id/162289
> >
>
>  Thank you for working on this; comments below.
>
>  First, I agree with you that this is a feature we want.  There is a class
> of users that do really need these kinds of warnings.  They have lots of
> code, used in environments where reliability requirements are high, and they
> want every warning they can get.  They are willing to modify their code base
> if necessary to avoid warnings -- even spurious ones.  I've worked with
> people who ran code through multiple compilers -- just to see if they could
> get more warnings.
>
>  And, clearly, from a PR point of view, this helps with the CERT issue.
>
>
> > +This option also allows the compiler to assume strict pointer
> > +semantics: given a pointer to an object, if adding an offset to that
> > +pointer does not produce a pointer to the same object, the addition is
> > +undefined.  This permits the compiler to conclude that @code{p + i >
> > +p} is always true for a pointer @code{p} and integer @code{i}.  This
> > +assumption is only valid if pointer wraparound is undefined, as the
> > +expression is false if @code{p + i} overflows.
> >
>
>  This is only true for non-negative integers "i".  I'm sure that's what the
> compiler does, but I think the documentation should say that.  I also think
> you say when pointer wraparound is undefined; otherwise, the reader doesn't
> know whether the condition in the last sentence applies.

Note that with POINTER_PLUS the offset argument is always unsigned for
all languages but Ada where it is always signed (it is of type sizetype which
has some weird semantics).  But in the optimization spcifically all offsets
are interpreted as signed values.  Note also that the case just before
the patched place also needs adjustment to handle the ptr + CST > ptr
case:

          /* We can fold this expression to a constant if the non-constant
             offset parts are equal.  */
          if (offset0 == offset1
              || (offset0 && offset1
...

>  As to the other issues:
>
>  * I think -fwrapv/-ftrapv should affect pointers as well as integers. Many
> people think of them the same, and I see no downside here.  Using -ftrapv
> says "break my code loudly if I do something weird with overflow".  Using
> -fwrapv says "make my code work if I do something weird with overflow, even
> at expense of speed".  I think treating these as applying to both pointers
> and integers makes sense.  If someone wants to create -fwrapv=int and
> -fwrapv=pointer options, that's fine with me, but I don't think we need to
> do it now.

Well, we try to educate people that -fwrapv affects singed ints and not
unsigned ints (and that overflow of signed ints is undefined).  Now we mix
this (difficult for some people) concept with pointers which technically
do not have a sign.  IMHO this adds to the confusion, at least the documentation
needs to be adjusted as now -fwrapv officially doesn't only affect
signed overflow.

The original code Ian mentions just looked at the sign of the pointer (which
was bogus anyway), so this argument is moot IMHO.

>  * I think Richard's right that this is a new feature for 4.2, which means
> we would have to bend the rules to put it in.  But, the patch seems very
> safe by its construction, since flag_strict_overflow is usually false.  So,
> I think it should be in 4.2.  Jakub, Joseph, what say you?
>
>  In any case, this is OK for mainline and 4.3.

I agree with Jakub that I'd like to see some statistics on the amount of (false)
positives this generates.

Thanks,
Richard.

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

Mark Mitchell <mark@codesourcery.com> writes:

> Ian Lance Taylor wrote:
>> Mark Mitchell <mark@codesourcery.com> writes:
>>
>>> * I think Richard's right that this is a new feature for 4.2, which
>>> means we would have to bend the rules to put it in.  But, the patch
>>> seems very safe by its construction, since flag_strict_overflow is
>>> usually false.  So, I think it should be in 4.2.  Jakub, Joseph, what
>>> say you?
>>
>> Note that flag_strict_overflow defaults to true when using -O2.
>
> Excellent point.
>
> That does up the risk a bit, since it means we are more likely to take
> the new code paths.  You're making optimization more aggressive, if I
> understand correctly, in that we'll now optimize based on pointers not
> overflowing.  I agree with Richard that in this case we should avoid
> this for 4.2; it seems like we might break existing programs in the
> field and it would be better to do that in 4.3.1, than on the 4.2
> branch.

No, I am making the optimization less aggressive.

In gcc 4.2, the optimization currently fires if -fno-wrapv and
-fno-trapv.  When I backport my patch to gcc 4.2, the optimization
will additionally require -fstrict-overflow.  That is, in current gcc
4.2, the optimization fires even at -O0 (in general--of course the
folder is called more often when optimizing).  When my patch is
backported, it will only fire when -fstrict-overflow is used, which
means in typical cases it will only fire at -O2.

In gcc 4.3 and 4.4, the optimization currently fires unconditionally.
With my current patch, it only fires if -fno-wrapv, and -fno-trapv,
and -fstrict-overflow.

I believe my patch is quite safe in the sense that it will optimize
less often.  However, it will cause more warnings when using
-Wstrict-overflow=3 or higher (not the default), and of course some
people might see the lack of optimization at -O1 as a regression.

Ian

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> I agree, both that it is a good idea to make -fwrapv/-ftrapv affect
> even pointer arithmetics and that we want this in 4.3 and maybe even
> in 4.2.  I'd just be interested to know how many warnings it will generate
> on a few popular packages to see whether the warnings might be actually
> useful for security investigators or whether there will be just too many
> to make them useless.

I ran the new compiler with -Wstrict-overflow=3 over an old set of cc1
.i files.  I got 95 warnings.  Of those, exactly one was the new
pointer wraparound warning.

../../trunk/gcc/sched-vis.c:49: warning: assuming pointer wraparound does not occur when comparing P +- C1 with P +- C2

It was for the "if (cur > end)" line in this function:

static char *
safe_concat (char *buf, char *cur, const char *str)
{
  char *end = buf + BUF_LEN - 2;	/* Leave room for null.  */
  int c;

  if (cur > end)
    {
      *end = '\0';
      return end;
    }


It is happening because, of course, this is being inlined, and cur is
based on buf.  This is a false positive.  It's easy to avoid by
writing the code as, e.g.,:
  if (cur - buf > BUF_LEN - 2)


For an aggressive warning like -Wstrict-overflow=3, I think that one
false positive on all of cc1 is not too bad.  Especially when you
consider that this is one false positive amidst 94 existing false
positives.

Ian

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Richard Guenther]

On Tue, Apr 8, 2008 at 10:56 PM, Ian Lance Taylor <iant@google.com> wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
>
>  > I agree, both that it is a good idea to make -fwrapv/-ftrapv affect
>  > even pointer arithmetics and that we want this in 4.3 and maybe even
>  > in 4.2.  I'd just be interested to know how many warnings it will generate
>  > on a few popular packages to see whether the warnings might be actually
>  > useful for security investigators or whether there will be just too many
>  > to make them useless.
>
>  I ran the new compiler with -Wstrict-overflow=3 over an old set of cc1
>  .i files.  I got 95 warnings.  Of those, exactly one was the new
>  pointer wraparound warning.
>
>  ../../trunk/gcc/sched-vis.c:49: warning: assuming pointer wraparound does not occur when comparing P +- C1 with P +- C2
>
>  It was for the "if (cur > end)" line in this function:
>
>  static char *
>  safe_concat (char *buf, char *cur, const char *str)
>  {
>   char *end = buf + BUF_LEN - 2;        /* Leave room for null.  */
>   int c;
>
>   if (cur > end)
>     {
>       *end = '\0';
>       return end;
>     }
>
>
>  It is happening because, of course, this is being inlined, and cur is
>  based on buf.  This is a false positive.  It's easy to avoid by
>  writing the code as, e.g.,:
>   if (cur - buf > BUF_LEN - 2)
>
>
>  For an aggressive warning like -Wstrict-overflow=3, I think that one
>  false positive on all of cc1 is not too bad.  Especially when you
>  consider that this is one false positive amidst 94 existing false
>  positives.

How about some C++ code using libstdc++?  (of course #pragma system_header
may save our day here)

Richard.

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Paul Brook]

> * I think -fwrapv/-ftrapv should affect pointers as well as integers.
> Many people think of them the same, and I see no downside here.  Using
> -ftrapv says "break my code loudly if I do something weird with
> overflow".  Using -fwrapv says "make my code work if I do something
> weird with overflow, even at expense of speed".

On some targets (e.g. ARM) I believe base+offset addressing modes are 
undefined if the calculation overflows.  So to implement -fwrapv for pointers 
reliably you may need to disable these adressing modes.

Paul

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Andrew Pinski]

On Tue, Apr 8, 2008 at 12:44 PM, Paul Brook <paul@codesourcery.com> wrote:
>  On some targets (e.g. ARM) I believe base+offset addressing modes are
>  undefined if the calculation overflows.  So to implement -fwrapv for pointers
>  reliably you may need to disable these adressing modes.

HPPA for sure.  It is one of the reasons why I created pointer plus
:).  The other reason is that everyother compiler does represents
pointer arithmetic like that and not with the additional cast to a
pointer type.

Thanks,
Andrew Pinski

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Mark Mitchell]

Andrew Pinski wrote:
> On Tue, Apr 8, 2008 at 12:44 PM, Paul Brook <paul@codesourcery.com> wrote:
>>  On some targets (e.g. ARM) I believe base+offset addressing modes are
>>  undefined if the calculation overflows.  So to implement -fwrapv for pointers
>>  reliably you may need to disable these adressing modes.
> 
> HPPA for sure. 

That seems like a pretty compelling argument.  Wrapping semantics are 
pretty easy for integers; everyone's hardware works that way.  If 
wrapping semantics are hard to implement for pointers, then maybe we 
shouldn't try to make -fwrapv work there.  In that case, the manual 
might be improved by saying "... integers (but not pointers)" so that we 
know that it really means it.  We might even have a note that mentions 
these kinds of hardware platforms.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Richard Guenther]

On Tue, Apr 8, 2008 at 11:42 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Andrew Pinski wrote:
>
> > On Tue, Apr 8, 2008 at 12:44 PM, Paul Brook <paul@codesourcery.com> wrote:
> >
> > >  On some targets (e.g. ARM) I believe base+offset addressing modes are
> > >  undefined if the calculation overflows.  So to implement -fwrapv for
> pointers
> > >  reliably you may need to disable these adressing modes.
> > >
> >
> > HPPA for sure.
> >
>
>  That seems like a pretty compelling argument.  Wrapping semantics are
> pretty easy for integers; everyone's hardware works that way.  If wrapping
> semantics are hard to implement for pointers, then maybe we shouldn't try to
> make -fwrapv work there.  In that case, the manual might be improved by
> saying "... integers (but not pointers)" so that we know that it really
> means it.  We might even have a note that mentions these kinds of hardware
> platforms.

Of course addressing mode issues only come into play if we use them for
address computation (like with lea on x86) - do we in this case?

Richard.

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

"Richard Guenther" <richard.guenther@gmail.com> writes:

> How about some C++ code using libstdc++?  (of course #pragma system_header
> may save our day here)

I built libstdc++ itself, and got two warnings with
-Wstrict-overflow=3, neither of which were a pointer wraparound
warning.

Ian

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Mark Mitchell]

Richard Guenther wrote:

>>>>  On some targets (e.g. ARM) I believe base+offset addressing modes are
>>>>  undefined if the calculation overflows. 
> 
> Of course addressing mode issues only come into play if we use them for
> address computation (like with lea on x86) - do we in this case?

They also come into play with code like:

   char *p = (char *) 0xffffffff;
   /* I am so clever.  This is just like loading from 0x2!  */
   char c = p[3];

With -fwrapv, as I'd suggested its semantics be, I think this would be 
defined code.  But, Paul is saying that would be hard to implement.  So, 
it seems bad to make -fwrapv imply something that we have a hard time 
implementing...

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Mark Mitchell]

Ian Lance Taylor wrote:

>> That does up the risk a bit, since it means we are more likely to take
>> the new code paths.  You're making optimization more aggressive, if I
>> understand correctly, in that we'll now optimize based on pointers not
>> overflowing.  I agree with Richard that in this case we should avoid
>> this for 4.2; it seems like we might break existing programs in the
>> field and it would be better to do that in 4.3.1, than on the 4.2
>> branch.
> 
> No, I am making the optimization less aggressive.

All right; I'm sorry to be dense.

I'm going to go back to plan A: ask others their opinion.  I think 
Richard is opposed to this in 4.2.  I'm mildly in favor.  Jakub, Joseph?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Joseph S. Myers]

On Tue, 8 Apr 2008, Mark Mitchell wrote:

> * I think -fwrapv/-ftrapv should affect pointers as well as integers. Many
> people think of them the same, and I see no downside here.  Using -ftrapv says
> "break my code loudly if I do something weird with overflow".  Using -fwrapv
> says "make my code work if I do something weird with overflow, even at expense
> of speed".  I think treating these as applying to both pointers and integers
> makes sense.  If someone wants to create -fwrapv=int and -fwrapv=pointer
> options, that's fine with me, but I don't think we need to do it now.

I don't think we can define meaningful -ftrapv or -fwrapv semantics for 
pointers.

My view of desired -ftrapv semantics is described in

http://gcc.gnu.org/ml/gcc-patches/2007-01/msg02026.html
http://gcc.gnu.org/ml/gcc/2007-09/msg00399.html

and -fwrapv is simpler: where signed integer arithmetic (not shifts, but 
including built-in abs and INT_MIN / -1 - and of course INT_MIN % -1 which 
arguably we should get right even without -fwrapv) has a mathematically 
defined result outside the range of the type, the result should be reduced 
modulo to within the range of the type.  (I haven't considered in detail 
how these options should apply to complex integers, but those are an 
obscure extension not widely used anyway.)

Those semantics are meaningful at more or less the level at which the 
languages are defined in the standard.  Such semantics for pointers 
aren't; the very concept of overflow is unclear and target-dependent.  
The most useful rule for trapping would be to trap if the arithmetic goes 
outside the bounds of the object in question (allowing a pointer just past 
the end), but you can't do that without tracking what the object bounds 
are somehow.  For wrapping, I very much doubt we're fixing all cases where 
it's presumed that a pointer stays within the same object; there may be a 
case for -fnaive-pointers for certain low-level code (pointers act like 
unsigned integers, all arithmetic on them allowed no matter what object 
they end up pointing to) that would disable many other optimizations, but 
not as part of -fwrapv.

Making this particular optimization dependent on flag_strict_overflow 
seems OK with me, but -fwrapv and -ftrapv shouldn't affect pointer 
arithmetic at all.

> * I think Richard's right that this is a new feature for 4.2, which means we
> would have to bend the rules to put it in.  But, the patch seems very safe by
> its construction, since flag_strict_overflow is usually false.  So, I think it
> should be in 4.2.  Jakub, Joseph, what say you?

I think disabling the optimization unless -fstrict-overflow is probably 
safe for 4.2; other aspects of the change are riskier.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[gsan]

My sense is that distinctions like integer overflow vs. pointer
overflow are lost on the kind of people who write code like that in
the CERT advisory.  And I don't see people crying out for one option
to control integer overflow and a separate option for pointer
overflow.  But I'm happy to hear what other people have to say.
[/quote]

I totally agree with you, but sorry, why do you want to add a more or less
useless warning option ?
From a user perspective, I would think the missing "unreachable code"
warning is the problem, shouldn't I ?
Isn't it possible to trigger this warning without adding a new feature to
stable code ?

Kind Regards
Guenter

[code]
gms2 ~ # cat x1.c
int main() {
  char buffer[100];
  char *buf=buffer;
  if ( buf+10 < buf ) {
    /* unreachable code */
    return 1;
  }
  return 0;
}
gms2 ~ # gcc-4.2.3 -o x1 x1.c -Wall -O0 -Wunreachable-code
x1.c: In function 'main':
x1.c:6: warning: will never be executed


gms2 ~ # cat x2.c
int main() {
  char buffer[100];
  char *buf=buffer;
  int len = 1 <<30;
  if ( buf+len < buf ) {
    /* unreachable code */
    return 1;
  }
  return 0;
}
gms2 ~ # gcc-4.2.3 -o x2 x2.c -Wall -O0 -Wunreachable-code
gms2 ~ # gcc-4.2.3 -o x2 x2.c -Wall -O2 -Wunreachable-code
gms2 ~ # gcc-4.2.3 -o x2 x2.c -Wall -O3 -Wunreachable-code
gms2 ~ #
[/code]

-- 
View this message in context: http://www.nabble.com/PATCH-RFC%3A-Warn-about-pointer-wraparound-with--Wstrict-overflow-tp16550259p16627181.html
Sent from the gcc - patches mailing list archive at Nabble.com.

Re: PATCH RFC: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

gsan <guenter.sandner@smartstream-stp.com> writes:

> I totally agree with you, but sorry, why do you want to add a more or less
> useless warning option ?

To satisfy security conscious users.

> From a user perspective, I would think the missing "unreachable code"
> warning is the problem, shouldn't I ?

I'm not opposed to making -Wunreachable-code better, but it will give
you even more false positives, when used with inline functions.

Ian

PATCH COMMITTED: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

Ian Lance Taylor <iant@google.com> writes:

> This patch treats undefined pointer wraparound optimizations as an
> instance of undefined signed overflow optimizations (they are of
> course different, but they seem similar to users not educated in
> standardese).  You will get a warning with -Wstrict-overflow, and you
> can disable the optimization with -fno-strict-overflow.

I have committed this patch to trunk as follows.  I belive this
accomodates all comments.

Bootstrapped and tested on i686-pc-linux-gnu.

Ian


gcc/ChangeLog:
2008-04-14  Ian Lance Taylor  <iant@google.com>

	* flags.h (POINTER_TYPE_OVERFLOW_UNDEFINED): Define.
	* fold-const.c (fold_comparison): If appropriate, test
	POINTER_TYPE_OVERFLOW_UNDEFINED, and issue an overflow warning.
	(fold_binary): Test POINTER_TYPE_OVERFLOW_UNDEFINED when
	reassociating a pointer type.
	* doc/invoke.texi (Optimize Options): Document that
	-fstrict-overflow applies to pointer wraparound.

gcc/testsuite/ChangeLog:
2008-04-14  Ian Lance Taylor  <iant@google.com>

	* gcc.dg/strict-overflow-6.c: New.
	* gcc.dg/no-strict-overflow-7.c: New.
	* gcc.dg/Wstrict-overflow-22.c: New.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Warn about pointer wraparound with -Wstrict-overflow --]
[-- Type: text/x-patch, Size: 5740 bytes --]

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 134283)
+++ doc/invoke.texi	(working copy)
@@ -6161,13 +6161,22 @@ using twos complement arithmetic.  When 
 attempt to determine whether an operation on signed numbers will
 overflow must be written carefully to not actually involve overflow.
 
+This option also allows the compiler to assume strict pointer
+semantics: given a pointer to an object, if adding an offset to that
+pointer does not produce a pointer to the same object, the addition is
+undefined.  This permits the compiler to conclude that @code{p + u >
+p} is always true for a pointer @code{p} and unsigned integer
+@code{u}.  This assumption is only valid because pointer wraparound is
+undefined, as the expression is false if @code{p + u} overflows using
+twos complement arithmetic.
+
 See also the @option{-fwrapv} option.  Using @option{-fwrapv} means
-that signed overflow is fully defined: it wraps.  When
+that integer signed overflow is fully defined: it wraps.  When
 @option{-fwrapv} is used, there is no difference between
-@option{-fstrict-overflow} and @option{-fno-strict-overflow}.  With
-@option{-fwrapv} certain types of overflow are permitted.  For
-example, if the compiler gets an overflow when doing arithmetic on
-constants, the overflowed value can still be used with
+@option{-fstrict-overflow} and @option{-fno-strict-overflow} for
+integers.  With @option{-fwrapv} certain types of overflow are
+permitted.  For example, if the compiler gets an overflow when doing
+arithmetic on constants, the overflowed value can still be used with
 @option{-fwrapv}, but not otherwise.
 
 The @option{-fstrict-overflow} option is enabled at levels
Index: flags.h
===================================================================
--- flags.h	(revision 134283)
+++ flags.h	(working copy)
@@ -332,6 +332,9 @@ extern bool flag_instrument_functions_ex
 #define TYPE_OVERFLOW_TRAPS(TYPE) \
   (!TYPE_UNSIGNED (TYPE) && flag_trapv)
 
+/* True if pointer types have undefined overflow.  */
+#define POINTER_TYPE_OVERFLOW_UNDEFINED (flag_strict_overflow)
+
 /* Names for the different levels of -Wstrict-overflow=N.  The numeric
    values here correspond to N.  */
 
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 134283)
+++ fold-const.c	(working copy)
@@ -8568,7 +8568,9 @@ fold_comparison (enum tree_code code, tr
 	     because pointer arithmetic is restricted to retain within an
 	     object and overflow on pointer differences is undefined as of
 	     6.5.6/8 and /9 with respect to the signed ptrdiff_t.  */
-	  else if (bitpos0 == bitpos1)
+	  else if (bitpos0 == bitpos1
+		   && ((code == EQ_EXPR || code == NE_EXPR)
+		       || POINTER_TYPE_OVERFLOW_UNDEFINED))
 	    {
 	      tree signed_size_type_node;
 	      signed_size_type_node = signed_type_for (size_type_node);
@@ -8587,6 +8589,12 @@ fold_comparison (enum tree_code code, tr
 	      else
 		offset1 = fold_convert (signed_size_type_node, offset1);
 
+	      if (code != EQ_EXPR && code != NE_EXPR)
+		fold_overflow_warning (("assuming pointer wraparound does not "
+					"occur when comparing P +- C1 with "
+					"P +- C2"),
+				       WARN_STRICT_OVERFLOW_COMPARISON);
+
 	      return fold_build2 (code, type, offset0, offset1);
 	    }
 	}
@@ -9711,7 +9719,7 @@ fold_binary (enum tree_code code, tree t
 
 	  /* With undefined overflow we can only associate constants
 	     with one variable.  */
-	  if ((POINTER_TYPE_P (type)
+	  if (((POINTER_TYPE_P (type) && POINTER_TYPE_OVERFLOW_UNDEFINED)
 	       || (INTEGRAL_TYPE_P (type) && !TYPE_OVERFLOW_WRAPS (type)))
 	      && var0 && var1)
 	    {
Index: testsuite/gcc.dg/strict-overflow-6.c
===================================================================
--- testsuite/gcc.dg/strict-overflow-6.c	(revision 0)
+++ testsuite/gcc.dg/strict-overflow-6.c	(revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-fstrict-overflow -O2 -fdump-tree-final_cleanup" } */
+
+/* Source: Ian Lance Taylor.  Dual of no-strict-overflow-7.c.  */
+
+/* We can only simplify the conditional when using strict overflow
+   semantics.  */
+
+int
+foo (char* p)
+{
+  return p + 1000 < p;
+}
+
+/* { dg-final { scan-tree-dump-not "\[+\]\[ \]*1000" "final_cleanup" } } */
+/* { dg-final { cleanup-tree-dump "final_cleanup" } } */
Index: testsuite/gcc.dg/no-strict-overflow-7.c
===================================================================
--- testsuite/gcc.dg/no-strict-overflow-7.c	(revision 0)
+++ testsuite/gcc.dg/no-strict-overflow-7.c	(revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-strict-overflow -O2 -fdump-tree-final_cleanup" } */
+
+/* Source: Ian Lance Taylor.  Dual of strict-overflow-6.c.  */
+
+/* We can only simplify the conditional when using strict overflow
+   semantics.  */
+
+int
+foo (char* p)
+{
+  return p + 1000 < p;
+}
+
+/* { dg-final { scan-tree-dump "\[+\]\[ \]*1000" "final_cleanup" } } */
+/* { dg-final { cleanup-tree-dump "final_cleanup" } } */
Index: testsuite/gcc.dg/Wstrict-overflow-22.c
===================================================================
--- testsuite/gcc.dg/Wstrict-overflow-22.c	(revision 0)
+++ testsuite/gcc.dg/Wstrict-overflow-22.c	(revision 0)
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-fstrict-overflow -O2 -Wstrict-overflow=3" } */
+
+/* Source: Ian Lance Taylor.  Based on strict-overflow-6.c.  */
+
+/* We can only simplify the conditional when using strict overflow
+   semantics.  */
+
+int
+foo (char* p)
+{
+  return p + 1000 < p; /* { dg-warning "assuming pointer wraparound does not occur" "correct warning" } */
+}

Re: PATCH COMMITTED: Warn about pointer wraparound with -Wstrict-overflow

[Richard Guenther]

On Mon, Apr 14, 2008 at 9:20 PM, Ian Lance Taylor <iant@google.com> wrote:
> > This patch treats undefined pointer wraparound optimizations as an
>  > instance of undefined signed overflow optimizations (they are of
>  > course different, but they seem similar to users not educated in
>  > standardese).  You will get a warning with -Wstrict-overflow, and you
>  > can disable the optimization with -fno-strict-overflow.
>
>  I have committed this patch to trunk as follows.  I belive this
>  accomodates all comments.

You didn't address my comment that

          /* We can fold this expression to a constant if the non-constant
             offset parts are equal.  */
          if (offset0 == offset1
              || (offset0 && offset1
                  && operand_equal_p (offset0, offset1, 0)))

needs similar handling.  This folds ptr + CST > ptr to a constant
(also &a[4] > &a[3] or &x->y > &x->z, so I guess warning there will cause
a lot of false positive warnings).

So, did you try that?

Thanks,
Richard.

Re: PATCH COMMITTED: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

"Richard Guenther" <richard.guenther@gmail.com> writes:

> On Mon, Apr 14, 2008 at 9:20 PM, Ian Lance Taylor <iant@google.com> wrote:
>> > This patch treats undefined pointer wraparound optimizations as an
>>  > instance of undefined signed overflow optimizations (they are of
>>  > course different, but they seem similar to users not educated in
>>  > standardese).  You will get a warning with -Wstrict-overflow, and you
>>  > can disable the optimization with -fno-strict-overflow.
>>
>>  I have committed this patch to trunk as follows.  I belive this
>>  accomodates all comments.
>
> You didn't address my comment that
>
>           /* We can fold this expression to a constant if the non-constant
>              offset parts are equal.  */
>           if (offset0 == offset1
>               || (offset0 && offset1
>                   && operand_equal_p (offset0, offset1, 0)))
>
> needs similar handling.  This folds ptr + CST > ptr to a constant
> (also &a[4] > &a[3] or &x->y > &x->z, so I guess warning there will cause
> a lot of false positive warnings).
>
> So, did you try that?

Sorry, I missed that comment.  The warning I added was sufficient for
the test cases I had.  I'll give it a try.

Ian

Re: PATCH COMMITTED: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

"Richard Guenther" <richard.guenther@gmail.com> writes:

> You didn't address my comment that
>
>           /* We can fold this expression to a constant if the non-constant
>              offset parts are equal.  */
>           if (offset0 == offset1
>               || (offset0 && offset1
>                   && operand_equal_p (offset0, offset1, 0)))
>
> needs similar handling.  This folds ptr + CST > ptr to a constant
> (also &a[4] > &a[3] or &x->y > &x->z, so I guess warning there will cause
> a lot of false positive warnings).
>
> So, did you try that?

By the way, I think this condition fires much less often than you
suggest.  It only fires when offset0 and offset1 are the same.  That
will happen if you have exactly the same value on both sides of the
comparison, or if both sides are ADDR_EXPR.  It won't fire when
comparing PTR + CST1 to PTR + CST2, because that is POINTER_PLUS_EXPR
with different offsets.  It won't fire when comparing &a[4] > &a[3];
that is also POINTER_PLUS_EXPR with different offsets.  Those cases
are both handled by the next condition, where I already added a
warning.  The case you mention will fire when comparing &x->y with
&x->z, but that does not seem like a common case.

In any case I am testing a patch.

Ian

Re: PATCH COMMITTED: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

"Richard Guenther" <richard.guenther@gmail.com> writes:

> On Mon, Apr 14, 2008 at 9:20 PM, Ian Lance Taylor <iant@google.com> wrote:
>> > This patch treats undefined pointer wraparound optimizations as an
>>  > instance of undefined signed overflow optimizations (they are of
>>  > course different, but they seem similar to users not educated in
>>  > standardese).  You will get a warning with -Wstrict-overflow, and you
>>  > can disable the optimization with -fno-strict-overflow.
>>
>>  I have committed this patch to trunk as follows.  I belive this
>>  accomodates all comments.
>
> You didn't address my comment that
>
>           /* We can fold this expression to a constant if the non-constant
>              offset parts are equal.  */
>           if (offset0 == offset1
>               || (offset0 && offset1
>                   && operand_equal_p (offset0, offset1, 0)))
>
> needs similar handling.  This folds ptr + CST > ptr to a constant
> (also &a[4] > &a[3] or &x->y > &x->z, so I guess warning there will cause
> a lot of false positive warnings).
>
> So, did you try that?

I've bootstrapped and tested this patch.  I ran it over the cc1 .i
files and confirmed that no new warnings appeared.

Ian



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: More pointer wraparound warnings --]
[-- Type: text/x-patch, Size: 3553 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 134307)
+++ fold-const.c	(working copy)
@@ -8389,6 +8389,66 @@ maybe_canonicalize_comparison (enum tree
   return t;
 }
 
+/* Return whether BASE + OFFSET + BITPOS may wrap around the address
+   space.  This is used to avoid issuing overflow warnings for
+   expressions like &p->x which can not wrap.  */
+
+static bool
+pointer_may_wrap_p (tree base, tree offset, HOST_WIDE_INT bitpos)
+{
+  tree size;
+  unsigned HOST_WIDE_INT offset_low, total_low;
+  HOST_WIDE_INT offset_high, total_high;
+
+  if (!POINTER_TYPE_P (TREE_TYPE (base)))
+    return true;
+
+  if (bitpos < 0)
+    return true;
+
+  size = size_in_bytes (TREE_TYPE (TREE_TYPE (base)));
+  if (size == NULL_TREE || TREE_CODE (size) != INTEGER_CST)
+    return true;
+
+  /* We can do slightly better for SIZE if we have an ADDR_EXPR of an
+     array.  */
+  if (TREE_CODE (base) == ADDR_EXPR)
+    {
+      tree base_size = size_in_bytes (TREE_TYPE (TREE_OPERAND (base, 0)));
+      if (base_size != NULL_TREE
+	  && TREE_CODE (base_size) == INTEGER_CST
+	  && INT_CST_LT_UNSIGNED (size, base_size))
+	size = base_size;
+    }
+
+  if (offset == NULL_TREE)
+    {
+      offset_low = 0;
+      offset_high = 0;
+    }
+  else if (TREE_CODE (offset) != INTEGER_CST || TREE_OVERFLOW (offset))
+    return true;
+  else
+    {
+      offset_low = TREE_INT_CST_LOW (offset);
+      offset_high = TREE_INT_CST_HIGH (offset);
+    }
+
+  if (add_double_with_sign (offset_low, offset_high,
+			    bitpos / BITS_PER_UNIT, 0,
+			    &total_low, &total_high,
+			    true))
+    return true;
+
+  if ((unsigned HOST_WIDE_INT) total_high
+      < (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (size))
+    return false;
+  if ((unsigned HOST_WIDE_INT) total_high
+      > (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (size))
+    return true;
+  return total_low > TREE_INT_CST_LOW (size);
+}
+
 /* Subroutine of fold_binary.  This routine performs all of the
    transformations that are common to the equality/inequality
    operators (EQ_EXPR and NE_EXPR) and the ordering operators
@@ -8539,10 +8599,24 @@ fold_comparison (enum tree_code code, tr
 	{
 	  /* We can fold this expression to a constant if the non-constant
 	     offset parts are equal.  */
-	  if (offset0 == offset1
-	      || (offset0 && offset1
-		  && operand_equal_p (offset0, offset1, 0)))
-	    {
+	  if ((offset0 == offset1
+	       || (offset0 && offset1
+		   && operand_equal_p (offset0, offset1, 0)))
+	      && (code == EQ_EXPR
+		  || code == NE_EXPR
+		  || POINTER_TYPE_OVERFLOW_UNDEFINED))
+		
+	    {
+	      if (code != EQ_EXPR
+		  && code != NE_EXPR
+		  && bitpos0 != bitpos1
+		  && (pointer_may_wrap_p (base0, offset0, bitpos0)
+		      || pointer_may_wrap_p (base1, offset1, bitpos1)))
+		fold_overflow_warning (("assuming pointer wraparound does not "
+					"occur when comparing P +- C1 with "
+					"P +- C2"),
+				       WARN_STRICT_OVERFLOW_CONDITIONAL);
+
 	      switch (code)
 		{
 		case EQ_EXPR:
@@ -8588,7 +8662,10 @@ fold_comparison (enum tree_code code, tr
 	      else
 		offset1 = fold_convert (signed_size_type_node, offset1);
 
-	      if (code != EQ_EXPR && code != NE_EXPR)
+	      if (code != EQ_EXPR
+		  && code != NE_EXPR
+		  && (pointer_may_wrap_p (base0, offset0, bitpos0)
+		      || pointer_may_wrap_p (base1, offset1, bitpos1)))
 		fold_overflow_warning (("assuming pointer wraparound does not "
 					"occur when comparing P +- C1 with "
 					"P +- C2"),

Re: PATCH COMMITTED: Warn about pointer wraparound with -Wstrict-overflow

[Ian Lance Taylor]

Ian Lance Taylor <iant@google.com> writes:

> I've bootstrapped and tested this patch.  I ran it over the cc1 .i
> files and confirmed that no new warnings appeared.

Committed with this ChangeLog entry.

Ian


2008-04-18  Ian Lance Taylor  <iant@google.com>

	* fold-const.c (pointer_may_wrap_p): New static function.
	(fold_comparison): Add another test for pointer overflow.  Use
	pointer_may_wrap_p to disable some false positives.