diff mbox series

C++ PATCH for c++/92106 - ICE with structured bindings and -Wreturn-local-addr

Message ID 20191015171717.GB2922@redhat.com
State New
Headers show
Series C++ PATCH for c++/92106 - ICE with structured bindings and -Wreturn-local-addr | expand

Commit Message

Marek Polacek Oct. 15, 2019, 5:17 p.m. UTC 
Here we are returning the address of a structure binding and since it's a
reference, we recursed on its initializer, but in this case there was none
and we crashed in cp_fold.  So don't recurse when we don't have an init to
recurse on.

Bootstrapped/regtested on x86_64-linux, ok for trunk/9?

2019-10-15  Marek Polacek  <polacek@redhat.com>

	PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr.
	* typeck.c (maybe_warn_about_returning_address_of_local): Avoid
	recursing on null initializer.

	* g++.dg/cpp1z/decomp50.C: New test.

Comments

Jakub Jelinek Oct. 15, 2019, 5:34 p.m. UTC | #1 
On Tue, Oct 15, 2019 at 01:17:17PM -0400, Marek Polacek wrote:
> 2019-10-15  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr.
> 	* typeck.c (maybe_warn_about_returning_address_of_local): Avoid
> 	recursing on null initializer.
> 
> 	* g++.dg/cpp1z/decomp50.C: New test.
> 
> diff --git gcc/cp/typeck.c gcc/cp/typeck.c
> index 141d86f50c9..1825540016f 100644
> --- gcc/cp/typeck.c
> +++ gcc/cp/typeck.c
> @@ -9354,10 +9354,8 @@ maybe_warn_about_returning_address_of_local (tree retval)
>  	     binding.  */
>  	  tree base = DECL_DECOMP_BASE (whats_returned);
>  	  if (TYPE_REF_P (TREE_TYPE (base)))
> -	    {
> -	      tree init = DECL_INITIAL (base);
> +	    if (tree init = DECL_INITIAL (base))
>  	      return maybe_warn_about_returning_address_of_local (init);
> -	    }

Actually, seeing the dg-warning in the testcase, I think we shouldn't warn,
for range-for it is just too hard to find out if it will be returning
address of a local or not. &value in itself is not address of a local
variable when the structured binding is a reference.
Well, in the testcase as is it actually is (perhaps just in the reduced
one and not original):
    const struct J & D.2293;
    const int name [value-expr: D.2293->name];
    const int value [value-expr: D.2293->value];
...
          struct reference D.2352;

          try
            {
              D.2352 = D<A::J*, int>::operator* (&__for_begin);
              D.2293 = &D.2352;
but a small change to the testcase:
 template <typename _Iterator, typename> class D {
 public:
-  typename B<_Iterator>::reference operator*();
+  typename B<_Iterator>::reference &operator*();
   void operator++();
 };
results in D.2293 = D<A::J*, int>::operator* (&__for_begin);
and then it might very well not be address of a local variable.

This isn't just about a false positive warning, when
maybe_warn_about_returning_address_of_local returns true, then
we actually return NULL pointer instead of the value user wanted.

So, I think you want to add and do else return false;
if init is NULL.

> +    for (const auto &[name, value] : members)
> +      return &value; // { dg-warning "address of local variable" }
> +    return nullptr;
> +  }
> +};
> +int main() {
> +  A a;
> +  a.find("");
> +}

	Jakub
