| Message ID | c97fed17-3bc6-eb8e-e9b8-35b11e70e50f@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Handle new operators with no arguments in DCE. | expand |
On Mon, 5 Aug 2019, Martin Liška wrote: > I'm sending fix for the ICE. The issue is that we can end up > with a ctor without an argument (when not being used). Ah, I didn't realize that after cloning and drastically changing the signature it would still count as operator new/delete. Is getting down to 0 arguments the only bad thing that can happen? Can't we have an operator delete (void*, void*) where the first argument gets optimized out and we end up optimizing as if the second argument was actually the memory being released? Should we do some sanity-checking when propagating the new/delete flags to clones?
On 8/5/19 9:07 AM, Marc Glisse wrote: > On Mon, 5 Aug 2019, Martin Liška wrote: > >> I'm sending fix for the ICE. The issue is that we can end up >> with a ctor without an argument (when not being used). > > Ah, I didn't realize that after cloning and drastically changing the signature it would still count as operator new/delete. Is getting down to 0 arguments the only bad thing that can happen? Can't we have an operator delete (void*, void*) where the first argument gets optimized out and we end up optimizing as if the second argument was actually the memory being released? Should we do some sanity-checking when propagating the new/delete flags to clones? > It can theoretically happen, but it should be properly handled in the following code: 810 if (is_delete_operator 811 || gimple_call_builtin_p (stmt, BUILT_IN_FREE)) 812 { 813 /* It can happen that a user delete operator has the pointer 814 argument optimized out already. */ 815 if (gimple_call_num_args (stmt) == 0) 816 continue; 817 818 tree ptr = gimple_call_arg (stmt, 0); 819 gimple *def_stmt; 820 tree def_callee; 821 /* If the pointer we free is defined by an allocation 822 function do not add the call to the worklist. */ 823 if (TREE_CODE (ptr) == SSA_NAME 824 && is_gimple_call (def_stmt = SSA_NAME_DEF_STMT (ptr)) 825 && (def_callee = gimple_call_fndecl (def_stmt)) 826 && ((DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL 827 && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_ALIGNED_ALLOC 828 || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC 829 || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) 830 || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee))) 831 { 832 /* Delete operators can have alignment and (or) size as next 833 arguments. When being a SSA_NAME, they must be marked 834 as necessary. */ 835 if (is_delete_operator && gimple_call_num_args (stmt) >= 2) 836 for (unsigned i = 1; i < gimple_call_num_args (stmt); i++) 837 { 838 tree arg = gimple_call_arg (stmt, i); 839 if (TREE_CODE (arg) == SSA_NAME) 840 mark_operand_necessary (arg); 841 } Where we verify that first argument of delete call is defined as a LHS of a new operator. Martin
On Mon, 5 Aug 2019, Martin Liška wrote: > On 8/5/19 9:07 AM, Marc Glisse wrote: >> On Mon, 5 Aug 2019, Martin Liška wrote: >> >>> I'm sending fix for the ICE. The issue is that we can end up >>> with a ctor without an argument (when not being used). >> >> Ah, I didn't realize that after cloning and drastically changing the signature it would still count as operator new/delete. Is getting down to 0 arguments the only bad thing that can happen? Can't we have an operator delete (void*, void*) where the first argument gets optimized out and we end up optimizing as if the second argument was actually the memory being released? Should we do some sanity-checking when propagating the new/delete flags to clones? >> > > It can theoretically happen, but it should be properly handled in the following > code: > > 810 if (is_delete_operator > 811 || gimple_call_builtin_p (stmt, BUILT_IN_FREE)) > 812 { > 813 /* It can happen that a user delete operator has the pointer > 814 argument optimized out already. */ > 815 if (gimple_call_num_args (stmt) == 0) > 816 continue; > 817 > 818 tree ptr = gimple_call_arg (stmt, 0); > 819 gimple *def_stmt; > 820 tree def_callee; > 821 /* If the pointer we free is defined by an allocation > 822 function do not add the call to the worklist. */ > 823 if (TREE_CODE (ptr) == SSA_NAME > 824 && is_gimple_call (def_stmt = SSA_NAME_DEF_STMT (ptr)) > 825 && (def_callee = gimple_call_fndecl (def_stmt)) > 826 && ((DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL > 827 && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_ALIGNED_ALLOC > 828 || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC > 829 || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) > 830 || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee))) > 831 { > 832 /* Delete operators can have alignment and (or) size as next > 833 arguments. When being a SSA_NAME, they must be marked > 834 as necessary. */ > 835 if (is_delete_operator && gimple_call_num_args (stmt) >= 2) > 836 for (unsigned i = 1; i < gimple_call_num_args (stmt); i++) > 837 { > 838 tree arg = gimple_call_arg (stmt, i); > 839 if (TREE_CODE (arg) == SSA_NAME) > 840 mark_operand_necessary (arg); > 841 } > > Where we verify that first argument of delete call is defined as a LHS of a new operator. What I am saying is that it may be the wrong operator new. Imagine something like: struct A { A(); __attribute__((malloc)) static void* operator new(unsigned long sz, void*,bool); static void operator delete(void* ptr, void*,bool); }; int main(){ int*p=new int; new(p,true) A; } At the beginning, it has D.2321 = A::operator new (1, p, 1); and A::operator delete (D.2321, p, 1); Now imagine we have access to the body of the functions, and the last one is transformed into operator delete.clone (p) (the first argument was removed) p does come from an operator new, so we do see a matched pair new+delete, but it is the wrong pair. I admit that's rather unlikely, but with clones that have a different signature, many assumptions we could make on the functions become unsafe, and I am not clear on the scale of this issue.
On Mon, Aug 5, 2019 at 8:44 AM Martin Liška <mliska@suse.cz> wrote: > > On 8/2/19 11:34 PM, H.J. Lu wrote: > > On Tue, Jul 2, 2019 at 4:50 AM Martin Liška <mliska@suse.cz> wrote: > >> > >> Second part. > >> > >> Martin > > > > This caused: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91334 > > > > Hi. > > I'm sending fix for the ICE. The issue is that we can end up > with a ctor without an argument (when not being used). > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? OK as a stop-gap for now, please discuss stuff further with Marc, I think he's right that this isn't a good solution. Can we check cgraph_node->clone_of or so and simply disregard all clones? We do not record the "original" parameter position... Richard. > Thanks, > Martin
On 8/5/19 1:57 PM, Marc Glisse wrote: > On Mon, 5 Aug 2019, Martin Liška wrote: > >> On 8/5/19 9:07 AM, Marc Glisse wrote: >>> On Mon, 5 Aug 2019, Martin Liška wrote: >>> >>>> I'm sending fix for the ICE. The issue is that we can end up >>>> with a ctor without an argument (when not being used). >>> >>> Ah, I didn't realize that after cloning and drastically changing the signature it would still count as operator new/delete. Is getting down to 0 arguments the only bad thing that can happen? Can't we have an operator delete (void*, void*) where the first argument gets optimized out and we end up optimizing as if the second argument was actually the memory being released? Should we do some sanity-checking when propagating the new/delete flags to clones? >>> >> >> It can theoretically happen, but it should be properly handled in the following >> code: >> >> 810 if (is_delete_operator >> 811 || gimple_call_builtin_p (stmt, BUILT_IN_FREE)) >> 812 { >> 813 /* It can happen that a user delete operator has the pointer >> 814 argument optimized out already. */ >> 815 if (gimple_call_num_args (stmt) == 0) >> 816 continue; >> 817 >> 818 tree ptr = gimple_call_arg (stmt, 0); >> 819 gimple *def_stmt; >> 820 tree def_callee; >> 821 /* If the pointer we free is defined by an allocation >> 822 function do not add the call to the worklist. */ >> 823 if (TREE_CODE (ptr) == SSA_NAME >> 824 && is_gimple_call (def_stmt = SSA_NAME_DEF_STMT (ptr)) >> 825 && (def_callee = gimple_call_fndecl (def_stmt)) >> 826 && ((DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL >> 827 && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_ALIGNED_ALLOC >> 828 || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC >> 829 || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) >> 830 || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee))) >> 831 { >> 832 /* Delete operators can have alignment and (or) size as next >> 833 arguments. When being a SSA_NAME, they must be marked >> 834 as necessary. */ >> 835 if (is_delete_operator && gimple_call_num_args (stmt) >= 2) >> 836 for (unsigned i = 1; i < gimple_call_num_args (stmt); i++) >> 837 { >> 838 tree arg = gimple_call_arg (stmt, i); >> 839 if (TREE_CODE (arg) == SSA_NAME) >> 840 mark_operand_necessary (arg); >> 841 } >> >> Where we verify that first argument of delete call is defined as a LHS of a new operator. > > What I am saying is that it may be the wrong operator new. > > Imagine something like: > > struct A { > A(); > __attribute__((malloc)) static void* operator new(unsigned long sz, void*,bool); > static void operator delete(void* ptr, void*,bool); > }; > int main(){ > int*p=new int; > new(p,true) A; > } > > At the beginning, it has > > D.2321 = A::operator new (1, p, 1); > > and > > A::operator delete (D.2321, p, 1); > > Now imagine we have access to the body of the functions, and the last one is transformed into > > operator delete.clone (p) You are right. It can really lead to confusion of the DCE. What we have is DECL_ABSTRACT_ORIGIN(decl) which we can use to indicate operators that were somehow modified by an IPA optimization. Do you believe it will be sufficient? Thanks, Martin > > (the first argument was removed) > p does come from an operator new, so we do see a matched pair new+delete, but it is the wrong pair. > > I admit that's rather unlikely, but with clones that have a different signature, many assumptions we could make on the functions become unsafe, and I am not clear on the scale of this issue. >
On Mon, 5 Aug 2019, Martin Liška wrote: > You are right. It can really lead to confusion of the DCE. > > What we have is DECL_ABSTRACT_ORIGIN(decl) which we can use to indicate operators > that were somehow modified by an IPA optimization. Looks similar to the cgraph_node->clone_of that Richard was talking about. I have no idea which one would be best. > Do you believe it will be sufficient? In DCE only consider the operator delete that are not clones? (possibly the same for operator new? I don't know how much we can change the return value of a function in a clone) I guess that should work, and it wouldn't impact the common case with default, global operator new/delete. An alternative would be to clear the DECL_IS_OPERATOR_DELETE flag when creating a clone (possibly only if it modified the first parameter?). There is probably not much information in the fact that a function that doesn't even take a pointer used to be an operator delete. By the way, I just thought of something, now that we handle class-specific operator new/delete (but you could do the same with the global replacable ones as well): #include <stdio.h> int count = 0; struct A { __attribute__((malloc,noinline)) static void* operator new(unsigned long sz){++count;return ::operator new(sz);} static void operator delete(void* ptr){--count;::operator delete(ptr);} }; int main(){ delete new A; printf("%d\n",count); } If we do not inline anything, we can remove the pair and nothing touches count. If we inline both new and delete, we can then remove the inner pair instead, count increases and decreases, fine. If we inline only one of them, and DCE the mismatched pair new/delete, we get something inconsistent (count is -1). This seems to indicate we should check that the new and delete match somehow...
On 8/5/19 3:46 PM, Marc Glisse wrote: > On Mon, 5 Aug 2019, Martin Liška wrote: > >> You are right. It can really lead to confusion of the DCE. >> >> What we have is DECL_ABSTRACT_ORIGIN(decl) which we can use to indicate operators >> that were somehow modified by an IPA optimization. > > Looks similar to the cgraph_node->clone_of that Richard was talking about. I have no idea which one would be best. Hm, strange that the ISRA clones don't have n->clone_of set. It's created here: #0 cgraph_node::create (decl=<function_decl 0x7ffff721c300 _ZN1AdlEPvd.isra.0>) at /home/marxin/Programming/gcc/gcc/profile-count.h:751 #1 0x0000000000bc8342 in cgraph_node::create_version_clone (this=<cgraph_node * const 0x7ffff7208000 "operator delete"/64>, new_decl=<optimized out>, redirect_callers=..., bbs_to_copy=0x0, suffix=0x1b74427 "isra") at /home/marxin/Programming/gcc/gcc/cgraphclones.c:960 #2 0x0000000000bc9b9a in cgraph_node::create_version_clone_with_body (this=this@entry=<cgraph_node * const 0x7ffff7208000 "operator delete"/64>, redirect_callers=redirect_callers@entry=..., tree_map=tree_map@entry=0x0, args_to_skip=args_to_skip@entry=0x0, skip_return=skip_return@entry=false, bbs_to_copy=bbs_to_copy@entry=0x0, new_entry_block=<optimized out>, suffix=<optimized out>, target_attributes=<optimized out>) at /home/marxin/Programming/gcc/gcc/cgraphclones.c:1071 #3 0x00000000010e4611 in modify_function (adjustments=..., node=<cgraph_node * 0x7ffff7208000 "operator delete"/64>) at /home/marxin/Programming/gcc/gcc/tree-sra.c:5493 #4 ipa_early_sra () at /home/marxin/Programming/gcc/gcc/tree-sra.c:5731 #5 (anonymous namespace)::pass_early_ipa_sra::execute (this=<optimized out>) at /home/marxin/Programming/gcc/gcc/tree-sra.c:5778 @Martin, @Honza: Why do we not set clone_of in this transformation? > >> Do you believe it will be sufficient? > > In DCE only consider the operator delete that are not clones? (possibly the same for operator new? I don't know how much we can change the return value of a function in a clone) I guess that should work, and it wouldn't impact the common case with default, global operator new/delete. I tent to limit that the only cgraph nodes that are not clones. I'm going to prepare a patch for it. > > An alternative would be to clear the DECL_IS_OPERATOR_DELETE flag when creating a clone (possibly only if it modified the first parameter?). There is probably not much information in the fact that a function that doesn't even take a pointer used to be an operator delete. > > > By the way, I just thought of something, now that we handle class-specific operator new/delete (but you could do the same with the global replacable ones as well): > > #include <stdio.h> > int count = 0; > struct A { > __attribute__((malloc,noinline)) > static void* operator new(unsigned long sz){++count;return ::operator new(sz);} > static void operator delete(void* ptr){--count;::operator delete(ptr);} > }; > int main(){ > delete new A; > printf("%d\n",count); > } > > If we do not inline anything, we can remove the pair and nothing touches count. If we inline both new and delete, we can then remove the inner pair instead, count increases and decreases, fine. If we inline only one of them, and DCE the mismatched pair new/delete, we get something inconsistent (count is -1). > > This seems to indicate we should check that the new and delete match somehow... >
On Tue, Aug 6, 2019 at 2:42 PM Martin Liška <mliska@suse.cz> wrote: > > On 8/5/19 3:46 PM, Marc Glisse wrote: > > On Mon, 5 Aug 2019, Martin Liška wrote: > > > >> You are right. It can really lead to confusion of the DCE. > >> > >> What we have is DECL_ABSTRACT_ORIGIN(decl) which we can use to indicate operators > >> that were somehow modified by an IPA optimization. > > > > Looks similar to the cgraph_node->clone_of that Richard was talking about. I have no idea which one would be best. > > > Hm, strange that the ISRA clones don't have n->clone_of set. It's created here: > > #0 cgraph_node::create (decl=<function_decl 0x7ffff721c300 _ZN1AdlEPvd.isra.0>) at /home/marxin/Programming/gcc/gcc/profile-count.h:751 > #1 0x0000000000bc8342 in cgraph_node::create_version_clone (this=<cgraph_node * const 0x7ffff7208000 "operator delete"/64>, new_decl=<optimized out>, redirect_callers=..., bbs_to_copy=0x0, suffix=0x1b74427 "isra") at /home/marxin/Programming/gcc/gcc/cgraphclones.c:960 > #2 0x0000000000bc9b9a in cgraph_node::create_version_clone_with_body (this=this@entry=<cgraph_node * const 0x7ffff7208000 "operator delete"/64>, redirect_callers=redirect_callers@entry=..., tree_map=tree_map@entry=0x0, args_to_skip=args_to_skip@entry=0x0, > skip_return=skip_return@entry=false, bbs_to_copy=bbs_to_copy@entry=0x0, new_entry_block=<optimized out>, suffix=<optimized out>, target_attributes=<optimized out>) at /home/marxin/Programming/gcc/gcc/cgraphclones.c:1071 > #3 0x00000000010e4611 in modify_function (adjustments=..., node=<cgraph_node * 0x7ffff7208000 "operator delete"/64>) at /home/marxin/Programming/gcc/gcc/tree-sra.c:5493 > #4 ipa_early_sra () at /home/marxin/Programming/gcc/gcc/tree-sra.c:5731 > #5 (anonymous namespace)::pass_early_ipa_sra::execute (this=<optimized out>) at /home/marxin/Programming/gcc/gcc/tree-sra.c:5778 > > @Martin, @Honza: Why do we not set clone_of in this transformation? > > > > >> Do you believe it will be sufficient? > > > > In DCE only consider the operator delete that are not clones? (possibly the same for operator new? I don't know how much we can change the return value of a function in a clone) I guess that should work, and it wouldn't impact the common case with default, global operator new/delete. > > I tent to limit that the only cgraph nodes that are not clones. I'm going to prepare a patch for it. I think the simplest way to achieve this is to not copy, aka clear, DECL_IS_OPERATOR_* when cloning and removing arguments (cloning for a constant align argument should be OK for example, as is for a constant address). Or simply always when cloning. Richard. > > > > An alternative would be to clear the DECL_IS_OPERATOR_DELETE flag when creating a clone (possibly only if it modified the first parameter?). There is probably not much information in the fact that a function that doesn't even take a pointer used to be an operator delete. > > > > > > By the way, I just thought of something, now that we handle class-specific operator new/delete (but you could do the same with the global replacable ones as well): > > > > #include <stdio.h> > > int count = 0; > > struct A { > > __attribute__((malloc,noinline)) > > static void* operator new(unsigned long sz){++count;return ::operator new(sz);} > > static void operator delete(void* ptr){--count;::operator delete(ptr);} > > }; > > int main(){ > > delete new A; > > printf("%d\n",count); > > } > > > > If we do not inline anything, we can remove the pair and nothing touches count. If we inline both new and delete, we can then remove the inner pair instead, count increases and decreases, fine. If we inline only one of them, and DCE the mismatched pair new/delete, we get something inconsistent (count is -1). > > > > This seems to indicate we should check that the new and delete match somehow... > > >
On 8/7/19 11:51 AM, Richard Biener wrote: > I think the simplest way to achieve this is to not copy, aka clear, > DECL_IS_OPERATOR_* when cloning and removing arguments > (cloning for a constant align argument should be OK for example, as is > for a constant address). Or simply always when cloning. Ok, then I'm suggesting following tested patch. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
On Wed, Aug 07, 2019 at 12:44:28PM +0200, Martin Liška wrote: > On 8/7/19 11:51 AM, Richard Biener wrote: > > I think the simplest way to achieve this is to not copy, aka clear, > > DECL_IS_OPERATOR_* when cloning and removing arguments > > (cloning for a constant align argument should be OK for example, as is > > for a constant address). Or simply always when cloning. > > Ok, then I'm suggesting following tested patch. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. What about LAMBDA_FUNCTION, doesn't cloning which changes arguments in any way invalidate that too, i.e. shouldn't it be just FUNCTION_DECL_DECL_TYPE (new_node->decl) = NONE; instead? On the other side, if the cloning doesn't change arguments in any way, do we still want to clear those flags? Jakub
On 8/7/19 12:51 PM, Jakub Jelinek wrote: > On Wed, Aug 07, 2019 at 12:44:28PM +0200, Martin Liška wrote: >> On 8/7/19 11:51 AM, Richard Biener wrote: >>> I think the simplest way to achieve this is to not copy, aka clear, >>> DECL_IS_OPERATOR_* when cloning and removing arguments >>> (cloning for a constant align argument should be OK for example, as is >>> for a constant address). Or simply always when cloning. >> >> Ok, then I'm suggesting following tested patch. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > What about LAMBDA_FUNCTION, doesn't cloning which changes arguments in any > way invalidate that too, i.e. shouldn't it be just > FUNCTION_DECL_DECL_TYPE (new_node->decl) = NONE; Well, how are lambdas involved in the new/delete DCE here? Lambdas with removed arguments should not interfere here. > instead? On the other side, if the cloning doesn't change arguments in any > way, do we still want to clear those flags? Well, I would consider it safer to drop it always. Martin > > Jakub >
On Wed, Aug 7, 2019 at 2:04 PM Martin Liška <mliska@suse.cz> wrote: > > On 8/7/19 12:51 PM, Jakub Jelinek wrote: > > On Wed, Aug 07, 2019 at 12:44:28PM +0200, Martin Liška wrote: > >> On 8/7/19 11:51 AM, Richard Biener wrote: > >>> I think the simplest way to achieve this is to not copy, aka clear, > >>> DECL_IS_OPERATOR_* when cloning and removing arguments > >>> (cloning for a constant align argument should be OK for example, as is > >>> for a constant address). Or simply always when cloning. > >> > >> Ok, then I'm suggesting following tested patch. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > > > What about LAMBDA_FUNCTION, doesn't cloning which changes arguments in any > > way invalidate that too, i.e. shouldn't it be just > > FUNCTION_DECL_DECL_TYPE (new_node->decl) = NONE; > > Well, how are lambdas involved in the new/delete DCE here? Lambdas with removed > arguments should not interfere here. But for coverage where we do gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl) && !DECL_FUNCTION_VERSIONED (current_function_decl) && !DECL_LAMBDA_FUNCTION_P (current_function_decl)); all clones should be considered artificial? Anyway, your patch is OK, we can think about lambdas separately. Can you simplify the DCE code after the patch? Thanks, Richard. > > instead? On the other side, if the cloning doesn't change arguments in any > > way, do we still want to clear those flags? > > Well, I would consider it safer to drop it always. > > Martin > > > > > Jakub > > >
On 8/7/19 4:12 PM, Richard Biener wrote: > On Wed, Aug 7, 2019 at 2:04 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 8/7/19 12:51 PM, Jakub Jelinek wrote: >>> On Wed, Aug 07, 2019 at 12:44:28PM +0200, Martin Liška wrote: >>>> On 8/7/19 11:51 AM, Richard Biener wrote: >>>>> I think the simplest way to achieve this is to not copy, aka clear, >>>>> DECL_IS_OPERATOR_* when cloning and removing arguments >>>>> (cloning for a constant align argument should be OK for example, as is >>>>> for a constant address). Or simply always when cloning. >>>> >>>> Ok, then I'm suggesting following tested patch. >>>> >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>> >>> What about LAMBDA_FUNCTION, doesn't cloning which changes arguments in any >>> way invalidate that too, i.e. shouldn't it be just >>> FUNCTION_DECL_DECL_TYPE (new_node->decl) = NONE; >> >> Well, how are lambdas involved in the new/delete DCE here? Lambdas with removed >> arguments should not interfere here. > > But for coverage where we do > > gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl) > && !DECL_FUNCTION_VERSIONED (current_function_decl) > && !DECL_LAMBDA_FUNCTION_P (current_function_decl)); > > all clones should be considered artificial? Well, from coverage perspective most of them are fine. > > Anyway, your patch is OK, we can think about lambdas separately. Can you > simplify the DCE code after the patch? I installed the patch and I'm sending the follow up cleanup. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin > > Thanks, > Richard. > >>> instead? On the other side, if the cloning doesn't change arguments in any >>> way, do we still want to clear those flags? >> >> Well, I would consider it safer to drop it always. >> >> Martin >> >>> >>> Jakub >>> >>
PING^1 On 8/8/19 10:43 AM, Martin Liška wrote: > On 8/7/19 4:12 PM, Richard Biener wrote: >> On Wed, Aug 7, 2019 at 2:04 PM Martin Liška <mliska@suse.cz> wrote: >>> >>> On 8/7/19 12:51 PM, Jakub Jelinek wrote: >>>> On Wed, Aug 07, 2019 at 12:44:28PM +0200, Martin Liška wrote: >>>>> On 8/7/19 11:51 AM, Richard Biener wrote: >>>>>> I think the simplest way to achieve this is to not copy, aka clear, >>>>>> DECL_IS_OPERATOR_* when cloning and removing arguments >>>>>> (cloning for a constant align argument should be OK for example, as is >>>>>> for a constant address). Or simply always when cloning. >>>>> >>>>> Ok, then I'm suggesting following tested patch. >>>>> >>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>>> >>>> What about LAMBDA_FUNCTION, doesn't cloning which changes arguments in any >>>> way invalidate that too, i.e. shouldn't it be just >>>> FUNCTION_DECL_DECL_TYPE (new_node->decl) = NONE; >>> >>> Well, how are lambdas involved in the new/delete DCE here? Lambdas with removed >>> arguments should not interfere here. >> >> But for coverage where we do >> >> gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl) >> && !DECL_FUNCTION_VERSIONED (current_function_decl) >> && !DECL_LAMBDA_FUNCTION_P (current_function_decl)); >> >> all clones should be considered artificial? > > Well, from coverage perspective most of them are fine. > >> >> Anyway, your patch is OK, we can think about lambdas separately. Can you >> simplify the DCE code after the patch? > > I installed the patch and I'm sending the follow up cleanup. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > >> >> Thanks, >> Richard. >> >>>> instead? On the other side, if the cloning doesn't change arguments in any >>>> way, do we still want to clear those flags? >>> >>> Well, I would consider it safer to drop it always. >>> >>> Martin >>> >>>> >>>> Jakub >>>> >>> >
On Thu, Aug 15, 2019 at 12:47 PM Martin Liška <mliska@suse.cz> wrote: > > PING^1 OK > On 8/8/19 10:43 AM, Martin Liška wrote: > > On 8/7/19 4:12 PM, Richard Biener wrote: > >> On Wed, Aug 7, 2019 at 2:04 PM Martin Liška <mliska@suse.cz> wrote: > >>> > >>> On 8/7/19 12:51 PM, Jakub Jelinek wrote: > >>>> On Wed, Aug 07, 2019 at 12:44:28PM +0200, Martin Liška wrote: > >>>>> On 8/7/19 11:51 AM, Richard Biener wrote: > >>>>>> I think the simplest way to achieve this is to not copy, aka clear, > >>>>>> DECL_IS_OPERATOR_* when cloning and removing arguments > >>>>>> (cloning for a constant align argument should be OK for example, as is > >>>>>> for a constant address). Or simply always when cloning. > >>>>> > >>>>> Ok, then I'm suggesting following tested patch. > >>>>> > >>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >>>> > >>>> What about LAMBDA_FUNCTION, doesn't cloning which changes arguments in any > >>>> way invalidate that too, i.e. shouldn't it be just > >>>> FUNCTION_DECL_DECL_TYPE (new_node->decl) = NONE; > >>> > >>> Well, how are lambdas involved in the new/delete DCE here? Lambdas with removed > >>> arguments should not interfere here. > >> > >> But for coverage where we do > >> > >> gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl) > >> && !DECL_FUNCTION_VERSIONED (current_function_decl) > >> && !DECL_LAMBDA_FUNCTION_P (current_function_decl)); > >> > >> all clones should be considered artificial? > > > > Well, from coverage perspective most of them are fine. > > > >> > >> Anyway, your patch is OK, we can think about lambdas separately. Can you > >> simplify the DCE code after the patch? > > > > I installed the patch and I'm sending the follow up cleanup. > > > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > > > Ready to be installed? > > Thanks, > > Martin > > > >> > >> Thanks, > >> Richard. > >> > >>>> instead? On the other side, if the cloning doesn't change arguments in any > >>>> way, do we still want to clear those flags? > >>> > >>> Well, I would consider it safer to drop it always. > >>> > >>> Martin > >>> > >>>> > >>>> Jakub > >>>> > >>> > > >
From 76d215d59f32c5f6cbcb0a0ad4ecbfff8f181770 Mon Sep 17 00:00:00 2001 From: Martin Liska <mliska@suse.cz> Date: Mon, 5 Aug 2019 06:55:45 +0200 Subject: [PATCH] Handle new operators with no arguments in DCE. gcc/ChangeLog: 2019-08-05 Martin Liska <mliska@suse.cz> PR c++/91334 * tree-ssa-dce.c (propagate_necessity): Handle new operators with not arguments. (eliminate_unnecessary_stmts): Likewise. gcc/testsuite/ChangeLog: 2019-08-05 Martin Liska <mliska@suse.cz> PR c++/91334 * g++.dg/torture/pr91334.C: New test. --- gcc/testsuite/g++.dg/torture/pr91334.C | 14 ++++++++++++++ gcc/tree-ssa-dce.c | 22 ++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/torture/pr91334.C diff --git a/gcc/testsuite/g++.dg/torture/pr91334.C b/gcc/testsuite/g++.dg/torture/pr91334.C new file mode 100644 index 00000000000..ba79d712b07 --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr91334.C @@ -0,0 +1,14 @@ +/* PR c++/91334. */ +/* { dg-do compile } */ + +#include <new> +#include <stdlib.h> + +struct A { + A() { throw 0; } + void* operator new(size_t size, double = 0.0) { return ::operator new(size);} + void operator delete(void* p, double) { exit(0); } + void operator delete(void* p) { abort(); } +}; + +int main() { try { new A; } catch(...) {} } diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index 80d5f5c30f7..afb7bd9dedc 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -810,6 +810,11 @@ propagate_necessity (bool aggressive) if (is_delete_operator || gimple_call_builtin_p (stmt, BUILT_IN_FREE)) { + /* It can happen that a user delete operator has the pointer + argument optimized out already. */ + if (gimple_call_num_args (stmt) == 0) + continue; + tree ptr = gimple_call_arg (stmt, 0); gimple *def_stmt; tree def_callee; @@ -1323,13 +1328,18 @@ eliminate_unnecessary_stmts (void) || (is_gimple_call (stmt) && gimple_call_operator_delete_p (as_a <gcall *> (stmt))))) { - tree ptr = gimple_call_arg (stmt, 0); - if (TREE_CODE (ptr) == SSA_NAME) + /* It can happen that a user delete operator has the pointer + argument optimized out already. */ + if (gimple_call_num_args (stmt) > 0) { - gimple *def_stmt = SSA_NAME_DEF_STMT (ptr); - if (!gimple_nop_p (def_stmt) - && !gimple_plf (def_stmt, STMT_NECESSARY)) - gimple_set_plf (stmt, STMT_NECESSARY, false); + tree ptr = gimple_call_arg (stmt, 0); + if (TREE_CODE (ptr) == SSA_NAME) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (ptr); + if (!gimple_nop_p (def_stmt) + && !gimple_plf (def_stmt, STMT_NECESSARY)) + gimple_set_plf (stmt, STMT_NECESSARY, false); + } } } -- 2.22.0