This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]