public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores
@ 2023-03-03 18:47 Alexandre Oliva
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2023-03-03 18:47 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:1e5b26d52826e7578eb841375066d71c429e2996
commit 1e5b26d52826e7578eb841375066d71c429e2996
Author: Alexandre Oliva <oliva@adacore.com>
Date: Fri Mar 3 01:47:07 2023 -0300
-Wdangling-pointer: don't mark SSA lhs sets as stores
check_dangling_stores has some weirdnesses that causes its behavior to
change when the target ABI requires C++ ctors to return this: while
scanning stmts backwards in e.g. the AS ctor on a target that returns
this in ctors, the scan first encounters a copy of this to the SSA
name used to hold the return value. m_ptr_query.get_ref resolves lhs
(the return SSA name) to the rhs (the default SSA name for this), does
not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
add it to stores, which seems to prevent later attempts to add stores
into *this from succeeding, which disables warnings that should have
triggered.
This is also the case when the backwards search finds unrelated stores
to other fields of *this before it reaches stores that IMHO should be
warned about. The store found first disables checking of other
stores, as if the store appearing later in the code would necessarily
overwrite the store that should be warned about. I've added an
xfailed variant of the existing test (struct An) that triggers this
problem, but I'm not sure how to go about fixing it.
Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
from being regarded as stores, which is enough to remove the
undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
returning this. Another variant of the existing test (struct Al) that
demonstrates the problem regardless of this aspect of the ABI, and
that gets the desired warning with the proposed patch, but not
without.
Curiously, this fix exposes yet another problem in
Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
p, not the store into possibly-overlapping *vpp2, that caused the
warning to not be issued for the store in *vpp1. I'm not sure whether
we should or should not warn in that case, but this patch adjusts the
test to reflect the behavior change.
for gcc/ChangeLog
* gimple-ssa-warn-access.cc
(pass_waccess::check_dangling_stores): Skip non-stores.
for gcc/testsuite/ChangeLog
* g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
two new variants, one fixed, one xfailed.
* c-c++-common/Wdangling-pointer-5.c
(nowarn_store_arg_store_arg): Add now-expected warnings.
Diff:
---
gcc/gimple-ssa-warn-access.cc | 3 ++-
gcc/testsuite/c-c++-common/Wdangling-pointer-5.c | 4 ++--
gcc/testsuite/g++.dg/warn/Wdangling-pointer.C | 29 +++++++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 2eab1d59abd..c0efb3fdb4e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
use the escaped locals. */
return;
- if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
+ if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
+ || !gimple_store_p (stmt))
continue;
access_ref lhs_ref;
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
index 2a165cea767..cb6da9e8639 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
@@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
{
- int x;
+ int x; // { dg-message "'x' declared here" }
void **p = (void**)sink (0);
- *vpp1 = &x; // warn here?
+ *vpp1 = &x; // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
*vpp2 = 0; // might overwrite *vpp1
return p;
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
index 22c559e4ada..a94477a6476 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
@@ -35,7 +35,34 @@ void warn_init_ref_member ()
{ }
} ai;
- sink (&as, &ai);
+ struct Al
+ {
+ const S &sref;
+ Al ():
+ // The temporary S object is destroyed when Al::Al() returns.
+ sref (S ()) // { dg-warning "storing the address" }
+ {
+ // Copying this to an SSA_NAME used to disable the warning:
+ Al *ptr = this;
+ asm ("" : "+r" (ptr));
+ }
+ } al;
+
+ struct An
+ {
+ An *next;
+ const S &sref;
+ An ():
+ next (0),
+ // The temporary S object is destroyed when An::An() returns.
+ sref (S ()) // { dg-warning "storing the address" "" { xfail *-*-* } }
+ {
+ // ??? Writing to another part of *this disables the warning:
+ next = 0;
+ }
+ } an;
+
+ sink (&as, &ai, &al, &an);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores
@ 2023-02-23 14:02 Alexandre Oliva
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2023-02-23 14:02 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:65c6196f88a1cda5dfa0f027e2be9bdf456b09c5
commit 65c6196f88a1cda5dfa0f027e2be9bdf456b09c5
Author: Alexandre Oliva <oliva@adacore.com>
Date: Thu Feb 23 11:01:18 2023 -0300
-Wdangling-pointer: don't mark SSA lhs sets as stores
check_dangling_stores has some weirdnesses that causes its behavior to
change when the target ABI requires C++ ctors to return this: while
scanning stmts backwards in e.g. the AS ctor on a target that returns
this in ctors, the scan first encounters a copy of this to the SSA
name used to hold the return value. m_ptr_query.get_ref resolves lhs
(the return SSA name) to the rhs (the default SSA name for this), does
not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
add it to stores, which seems to prevent later attempts to add stores
into *this from succeeding, which disables warnings that should have
triggered.
This is also the case when the backwards search finds unrelated stores
to other fields of *this before it reaches stores that IMHO should be
warned about. The store found first disables checking of other
stores, as if the store appearing later in the code would necessarily
overwrite the store that should be warned about. I've added an
xfailed variant of the existing test (struct An) that triggers this
problem, but I'm not sure how to go about fixing it.
Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
from being regarded as stores, which is enough to remove the
undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
returning this. Another variant of the existing test (struct Al) that
demonstrates the problem regardless of this aspect of the ABI, and
that gets the desired warning with the proposed patch, but not
without.
Curiously, this fix exposes yet another problem in
Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
p, not the store into possibly-overlapping *vpp2, that caused the
warning to not be issued for the store in *vpp1. I'm not sure whether
we should or should not warn in that case, but this patch adjusts the
test to reflect the behavior change.
for gcc/ChangeLog
* gimple-ssa-warn-access.cc
(pass_waccess::check_dangling_stores): Skip non-stores.
for gcc/testsuite/ChangeLog
* g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
two new variants, one fixed, one xfailed.
* c-c++-common/Wdangling-pointer-5.c
(nowarn_store_arg_store_arg): Add now-expected warnings.
Diff:
---
gcc/gimple-ssa-warn-access.cc | 3 ++-
gcc/testsuite/c-c++-common/Wdangling-pointer-5.c | 4 ++--
gcc/testsuite/g++.dg/warn/Wdangling-pointer.C | 29 +++++++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 2eab1d59abd..c0efb3fdb4e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
use the escaped locals. */
return;
- if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
+ if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
+ || !gimple_store_p (stmt))
continue;
access_ref lhs_ref;
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
index 2a165cea767..cb6da9e8639 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
@@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
{
- int x;
+ int x; // { dg-message "'x' declared here" }
void **p = (void**)sink (0);
- *vpp1 = &x; // warn here?
+ *vpp1 = &x; // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
*vpp2 = 0; // might overwrite *vpp1
return p;
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
index 22c559e4ada..a94477a6476 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
@@ -35,7 +35,34 @@ void warn_init_ref_member ()
{ }
} ai;
- sink (&as, &ai);
+ struct Al
+ {
+ const S &sref;
+ Al ():
+ // The temporary S object is destroyed when Al::Al() returns.
+ sref (S ()) // { dg-warning "storing the address" }
+ {
+ // Copying this to an SSA_NAME used to disable the warning:
+ Al *ptr = this;
+ asm ("" : "+r" (ptr));
+ }
+ } al;
+
+ struct An
+ {
+ An *next;
+ const S &sref;
+ An ():
+ next (0),
+ // The temporary S object is destroyed when An::An() returns.
+ sref (S ()) // { dg-warning "storing the address" "" { xfail *-*-* } }
+ {
+ // ??? Writing to another part of *this disables the warning:
+ next = 0;
+ }
+ } an;
+
+ sink (&as, &ai, &al, &an);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores
@ 2023-02-23 13:57 Alexandre Oliva
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2023-02-23 13:57 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:0959c74dcb583b8803e0591d1745dd250999d11f
commit 0959c74dcb583b8803e0591d1745dd250999d11f
Author: Alexandre Oliva <oliva@adacore.com>
Date: Thu Feb 23 10:30:51 2023 -0300
-Wdangling-pointer: don't mark SSA lhs sets as stores
check_dangling_stores has some weirdnesses that causes its behavior to
change when the target ABI requires C++ ctors to return this: while
scanning stmts backwards in e.g. the AS ctor on a target that returns
this in ctors, the scan first encounters a copy of this to the SSA
name used to hold the return value. m_ptr_query.get_ref resolves lhs
(the return SSA name) to the rhs (the default SSA name for this), does
not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
add it to stores, which seems to prevent later attempts to add stores
into *this from succeeding, which disables warnings that should have
triggered.
This is also the case when the backwards search finds unrelated stores
to other fields of *this before it reaches stores that IMHO should be
warned about. The store found first disables checking of other
stores, as if the store appearing later in the code would necessarily
overwrite the store that should be warned about. I've added an
xfailed variant of the existing test (struct An) that triggers this
problem, but I'm not sure how to go about fixing it.
Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
from being regarded as stores, which is enough to remove the
undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
returning this. Another variant of the existing test (struct Al) that
demonstrates the problem regardless of this aspect of the ABI, and
that gets the desired warning with the proposed patch, but not
without.
Curiously, this fix exposes yet another problem in
Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
p, not the store into possibly-overlapping *vpp2, that caused the
warning to not be issued for the store in *vpp1. I'm not sure whether
we should or should not warn in that case, but this patch adjusts the
test to reflect the behavior change.
for gcc/ChangeLog
* gimple-ssa-warn-access.cc
(pass_waccess::check_dangling_stores): Skip non-stores.
for gcc/testsuite/ChangeLog
* g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
two new variants, one fixed, one xfailed.
* c-c++-common/Wdangling-pointer-5.c
(nowarn_store_arg_store_arg): Add now-expected warnings.
Diff:
---
gcc/gimple-ssa-warn-access.cc | 3 ++-
gcc/testsuite/c-c++-common/Wdangling-pointer-5.c | 4 ++--
gcc/testsuite/g++.dg/warn/Wdangling-pointer.C | 29 +++++++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 2eab1d59abd..c0efb3fdb4e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
use the escaped locals. */
return;
- if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
+ if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
+ || !gimple_store_p (stmt))
continue;
access_ref lhs_ref;
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
index 2a165cea767..cb6da9e8639 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
@@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
{
- int x;
+ int x; // { dg-message "'x' declared here" }
void **p = (void**)sink (0);
- *vpp1 = &x; // warn here?
+ *vpp1 = &x; // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
*vpp2 = 0; // might overwrite *vpp1
return p;
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
index 22c559e4ada..a94477a6476 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
@@ -35,7 +35,34 @@ void warn_init_ref_member ()
{ }
} ai;
- sink (&as, &ai);
+ struct Al
+ {
+ const S &sref;
+ Al ():
+ // The temporary S object is destroyed when Al::Al() returns.
+ sref (S ()) // { dg-warning "storing the address" }
+ {
+ // Copying this to an SSA_NAME used to disable the warning:
+ Al *ptr = this;
+ asm ("" : "+r" (ptr));
+ }
+ } al;
+
+ struct An
+ {
+ An *next;
+ const S &sref;
+ An ():
+ next (0),
+ // The temporary S object is destroyed when An::An() returns.
+ sref (S ()) // { dg-warning "storing the address" "" { xfail *-*-* } }
+ {
+ // ??? Writing to another part of *this disables the warning:
+ next = 0;
+ }
+ } an;
+
+ sink (&as, &ai, &al, &an);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores
@ 2023-02-23 13:49 Alexandre Oliva
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2023-02-23 13:49 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:ecab8447f39b81fcef31926f466b244430fd2876
commit ecab8447f39b81fcef31926f466b244430fd2876
Author: Alexandre Oliva <oliva@adacore.com>
Date: Thu Feb 23 10:30:51 2023 -0300
-Wdangling-pointer: don't mark SSA lhs sets as stores
check_dangling_stores has some weirdnesses that causes its behavior to
change when the target ABI requires C++ ctors to return this: while
scanning stmts backwards in e.g. the AS ctor on a target that returns
this in ctors, the scan first encounters a copy of this to the SSA
name used to hold the return value. m_ptr_query.get_ref resolves lhs
(the return SSA name) to the rhs (the default SSA name for this), does
not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
add it to stores, which seems to prevent later attempts to add stores
into *this from succeeding, which disables warnings that should have
triggered.
This is also the case when the backwards search finds unrelated stores
to other fields of *this before it reaches stores that IMHO should be
warned about. The store found first disables checking of other
stores, as if the store appearing later in the code would necessarily
overwrite the store that should be warned about. I've added an
xfailed variant of the existing test (struct An) that triggers this
problem, but I'm not sure how to go about fixing it.
Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
from being regarded as stores, which is enough to remove the
undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
returning this. Another variant of the existing test (struct Al) that
demonstrates the problem regardless of this aspect of the ABI, and
that gets the desired warning with the proposed patch, but not
without.
Curiously, this fix exposes yet another problem in
Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
p, not the store into possibly-overlapping *vpp2, that caused the
warning to not be issued for the store in *vpp1. I'm not sure whether
we should or should not warn in that case, but this patch adjusts the
test to reflect the behavior change.
for gcc/ChangeLog
* gimple-ssa-warn-access.cc
(pass_waccess::check_dangling_stores): Skip non-stores.
for gcc/testsuite/ChangeLog
* g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
two new variants, one fixed, one xfailed.
* c-c++-common/Wdangling-pointer-5.c
(nowarn_store_arg_store_arg): Add now-expected warnings.
Diff:
---
gcc/gimple-ssa-warn-access.cc | 3 ++-
gcc/testsuite/c-c++-common/Wdangling-pointer-5.c | 4 ++--
gcc/testsuite/g++.dg/warn/Wdangling-pointer.C | 29 +++++++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 2eab1d59abd..c0efb3fdb4e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
use the escaped locals. */
return;
- if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
+ if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
+ || !gimple_store_p (stmt))
continue;
access_ref lhs_ref;
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
index 2a165cea767..cb6da9e8639 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
@@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
{
- int x;
+ int x; // { dg-message "'x' declared here" }
void **p = (void**)sink (0);
- *vpp1 = &x; // warn here?
+ *vpp1 = &x; // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
*vpp2 = 0; // might overwrite *vpp1
return p;
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
index 22c559e4ada..a94477a6476 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
@@ -35,7 +35,34 @@ void warn_init_ref_member ()
{ }
} ai;
- sink (&as, &ai);
+ struct Al
+ {
+ const S &sref;
+ Al ():
+ // The temporary S object is destroyed when Al::Al() returns.
+ sref (S ()) // { dg-warning "storing the address" }
+ {
+ // Copying this to an SSA_NAME used to disable the warning:
+ Al *ptr = this;
+ asm ("" : "+r" (ptr));
+ }
+ } al;
+
+ struct An
+ {
+ An *next;
+ const S &sref;
+ An ():
+ next (0),
+ // The temporary S object is destroyed when An::An() returns.
+ sref (S ()) // { dg-warning "storing the address" "" { xfail *-*-* } }
+ {
+ // ??? Writing to another part of *this disables the warning:
+ next = 0;
+ }
+ } an;
+
+ sink (&as, &ai, &al, &an);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores
@ 2023-02-23 13:26 Alexandre Oliva
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2023-02-23 13:26 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:447d95265dfa0aebed75ce7e32906f80fdf4cc73
commit 447d95265dfa0aebed75ce7e32906f80fdf4cc73
Author: Alexandre Oliva <oliva@adacore.com>
Date: Thu Feb 16 06:52:30 2023 -0300
-Wdangling-pointer: don't mark SSA lhs sets as stores
check_dangling_stores has some weirdnesses that causes its behavior to
change when the target ABI requires C++ ctors to return this: while
scanning stmts backwards in e.g. the AS ctor on a target that returns
this in ctors, the scan first encounters a copy of this to the SSA
name used to hold the return value. m_ptr_query.get_ref resolves lhs
(the return SSA name) to the rhs (the default SSA name for this), does
not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
add it to stores, which seems to prevent later attempts to add stores
into *this from succeeding, which disables warnings that should have
triggered.
This is also the case when the backwards search finds unrelated stores
to other fields of *this before it reaches stores that IMHO should be
warned about. The store found first disables checking of other
stores, as if the store appearing later in the code would necessarily
overwrite the store that should be warned about. I've added an
xfailed variant of the existing test (struct An) that triggers this
problem, but I'm not sure how to go about fixing it.
Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
from being regarded as stores, which is enough to remove the
undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
returning this. Another variant of the existing test (struct Al) that
demonstrates the problem regardless of this aspect of the ABI, and
that gets the desired warning with the proposed patch, but not
without.
Curiously, this fix exposes yet another problem in
Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
p, not the store into possibly-overlapping *vpp2, that caused the
warning to not be issued for the store in *vpp1. I'm not sure whether
we should or should not warn in that case, but this patch adjusts the
test to reflect the behavior change.
for gcc/ChangeLog
* gimple-ssa-warn-access.cc
(pass_waccess::check_dangling_stores): Skip non-stores.
for gcc/testsuite/ChangeLog
* g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
two new variants, one fixed, one xfailed.
* c-c++-common/Wdangling-pointer-5.c
(nowarn_store_arg_store_arg): Add now-expected warnings.
Diff:
---
gcc/gimple-ssa-warn-access.cc | 3 ++-
gcc/testsuite/c-c++-common/Wdangling-pointer-5.c | 4 ++--
gcc/testsuite/g++.dg/warn/Wdangling-pointer.C | 29 +++++++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 2eab1d59abd..c0efb3fdb4e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
use the escaped locals. */
return;
- if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
+ if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
+ || !gimple_store_p (stmt))
continue;
access_ref lhs_ref;
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
index 2a165cea767..cb6da9e8639 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
@@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
{
- int x;
+ int x; // { dg-message "'x' declared here" }
void **p = (void**)sink (0);
- *vpp1 = &x; // warn here?
+ *vpp1 = &x; // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
*vpp2 = 0; // might overwrite *vpp1
return p;
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
index 22c559e4ada..a94477a6476 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
@@ -35,7 +35,34 @@ void warn_init_ref_member ()
{ }
} ai;
- sink (&as, &ai);
+ struct Al
+ {
+ const S &sref;
+ Al ():
+ // The temporary S object is destroyed when Al::Al() returns.
+ sref (S ()) // { dg-warning "storing the address" }
+ {
+ // Copying this to an SSA_NAME used to disable the warning:
+ Al *ptr = this;
+ asm ("" : "+r" (ptr));
+ }
+ } al;
+
+ struct An
+ {
+ An *next;
+ const S &sref;
+ An ():
+ next (0),
+ // The temporary S object is destroyed when An::An() returns.
+ sref (S ()) // { dg-warning "storing the address" "" { xfail *-*-* } }
+ {
+ // ??? Writing to another part of *this disables the warning:
+ next = 0;
+ }
+ } an;
+
+ sink (&as, &ai, &al, &an);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores
@ 2023-02-16 11:13 Alexandre Oliva
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2023-02-16 11:13 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:47c2eab7e31f6dc211f970697174125a1b7af9ba
commit 47c2eab7e31f6dc211f970697174125a1b7af9ba
Author: Alexandre Oliva <oliva@adacore.com>
Date: Thu Feb 16 06:52:30 2023 -0300
-Wdangling-pointer: don't mark SSA lhs sets as stores
check_dangling_stores has some weirdnesses that causes its behavior to
change when the target ABI requires C++ ctors to return this: while
scanning stmts backwards in e.g. the AS ctor on a target that returns
this in ctors, the scan first encounters a copy of this to the SSA
name used to hold the return value. m_ptr_query.get_ref resolves lhs
(the return SSA name) to the rhs (the default SSA name for this), does
not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
add it to stores, which seems to prevent later attempts to add stores
into *this from succeeding, which disables warnings that should have
triggered.
This is also the case when the backwards search finds unrelated stores
to other fields of *this before it reaches stores that IMHO should be
warned about. The store found first disables checking of other
stores, as if the store appearing later in the code would necessarily
overwrite the store that should be warned about. I've added an
xfailed variant of the existing test (struct An) that triggers this
problem, but I'm not sure how to go about fixing it.
Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
from being regarded as stores, which is enough to remove the
undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
returning this. Another variant of the existing test (struct Al) that
demonstrates the problem regardless of this aspect of the ABI, and
that gets the desired warning with the proposed patch, but not
without.
Curiously, this fix exposes yet another problem in
Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
p, not the store into possibly-overlapping *vpp2, that caused the
warning to not be issued for the store in *vpp1. I'm not sure whether
we should or should not warn in that case, but this patch adjusts the
test to reflect the behavior change.
for gcc/ChangeLog
* gimple-ssa-warn-access.cc
(pass_waccess::check_dangling_stores): Skip non-stores.
for gcc/testsuite/ChangeLog
* g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
two new variants, one fixed, one xfailed.
* c-c++-common/Wdangling-pointer-5.c
(nowarn_store_arg_store_arg): Add now-expected warnings.
Diff:
---
gcc/gimple-ssa-warn-access.cc | 3 ++-
gcc/testsuite/c-c++-common/Wdangling-pointer-5.c | 4 ++--
gcc/testsuite/g++.dg/warn/Wdangling-pointer.C | 29 +++++++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 2eab1d59abd..c0efb3fdb4e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
use the escaped locals. */
return;
- if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
+ if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
+ || !gimple_store_p (stmt))
continue;
access_ref lhs_ref;
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
index 2a165cea767..cb6da9e8639 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
@@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
{
- int x;
+ int x; // { dg-message "'x' declared here" }
void **p = (void**)sink (0);
- *vpp1 = &x; // warn here?
+ *vpp1 = &x; // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
*vpp2 = 0; // might overwrite *vpp1
return p;
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
index 22c559e4ada..a94477a6476 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
@@ -35,7 +35,34 @@ void warn_init_ref_member ()
{ }
} ai;
- sink (&as, &ai);
+ struct Al
+ {
+ const S &sref;
+ Al ():
+ // The temporary S object is destroyed when Al::Al() returns.
+ sref (S ()) // { dg-warning "storing the address" }
+ {
+ // Copying this to an SSA_NAME used to disable the warning:
+ Al *ptr = this;
+ asm ("" : "+r" (ptr));
+ }
+ } al;
+
+ struct An
+ {
+ An *next;
+ const S &sref;
+ An ():
+ next (0),
+ // The temporary S object is destroyed when An::An() returns.
+ sref (S ()) // { dg-warning "storing the address" "" { xfail *-*-* } }
+ {
+ // ??? Writing to another part of *this disables the warning:
+ next = 0;
+ }
+ } an;
+
+ sink (&as, &ai, &al, &an);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-03 18:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-03 18:47 [gcc(refs/users/aoliva/heads/testme)] -Wdangling-pointer: don't mark SSA lhs sets as stores Alexandre Oliva
-- strict thread matches above, loose matches on Subject: below --
2023-02-23 14:02 Alexandre Oliva
2023-02-23 13:57 Alexandre Oliva
2023-02-23 13:49 Alexandre Oliva
2023-02-23 13:26 Alexandre Oliva
2023-02-16 11:13 Alexandre Oliva
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).