[PATCH] enhance buffer overflow warnings (and c/53562)

[Jakub Jelinek]

On Wed, Nov 02, 2016 at 10:55:23AM -0600, Martin Sebor wrote:
> >That's an unfair assertion in light of the numbers above.
> >
> >>If you want a warning for suspicious calls, sure, but
> >>1) it has to be clearly worded significantly differently from how do you
> >>   word it, so that users really understand you are warning about
> >>   suspicious code (though, I really believe it is very common and there
> >>   is really nothing the users can do about it)
> >>2) it really shouldn't be enabled in -Wall, and I think not even in
> >>-Wextra
> >
> >As you have argued yourself recently in our discussion of
> >-Wimplicit-fallthrough, warnings that aren't enabled by either
> >of these options don't generally benefit users because very few
> >turn them on explicitly.  I agree with that argument although I
> >would be in favor of rolling out a new warning gradually over
> >two or more releases if it were known to be prone to high rates
> >of false positive. The -Wstringop-overflow warning clearly isn't
> >in that category so there's no need for it.  My offer to do
> >additional testing is still good if you'd like to see more data.

But obviously not all levels of the warning can/should be enabled
with -Wall/-Werror.  There are cases which are worth warning by default
(the case where we want to inform the user if you reach this stmt,
you'll get your program killed (will call __chk_fail)) is something
that ought like before be enabled by default; can have a warning
switch users can disable.
Then there is the case where there is a sure buffer overflow (not using
-D_FORTIFY_SOURCE, but still __bos (, 0) tells the buffer is too short,
and it is unconditional (no tricks with PHIs where one path has short
and another part has long size).  This is something that is useful
in -Wall.
The rest I'm very doubtful about even for -Wextra.

One thing is that for useful warnings users have to be able to do something
to quiet them.  For -Wmisleading-indentation you indent your code properly,
for -Wimplicit-fallthrough you add attributes or comments, etc.
But I really don't know what you want people to do to tell the compiler
the warning is a false positive.  Add asm ("" : "+g" (ptr)); to hide
everything from the compiler?  Too ugly, too unportable, pessimizing correct
code.

Another thing is that __builtin_object_size (x, 1) is very fragile thing
especially since the introduction of MEM_REF, but also various foldings etc.
Just look up the numerous open PRs about it.  We try to mitigate it in some
early optimization passes, and had to introduce an early objsz pass copy to
deal with that.  But further down the optimization passes the distinctions
that __bos (x, 1) needs to do are very often lost, optimizers fold
(char *)&s + offsetof (struct S, fld) into &s.fld and vice versa, MEM_REF
just doesn't tell you anything about what field is used, etc.  It is
unreliable in both directions.  So trying to use it for anything very late
in the GIMPLE opts is just a bad idea.

	Jakub