[PING 2][POC v2 PATCH] __builtin_warning
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01015.html
It feels to me like it's getting a little late for this but it
would still be helpful to get some feedback. Jeff, you were
very interested in this work when we discussed it offline. Do
you have any comments?
On 10/24/19 8:42 AM, Martin Sebor wrote:
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
[PING][POC v2 PATCH] __builtin_warning
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 po
Re: [POC v2 PATCH] __builtin_warning
On Tue, 15 Oct 2019, Richard Sandiford wrote: > > No. I'd expect the code generating the IR from GCC diagnostics should > > arrange for the message to be translated first (while in the case of > > __builtin_warning in user code, it would not be translated), and the code > > subsequently using the IR to use diagnostic_set_info_translated or > > functions that end up calling it without trying to translate. > > Might be a daft question, but would we ever want GCC to introduce new > calls before LTO streaming? I guess we shouldn't assume that the user > who builds the final object will be using the same locale. In that case, we'd need the IR to distinguish warnings to be translated (store original message text and arguments) and warnings from user code not to be translated. -- Joseph S. Myers jos...@codesourcery.com
Re: [POC v2 PATCH] __builtin_warning
Joseph Myers writes: > On Mon, 14 Oct 2019, Martin Sebor wrote: > >> On 10/14/19 4:03 PM, Joseph Myers wrote: >> > How does this interact with translation? >> > >> > My expectation would be that in user code, the message is taken literally >> > as-is; it is not looked up in the GCC message catalog even if it is >> > identical to some GCC diagnostic. However, when used internally for GCC >> > diagnostics, they should be translated, meaning the translation for GCC >> > diagnostics using this mechanism should occur before the built-in call is >> > stored in the IR. >> >> Right, that would make sense. Does something make you think it will >> be hard to do? > > No. I'd expect the code generating the IR from GCC diagnostics should > arrange for the message to be translated first (while in the case of > __builtin_warning in user code, it would not be translated), and the code > subsequently using the IR to use diagnostic_set_info_translated or > functions that end up calling it without trying to translate. Might be a daft question, but would we ever want GCC to introduce new calls before LTO streaming? I guess we shouldn't assume that the user who builds the final object will be using the same locale. Richard
Re: [POC v2 PATCH] __builtin_warning
On Mon, 14 Oct 2019, Martin Sebor wrote: > On 10/14/19 4:03 PM, Joseph Myers wrote: > > How does this interact with translation? > > > > My expectation would be that in user code, the message is taken literally > > as-is; it is not looked up in the GCC message catalog even if it is > > identical to some GCC diagnostic. However, when used internally for GCC > > diagnostics, they should be translated, meaning the translation for GCC > > diagnostics using this mechanism should occur before the built-in call is > > stored in the IR. > > Right, that would make sense. Does something make you think it will > be hard to do? No. I'd expect the code generating the IR from GCC diagnostics should arrange for the message to be translated first (while in the case of __builtin_warning in user code, it would not be translated), and the code subsequently using the IR to use diagnostic_set_info_translated or functions that end up calling it without trying to translate. > I suppose we'll need to make sure these strings are translated just > like for warning_at. Come to think of it, how does it happen there? > Is it just via the __gcc_*diag__ attributes? Those attributes control the checks done by the compiler used to build GCC. The naming of parameters ending in "msgid" controls extraction for translation (see ABOUT-GCC-NLS). Thus N_ and G_ provide for extraction without actually translating at the call site (if the string isn't being passed directly as a so-named parameter of a function), while _ provides for extraction with translation occurring at the call site. -- Joseph S. Myers jos...@codesourcery.com
Re: [POC v2 PATCH] __builtin_warning
On 10/14/19 4:03 PM, Joseph Myers wrote: How does this interact with translation? My expectation would be that in user code, the message is taken literally as-is; it is not looked up in the GCC message catalog even if it is identical to some GCC diagnostic. However, when used internally for GCC diagnostics, they should be translated, meaning the translation for GCC diagnostics using this mechanism should occur before the built-in call is stored in the IR. Right, that would make sense. Does something make you think it will be hard to do? I note the patch has +return gimple_build_warning (stmt, OPT_Wnonnull, +"operand null where non-null expected"); with the message not being marked for extraction for translation. I suppose we'll need to make sure these strings are translated just like for warning_at. Come to think of it, how does it happen there? Is it just via the __gcc_*diag__ attributes? Martin PS Another unfinished part is handling different execution charsets in the directive parser.
Re: [POC v2 PATCH] __builtin_warning
How does this interact with translation? My expectation would be that in user code, the message is taken literally as-is; it is not looked up in the GCC message catalog even if it is identical to some GCC diagnostic. However, when used internally for GCC diagnostics, they should be translated, meaning the translation for GCC diagnostics using this mechanism should occur before the built-in call is stored in the IR. I note the patch has > +return gimple_build_warning (stmt, OPT_Wnonnull, > +"operand null where non-null expected"); with the message not being marked for extraction for translation. -- Joseph S. Myers jos...@codesourcery.com
[POC v2 PATCH] __builtin_warning
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: po
