UX ideas for diagnostics involving ranges (was Re: [patch] convert -Walloca pass to ranger)
David Malcolm
dmalcolm@redhat.com
Mon Oct 5 15:28:09 GMT 2020
On Mon, 2020-10-05 at 11:51 +0200, Aldy Hernandez via Gcc-patches
wrote:
> The walloca pass is a mess. It has all sorts of heuristics to
> divine
> problematic ranges fed into alloca, none of them very good, and all
> of
> them unreadable. The mess therein was actually one of the original
> motivators for the ranger project (along with array bounds checking).
>
> The attached patch is a conversion of the pass to ranger. It's
> mostly
> an exercise in removing code. The entire pass almost reduces to:
>
> + // If the user specified a limit, use it.
> + int_range_max r;
> + if (warn_limit_specified_p (is_vla)
> + && TREE_CODE (len) == SSA_NAME
> + && query.range_of_expr (r, len, stmt)
> + && !r.varying_p ())
> + {
> + // The invalid bits are anything outside of [0, MAX_SIZE].
> + static int_range<2> invalid_range (build_int_cst
> (size_type_node, 0),
> + build_int_cst
> (size_type_node,
> + max_size),
> + VR_ANTI_RANGE);
> +
> + r.intersect (invalid_range);
> + if (r.undefined_p ())
> + return alloca_type_and_limit (ALLOCA_OK);
> +
> + return alloca_type_and_limit (ALLOCA_BOUND_MAYBE_LARGE,
> + wi::to_wide (integer_zero_node));
> }
>
> That is, if the range of the integer passed to alloca is outside of
> [0,MAX_SIZE], warn, otherwise it's ok. Plain and simple.
>
> You will notice I removed the nuanced errors we gave before-- like
> trying to guess whether the problematic range came by virtue of a
> signed
> cast conversion. These specific errors were never part of the
> original
> design, they were just stuff we could guess by how the IL
> looked. It
> was non-exact and fragile. Now we just say the alloca argument may
> be
> too large, period.
>
> It the future, I would even like to remove the specific range the
> ranger
> was able to compute from the error message itself. As will become
> obvious, the ranger can get pretty outrageous ranges that are
> entirely
> non-obvious by looking at the code. Peppering the error messages
> with
> these ranges will ultimately just confuse the user. But alas, that's
> a
> problem for another patch to solve.
I can't comment on the content of the patch itself, but with my "user
experience hat" on I'm wondering what diagnostic messages involving
ranges ought to look like. I worry that if we simply drop range
information altogether from the messages, we're not giving the user
enough information to make a judgment call on whether to pay attention
to the diagnostic. That said, I'm unhappy with the status quo of our
range-based messages, so I don't object to the patch.
Some possible ideas:
I added support for capturing and printing control-flow paths for
diagnostics in gcc 10; see gcc/diagnostic-path.h. I added this for the
analyzer code, but it's usable outside of it. It's possible to use
this to associate a chain of events with a diagnostic, by building a
diagnostic_path and associating it with a rich_location. Perhaps the
ranger code could have a mode which captures diagnostic_paths, where
the events in the path give pertinent information about ranges - e.g.
the various conditionals that lead to a particular range. There's a
pre-canned simple_diagnostic_path that can be populated with
simple_diagnostic_event, or you could create your own ranger-specific
event and path subclasses. These can be temporary objects that live on
the stack next to the rich_location, just for the lifetime of emitting
the diagnostic. Perhaps a "class ranger_rich_location" which
implicitly supplies an instance of "class ranger_diagnostic_path" to
the diagnostic, describing where the range information comes from?
Here are some more out-there ideas I had some years ago (I can't
remember if these ever made it to the mailing list). These ideas focus
more on how the ranges are used, rather than where the ranges come
from. There's an interaction, though, so I think it's on-topic - can
the ranger code supply this kind of information?
Consider this problematic call to sprintf:
$ cat demo.c
#include <stdio.h>
const char *test_1 (const char *msg)
{
static char buf[16];
sprintf (buf, "msg: %s\n", msg);
return buf;
}
void test_2 ()
{
test_1 ("this is long enough to cause trouble");
}
Right now, we emit this (this is trunk, plus some fixes for line-
numbering bugs):
$ ./xgcc -B. -c demo.c -Wall -O2
demo.c: In function ‘test_2’:
demo.c:6:23: warning: ‘%s’ directive writing 36 bytes into a region of
size 11 [-Wformat-overflow=]
6 | sprintf (buf, "msg: %s\n", msg);
| ^~
demo.c:12:11:
12 | test_1 ("this is long enough to cause trouble");
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
demo.c:6:3: note: ‘sprintf’ output 43 bytes into a destination of size
16
6 | sprintf (buf, "msg: %s\n", msg);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I brainstormed some ideas on making these kinds of warning easier for
the user to understand.
We could use the labeling-of-source-ranges to print something like:
demo.c: In function ‘test_2’:
demo.c:6:23: warning: ‘%s’ directive writing 36 bytes into a region of size 11 [-Wformat-overflow=]
6 | sprintf (buf, "msg: %s\n", msg);
| ~~~ ^~
| | |
| | required space: 36 bytes
| remaining capacity: 11 bytes
demo.c:12:11:
12 | test_1 ("this is long enough to cause trouble");
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| required space: 36 bytes
demo.c:6:3: note: ‘sprintf’ output 43 bytes into a destination of size 16
6 | sprintf (buf, "msg: %s\n", msg);
| ~~~ ^~~~~~~~~
| | |
| | required space: 43 bytes
| size: 16 bytes
(making a distinction between "size" and "remaining capacity",
depending on whether the code is writing to the start of the buffer or
not)
Underlining "buf" requires access to its source location, which might
not be available yet in the C frontend.
A tweak to this would be to show the point where the overflow occurs
(if the substring location code is up to it...):
demo.c: In function ‘test_2’:
demo.c:6:23: warning: ‘%s’ directive writing 36 bytes into a region of
size 11 [-Wformat-overflow=]
6 | sprintf (buf, "msg: %s\n", msg);
| ~~~ ^~
| | |
| | required space: 36 bytes
| remaining capacity: 11 bytes
demo.c:12:11:
12 | test_1 ("this is long enough to cause trouble");
| ^~~~~~~~~~~~~~~~~~~~~~~~~
| |
| overflow occurs here
demo.c:6:3: note: ‘sprintf’ output 43 bytes into a destination of size
16
6 | sprintf (buf, "msg: %s\n", msg);
| ~~~ ^~~~~~~~~
| | |
| | required space: 43 bytes
| size: 16 bytes
Getting really fancy, we could emit an ASCII art visualization to
(hopefully) make the buffer overflow crystal-clear (with an option to
disable it):
demo.c:6:3: note: buffer overflow [-fdiagnostics-show-buffer-overflow]
snprintf of "%s" from:
|+---+---+ ... +---+---+|+---+---+ ... +---+---+|
|| 0| 1| | 9| 10||| 11| 12| | 41| 42||
||'t'|'h'| |'o'|'n'|||'g'|' '| |'e'|NUL||
|+---+---+ ... +---+---+|+---+---+ ... +---+---+|
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
|<--- ok --->|<-- overflow -->|
| | |
to 'buf':
| | |
++---+---+---+---+---+|+---+---+ ... +---+---+|
|| 0| 1| 2| 3| 4||5 | 6| | 14| 15||
||'m'|'s'|'g'|':'|' '|||'t'|'h'| |'o'|'n'||
++---+---+---+---+---+|+---+---+ ... +---+---+|
| |
(thus showing the buffer content where it's known, eliding the middle
when it goes above 5 elements)
The parts on the right-hand side ("overflow" etc) could be colorized in
red.
Here's another version of the same UI idea, for the same diagnostic,
which tries to show the string data (in the name of readability; I
think this one is better than the above):
demo.c:6:3: note: buffer overflow...
snprintf of "%s" from:
|+-------------+|+---------------------------++---+|
|| 0 - 10 ||| 11 - 41 || 42||
||"this is lon"|||"g enough to cause trouble"||NUL||
|+-------------+|+---------------------------++---+|
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
|<--- ok --->|<-- overflow -->|
| | |
to 'buf':
| | |
++-------+|+-------------+|
|| 0 - 4 ||| 5 - 15 ||
||"msg: "|||"this is lon"||
++-------+|+-------------+|
Here's another possible visualization, of a different overflow:
snprintf of "%s" from:
|+---+ ... +---+|+---+ ... +---|
|| 0 | | n |||n+1| | 31|
|+---+ ... +---+|+---+ ... +---|
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
|<--- ok --->|<--overflow-->|
| | |
to 'buf':
| | |
++---+ ... +---+|+---+ ... +---+|
|| 0 | | x |||x+1| | 15||
++---+ ... +---+|+---+ ... +---+|
In this one, 32 bytes are being written into an unknown point ("x+1")
within a buffer of size 16.
A simple example where the overflowing write is to the start of the
buffer:
sprintf of an unbounded string to a fixed-size buf[100]:
demo.c:6:3: note: buffer overflow...
snprintf of "%s" from:
|+------+|+--------------++-------+|
||0...99|||100...strlen-1|| strlen||
|| ||| || NUL ||
|+------+|+--------------++-------+|
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
|<--ok-->|<-- overflow -->|
| | |
to 'buf':
| |
|+------+|
||0...99||
|+------+|
Copying a string to a buffer allocated with strlen(), rather than
strlen() + 1:
| | |
|+----------------+|+--------+|
||0...strlen() - 1|| NUL ||
|+----------------+|+--------+|
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
|<--- ok -->|<overflow>|
| | |
to 'buf':
| |
|+----------------+|
||0...strlen() - 1||
|+----------------+|
Further random UI ideas (brainstormed in Emacs; I have no idea if these
are *good* ideas):
warning: buffer overflow: writing 9-110 bytes into a buffer with capacity 80 [-Wformat-overflow=]
7 | __builtin_sprintf (buf, "/%s/%s-%i.tmp", tmpdir, fname, num);
| ~~~ ~~~^~~~~~~~~~
| | |
| | writing 9...110 bytes
| capacity: 80 bytes
note: details
7 | __builtin_sprintf (buf, "/%s/%s-%i.tmp", tmpdir, fname, num);
| ~~~ ab~cd~ef~g~~~h
| | || || || | |
| | || || || | 1 byte (NUL terminator)
| | || || || 4 bytes (".tmp")
| | || || |1...16 bytes ("%i" on 'num')
| | || || 1 byte ("-")
| | || |0-8 bytes ("%s" on 'fname')
| | || 1 byte ("/")
| | |0-79 bytes ("%s" on 'tmpdir')
| | 1 byte ("/")
| capacity: 80 bytes
note: layout [-fsome-option-enabling-this]
(with alternating colorization to better distinguish all those ranges
and labels)
|+---+--------------+----+-------------+-----+-----------+------+------+
start @||0 |1 |1-80|2-81 |10-89|11-90 |12-106|16-110|
size:|| 1| 0 - 79 | 1| 8| 1| 1-16???| 4| 1|
||"/"|%s on 'tmpdir'| "/"|%s on 'fname'| "-" |%i on 'num'|".tmp"| NUL |
|+---+--------------+----+-------------+-----+-----------+------+------+
or with a vertical orientation:
note: layout [-fsome-option-enabling-this]
+----------------+-----------+--------+---------------+
|element |starting at| size|cumulative size|
+----------------+-----------+--------+---------------+
|"/" | 0 | 1 | 1 |
|"%s" on 'tmpdir'| 1 | 0 - 79 | 1 - 80 |
|"/" | 1 - 80 | 1 | 2 - 81 |
|"%s" on 'fname' | 2 - 81 | 0 - 7 | 2 - 88 |
|"-" | 2 - 88 | 1 | 3 - 89 |
|"%i" on 'num' | 3 - 89 | 1- 16 | 4 - 105 |
|".tmp" | 4 - 105 | 4 | 8 - 109 |
|NUL terminator | 8 - 109 | 1 | 9 - 110 |
+----------------+-----------+--------+---------------+
(I've probably got some of the numbers wrong above, but hopefully you
get the idea of where I'm going with this).
Maybe some kind of highlight to show where we can exceed the buffer
capacity.
I like calling out the NUL terminator explicitly (as it's so easy to
get wrong), and putting "buffer overflow" upfront in the text of the
warning, as I did above.
[...snip...]
Hope this is constructive
Dave
More information about the Gcc-patches
mailing list