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.
If you have say on 32-bit target
#include <stdlib.h>
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;
}
then the loop does the same thing as will memset (p, 6, 3U * 1024 * 1024 *
1024);
do. On such large objects some operations may not work properly, e.g.
&p[i] - &p[0] might be negative etc., but that is not something the above
loop does or memset will do internally. If the loop doesn't use just 3/4 of
the address space, but much more, e.g. more than whole address space minus
one page, which is what happens in the testcase, it is indeed quite sure it
will crash if invoked, but the problem with the warning is the same with
many other late warnings or warnings excessively using VRP etc.
If we are given
void
foo (char *p, size_t len)
{
memset (p, '\0', len);
}
then it also can invoke UB if somebody calls it with a huge len
(~(size_t)0), but we rightly don't want to warn because we can't prove it
will be called with that. If some unrelated or related test somewhere else
makes the value range smaller on some path or turns it constant on some
path, the warning does warn, even when there is a sometimes low, sometimes
very high possibility the warning is on dead code.
Which is why IMHO it is a very bad idea to enable this warning by default
even without any -W* option, it just has too many false positives.
Turning extra large memcpy (as memcpy can't have overlap in between the
objects, already starting from half of address space is fine; for memset
I'd think only for something like -4096 or something similar that is just
unlikely in hosted environment to be a single object) into a trap or
unreachable might be a nice optimization and allow optimizing away code that
would follow the spot that would always crash. But with warning above it
the problem is that it is hard to figure out if it is reachable or not.
Jakub
