| Message ID | 20240409114315.356573-1-j@lambda.is |
|---|---|
| State | New |
| Headers | show |
| Series | Guard function->cond_uids access [PR114601] | expand |
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));
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)); >
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));