Message ID | CADzB+2kk=5ewAy+5-uGdcEPDoO4+1NNTOoo3YERkp7g9PoaqMg@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | RFA (ipa-prop): PATCHes to avoid use of deprecated copy ctor and op= | expand |
On Wed, May 16, 2018 at 2:58 AM Jason Merrill <jason@redhat.com> wrote: > In C++11 and up, the implicitly-declared copy constructor and > assignment operator are deprecated if one of them, or the destructor, > is user-provided. Implementing that in G++ turned up a few dodgy uses > in the compiler. > In general it's unsafe to copy an ipa_edge_args, because if one of the > pointers is non-null you get two copies of a vec pointer, and when one > of the objects is destroyed it frees the vec and leaves the other > object pointing to freed memory. This specific example is safe > because it only copies from an object with null pointers, but it would > be better to avoid the copy. OK for trunk? > It's unsafe to copy a releasing_vec for the same reason. There are a > few places where the copy constructor is nominally used to initialize > a releasing_vec variable from a temporary returned from a function; in > these cases no actual copy is done, and the function directly > initializes the variable, so it's safe. I made this clearer by > declaring the copy constructor but not defining it, so uses that get > elided are accepted, but uses that actually want to copy will fail to > link. > In cp_expr we defined the copy constructor to do the same thing that > the implicit definition would do, causing the copy assignment operator > to be deprecated. We don't need the copy constructor, so let's remove > it. > Tested x86_64-pc-linux-gnu. Are the ipa-prop bits OK for trunk? Yes. Richard.
On Mai 15 2018, Jason Merrill <jason@redhat.com> wrote: > commit 648ffd02e23ac2695de04ab266b4f8862df6c2ed > Author: Jason Merrill <jason@redhat.com> > Date: Tue May 15 20:46:54 2018 -0400 > > * cp-tree.h (cp_expr): Remove copy constructor. > > * mangle.c (struct releasing_vec): Declare copy constructor. I'm getting an ICE on ia64 during the stage1 build of libstdc++ (perhaps related that this uses gcc 4.8 as the bootstrap compiler): /usr/local/gcc/gcc-20180516/Build/./gcc/xgcc -shared-libgcc -B/usr/local/gcc/gcc-20180516/Build/./gcc -nostdinc++ -L/usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/src -L/usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/src/.libs -L/usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/libsupc++/.libs -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include -fno-checking -x c++-header -nostdinc++ -O2 -g -D_GNU_SOURCE -I/usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/include/ia64-suse-linux -I/usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/include -I/usr/local/gcc/gcc-20180516/libstdc++-v3/libsupc++ -O2 -g -std=gnu++0x /usr/local/gcc/gcc-20180516/libstdc++-v3/include/precompiled/stdc++.h \ -o ia64-suse-linux/bits/stdc++.h.gch/O2ggnu++0x.gch In file included from /usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/include/cmath:42, from /usr/local/gcc/gcc-20180516/libstdc++-v3/include/precompiled/stdc++.h:41: /usr/local/gcc/gcc-20180516/Build/ia64-suse-linux/libstdc++-v3/include/bits/cpp_type_traits.h:89:34: internal compiler error: in cp_parser_lookup_name, at cp/parser.c:26107 enum { __value = bool(_Sp::__value) || bool(_Tp::__value) }; ^~~~~~~ 0x40000000003a5cff cp_parser_lookup_name ../../gcc/cp/parser.c:26107 0x40000000003d576f cp_parser_primary_expression ../../gcc/cp/parser.c:5517 0x40000000003f1daf cp_parser_postfix_expression ../../gcc/cp/parser.c:7008 0x400000000040534f cp_parser_unary_expression ../../gcc/cp/parser.c:8300 0x40000000003b2abf cp_parser_cast_expression ../../gcc/cp/parser.c:9068 0x40000000003b3f9f cp_parser_binary_expression ../../gcc/cp/parser.c:9170 0x40000000003b518f cp_parser_assignment_expression ../../gcc/cp/parser.c:9466 0x40000000003bbf9f cp_parser_parenthesized_expression_list ../../gcc/cp/parser.c:7740 0x40000000003bde1f cp_parser_functional_cast ../../gcc/cp/parser.c:27387 0x40000000003f1c9f cp_parser_postfix_expression ../../gcc/cp/parser.c:6931 0x400000000040534f cp_parser_unary_expression ../../gcc/cp/parser.c:8300 0x40000000003b2abf cp_parser_cast_expression ../../gcc/cp/parser.c:9068 0x40000000003b3f9f cp_parser_binary_expression ../../gcc/cp/parser.c:9170 0x40000000003b518f cp_parser_assignment_expression ../../gcc/cp/parser.c:9466 0x40000000003b775f cp_parser_constant_expression ../../gcc/cp/parser.c:9748 0x40000000003ce8bf cp_parser_enumerator_definition ../../gcc/cp/parser.c:18410 0x40000000003ce8bf cp_parser_enumerator_list ../../gcc/cp/parser.c:18350 0x40000000003ce8bf cp_parser_enum_specifier ../../gcc/cp/parser.c:18277 0x40000000003ce8bf cp_parser_type_specifier ../../gcc/cp/parser.c:16736 0x40000000003fad7f cp_parser_decl_specifier_seq ../../gcc/cp/parser.c:13606 Andreas.
Hi, On Tue, May 15 2018, Jason Merrill wrote: > In C++11 and up, the implicitly-declared copy constructor and > assignment operator are deprecated if one of them, or the destructor, > is user-provided. Implementing that in G++ turned up a few dodgy uses > in the compiler. > > In general it's unsafe to copy an ipa_edge_args, because if one of the > pointers is non-null you get two copies of a vec pointer, and when one > of the objects is destroyed it frees the vec and leaves the other > object pointing to freed memory. This specific example is safe > because it only copies from an object with null pointers, but it would > be better to avoid the copy. OK for trunk? I have had a look and found out that the function in question (ipa_free_edge_args_substructures) has no uses, apparently I forgot to remove it when I did the conversion of jump functions to be stored in call graph edge summaries. So thanks lot for spotting this but I'd prefer the following (compiled but untested) patch: Martin 2018-05-16 Martin Jambor <mjambor@suse.cz> * ipa-prop.c (ipa_free_all_edge_args): Remove. * ipa-prop.h (ipa_free_all_edge_args): Likewise. diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 38441cc49bc..19d55cda009 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -3708,16 +3708,6 @@ ipa_check_create_edge_args (void) ipa_vr_hash_table = hash_table<ipa_vr_ggc_hash_traits>::create_ggc (37); } -/* Frees all dynamically allocated structures that the argument info points - to. */ - -void -ipa_free_edge_args_substructures (struct ipa_edge_args *args) -{ - vec_free (args->jump_functions); - *args = ipa_edge_args (); -} - /* Free all ipa_edge structures. */ void diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index a61e06135e3..dc45cea9c71 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -664,7 +664,6 @@ extern GTY(()) vec<ipcp_transformation_summary, va_gc> *ipcp_transformations; void ipa_create_all_node_params (void); void ipa_create_all_edge_args (void); void ipa_check_create_edge_args (void); -void ipa_free_edge_args_substructures (struct ipa_edge_args *); void ipa_free_all_node_params (void); void ipa_free_all_edge_args (void); void ipa_free_all_structures_after_ipa_cp (void);
On Wed, May 16, 2018 at 6:35 AM, Martin Jambor <mjambor@suse.cz> wrote: > On Tue, May 15 2018, Jason Merrill wrote: >> In C++11 and up, the implicitly-declared copy constructor and >> assignment operator are deprecated if one of them, or the destructor, >> is user-provided. Implementing that in G++ turned up a few dodgy uses >> in the compiler. >> >> In general it's unsafe to copy an ipa_edge_args, because if one of the >> pointers is non-null you get two copies of a vec pointer, and when one >> of the objects is destroyed it frees the vec and leaves the other >> object pointing to freed memory. This specific example is safe >> because it only copies from an object with null pointers, but it would >> be better to avoid the copy. OK for trunk? > > I have had a look and found out that the function in question > (ipa_free_edge_args_substructures) has no uses, apparently I forgot to > remove it when I did the conversion of jump functions to be stored in > call graph edge summaries. So thanks lot for spotting this but I'd > prefer the following (compiled but untested) patch: > > Martin > > > 2018-05-16 Martin Jambor <mjambor@suse.cz> > > * ipa-prop.c (ipa_free_all_edge_args): Remove. > * ipa-prop.h (ipa_free_all_edge_args): Likewise. That works for me, too. Jason
Hi, On Wed, May 16 2018, Jason Merrill wrote: > On Wed, May 16, 2018 at 6:35 AM, Martin Jambor <mjambor@suse.cz> wrote: >> On Tue, May 15 2018, Jason Merrill wrote: >>> In C++11 and up, the implicitly-declared copy constructor and >>> assignment operator are deprecated if one of them, or the destructor, >>> is user-provided. Implementing that in G++ turned up a few dodgy uses >>> in the compiler. >>> >>> In general it's unsafe to copy an ipa_edge_args, because if one of the >>> pointers is non-null you get two copies of a vec pointer, and when one >>> of the objects is destroyed it frees the vec and leaves the other >>> object pointing to freed memory. This specific example is safe >>> because it only copies from an object with null pointers, but it would >>> be better to avoid the copy. OK for trunk? >> >> I have had a look and found out that the function in question >> (ipa_free_edge_args_substructures) has no uses, apparently I forgot to >> remove it when I did the conversion of jump functions to be stored in >> call graph edge summaries. So thanks lot for spotting this but I'd >> prefer the following (compiled but untested) patch: >> >> Martin >> >> >> 2018-05-16 Martin Jambor <mjambor@suse.cz> >> >> * ipa-prop.c (ipa_free_all_edge_args): Remove. >> * ipa-prop.h (ipa_free_all_edge_args): Likewise. > > That works for me, too. > > Jason OK, so after bootstrapping and testing on x86_64, I committed the patch as obvious. Thanks, Martin 2018-05-16 Martin Jambor <mjambor@suse.cz> * ipa-prop.c (ipa_free_all_edge_args): Remove. * ipa-prop.h (ipa_free_all_edge_args): Likewise. --- gcc/ipa-prop.c | 10 ---------- gcc/ipa-prop.h | 1 - 2 files changed, 11 deletions(-) diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 38441cc49bc..19d55cda009 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -3708,16 +3708,6 @@ ipa_check_create_edge_args (void) ipa_vr_hash_table = hash_table<ipa_vr_ggc_hash_traits>::create_ggc (37); } -/* Frees all dynamically allocated structures that the argument info points - to. */ - -void -ipa_free_edge_args_substructures (struct ipa_edge_args *args) -{ - vec_free (args->jump_functions); - *args = ipa_edge_args (); -} - /* Free all ipa_edge structures. */ void diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index a61e06135e3..dc45cea9c71 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -664,7 +664,6 @@ extern GTY(()) vec<ipcp_transformation_summary, va_gc> *ipcp_transformations; void ipa_create_all_node_params (void); void ipa_create_all_edge_args (void); void ipa_check_create_edge_args (void); -void ipa_free_edge_args_substructures (struct ipa_edge_args *); void ipa_free_all_node_params (void); void ipa_free_all_edge_args (void); void ipa_free_all_structures_after_ipa_cp (void);
On Mai 16 2018, Andreas Schwab <schwab@suse.de> wrote: > On Mai 15 2018, Jason Merrill <jason@redhat.com> wrote: > >> commit 648ffd02e23ac2695de04ab266b4f8862df6c2ed >> Author: Jason Merrill <jason@redhat.com> >> Date: Tue May 15 20:46:54 2018 -0400 >> >> * cp-tree.h (cp_expr): Remove copy constructor. >> >> * mangle.c (struct releasing_vec): Declare copy constructor. > > I'm getting an ICE on ia64 during the stage1 build of libstdc++ (perhaps > related that this uses gcc 4.8 as the bootstrap compiler): I have now switched to gcc 5 as the bootstrap compiler, which doesn't have this issue. Andreas.
On Thu, May 17, 2018 at 4:14 AM, Andreas Schwab <schwab@suse.de> wrote: > On Mai 16 2018, Andreas Schwab <schwab@suse.de> wrote: >> On Mai 15 2018, Jason Merrill <jason@redhat.com> wrote: >> >>> commit 648ffd02e23ac2695de04ab266b4f8862df6c2ed >>> Author: Jason Merrill <jason@redhat.com> >>> Date: Tue May 15 20:46:54 2018 -0400 >>> >>> * cp-tree.h (cp_expr): Remove copy constructor. >>> >>> * mangle.c (struct releasing_vec): Declare copy constructor. >> >> I'm getting an ICE on ia64 during the stage1 build of libstdc++ (perhaps >> related that this uses gcc 4.8 as the bootstrap compiler): > > I have now switched to gcc 5 as the bootstrap compiler, which doesn't > have this issue. Aha. Is it the cp_expr change that confused 4.8? Jason
On Mai 17 2018, Jason Merrill <jason@redhat.com> wrote: > On Thu, May 17, 2018 at 4:14 AM, Andreas Schwab <schwab@suse.de> wrote: >> On Mai 16 2018, Andreas Schwab <schwab@suse.de> wrote: >>> On Mai 15 2018, Jason Merrill <jason@redhat.com> wrote: >>> >>>> commit 648ffd02e23ac2695de04ab266b4f8862df6c2ed >>>> Author: Jason Merrill <jason@redhat.com> >>>> Date: Tue May 15 20:46:54 2018 -0400 >>>> >>>> * cp-tree.h (cp_expr): Remove copy constructor. >>>> >>>> * mangle.c (struct releasing_vec): Declare copy constructor. >>> >>> I'm getting an ICE on ia64 during the stage1 build of libstdc++ (perhaps >>> related that this uses gcc 4.8 as the bootstrap compiler): >> >> I have now switched to gcc 5 as the bootstrap compiler, which doesn't >> have this issue. > > Aha. Is it the cp_expr change that confused 4.8? I haven't looked closer. Andreas.
On Thu, May 17, 2018 at 3:25 PM Andreas Schwab <schwab@suse.de> wrote: > On Mai 17 2018, Jason Merrill <jason@redhat.com> wrote: > > On Thu, May 17, 2018 at 4:14 AM, Andreas Schwab <schwab@suse.de> wrote: > >> On Mai 16 2018, Andreas Schwab <schwab@suse.de> wrote: > >>> On Mai 15 2018, Jason Merrill <jason@redhat.com> wrote: > >>> > >>>> commit 648ffd02e23ac2695de04ab266b4f8862df6c2ed > >>>> Author: Jason Merrill <jason@redhat.com> > >>>> Date: Tue May 15 20:46:54 2018 -0400 > >>>> > >>>> * cp-tree.h (cp_expr): Remove copy constructor. > >>>> > >>>> * mangle.c (struct releasing_vec): Declare copy constructor. > >>> > >>> I'm getting an ICE on ia64 during the stage1 build of libstdc++ (perhaps > >>> related that this uses gcc 4.8 as the bootstrap compiler): > >> > >> I have now switched to gcc 5 as the bootstrap compiler, which doesn't > >> have this issue. > > > > Aha. Is it the cp_expr change that confused 4.8? > I haven't looked closer. I have no problems with GCC 4.8 on trunk x86_64. Richard. > Andreas. > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different."
commit 648ffd02e23ac2695de04ab266b4f8862df6c2ed Author: Jason Merrill <jason@redhat.com> Date: Tue May 15 20:46:54 2018 -0400 * cp-tree.h (cp_expr): Remove copy constructor. * mangle.c (struct releasing_vec): Declare copy constructor. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 9a2eb3be4d1..cab926028b8 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -59,9 +59,6 @@ public: cp_expr (tree value, location_t loc): m_value (value), m_loc (loc) {} - cp_expr (const cp_expr &other) : - m_value (other.m_value), m_loc (other.m_loc) {} - /* Implicit conversions to tree. */ operator tree () const { return m_value; } tree & operator* () { return m_value; } diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c index 6a7df804caf..59a3111fba2 100644 --- a/gcc/cp/mangle.c +++ b/gcc/cp/mangle.c @@ -1555,6 +1555,10 @@ struct releasing_vec releasing_vec (vec_t *v): v(v) { } releasing_vec (): v(make_tree_vector ()) { } + /* Copy constructor is deliberately declared but not defined, + copies must always be elided. */ + releasing_vec (const releasing_vec &); + vec_t &operator* () const { return *v; } vec_t *operator-> () const { return v; } vec_t *get () const { return v; }