diff mbox series

RFA (ipa-prop): PATCHes to avoid use of deprecated copy ctor and op=

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

Commit Message

Jason Merrill May 16, 2018, 12:57 a.m. UTC 
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?

Comments

Richard Biener May 16, 2018, 9:36 a.m. UTC | #1 
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.
Andreas Schwab May 16, 2018, 10:29 a.m. UTC | #2 
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.
Martin Jambor May 16, 2018, 10:35 a.m. UTC | #3 
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);
Jason Merrill May 16, 2018, 12:42 p.m. UTC | #4 
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
Martin Jambor May 16, 2018, 4:24 p.m. UTC | #5 
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);
Andreas Schwab May 17, 2018, 8:14 a.m. UTC | #6 
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.
Jason Merrill May 17, 2018, 1 p.m. UTC | #7 
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
Andreas Schwab May 17, 2018, 1:25 p.m. UTC | #8 
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.
Richard Biener May 17, 2018, 1:40 p.m. UTC | #9 
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."
diff mbox series

Patch

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; }