On 07/21/2016 03:48 PM, Jeff Law wrote:
Sorry about the delay in responding...
I saw a few places in GCC itself where you increased buffer sizes. Were
any of those level 1 failures?
Yes. With optimization, even level 1 uses range information when
it's available. The idea is that when the range of values of the
variable has been constrained (possibly to avoid a sprintf buffer
overflow) but wasn't constrained enough, it's a possible sign of
a bug.
In some cases such as when the resulting range is the result of
a cast from signed to unsigned (as in cp/mangle.c) it triggers
a diagnostic that wouldn't be issued in its absence. Here's
a small example:
char d [9];
void f (long *x)
{
// warning here
__builtin_sprintf (d, "%08lx", (unsigned long)*x);
}
void g (long *x)
{
// no warning here
__builtin_sprintf (d, "%08lx", *x);
}
It's probably possible to tweak the pass to try to detect this
case and avoid the warning, but I'm not sure it's worth it. What
do you think?
In c.opt:
+C ObjC C++ ObjC++ Warning Alias (Wformat-length=, 1, 0)
Can't this take on values 0, 1, 2? And if so, is the "1" above correct?
In invoke.texi we have:
+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=2 @gol
Note Wformat-length=2.
Then in the actual documentation it says the default level is 1.
+in output truncation. GCC counts the number of bytes that each format
+string and directive within it writes into the provided buffer and, when
+it detects that more bytes that fit in the destination buffer may be
output,
+it emits a warning.
s/that fit/than fit
In that same section is the text which says the default level is 1
Good catch! The default level is meant to be 1, so the manual was
wrong. I've fixed it.
I'm curious about the heuristics for level 1. I guess if you get a
warning at level 1, then you've got a situation where we know we'll
write out of bounds. level 2 is a may write out of bounds. Which makes
me wonder if rather than levels we should use symbolic names?
I guess my worry here is that if we don't message this right folks will
make their code level 1 clean and think they're done. When in reality
all they've done is mitigate the most egregious problems.
Right. I'm not sure what the right solution is. The biggest
difference between the levels is in the values they assume
unknown integers can take on. Level one assumes 1, level two
assumes a value that results in the longest output (such as
INT_MIN). This can cause false positives but in my experience
there aren't too many of those. I would be comfortable making
level 2 the default if we had good range information. Without
it, though, even some obviously safe cases tend to be diagnosed,
like this one:
char d [4];
void g (int i)
{
if (0 <= i && i < 1000)
__builtin_sprintf (d, "%i", i);
}
warning: ‘%i’ directive writing between 1 and 11 bytes into a region
of size 4 [-Wformat-length=]
__builtin_sprintf (d, "%i", i);
^~
note: using the range [‘1’, ‘-2147483648’] for directive argument
note: format output between 2 and 12 bytes into a destination of size 4
Maybe leave level 1 the default for now and bump it up to 2 when
the new and improved VRP pass is in?
In passes.c:
+
+ // inform (0, "executing pass: %s", pass->name);
+
if (execute_one_pass (pass) && pass->sub)
- execute_pass_list_1 (pass->sub);
+ {
+ // inform (0, "executing subpass: %s", pass->sub->name);
+ execute_pass_list_1 (pass->sub);
+ }
+
The comments left over from debugging?
Yes. Someone else pointed it out in the first patch and I forgot
to remove it. It's gone now.
Looks like ~1/3 of the patch are tests. Good stuff. I'm going to
assume those are correct.
They should be for the most part. As we've discussed, I used actual
sprintf calls to validate them within GCC itself. Now that I've
removed those calls from the GCC source I need to add tests to verify
that the return value computed by the pass for the sprintf calls is
what the call actually returns. I have added a new test that does
that. I'll post an updated patch soon.
Martin
