* [PATCH] Restore input_location after recursive expand_call_inline
@ 2021-01-04 20:12 Bernd Edlinger
2021-01-04 21:23 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: Bernd Edlinger @ 2021-01-04 20:12 UTC (permalink / raw)
To: gcc-patches, Richard Biener
[-- Attachment #1: Type: text/plain, Size: 466 bytes --]
Hi,
I spotted a place where input_location is clobbered accidentally.
That is in a recursive call to expand_call_inline. The input_location
is usually restored by goto egress in this function.
Additionally the return value of the recursive expand call is thrown
away, which does not look like a good idea.
Although this causes no problems ATM, I wanted to fix it anyway.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-input_location-after-recursive-expand_call_i.patch --]
[-- Type: text/x-patch; name="0001-Restore-input_location-after-recursive-expand_call_i.patch", Size: 1158 bytes --]
From 88b963bba7b32972abf0ea44a01c03d643d7c6ca Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Mon, 4 Jan 2021 11:35:31 +0100
Subject: [PATCH] Restore input_location after recursive expand_call_inline
This is just a precautionary fix.
2021-01-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
* tree-inline.c (expand_call_inline): Restore input_location.
Return result from recursive call.
---
gcc/tree-inline.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 360b85f..9f7d914 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -4840,9 +4840,9 @@ expand_call_inline (basic_block bb, gimple *stmt, copy_body_data *id,
gimple_call_set_fndecl (stmt, edge->callee->decl);
update_stmt (stmt);
id->src_node->remove ();
- expand_call_inline (bb, stmt, id, to_purge);
+ successfully_inlined = expand_call_inline (bb, stmt, id, to_purge);
maybe_remove_unused_call_args (cfun, stmt);
- return true;
+ goto egress;
}
fn = cg_edge->callee->decl;
cg_edge->callee->get_untransformed_body ();
--
1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Restore input_location after recursive expand_call_inline
2021-01-04 20:12 [PATCH] Restore input_location after recursive expand_call_inline Bernd Edlinger
@ 2021-01-04 21:23 ` Jeff Law
2021-01-05 6:44 ` Bernd Edlinger
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2021-01-04 21:23 UTC (permalink / raw)
To: Bernd Edlinger, gcc-patches, Richard Biener
On 1/4/21 1:12 PM, Bernd Edlinger wrote:
> Hi,
>
> I spotted a place where input_location is clobbered accidentally.
>
> That is in a recursive call to expand_call_inline. The input_location
> is usually restored by goto egress in this function.
>
> Additionally the return value of the recursive expand call is thrown
> away, which does not look like a good idea.
>
> Although this causes no problems ATM, I wanted to fix it anyway.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
> 0001-Restore-input_location-after-recursive-expand_call_i.patch
>
> From 88b963bba7b32972abf0ea44a01c03d643d7c6ca Mon Sep 17 00:00:00 2001
> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Date: Mon, 4 Jan 2021 11:35:31 +0100
> Subject: [PATCH] Restore input_location after recursive expand_call_inline
>
> This is just a precautionary fix.
>
> 2021-01-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
>
> * tree-inline.c (expand_call_inline): Restore input_location.
> Return result from recursive call.
I suspect that we're always supposed to inline in this case. As
asserting that successfully_inlined is true before jumping to "egress"
seems wise.
OK with that change after the usual testing.
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Restore input_location after recursive expand_call_inline
2021-01-04 21:23 ` Jeff Law
@ 2021-01-05 6:44 ` Bernd Edlinger
2021-01-05 8:05 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Bernd Edlinger @ 2021-01-05 6:44 UTC (permalink / raw)
To: Jeff Law, gcc-patches, Richard Biener
On 1/4/21 10:23 PM, Jeff Law wrote:
>
>
> On 1/4/21 1:12 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> I spotted a place where input_location is clobbered accidentally.
>>
>> That is in a recursive call to expand_call_inline. The input_location
>> is usually restored by goto egress in this function.
>>
>> Additionally the return value of the recursive expand call is thrown
>> away, which does not look like a good idea.
>>
>> Although this causes no problems ATM, I wanted to fix it anyway.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>> 0001-Restore-input_location-after-recursive-expand_call_i.patch
>>
>> From 88b963bba7b32972abf0ea44a01c03d643d7c6ca Mon Sep 17 00:00:00 2001
>> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
>> Date: Mon, 4 Jan 2021 11:35:31 +0100
>> Subject: [PATCH] Restore input_location after recursive expand_call_inline
>>
>> This is just a precautionary fix.
>>
>> 2021-01-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> * tree-inline.c (expand_call_inline): Restore input_location.
>> Return result from recursive call.
> I suspect that we're always supposed to inline in this case. As
> asserting that successfully_inlined is true before jumping to "egress"
> seems wise.
>
> OK with that change after the usual testing.
>
No this does not work:
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++98 compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++14 compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++17 compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++2a compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++98 compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++14 compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++17 compilation failed to produce executable
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (test for excess errors)
+UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++2a compilation failed to produce executable
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 compilation failed to produce executable
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 compilation failed to produce executable
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (test for excess errors)
+UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 compilation failed to produce executable
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (test for excess errors)
+UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a compilation failed to produce executable
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (test for excess errors)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (test for excess errors)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (test for excess errors)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (test for excess errors)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (test for excess errors)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (test for excess errors)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (test for excess errors)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (test for excess errors)
+FAIL: g++.dg/ipa/pr85421.C (internal compiler error)
+FAIL: g++.dg/ipa/pr85421.C (test for excess errors)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (test for excess errors)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (test for excess errors)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (test for excess errors)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (test for excess errors)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (test for excess errors)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (internal compiler error)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (test for excess errors)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (internal compiler error)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (test for excess errors)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (internal compiler error)
+FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (test for excess errors)
FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin -flto-partition=none line 23 p == 40
+FAIL: g++.dg/lto/devirt-5 cp_lto_devirt-5_0.o-cp_lto_devirt-5_0.o link, -O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized -flto (internal compiler error)
Is it OK in the original form?
Thanks
Bernd.
> Jeff
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Restore input_location after recursive expand_call_inline
2021-01-05 6:44 ` Bernd Edlinger
@ 2021-01-05 8:05 ` Richard Biener
2021-01-05 16:51 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2021-01-05 8:05 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Jeff Law, gcc-patches
On Tue, 5 Jan 2021, Bernd Edlinger wrote:
>
>
> On 1/4/21 10:23 PM, Jeff Law wrote:
> >
> >
> > On 1/4/21 1:12 PM, Bernd Edlinger wrote:
> >> Hi,
> >>
> >> I spotted a place where input_location is clobbered accidentally.
> >>
> >> That is in a recursive call to expand_call_inline. The input_location
> >> is usually restored by goto egress in this function.
> >>
> >> Additionally the return value of the recursive expand call is thrown
> >> away, which does not look like a good idea.
> >>
> >> Although this causes no problems ATM, I wanted to fix it anyway.
> >>
> >>
> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk?
> >>
> >>
> >> Thanks
> >> Bernd.
> >>
> >> 0001-Restore-input_location-after-recursive-expand_call_i.patch
> >>
> >> From 88b963bba7b32972abf0ea44a01c03d643d7c6ca Mon Sep 17 00:00:00 2001
> >> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> >> Date: Mon, 4 Jan 2021 11:35:31 +0100
> >> Subject: [PATCH] Restore input_location after recursive expand_call_inline
> >>
> >> This is just a precautionary fix.
> >>
> >> 2021-01-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
> >>
> >> * tree-inline.c (expand_call_inline): Restore input_location.
> >> Return result from recursive call.
> > I suspect that we're always supposed to inline in this case. As
> > asserting that successfully_inlined is true before jumping to "egress"
> > seems wise.
> >
> > OK with that change after the usual testing.
> >
>
> No this does not work:
>
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++98 compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++14 compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++17 compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++2a compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++98 compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++14 compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++17 compilation failed to produce executable
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++2a compilation failed to produce executable
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 compilation failed to produce executable
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 compilation failed to produce executable
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 compilation failed to produce executable
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (test for excess errors)
> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a compilation failed to produce executable
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (test for excess errors)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (test for excess errors)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (test for excess errors)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (test for excess errors)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (test for excess errors)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (test for excess errors)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (test for excess errors)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (test for excess errors)
> +FAIL: g++.dg/ipa/pr85421.C (internal compiler error)
> +FAIL: g++.dg/ipa/pr85421.C (test for excess errors)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (test for excess errors)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (test for excess errors)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (test for excess errors)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (test for excess errors)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (internal compiler error)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (test for excess errors)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (internal compiler error)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (test for excess errors)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (internal compiler error)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (test for excess errors)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (internal compiler error)
> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (test for excess errors)
> FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin -flto-partition=none line 23 p == 40
> +FAIL: g++.dg/lto/devirt-5 cp_lto_devirt-5_0.o-cp_lto_devirt-5_0.o link, -O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized -flto (internal compiler error)
>
> Is it OK in the original form?
Can you add a comment like
/* This used to return true even though we do fail to inline in
some cases. See PRXYZ. */
and file a bug?
OK with that change.
Some RAII-style auto_input_location class might be useful -
temporarily setting input_location for some "legacy" code is
the only valid use of input_location in the middle-end.
Thanks,
Richard.
>
> Thanks
> Bernd.
>
>
> > Jeff
> >
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Restore input_location after recursive expand_call_inline
2021-01-05 8:05 ` Richard Biener
@ 2021-01-05 16:51 ` Jeff Law
2021-01-05 17:36 ` Bernd Edlinger
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2021-01-05 16:51 UTC (permalink / raw)
To: Richard Biener, Bernd Edlinger; +Cc: gcc-patches
On 1/5/21 1:05 AM, Richard Biener wrote:
> On Tue, 5 Jan 2021, Bernd Edlinger wrote:
>
>>
>> On 1/4/21 10:23 PM, Jeff Law wrote:
>>>
>>> On 1/4/21 1:12 PM, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>> I spotted a place where input_location is clobbered accidentally.
>>>>
>>>> That is in a recursive call to expand_call_inline. The input_location
>>>> is usually restored by goto egress in this function.
>>>>
>>>> Additionally the return value of the recursive expand call is thrown
>>>> away, which does not look like a good idea.
>>>>
>>>> Although this causes no problems ATM, I wanted to fix it anyway.
>>>>
>>>>
>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>> Is it OK for trunk?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>> 0001-Restore-input_location-after-recursive-expand_call_i.patch
>>>>
>>>> From 88b963bba7b32972abf0ea44a01c03d643d7c6ca Mon Sep 17 00:00:00 2001
>>>> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>> Date: Mon, 4 Jan 2021 11:35:31 +0100
>>>> Subject: [PATCH] Restore input_location after recursive expand_call_inline
>>>>
>>>> This is just a precautionary fix.
>>>>
>>>> 2021-01-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>
>>>> * tree-inline.c (expand_call_inline): Restore input_location.
>>>> Return result from recursive call.
>>> I suspect that we're always supposed to inline in this case. As
>>> asserting that successfully_inlined is true before jumping to "egress"
>>> seems wise.
>>>
>>> OK with that change after the usual testing.
>>>
>> No this does not work:
>>
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++98 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++14 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++17 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++2a compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++98 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++14 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++17 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++2a compilation failed to produce executable
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 compilation failed to produce executable
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (test for excess errors)
>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a compilation failed to produce executable
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (test for excess errors)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (test for excess errors)
>> +FAIL: g++.dg/ipa/pr85421.C (internal compiler error)
>> +FAIL: g++.dg/ipa/pr85421.C (test for excess errors)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (test for excess errors)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (internal compiler error)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (test for excess errors)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (internal compiler error)
>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (test for excess errors)
>> FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin -flto-partition=none line 23 p == 40
>> +FAIL: g++.dg/lto/devirt-5 cp_lto_devirt-5_0.o-cp_lto_devirt-5_0.o link, -O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized -flto (internal compiler error)
>>
>> Is it OK in the original form?
> Can you add a comment like
>
> /* This used to return true even though we do fail to inline in
> some cases. See PRXYZ. */
>
> and file a bug?
>
> OK with that change.
But what about the failures above. What's effectively going on is we're
assuming inlining was successful and Bernd's patch shows that there's
cases where it isn't. That probably needs deeper investigation.
>
> Some RAII-style auto_input_location class might be useful -
> temporarily setting input_location for some "legacy" code is
> the only valid use of input_location in the middle-end.
Agreed.
jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Restore input_location after recursive expand_call_inline
2021-01-05 16:51 ` Jeff Law
@ 2021-01-05 17:36 ` Bernd Edlinger
0 siblings, 0 replies; 6+ messages in thread
From: Bernd Edlinger @ 2021-01-05 17:36 UTC (permalink / raw)
To: Jeff Law, Richard Biener; +Cc: gcc-patches
On 1/5/21 5:51 PM, Jeff Law wrote:
>
>
> On 1/5/21 1:05 AM, Richard Biener wrote:
>> On Tue, 5 Jan 2021, Bernd Edlinger wrote:
>>
>>>
>>> On 1/4/21 10:23 PM, Jeff Law wrote:
>>>>
>>>> On 1/4/21 1:12 PM, Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> I spotted a place where input_location is clobbered accidentally.
>>>>>
>>>>> That is in a recursive call to expand_call_inline. The input_location
>>>>> is usually restored by goto egress in this function.
>>>>>
>>>>> Additionally the return value of the recursive expand call is thrown
>>>>> away, which does not look like a good idea.
>>>>>
>>>>> Although this causes no problems ATM, I wanted to fix it anyway.
>>>>>
>>>>>
>>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>>> Is it OK for trunk?
>>>>>
>>>>>
>>>>> Thanks
>>>>> Bernd.
>>>>>
>>>>> 0001-Restore-input_location-after-recursive-expand_call_i.patch
>>>>>
>>>>> From 88b963bba7b32972abf0ea44a01c03d643d7c6ca Mon Sep 17 00:00:00 2001
>>>>> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>> Date: Mon, 4 Jan 2021 11:35:31 +0100
>>>>> Subject: [PATCH] Restore input_location after recursive expand_call_inline
>>>>>
>>>>> This is just a precautionary fix.
>>>>>
>>>>> 2021-01-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>>
>>>>> * tree-inline.c (expand_call_inline): Restore input_location.
>>>>> Return result from recursive call.
>>>> I suspect that we're always supposed to inline in this case. As
>>>> asserting that successfully_inlined is true before jumping to "egress"
>>>> seems wise.
>>>>
>>>> OK with that change after the usual testing.
>>>>
>>> No this does not work:
>>>
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++98 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++98 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++14 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++14 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++17 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++17 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-5.C -std=gnu++2a (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-5.C -std=gnu++2a compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++98 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++98 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++14 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++14 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++17 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++17 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/devirt-c-4.C -std=gnu++2a (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/devirt-c-4.C -std=gnu++2a compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++98 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++14 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++17 compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a (test for excess errors)
>>> +UNRESOLVED: g++.dg/ipa/imm-devirt-2.C -std=gnu++2a compilation failed to produce executable
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++98 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++14 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++17 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr71146.C -std=gnu++2a (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++98 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++14 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++17 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr79776.C -std=gnu++2a (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr85421.C (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr85421.C (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++98 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++14 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++17 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr91969.C -std=gnu++2a (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++98 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++14 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++17 (test for excess errors)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (internal compiler error)
>>> +FAIL: g++.dg/ipa/pr92454.C -std=gnu++2a (test for excess errors)
>>> FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin -flto-partition=none line 23 p == 40
>>> +FAIL: g++.dg/lto/devirt-5 cp_lto_devirt-5_0.o-cp_lto_devirt-5_0.o link, -O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized -flto (internal compiler error)
>>>
>>> Is it OK in the original form?
>> Can you add a comment like
>>
>> /* This used to return true even though we do fail to inline in
>> some cases. See PRXYZ. */
>>
>> and file a bug?
>>
>> OK with that change.
> But what about the failures above. What's effectively going on is we're
> assuming inlining was successful and Bernd's patch shows that there's
> cases where it isn't. That probably needs deeper investigation.
>
Yes. I opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98525
for that.
Bernd.
>>
>> Some RAII-style auto_input_location class might be useful -
>> temporarily setting input_location for some "legacy" code is
>> the only valid use of input_location in the middle-end.
> Agreed.
>
> jeff
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-05 17:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 20:12 [PATCH] Restore input_location after recursive expand_call_inline Bernd Edlinger
2021-01-04 21:23 ` Jeff Law
2021-01-05 6:44 ` Bernd Edlinger
2021-01-05 8:05 ` Richard Biener
2021-01-05 16:51 ` Jeff Law
2021-01-05 17:36 ` Bernd Edlinger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).