handle "invisible" reference in -Wdangling-pointer (PR104436)
Commit Message
Transforming a by-value arguments to by-reference as GCC does for some
class types can trigger -Wdangling-pointer when the argument is used
to store the address of a local variable. Since the stored value is
not accessible in the caller the warning is a false positive.
The attached patch handles this case by excluding PARM_DECLs with
the DECL_BY_REFERENCE bit set from consideration.
While testing the patch I noticed some instances of the warning are
uninitentionally duplicated as the pass runs more than once. To avoid
that, I also introduce warning suppression into the handler for this
instance of the warning. (There might still be others.)
Tested on x86_64-linux.
Martin
Comments
On 2/8/22 16:59, Martin Sebor wrote:
> Transforming a by-value arguments to by-reference as GCC does for some
> class types can trigger -Wdangling-pointer when the argument is used
> to store the address of a local variable. Since the stored value is
> not accessible in the caller the warning is a false positive.
>
> The attached patch handles this case by excluding PARM_DECLs with
> the DECL_BY_REFERENCE bit set from consideration.
>
> While testing the patch I noticed some instances of the warning are
> uninitentionally duplicated as the pass runs more than once. To avoid
> that, I also introduce warning suppression into the handler for this
> instance of the warning. (There might still be others.)
The second test should verify that we do warn about returning 't' from a
function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
> + tree var = SSA_NAME_VAR (lhs_ref.ref);
> + if (DECL_BY_REFERENCE (var))
> + /* Avoid by-value arguments transformed into by-reference. */
> + continue;
I wonder if we can we express this property of invisiref parms somewhere
more general? I imagine optimizations would find it useful as well.
Could pointer_query somehow treat the reference as pointing to a
function-local object?
I previously tried to express this by marking the reference as
'restrict', but that was wrong
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
Jason
On Tue, Feb 8, 2022 at 11:38 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 2/8/22 16:59, Martin Sebor wrote:
> > Transforming a by-value arguments to by-reference as GCC does for some
> > class types can trigger -Wdangling-pointer when the argument is used
> > to store the address of a local variable. Since the stored value is
> > not accessible in the caller the warning is a false positive.
> >
> > The attached patch handles this case by excluding PARM_DECLs with
> > the DECL_BY_REFERENCE bit set from consideration.
> >
> > While testing the patch I noticed some instances of the warning are
> > uninitentionally duplicated as the pass runs more than once. To avoid
> > that, I also introduce warning suppression into the handler for this
> > instance of the warning. (There might still be others.)
>
> The second test should verify that we do warn about returning 't' from a
> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>
> > + tree var = SSA_NAME_VAR (lhs_ref.ref);
> > + if (DECL_BY_REFERENCE (var))
I think you need to test var && TREE_CODE (var) == PARM_DECL here since
for DECL_BY_REFERENCE RESULT_DECL we _do_ escape to the caller. Also
SSA_NAME_VAR var might be NULL.
> > + /* Avoid by-value arguments transformed into by-reference. */
> > + continue;
>
> I wonder if we can we express this property of invisiref parms somewhere
> more general? I imagine optimizations would find it useful as well.
> Could pointer_query somehow treat the reference as pointing to a
> function-local object?
I think points-to analysis got this correct when the reference was marked
restrict but now it also fails at this, making DSE fail to eliminate the
store in
struct A { A(); ~A(); int *p; };
void foo (struct A a, int *p)
{
a.p = p;
}
> I previously tried to express this by marking the reference as
> 'restrict', but that was wrong
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>
> Jason
>
On 2/9/22 03:30, Richard Biener wrote:
> On Tue, Feb 8, 2022 at 11:38 PM Jason Merrill via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 2/8/22 16:59, Martin Sebor wrote:
>>> Transforming a by-value arguments to by-reference as GCC does for some
>>> class types can trigger -Wdangling-pointer when the argument is used
>>> to store the address of a local variable. Since the stored value is
>>> not accessible in the caller the warning is a false positive.
>>>
>>> The attached patch handles this case by excluding PARM_DECLs with
>>> the DECL_BY_REFERENCE bit set from consideration.
>>>
>>> While testing the patch I noticed some instances of the warning are
>>> uninitentionally duplicated as the pass runs more than once. To avoid
>>> that, I also introduce warning suppression into the handler for this
>>> instance of the warning. (There might still be others.)
>>
>> The second test should verify that we do warn about returning 't' from a
>> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>>
>>> + tree var = SSA_NAME_VAR (lhs_ref.ref);
>>> + if (DECL_BY_REFERENCE (var))
>
> I think you need to test var && TREE_CODE (var) == PARM_DECL here since
> for DECL_BY_REFERENCE RESULT_DECL we _do_ escape to the caller. Also
> SSA_NAME_VAR var might be NULL.
>
>>> + /* Avoid by-value arguments transformed into by-reference. */
>>> + continue;
>>
>> I wonder if we can we express this property of invisiref parms somewhere
>> more general? I imagine optimizations would find it useful as well.
>> Could pointer_query somehow treat the reference as pointing to a
>> function-local object?
>
> I think points-to analysis got this correct when the reference was marked
> restrict but now it also fails at this, making DSE fail to eliminate the
> store in
>
> struct A { A(); ~A(); int *p; };
>
> void foo (struct A a, int *p)
> {
> a.p = p;
> }
Well, that's conservatively correct; since we don't know the definition
of ~A, we don't know whether it copies p somewhere, e.g.
int *global_p;
A::~A() { global_p = p; }
in which case eliminating the store would be an invalid optimization,
just as it would be if 'a' were a local variable.
>> I previously tried to express this by marking the reference as
>> 'restrict', but that was wrong
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
On 2/8/22 15:37, Jason Merrill wrote:
> On 2/8/22 16:59, Martin Sebor wrote:
>> Transforming a by-value arguments to by-reference as GCC does for some
>> class types can trigger -Wdangling-pointer when the argument is used
>> to store the address of a local variable. Since the stored value is
>> not accessible in the caller the warning is a false positive.
>>
>> The attached patch handles this case by excluding PARM_DECLs with
>> the DECL_BY_REFERENCE bit set from consideration.
>>
>> While testing the patch I noticed some instances of the warning are
>> uninitentionally duplicated as the pass runs more than once. To avoid
>> that, I also introduce warning suppression into the handler for this
>> instance of the warning. (There might still be others.)
>
> The second test should verify that we do warn about returning 't' from a
> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
The indirect aggregate case isn't handled and needs more work but
since you brought it up I thought I should look into finishing it.
The attached patch #2 adds support for it. It also incorporates
Richard's suggestion to test PARM_DECL. Patch #2 applies on top
of patch #1 which is unchanged from the first revision.
I have retested it on x86_64-linux and by building Glibc and
Binutils + GDB.
If now is too late for the aggregate enhancement I'm okay with
deferring it until stage 1.
>
>> + tree var = SSA_NAME_VAR (lhs_ref.ref);
>> + if (DECL_BY_REFERENCE (var))
>> + /* Avoid by-value arguments transformed into by-reference. */
>> + continue;
>
> I wonder if we can we express this property of invisiref parms somewhere
> more general? I imagine optimizations would find it useful as well.
> Could pointer_query somehow treat the reference as pointing to a
> function-local object?
I don't quite see where in the pointer_query class this would be
useful (the class also isn't used for optimization). I could add
a helper to the access_ref class to query this property in warning
code but as this is the only potential client I'm also not sure
that's quite what you had in mind. I'd need some guidance as to
what you're thinking of here.
Martin
>
> I previously tried to express this by marking the reference as
> 'restrict', but that was wrong
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>
> Jason
>
Pinging the two patches here:
https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590230.html
On 2/10/22 16:04, Martin Sebor wrote:
> On 2/8/22 15:37, Jason Merrill wrote:
>> On 2/8/22 16:59, Martin Sebor wrote:
>>> Transforming a by-value arguments to by-reference as GCC does for some
>>> class types can trigger -Wdangling-pointer when the argument is used
>>> to store the address of a local variable. Since the stored value is
>>> not accessible in the caller the warning is a false positive.
>>>
>>> The attached patch handles this case by excluding PARM_DECLs with
>>> the DECL_BY_REFERENCE bit set from consideration.
>>>
>>> While testing the patch I noticed some instances of the warning are
>>> uninitentionally duplicated as the pass runs more than once. To avoid
>>> that, I also introduce warning suppression into the handler for this
>>> instance of the warning. (There might still be others.)
>>
>> The second test should verify that we do warn about returning 't' from
>> a function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>
> The indirect aggregate case isn't handled and needs more work but
> since you brought it up I thought I should look into finishing it.
> The attached patch #2 adds support for it. It also incorporates
> Richard's suggestion to test PARM_DECL. Patch #2 applies on top
> of patch #1 which is unchanged from the first revision.
>
> I have retested it on x86_64-linux and by building Glibc and
> Binutils + GDB.
>
> If now is too late for the aggregate enhancement I'm okay with
> deferring it until stage 1.
>
>>
>>> + tree var = SSA_NAME_VAR (lhs_ref.ref);
>>> + if (DECL_BY_REFERENCE (var))
>>> + /* Avoid by-value arguments transformed into by-reference. */
>>> + continue;
>>
>> I wonder if we can we express this property of invisiref parms
>> somewhere more general? I imagine optimizations would find it useful
>> as well. Could pointer_query somehow treat the reference as pointing
>> to a function-local object?
>
> I don't quite see where in the pointer_query class this would be
> useful (the class also isn't used for optimization). I could add
> a helper to the access_ref class to query this property in warning
> code but as this is the only potential client I'm also not sure
> that's quite what you had in mind. I'd need some guidance as to
> what you're thinking of here.
>
> Martin
>
>
>>
>> I previously tried to express this by marking the reference as
>> 'restrict', but that was wrong
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>>
>> Jason
>>
On Fri, Feb 11, 2022 at 12:05 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 2/8/22 15:37, Jason Merrill wrote:
> > On 2/8/22 16:59, Martin Sebor wrote:
> >> Transforming a by-value arguments to by-reference as GCC does for some
> >> class types can trigger -Wdangling-pointer when the argument is used
> >> to store the address of a local variable. Since the stored value is
> >> not accessible in the caller the warning is a false positive.
> >>
> >> The attached patch handles this case by excluding PARM_DECLs with
> >> the DECL_BY_REFERENCE bit set from consideration.
> >>
> >> While testing the patch I noticed some instances of the warning are
> >> uninitentionally duplicated as the pass runs more than once. To avoid
> >> that, I also introduce warning suppression into the handler for this
> >> instance of the warning. (There might still be others.)
> >
> > The second test should verify that we do warn about returning 't' from a
> > function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>
> The indirect aggregate case isn't handled and needs more work but
> since you brought it up I thought I should look into finishing it.
> The attached patch #2 adds support for it. It also incorporates
> Richard's suggestion to test PARM_DECL. Patch #2 applies on top
> of patch #1 which is unchanged from the first revision.
patch #1 would be OK if you'd do the PARM_DECL check there - I'd
rather defer #2 to stage1.
Richard.
>
> I have retested it on x86_64-linux and by building Glibc and
> Binutils + GDB.
>
> If now is too late for the aggregate enhancement I'm okay with
> deferring it until stage 1.
>
> >
> >> + tree var = SSA_NAME_VAR (lhs_ref.ref);
> >> + if (DECL_BY_REFERENCE (var))
> >> + /* Avoid by-value arguments transformed into by-reference. */
> >> + continue;
> >
> > I wonder if we can we express this property of invisiref parms somewhere
> > more general? I imagine optimizations would find it useful as well.
> > Could pointer_query somehow treat the reference as pointing to a
> > function-local object?
>
> I don't quite see where in the pointer_query class this would be
> useful (the class also isn't used for optimization). I could add
> a helper to the access_ref class to query this property in warning
> code but as this is the only potential client I'm also not sure
> that's quite what you had in mind. I'd need some guidance as to
> what you're thinking of here.
>
> Martin
>
>
> >
> > I previously tried to express this by marking the reference as
> > 'restrict', but that was wrong
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
> >
> > Jason
> >
On 3/9/22 06:17, Richard Biener wrote:
> On Fri, Feb 11, 2022 at 12:05 AM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 2/8/22 15:37, Jason Merrill wrote:
>>> On 2/8/22 16:59, Martin Sebor wrote:
>>>> Transforming a by-value arguments to by-reference as GCC does for some
>>>> class types can trigger -Wdangling-pointer when the argument is used
>>>> to store the address of a local variable. Since the stored value is
>>>> not accessible in the caller the warning is a false positive.
>>>>
>>>> The attached patch handles this case by excluding PARM_DECLs with
>>>> the DECL_BY_REFERENCE bit set from consideration.
>>>>
>>>> While testing the patch I noticed some instances of the warning are
>>>> uninitentionally duplicated as the pass runs more than once. To avoid
>>>> that, I also introduce warning suppression into the handler for this
>>>> instance of the warning. (There might still be others.)
>>>
>>> The second test should verify that we do warn about returning 't' from a
>>> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>>
>> The indirect aggregate case isn't handled and needs more work but
>> since you brought it up I thought I should look into finishing it.
>> The attached patch #2 adds support for it. It also incorporates
>> Richard's suggestion to test PARM_DECL. Patch #2 applies on top
>> of patch #1 which is unchanged from the first revision.
>
> patch #1 would be OK if you'd do the PARM_DECL check there - I'd
> rather defer #2 to stage1.
>
> Richard.
>
>>
>> I have retested it on x86_64-linux and by building Glibc and
>> Binutils + GDB.
>>
>> If now is too late for the aggregate enhancement I'm okay with
>> deferring it until stage 1.
I committed the adjusted patch in r12-7650. I'll try to remember to
ping the second patch in stage 1.
Martin
>>
>>>
>>>> + tree var = SSA_NAME_VAR (lhs_ref.ref);
>>>> + if (DECL_BY_REFERENCE (var))
>>>> + /* Avoid by-value arguments transformed into by-reference. */
>>>> + continue;
>>>
>>> I wonder if we can we express this property of invisiref parms somewhere
>>> more general? I imagine optimizations would find it useful as well.
>>> Could pointer_query somehow treat the reference as pointing to a
>>> function-local object?
>>
>> I don't quite see where in the pointer_query class this would be
>> useful (the class also isn't used for optimization). I could add
>> a helper to the access_ref class to query this property in warning
>> code but as this is the only potential client I'm also not sure
>> that's quite what you had in mind. I'd need some guidance as to
>> what you're thinking of here.
>>
>> Martin
>>
>>
>>>
>>> I previously tried to express this by marking the reference as
>>> 'restrict', but that was wrong
>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>>>
>>> Jason
>>>
Avoid -Wdangling-pointer for by-transparent-reference arguments [PR104436].
Resolves:
PR middle-end/104436 - spurious -Wdangling-pointer assigning local address to a class passed by value
gcc/ChangeLog:
PR middle-end/104436
* gimple-ssa-warn-access.cc (pass_waccess::check_dangling_stores):
Check for warning suppression. Avoid by-value arguments transformed
into by-transparent-reference.
gcc/testsuite/ChangeLog:
PR middle-end/104436
* c-c++-common/Wdangling-pointer-7.c: New test.
* g++.dg/warn/Wdangling-pointer-4.C: New test.
@@ -4517,6 +4517,9 @@ pass_waccess::check_dangling_stores (basic_block bb,
if (!stmt)
break;
+ if (warning_suppressed_p (stmt, OPT_Wdangling_pointer_))
+ continue;
+
if (is_gimple_call (stmt)
&& !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
/* Avoid looking before nonconst, nonpure calls since those might
@@ -4542,10 +4545,16 @@ pass_waccess::check_dangling_stores (basic_block bb,
}
else if (TREE_CODE (lhs_ref.ref) == SSA_NAME)
{
- /* Avoid looking at or before stores into unknown objects. */
gimple *def_stmt = SSA_NAME_DEF_STMT (lhs_ref.ref);
if (!gimple_nop_p (def_stmt))
+ /* Avoid looking at or before stores into unknown objects. */
return;
+
+ tree var = SSA_NAME_VAR (lhs_ref.ref);
+ if (DECL_BY_REFERENCE (var))
+ /* Avoid by-value arguments transformed into by-reference. */
+ continue;
+
}
else if (TREE_CODE (lhs_ref.ref) == MEM_REF)
{
@@ -4578,6 +4587,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
"storing the address of local variable %qD in %qE",
rhs_ref.ref, lhs))
{
+ suppress_warning (stmt, OPT_Wdangling_pointer_);
+
location_t loc = DECL_SOURCE_LOCATION (rhs_ref.ref);
inform (loc, "%qD declared here", rhs_ref.ref);
new file mode 100644
@@ -0,0 +1,20 @@
+/* Verify -Wdangling-pointer is issued only once.
+ { dg-do compile }
+ { dg-options "-O -Wall" } */
+
+void *p;
+
+void escape_global_warn_once (void)
+{
+ int x[5];
+
+ p = &x[3]; // { dg-regexp "\[^\n\r\]+: warning: \[^\n\r\]+ \\\[-Wdangling-pointer.?\\\]" "message" }
+}
+
+
+void escape_param_warn_once (void **p)
+{
+ int x[5];
+
+ *p = &x[3]; // { dg-regexp "\[^\n\r\]+: warning: \[^\n\r\]+ \\\[-Wdangling-pointer.?\\\]" "message" }
+}
new file mode 100644
@@ -0,0 +1,34 @@
+/* PR middle-end/104436 - spurious -Wdangling-pointer assigning local
+ address to a class passed by value
+ { dg-do compile }
+ { dg-options "-O1 -Wall" } */
+
+struct S
+{
+ S (void *p): p (p) { }
+ S (const S &s): p (s.p) { }
+
+ void *p;
+};
+
+
+void nowarn_assign_by_value (S s)
+{
+ int i;
+ S t (&i);
+ s = t; // { dg-bogus "-Wdangling-pointer" }
+}
+
+void nowarn_assign_by_value_arg (S s)
+{
+ S t (&s);
+ s = t; // { dg-bogus "-Wdangling-pointer" }
+}
+
+
+void warn_assign_local_by_reference (S &s)
+{
+ int i;
+ S t (&i);
+ s = t; // { dg-warning "-Wdangling-pointer" }
+}