Other than the comments from Joseph any feedback on the patch
itself and my questions?
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01015.html
On 10/14/2019 02:41 PM, Martin Sebor wrote:
Attached is a more fleshed out version of the built-in implemented
to run in a pass of its own. I did this in anticipation of looking
at the CFG to help eliminate false positives due to ASAN
instrumentation (e.g., PR 91707).
The built-in now handles a decent number of C and GCC formatting
directives.
The patch introduces a convenience API to create calls to the built-in
(gimple_build_warning). It also avoids duplicate warnings emitted as
a result of redundant calls to the built-in for the same code (e.g.,
by different passes detecting the same out-of-bounds access).
To show how to use the built-in and the APIs within GCC the patch
modifies path isolation and CCP to inject calls to it into the CFG.
A couple of new tests exercise using the built-in from user code.
The patch triggers a number of -Wnonnull instances during bootstrap
and failures in tests that exercise the warnings modified by using
the built-in. The GCC warnings are mostly potential bugs that
I will submit patches for, but they're in general unrelated to
the built-in itself.
At this point I want to know if there is support for a) including
the built-in in GCC 10, b) the path isolation changes to make use
of it, and c) the CCP -Wnonnull changes. If (a), I will submit
a final patch in the next few weeks. If also (b) and/or (c)
I will also work on cleaning up the GCC warnings.
Martin
PS The patch introduces a general mechanism for processing vararg
formatting functions. It's only used to handle the built-in but
once it's accepted I expect to replace the gimple-ssa-printf.c
parser with it.
On 8/9/19 3:26 PM, Martin Sebor wrote:
Attached is a very rough and only superficially barely tested
prototype of the __builtin_warning intrinsic we talked about
the other day. The built-in is declared like so:
int __builtin_warning (int loc,
const char *option,
const char *txt, ...);
If it's still present during RTL expansion the built-in calls
bool ret = warning_at (LOC, find_opt (option), txt, ...);
and expands to the constant value of RET (which could be used
to issue __builtin_note but there may be better ways to deal
with those than that).
LOC is the location of the warning or zero for the location of
the built-in itself (when called by user code), OPTION is either
the name of the warning option (e.g., "-Wnonnull", when called
by user code) or the index of the option (e.g., OPT_Wnonnull,
when emitted by GCC into the IL), and TXT is the format string
to format the warning text. The rest of the arguments are not
used but I expect to extract and pass them to the diagnostic
pretty printer to format the text of the warning.
Using the built-in in user code should be obvious. To show
how it might be put to use within GCC, I added code to
gimple-ssa-isolate-paths.c to emit -Wnonnull in response to
invalid null pointer accesses. For this demo, when compiled
with the patch applied and with -Wnull-dereference (which is
not in -Wall or -Wextra), GCC issues three warnings: two
instances of -Wnull-dereference one of which is a false positive,
and one -Wnonnull (the one I added, which is included in -Wall),
which is a true positive:
int f (void)
{
char a[4] = "12";
char *p = __builtin_strlen (a) < 3 ? a : 0;
return *p;
}
int g (void)
{
char a[4] = "12";
char *p = __builtin_strlen (a) > 3 ? a : 0;
return *p;
}
In function ‘f’:
warning: potential null pointer dereference [-Wnull-dereference]
7 | return *p;
| ^~
In function ‘g’:
warning: null pointer dereference [-Wnull-dereference]
14 | return *p;
| ^~
warning: invalid use of a null pointer [-Wnonnull]
The -Wnull-dereference instance in f is a false positive because
the strlen result is clearly less than two. The strlen pass folds
the strlen result to a constant but it runs after path isolation
which will have already issued the bogus warning.
Martin
PS I tried compiling GCC with the patch. It fails in libgomp
with:
libgomp/oacc-mem.c: In function ‘gomp_acc_remove_pointer’:
cc1: warning: invalid use of a null pointer [-Wnonnull]
so clearly it's missing location information. With
-Wnull-dereference enabled we get more detail:
libgomp/oacc-mem.c: In function ‘gomp_acc_remove_pointer’:
libgomp/oacc-mem.c:1013:31: warning: potential null pointer
dereference [-Wnull-dereference]
1013 | for (size_t i = 0; i < t->list_count; i++)
| ~^~~~~~~~~~~~
libgomp/oacc-mem.c:1012:19: warning: potential null pointer
dereference [-Wnull-dereference]
1012 | t->refcount = minrefs;
| ~~~~~~~~~~~~^~~~~~~~~
libgomp/oacc-mem.c:1013:31: warning: potential null pointer
dereference [-Wnull-dereference]
1013 | for (size_t i = 0; i < t->list_count; i++)
| ~^~~~~~~~~~~~
libgomp/oacc-mem.c:1012:19: warning: potential null pointer
dereference [-Wnull-dereference]
1012 | t->refcount = minrefs;
| ~~~~~~~~~~~~^~~~~~~~~
cc1: warning: invalid use of a null pointer [-Wnonnull]
I didn't spend too long examining the code but it seems like
the warnings might actually be justified. When the first loop
terminates with t being null the subsequent dereferences are
invalid:
if (t->refcount == minrefs)
{
/* This is the last reference, so pull the descriptor off the
chain. This prevents gomp_unmap_vars via gomp_unmap_tgt from
freeing the device memory. */
struct target_mem_desc *tp;
for (tp = NULL, t = acc_dev->openacc.data_environ; t != NULL;
tp = t, t = t->prev)
{
if (n->tgt == t)
{
if (tp)
tp->prev = t->prev;
else
acc_dev->openacc.data_environ = t->prev;
break;
}
}
}
/* Set refcount to 1 to allow gomp_unmap_vars to unmap it. */
n->refcount = 1;
t->refcount = minrefs;
for (size_t i = 0; i < t->list_count; i++)