Marek Polacek Oct. 15, 2019, 6:28 p.m. UTC | #2 
On Tue, Oct 15, 2019 at 07:34:31PM +0200, Jakub Jelinek wrote:
> On Tue, Oct 15, 2019 at 01:17:17PM -0400, Marek Polacek wrote:
> > 2019-10-15  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr.
> > 	* typeck.c (maybe_warn_about_returning_address_of_local): Avoid
> > 	recursing on null initializer.
> > 
> > 	* g++.dg/cpp1z/decomp50.C: New test.
> > 
> > diff --git gcc/cp/typeck.c gcc/cp/typeck.c
> > index 141d86f50c9..1825540016f 100644
> > --- gcc/cp/typeck.c
> > +++ gcc/cp/typeck.c
> > @@ -9354,10 +9354,8 @@ maybe_warn_about_returning_address_of_local (tree retval)
> >  	     binding.  */
> >  	  tree base = DECL_DECOMP_BASE (whats_returned);
> >  	  if (TYPE_REF_P (TREE_TYPE (base)))
> > -	    {
> > -	      tree init = DECL_INITIAL (base);
> > +	    if (tree init = DECL_INITIAL (base))
> >  	      return maybe_warn_about_returning_address_of_local (init);
> > -	    }
> 
> Actually, seeing the dg-warning in the testcase, I think we shouldn't warn,
> for range-for it is just too hard to find out if it will be returning
> address of a local or not. &value in itself is not address of a local
> variable when the structured binding is a reference.
> Well, in the testcase as is it actually is (perhaps just in the reduced
> one and not original):
>     const struct J & D.2293;
>     const int name [value-expr: D.2293->name];
>     const int value [value-expr: D.2293->value];
> ...
>           struct reference D.2352;
> 
>           try
>             {
>               D.2352 = D<A::J*, int>::operator* (&__for_begin);
>               D.2293 = &D.2352;
> but a small change to the testcase:
>  template <typename _Iterator, typename> class D {
>  public:
> -  typename B<_Iterator>::reference operator*();
> +  typename B<_Iterator>::reference &operator*();
>    void operator++();
>  };
> results in D.2293 = D<A::J*, int>::operator* (&__for_begin);
> and then it might very well not be address of a local variable.
> 
> This isn't just about a false positive warning, when
> maybe_warn_about_returning_address_of_local returns true, then
> we actually return NULL pointer instead of the value user wanted.
> 
> So, I think you want to add and do else return false;
> if init is NULL.

That's an interesting point, thanks.  Here's a patch with that return
added.

Bootstrapped/regtested on x86_64-linux, ok for trunk / 9?

2019-10-15  Marek Polacek  <polacek@redhat.com>

	PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr.
	* typeck.c (maybe_warn_about_returning_address_of_local): Avoid
	recursing on null initializer and return false instead.

	* g++.dg/cpp1z/decomp50.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 141d86f50c9..90bd5bb9cdc 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9355,8 +9355,10 @@ maybe_warn_about_returning_address_of_local (tree retval)
 	  tree base = DECL_DECOMP_BASE (whats_returned);
 	  if (TYPE_REF_P (TREE_TYPE (base)))
 	    {
-	      tree init = DECL_INITIAL (base);
-	      return maybe_warn_about_returning_address_of_local (init);
+	      if (tree init = DECL_INITIAL (base))
+		return maybe_warn_about_returning_address_of_local (init);
+	      else
+		return false;
 	    }
 	}
       bool w = false;
diff --git gcc/testsuite/g++.dg/cpp1z/decomp50.C gcc/testsuite/g++.dg/cpp1z/decomp50.C
new file mode 100644
index 00000000000..13b40146379
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1z/decomp50.C
@@ -0,0 +1,51 @@
+// PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr. 
+// { dg-do compile { target c++17 } }
+
+template <typename> struct B;
+template <typename _Tp> struct B<_Tp *> { typedef _Tp reference; };
+struct C {
+  template <typename _Up> using rebind = _Up *;
+};
+template <typename _Iterator, typename> class D {
+public:
+  typename B<_Iterator>::reference operator*();
+  void operator++();
+};
+
+template <typename _Iterator, typename _Container>
+bool operator!=(D<_Iterator, _Container>, D<_Iterator, _Container>);
+template <typename _Tp> class F {
+public:
+  typedef _Tp value_type;
+};
+
+template <typename _Alloc> struct G {
+  template <typename _Tp> struct H { using type = C::rebind<_Tp>; };
+  using const_pointer = typename H<typename _Alloc::value_type>::type;
+};
+template <typename _Tp, typename _Alloc = F<_Tp>> class I {
+  typedef D<typename G<_Alloc>::const_pointer, int> const_iterator;
+
+public:
+  const_iterator begin();
+  const_iterator end();
+};
+
+struct A {
+  struct J {
+    int name;
+    int value;
+  };
+  I<J> members;
+  template <typename Key> const int *find(Key) {
+    for (const auto &[name, value] : members)
+      // See <https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01107.html>
+      // for why we don't warn here.
+      return &value; // { dg-bogus "address of local variable" }
+    return nullptr;
+  }
+};
+int main() {
+  A a;
+  a.find("");
+}
Jakub Jelinek Oct. 15, 2019, 6:41 p.m. UTC | #3 
On Tue, Oct 15, 2019 at 02:28:12PM -0400, Marek Polacek wrote:
> --- /dev/null
> +++ gcc/testsuite/g++.dg/cpp1z/decomp50.C
> @@ -0,0 +1,51 @@
> +// PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr. 
> +// { dg-do compile { target c++17 } }
> +
> +template <typename> struct B;
> +template <typename _Tp> struct B<_Tp *> { typedef _Tp reference; };

