Message ID | Yh5o8gZ3FoavGg20@tucnak |
---|---|
State | New |
Headers | show |
Series | warn-access: Fix up check_pointer_uses [PR104715] | expand |
On 3/1/22 11:41, Jakub Jelinek wrote: > Hi! > > The following testcase emits bogus -Wdangling-pointer warnings. > The bug is that when it sees that ptr immediate use is a call that > returns one of its arguments, it will assume that the return value > is based on ptr, but that is the case only if ptr is passed to the > argument that is actually returned (so e.g. for memcpy the first argument, > etc.). When the builtins guarantee e.g. that the result is based on the > first argument (either ERF_RETURNS_ARG 0 in which case it will always > just returns the first argument as is, or when it is something like > strstr or strpbrk or mempcpy that it returns some pointer based on the > first argument), it means the result is not based on second or following > argument if any. The second hunk fixes this. > > The first hunk just removes an unnecessary TREE_CODE check, the code only > pushes SSA_NAMEs into the pointers vector and if it didn't, it uses > FOR_EACH_IMM_USE_FAST (use_p, iter, ptr) > a few lines below this, which of course requires that ptr is a SSA_NAME. > Tree checking on SSA_NAME_VERSION will already ensure that if it wasn't > a SSA_NAME, we'd ICE. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Thanks for the fix. It makes sense to me. Besides the test for the false positives I would suggest to add one to verify that using the first argument to a strstr() call is diagnosed if it's dangling (both as is, as well as with an offset from the first element). There are tests for memchr and strchr in the -Wdangling-pointer test suite but none for strstr. Martin > > 2022-03-01 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/104715 > * gimple-ssa-warn-access.cc (pass_waccess::check_pointer_uses): Don't > unnecessarily test if ptr is a SSA_NAME, it has to be. Only push lhs > of a call if gimple_call_return_arg is equal to ptr, not just when it > is non-NULL. > > * c-c++-common/Wdangling-pointer-7.c: New test. > > --- gcc/gimple-ssa-warn-access.cc.jj 2022-02-28 16:22:40.860520930 +0100 > +++ gcc/gimple-ssa-warn-access.cc 2022-02-28 16:55:01.242272499 +0100 > @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple > for (unsigned i = 0; i != pointers.length (); ++i) > { > tree ptr = pointers[i]; > - if (TREE_CODE (ptr) == SSA_NAME > - && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) > + if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) > /* Avoid revisiting the same pointer. */ > continue; > > @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple > > if (gcall *call = dyn_cast <gcall *>(use_stmt)) > { > - if (gimple_call_return_arg (call)) > + if (gimple_call_return_arg (call) == ptr) > if (tree lhs = gimple_call_lhs (call)) > if (TREE_CODE (lhs) == SSA_NAME) > pointers.safe_push (lhs); > --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj 2022-02-28 17:09:09.906355082 +0100 > +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c 2022-02-28 17:03:50.533839892 +0100 > @@ -0,0 +1,36 @@ > +/* PR tree-optimization/104715 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wdangling-pointer" } */ > + > +char * > +foo (char *p) > +{ > + { > + char q[61] = "012345678901234567890123456789012345678901234567890123456789"; > + char *r = q; > + p = __builtin_strcat (p, r); > + } > + return p; /* { dg-bogus "using dangling pointer" } */ > +} > + > +char * > +bar (char *p) > +{ > + { > + char q[] = "0123456789"; > + char *r = q; > + p = __builtin_strstr (p, r); > + } > + return p; /* { dg-bogus "using dangling pointer" } */ > +} > + > +char * > +baz (char *p) > +{ > + { > + char q[] = "0123456789"; > + char *r = q; > + p = __builtin_strpbrk (p, r); > + } > + return p; /* { dg-bogus "using dangling pointer" } */ > +} > > Jakub >
> Am 01.03.2022 um 20:08 schrieb Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org>: > > On 3/1/22 11:41, Jakub Jelinek wrote: >> Hi! >> The following testcase emits bogus -Wdangling-pointer warnings. >> The bug is that when it sees that ptr immediate use is a call that >> returns one of its arguments, it will assume that the return value >> is based on ptr, but that is the case only if ptr is passed to the >> argument that is actually returned (so e.g. for memcpy the first argument, >> etc.). When the builtins guarantee e.g. that the result is based on the >> first argument (either ERF_RETURNS_ARG 0 in which case it will always >> just returns the first argument as is, or when it is something like >> strstr or strpbrk or mempcpy that it returns some pointer based on the >> first argument), it means the result is not based on second or following >> argument if any. The second hunk fixes this. >> The first hunk just removes an unnecessary TREE_CODE check, the code only >> pushes SSA_NAMEs into the pointers vector and if it didn't, it uses >> FOR_EACH_IMM_USE_FAST (use_p, iter, ptr) >> a few lines below this, which of course requires that ptr is a SSA_NAME. >> Tree checking on SSA_NAME_VERSION will already ensure that if it wasn't >> a SSA_NAME, we'd ICE. >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Ok Richard > Thanks for the fix. It makes sense to me. Besides the test for > the false positives I would suggest to add one to verify that using > the first argument to a strstr() call is diagnosed if it's dangling > (both as is, as well as with an offset from the first element). > There are tests for memchr and strchr in the -Wdangling-pointer > test suite but none for strstr. > > Martin > >> 2022-03-01 Jakub Jelinek <jakub@redhat.com> >> PR tree-optimization/104715 >> * gimple-ssa-warn-access.cc (pass_waccess::check_pointer_uses): Don't >> unnecessarily test if ptr is a SSA_NAME, it has to be. Only push lhs >> of a call if gimple_call_return_arg is equal to ptr, not just when it >> is non-NULL. >> * c-c++-common/Wdangling-pointer-7.c: New test. >> --- gcc/gimple-ssa-warn-access.cc.jj 2022-02-28 16:22:40.860520930 +0100 >> +++ gcc/gimple-ssa-warn-access.cc 2022-02-28 16:55:01.242272499 +0100 >> @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple >> for (unsigned i = 0; i != pointers.length (); ++i) >> { >> tree ptr = pointers[i]; >> - if (TREE_CODE (ptr) == SSA_NAME >> - && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) >> + if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) >> /* Avoid revisiting the same pointer. */ >> continue; >> @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple >> if (gcall *call = dyn_cast <gcall *>(use_stmt)) >> { >> - if (gimple_call_return_arg (call)) >> + if (gimple_call_return_arg (call) == ptr) >> if (tree lhs = gimple_call_lhs (call)) >> if (TREE_CODE (lhs) == SSA_NAME) >> pointers.safe_push (lhs); >> --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj 2022-02-28 17:09:09.906355082 +0100 >> +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c 2022-02-28 17:03:50.533839892 +0100 >> @@ -0,0 +1,36 @@ >> +/* PR tree-optimization/104715 */ >> +/* { dg-do compile } */ >> +/* { dg-options "-Wdangling-pointer" } */ >> + >> +char * >> +foo (char *p) >> +{ >> + { >> + char q[61] = "012345678901234567890123456789012345678901234567890123456789"; >> + char *r = q; >> + p = __builtin_strcat (p, r); >> + } >> + return p; /* { dg-bogus "using dangling pointer" } */ >> +} >> + >> +char * >> +bar (char *p) >> +{ >> + { >> + char q[] = "0123456789"; >> + char *r = q; >> + p = __builtin_strstr (p, r); >> + } >> + return p; /* { dg-bogus "using dangling pointer" } */ >> +} >> + >> +char * >> +baz (char *p) >> +{ >> + { >> + char q[] = "0123456789"; >> + char *r = q; >> + p = __builtin_strpbrk (p, r); >> + } >> + return p; /* { dg-bogus "using dangling pointer" } */ >> +} >> Jakub >
On Tue, Mar 01, 2022 at 12:07:49PM -0700, Martin Sebor wrote: > Thanks for the fix. It makes sense to me. Besides the test for > the false positives I would suggest to add one to verify that using > the first argument to a strstr() call is diagnosed if it's dangling > (both as is, as well as with an offset from the first element). > There are tests for memchr and strchr in the -Wdangling-pointer > test suite but none for strstr. Thanks for adding that test. Note, as I wrote in the PR, I think we should handle BUILT_IN_STRPBRK like BUILT_IN_STRSTR in pass_waccess::gimple_call_return_arg, but as that would emit further warnings, I think that has to be a GCC 13 material. Jakub
--- gcc/gimple-ssa-warn-access.cc.jj 2022-02-28 16:22:40.860520930 +0100 +++ gcc/gimple-ssa-warn-access.cc 2022-02-28 16:55:01.242272499 +0100 @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple for (unsigned i = 0; i != pointers.length (); ++i) { tree ptr = pointers[i]; - if (TREE_CODE (ptr) == SSA_NAME - && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) + if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) /* Avoid revisiting the same pointer. */ continue; @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple if (gcall *call = dyn_cast <gcall *>(use_stmt)) { - if (gimple_call_return_arg (call)) + if (gimple_call_return_arg (call) == ptr) if (tree lhs = gimple_call_lhs (call)) if (TREE_CODE (lhs) == SSA_NAME) pointers.safe_push (lhs); --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj 2022-02-28 17:09:09.906355082 +0100 +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c 2022-02-28 17:03:50.533839892 +0100 @@ -0,0 +1,36 @@ +/* PR tree-optimization/104715 */ +/* { dg-do compile } */ +/* { dg-options "-Wdangling-pointer" } */ + +char * +foo (char *p) +{ + { + char q[61] = "012345678901234567890123456789012345678901234567890123456789"; + char *r = q; + p = __builtin_strcat (p, r); + } + return p; /* { dg-bogus "using dangling pointer" } */ +} + +char * +bar (char *p) +{ + { + char q[] = "0123456789"; + char *r = q; + p = __builtin_strstr (p, r); + } + return p; /* { dg-bogus "using dangling pointer" } */ +} + +char * +baz (char *p) +{ + { + char q[] = "0123456789"; + char *r = q; + p = __builtin_strpbrk (p, r); + } + return p; /* { dg-bogus "using dangling pointer" } */ +}