Message ID | 20160210223208.GY3017@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
On Wed, 10 Feb 2016, Jakub Jelinek wrote: > Hi! > > Markus has pointed out to a reduced testcase which still ICEs even with the > PR69241 fix. In that case the function with TREE_ADDRESSABLE return type > does not return at all (and -Wreturn-type properly diagnoses it). > For that case the following patch just forces the lhs on the *.part.* > call, so that we don't ICE in assign_temp. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Um... I wonder if it wouldn't be better to force such functions to be noreturn by say, placing a __builtin_unreachable () at the missing return and maybe even adjust the return type. In the frontend I mean. After all, what would the code below do at runtime? Unhelpfully write to a random memory location. So if then I'd rather dereference literal zero here (well, not sure what to do for -fno-delete-null-pointer targets). Otherwise this becomes a bigger security issue than not initializing the not used return value? The other option is to simply not split the function in this case. Richard. > 2016-02-10 Jakub Jelinek <jakub@redhat.com> > > PR ipa/69241 > * ipa-split.c (split_function): If split part returns TREE_ADDRESSABLE > type by reference, force lhs on the call. > > * g++.dg/ipa/pr69241-4.C: New test. > > --- gcc/ipa-split.c.jj 2016-02-10 16:05:37.000000000 +0100 > +++ gcc/ipa-split.c 2016-02-10 17:18:12.553061670 +0100 > @@ -1589,7 +1589,20 @@ split_function (basic_block return_bb, s > } > } > else > - gsi_insert_after (&gsi, call, GSI_NEW_STMT); > + { > + /* Force a lhs if the split part has to return a value. */ > + if (split_point->split_part_set_retval > + && DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) > + { > + retval = DECL_RESULT (current_function_decl); > + if (TREE_ADDRESSABLE (TREE_TYPE (TREE_TYPE (retval)))) > + { > + retval = get_or_create_ssa_default_def (cfun, retval); > + gimple_call_set_lhs (call, build_simple_mem_ref (retval)); > + } > + } > + gsi_insert_after (&gsi, call, GSI_NEW_STMT); > + } > if (tsan_func_exit_call) > gsi_insert_after (&gsi, tsan_func_exit_call, GSI_NEW_STMT); > } > --- gcc/testsuite/g++.dg/ipa/pr69241-4.C.jj 2016-02-10 17:22:03.977866326 +0100 > +++ gcc/testsuite/g++.dg/ipa/pr69241-4.C 2016-02-10 17:22:00.073920229 +0100 > @@ -0,0 +1,55 @@ > +// PR ipa/69241 > +// { dg-do compile { target c++11 } } > +// { dg-options "-O2 -Wno-return-type" } > + > +template <typename> class A; > +struct B { > + using pointer = int *; > +}; > +template <typename _CharT, typename = A<_CharT>> class basic_string { > + long _M_string_length; > + enum { _S_local_capacity = 15 } _M_local_buf[_S_local_capacity]; > + B::pointer _M_local_data; > + > +public: > + ~basic_string(); > +}; > +template <typename _CharT, typename _Traits, typename _Alloc> > +int operator<<(_Traits, basic_string<_CharT, _Alloc>); > +class C { > + basic_string<A<char>> _M_string; > +}; > +class D { > + C _M_stringbuf; > +}; > +class F { > + int stream; > + D stream_; > +}; > +class G { > +public: > + void operator&(int); > +}; > +class H { > +public: > + H(unsigned); > + H(H &&); > + bool m_fn1(); > +}; > +class I { > + void m_fn2(const int &&); > + static H m_fn3(const int &); > +}; > +template <typename Functor> void Bind(Functor); > +class J { > +public: > + static basic_string<char> m_fn4(); > +}; > +int a; > +void I::m_fn2(const int &&) { Bind(m_fn3); } > +H I::m_fn3(const int &) { > + !false ? (void)0 : G() & F() << J::m_fn4(); > + H b(a); > + if (b.m_fn1()) > + F(); > +} > > Jakub > >
On 02/11/2016 02:22 AM, Richard Biener wrote: > On Wed, 10 Feb 2016, Jakub Jelinek wrote: > >> Hi! >> >> Markus has pointed out to a reduced testcase which still ICEs even with the >> PR69241 fix. In that case the function with TREE_ADDRESSABLE return type >> does not return at all (and -Wreturn-type properly diagnoses it). >> For that case the following patch just forces the lhs on the *.part.* >> call, so that we don't ICE in assign_temp. >> >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Um... I wonder if it wouldn't be better to force such functions to > be noreturn by say, placing a __builtin_unreachable () at the missing > return and maybe even adjust the return type. Remember that __builtin_unreachable doesn't actually insert any real code, so if by some means we get to that point, we'll happily continue executing whatever code is there, which opens a potential security hole. I generally prefer __builtin_trap over __builtin_unreachable. The single instruction is of marginal cost and provides a level of safety if we do manage to get into "unreachable" code. Jeff
--- gcc/ipa-split.c.jj 2016-02-10 16:05:37.000000000 +0100 +++ gcc/ipa-split.c 2016-02-10 17:18:12.553061670 +0100 @@ -1589,7 +1589,20 @@ split_function (basic_block return_bb, s } } else - gsi_insert_after (&gsi, call, GSI_NEW_STMT); + { + /* Force a lhs if the split part has to return a value. */ + if (split_point->split_part_set_retval + && DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) + { + retval = DECL_RESULT (current_function_decl); + if (TREE_ADDRESSABLE (TREE_TYPE (TREE_TYPE (retval)))) + { + retval = get_or_create_ssa_default_def (cfun, retval); + gimple_call_set_lhs (call, build_simple_mem_ref (retval)); + } + } + gsi_insert_after (&gsi, call, GSI_NEW_STMT); + } if (tsan_func_exit_call) gsi_insert_after (&gsi, tsan_func_exit_call, GSI_NEW_STMT); } --- gcc/testsuite/g++.dg/ipa/pr69241-4.C.jj 2016-02-10 17:22:03.977866326 +0100 +++ gcc/testsuite/g++.dg/ipa/pr69241-4.C 2016-02-10 17:22:00.073920229 +0100 @@ -0,0 +1,55 @@ +// PR ipa/69241 +// { dg-do compile { target c++11 } } +// { dg-options "-O2 -Wno-return-type" } + +template <typename> class A; +struct B { + using pointer = int *; +}; +template <typename _CharT, typename = A<_CharT>> class basic_string { + long _M_string_length; + enum { _S_local_capacity = 15 } _M_local_buf[_S_local_capacity]; + B::pointer _M_local_data; + +public: + ~basic_string(); +}; +template <typename _CharT, typename _Traits, typename _Alloc> +int operator<<(_Traits, basic_string<_CharT, _Alloc>); +class C { + basic_string<A<char>> _M_string; +}; +class D { + C _M_stringbuf; +}; +class F { + int stream; + D stream_; +}; +class G { +public: + void operator&(int); +}; +class H { +public: + H(unsigned); + H(H &&); + bool m_fn1(); +}; +class I { + void m_fn2(const int &&); + static H m_fn3(const int &); +}; +template <typename Functor> void Bind(Functor); +class J { +public: + static basic_string<char> m_fn4(); +}; +int a; +void I::m_fn2(const int &&) { Bind(m_fn3); } +H I::m_fn3(const int &) { + !false ? (void)0 : G() & F() << J::m_fn4(); + H b(a); + if (b.m_fn1()) + F(); +}