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
Related show

Commit Message

Marek Polacek Oct. 15, 2019, 5:17 p.m.
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. | #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. | #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. | #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. | #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

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("");
+}