* [PATCH] Guard function->cond_uids access [PR114601]
@ 2024-04-09 11:43 Jørgen Kvalsvik
2024-04-09 11:45 ` Jørgen Kvalsvik
2024-04-09 11:47 ` Richard Biener
0 siblings, 2 replies; 3+ messages in thread
From: Jørgen Kvalsvik @ 2024-04-09 11:43 UTC (permalink / raw)
To: gcc-patches; +Cc: rguenther, hubicka, zsojka, Jørgen Kvalsvik
PR114601 shows that it is possible to reach the condition_uid lookup
without having also created the fn->cond_uids, through
compiler-generated conditionals. Consider all lookups on non-existing
maps misses, which they are from the perspective of the source code, to
avoid the NULL access.
PR gcov-profile/114601
gcc/ChangeLog:
* tree-profile.cc (condition_uid): Guard fn->cond_uids access.
gcc/testsuite/ChangeLog:
* gcc.misc-tests/gcov-pr114601.c: New test.
---
gcc/testsuite/gcc.misc-tests/gcov-pr114601.c | 11 +++++++++++
gcc/tree-profile.cc | 9 +++++++--
2 files changed, 18 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c b/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
new file mode 100644
index 00000000000..72248c8fd25
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
@@ -0,0 +1,11 @@
+/* PR gcov-profile/114601 */
+/* { dg-do compile } */
+/* { dg-options "-fcondition-coverage -finstrument-functions-once" } */
+
+/* -finstrument-functions-once inserts a hidden conditional expression into
+ this function which otherwise has none. This caused a crash on looking up
+ the condition as the cond->expr map is not created unless it necessary. */
+void
+empty (void)
+{
+}
diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index b85111624fe..b87c121790c 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -359,12 +359,17 @@ condition_index (unsigned flag)
min-max, etc., which leaves ghost identifiers in basic blocks that do not
end with a conditional jump. They are not really meaningful for condition
coverage anymore, but since coverage is unreliable under optimization anyway
- this is not a big problem. */
+ this is not a big problem.
+
+ The cond_uids map in FN cannot be expected to exist. It will only be
+ created if it is needed, and a function may have gconds even though there
+ are none in source. This can be seen in PR gcov-profile/114601, when
+ -finstrument-functions-once is used and the function has no conditions. */
unsigned
condition_uid (struct function *fn, basic_block b)
{
gimple *stmt = gsi_stmt (gsi_last_bb (b));
- if (!safe_is_a<gcond *> (stmt))
+ if (!safe_is_a <gcond*> (stmt) || !fn->cond_uids)
return 0;
unsigned *v = fn->cond_uids->get (as_a <gcond*> (stmt));
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Guard function->cond_uids access [PR114601]
2024-04-09 11:43 [PATCH] Guard function->cond_uids access [PR114601] Jørgen Kvalsvik
@ 2024-04-09 11:45 ` Jørgen Kvalsvik
2024-04-09 11:47 ` Richard Biener
1 sibling, 0 replies; 3+ messages in thread
From: Jørgen Kvalsvik @ 2024-04-09 11:45 UTC (permalink / raw)
To: gcc-patches; +Cc: rguenther, hubicka, zsojka
On 09/04/2024 13:43, Jørgen Kvalsvik wrote:
> PR114601 shows that it is possible to reach the condition_uid lookup
> without having also created the fn->cond_uids, through
> compiler-generated conditionals. Consider all lookups on non-existing
> maps misses, which they are from the perspective of the source code, to
> avoid the NULL access.
>
> PR gcov-profile/114601
>
> gcc/ChangeLog:
>
> * tree-profile.cc (condition_uid): Guard fn->cond_uids access.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.misc-tests/gcov-pr114601.c: New test.
I tested this with Zdenek's configuration on x86_64/linux, but I would
mostly consider it obvious (with the test case).
Thanks for the report, Zdenek.
> ---
> gcc/testsuite/gcc.misc-tests/gcov-pr114601.c | 11 +++++++++++
> gcc/tree-profile.cc | 9 +++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
>
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c b/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
> new file mode 100644
> index 00000000000..72248c8fd25
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
> @@ -0,0 +1,11 @@
> +/* PR gcov-profile/114601 */
> +/* { dg-do compile } */
> +/* { dg-options "-fcondition-coverage -finstrument-functions-once" } */
> +
> +/* -finstrument-functions-once inserts a hidden conditional expression into
> + this function which otherwise has none. This caused a crash on looking up
> + the condition as the cond->expr map is not created unless it necessary. */
> +void
> +empty (void)
> +{
> +}
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index b85111624fe..b87c121790c 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -359,12 +359,17 @@ condition_index (unsigned flag)
> min-max, etc., which leaves ghost identifiers in basic blocks that do not
> end with a conditional jump. They are not really meaningful for condition
> coverage anymore, but since coverage is unreliable under optimization anyway
> - this is not a big problem. */
> + this is not a big problem.
> +
> + The cond_uids map in FN cannot be expected to exist. It will only be
> + created if it is needed, and a function may have gconds even though there
> + are none in source. This can be seen in PR gcov-profile/114601, when
> + -finstrument-functions-once is used and the function has no conditions. */
> unsigned
> condition_uid (struct function *fn, basic_block b)
> {
> gimple *stmt = gsi_stmt (gsi_last_bb (b));
> - if (!safe_is_a<gcond *> (stmt))
> + if (!safe_is_a <gcond*> (stmt) || !fn->cond_uids)
> return 0;
>
> unsigned *v = fn->cond_uids->get (as_a <gcond*> (stmt));
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Guard function->cond_uids access [PR114601]
2024-04-09 11:43 [PATCH] Guard function->cond_uids access [PR114601] Jørgen Kvalsvik
2024-04-09 11:45 ` Jørgen Kvalsvik
@ 2024-04-09 11:47 ` Richard Biener
1 sibling, 0 replies; 3+ messages in thread
From: Richard Biener @ 2024-04-09 11:47 UTC (permalink / raw)
To: Jørgen Kvalsvik; +Cc: gcc-patches, hubicka, zsojka
[-- Attachment #1: Type: text/plain, Size: 2807 bytes --]
On Tue, 9 Apr 2024, Jørgen Kvalsvik wrote:
> PR114601 shows that it is possible to reach the condition_uid lookup
> without having also created the fn->cond_uids, through
> compiler-generated conditionals. Consider all lookups on non-existing
> maps misses, which they are from the perspective of the source code, to
> avoid the NULL access.
OK.
> PR gcov-profile/114601
>
> gcc/ChangeLog:
>
> * tree-profile.cc (condition_uid): Guard fn->cond_uids access.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.misc-tests/gcov-pr114601.c: New test.
> ---
> gcc/testsuite/gcc.misc-tests/gcov-pr114601.c | 11 +++++++++++
> gcc/tree-profile.cc | 9 +++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
>
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c b/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
> new file mode 100644
> index 00000000000..72248c8fd25
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr114601.c
> @@ -0,0 +1,11 @@
> +/* PR gcov-profile/114601 */
> +/* { dg-do compile } */
> +/* { dg-options "-fcondition-coverage -finstrument-functions-once" } */
> +
> +/* -finstrument-functions-once inserts a hidden conditional expression into
> + this function which otherwise has none. This caused a crash on looking up
> + the condition as the cond->expr map is not created unless it necessary. */
> +void
> +empty (void)
> +{
> +}
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index b85111624fe..b87c121790c 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -359,12 +359,17 @@ condition_index (unsigned flag)
> min-max, etc., which leaves ghost identifiers in basic blocks that do not
> end with a conditional jump. They are not really meaningful for condition
> coverage anymore, but since coverage is unreliable under optimization anyway
> - this is not a big problem. */
> + this is not a big problem.
> +
> + The cond_uids map in FN cannot be expected to exist. It will only be
> + created if it is needed, and a function may have gconds even though there
> + are none in source. This can be seen in PR gcov-profile/114601, when
> + -finstrument-functions-once is used and the function has no conditions. */
> unsigned
> condition_uid (struct function *fn, basic_block b)
> {
> gimple *stmt = gsi_stmt (gsi_last_bb (b));
> - if (!safe_is_a<gcond *> (stmt))
> + if (!safe_is_a <gcond*> (stmt) || !fn->cond_uids)
> return 0;
>
> unsigned *v = fn->cond_uids->get (as_a <gcond*> (stmt));
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-09 11:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-09 11:43 [PATCH] Guard function->cond_uids access [PR114601] Jørgen Kvalsvik
2024-04-09 11:45 ` Jørgen Kvalsvik
2024-04-09 11:47 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).