I've looked at the unreduced testcase and it seems to have
template <typename _Tp> struct B<_Tp *> { typedef _Tp& reference; };
for this line instead.  I'd think it would be better to change the testcase,
so that it is well defined rather than undefined then.
It is struct iterator_traits<_Tp*>, right?

Otherwise, the patch LGTM, but I'll defer to Jason as the maintainer.

	Jakub
Jason Merrill Oct. 21, 2019, 6:11 p.m. UTC | #4 
On 10/15/19 2:41 PM, Jakub Jelinek wrote:
> On Tue, Oct 15, 2019 at 02:28:12PM -0400, Marek Polacek wrote:
>> --- /dev/null
>> +++ gcc/testsuite/g++.dg/cpp1z/decomp50.C
>> @@ -0,0 +1,51 @@
>> +// PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr.
>> +// { dg-do compile { target c++17 } }
>> +
>> +template <typename> struct B;
>> +template <typename _Tp> struct B<_Tp *> { typedef _Tp reference; };
> 
> I've looked at the unreduced testcase and it seems to have
> template <typename _Tp> struct B<_Tp *> { typedef _Tp& reference; };
> for this line instead.  I'd think it would be better to change the testcase,
> so that it is well defined rather than undefined then.
> It is struct iterator_traits<_Tp*>, right?
> 
> Otherwise, the patch LGTM, but I'll defer to Jason as the maintainer.

OK with that change.

Jason
diff mbox series

Patch

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 141d86f50c9..1825540016f 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9354,10 +9354,8 @@  maybe_warn_about_returning_address_of_local (tree retval)
 	     binding.  */
 	  tree base = DECL_DECOMP_BASE (whats_returned);
 	  if (TYPE_REF_P (TREE_TYPE (base)))
-	    {
-	      tree init = DECL_INITIAL (base);
+	    if (tree init = DECL_INITIAL (base))
 	      return maybe_warn_about_returning_address_of_local (init);
-	    }
 	}
       bool w = false;
       auto_diagnostic_group d;
diff --git gcc/testsuite/g++.dg/cpp1z/decomp50.C gcc/testsuite/g++.dg/cpp1z/decomp50.C
new file mode 100644
index 00000000000..c41c0d8cbbf
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1z/decomp50.C
@@ -0,0 +1,49 @@ 
+// PR c++/92106 - ICE with structured bindings and -Wreturn-local-addr. 
+// { dg-do compile { target c++17 } }
+
+template <typename> struct B;
+template <typename _Tp> struct B<_Tp *> { typedef _Tp reference; };
+struct C {
+  template <typename _Up> using rebind = _Up *;
+};
+template <typename _Iterator, typename> class D {
+public:
+  typename B<_Iterator>::reference operator*();
+  void operator++();
+};
+
+template <typename _Iterator, typename _Container>
+bool operator!=(D<_Iterator, _Container>, D<_Iterator, _Container>);
+template <typename _Tp> class F {
+public:
+  typedef _Tp value_type;
+};
+
+template <typename _Alloc> struct G {
+  template <typename _Tp> struct H { using type = C::rebind<_Tp>; };
+  using const_pointer = typename H<typename _Alloc::value_type>::type;
+};
+template <typename _Tp, typename _Alloc = F<_Tp>> class I {
+  typedef D<typename G<_Alloc>::const_pointer, int> const_iterator;
+
+public:
+  const_iterator begin();
+  const_iterator end();
+};
+
+struct A {
+  struct J {
+    int name;
+    int value;
+  };
+  I<J> members;
+  template <typename Key> const int *find(Key) {
+    for (const auto &[name, value] : members)
+      return &value; // { dg-warning "address of local variable" }
+    return nullptr;
+  }
+};
+int main() {
+  A a;
+  a.find("");
+}