* [PATCH] Early LTO debug TODO
@ 2017-08-31 14:12 Richard Biener
2017-08-31 15:18 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2017-08-31 14:12 UTC (permalink / raw)
To: gcc-patches
As suspected during review the DECL_ABSTRACT_P handling in
gen_formal_parameter_die is no longer necessary so the following
patch removes it.
[LTO] bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
The gdb testsuite shows no regression.
Will apply after testing finished.
Richard.
2017-08-31 Richard Biener <rguenther@suse.de>
* dwarf2out.c (gen_formal_parameter_die): Remove no longer
needed DECL_ABSTRACT_P handling.
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c (revision 251559)
+++ gcc/dwarf2out.c (working copy)
@@ -21288,28 +21288,9 @@ gen_formal_parameter_die (tree node, tre
tree ultimate_origin;
dw_die_ref parm_die = NULL;
- if (TREE_CODE_CLASS (TREE_CODE (node_or_origin)) == tcc_declaration)
+ if (DECL_P (node_or_origin))
{
parm_die = lookup_decl_die (node);
-
- /* If the contexts differ, we may not be talking about the same
- thing.
- ??? When in LTO the DIE parent is the "abstract" copy and the
- context_die is the specification "copy". But this whole block
- should eventually be no longer needed. */
- if (parm_die && parm_die->die_parent != context_die && !in_lto_p)
- {
- if (!DECL_ABSTRACT_P (node))
- {
- /* This can happen when creating an inlined instance, in
- which case we need to create a new DIE that will get
- annotated with DW_AT_abstract_origin. */
- parm_die = NULL;
- }
- else
- gcc_unreachable ();
- }
-
if (parm_die && parm_die->die_parent == NULL)
{
/* Check that parm_die already has the right attributes that
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Early LTO debug TODO
2017-08-31 14:12 [PATCH] Early LTO debug TODO Richard Biener
@ 2017-08-31 15:18 ` Richard Biener
2017-09-09 14:01 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2017-08-31 15:18 UTC (permalink / raw)
To: gcc-patches; +Cc: jason
On Thu, 31 Aug 2017, Richard Biener wrote:
>
> As suspected during review the DECL_ABSTRACT_P handling in
> gen_formal_parameter_die is no longer necessary so the following
> patch removes it.
>
> [LTO] bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> The gdb testsuite shows no regression.
>
> Will apply after testing finished.
Ok, so it doesn't work - it regresses for example
gcc.dg/debug/dwarf2/inline1.c because we end up annotating the
abstract DIE with a location. This is because in gen_subprogram_die
(with decl set as abstract-self and thus generating a concrete
instance subroutine die) we call
else if (parm && !POINTER_BOUNDS_P (parm))
{
dw_die_ref parm_die = gen_decl_die (parm, NULL, NULL,
subr_die);
thus unconditionally pass NULL as origin where gen_formal_parameter_die
just has
/* If we have a previously generated DIE, use it, unless this is an
concrete instance (origin != NULL), in which case we need a new
DIE with a corresponding DW_AT_abstract_origin. */
bool reusing_die;
if (parm_die && origin == NULL)
reusing_die = true;
else
{
parm_die = new_die (DW_TAG_formal_parameter, context_die, node);
reusing_die = false;
}
but obviously that logic is flawed with us passing origin as NULL...
What saved us here is the check whether the existing param_die has
the correct parent, if not we didn't re-use it. OTOH for die_parent
== NULL we have the extra check that would have been dead code.
Any hint as to whether we should pass in anything as origin or
whether we should keep the existing die_parent logic somehow?
(do we ever get a NULL context_die passed?)
Anyway, retracting the patch for now -- at least the comment above
doesn't match reality (we're getting origin == NULL for the
concrete instance).
Thanks,
Richard.
> Richard.
>
> 2017-08-31 Richard Biener <rguenther@suse.de>
>
> * dwarf2out.c (gen_formal_parameter_die): Remove no longer
> needed DECL_ABSTRACT_P handling.
>
> Index: gcc/dwarf2out.c
> ===================================================================
> --- gcc/dwarf2out.c (revision 251559)
> +++ gcc/dwarf2out.c (working copy)
> @@ -21288,28 +21288,9 @@ gen_formal_parameter_die (tree node, tre
> tree ultimate_origin;
> dw_die_ref parm_die = NULL;
>
> - if (TREE_CODE_CLASS (TREE_CODE (node_or_origin)) == tcc_declaration)
> + if (DECL_P (node_or_origin))
> {
> parm_die = lookup_decl_die (node);
> -
> - /* If the contexts differ, we may not be talking about the same
> - thing.
> - ??? When in LTO the DIE parent is the "abstract" copy and the
> - context_die is the specification "copy". But this whole block
> - should eventually be no longer needed. */
> - if (parm_die && parm_die->die_parent != context_die && !in_lto_p)
> - {
> - if (!DECL_ABSTRACT_P (node))
> - {
> - /* This can happen when creating an inlined instance, in
> - which case we need to create a new DIE that will get
> - annotated with DW_AT_abstract_origin. */
> - parm_die = NULL;
> - }
> - else
> - gcc_unreachable ();
> - }
> -
> if (parm_die && parm_die->die_parent == NULL)
> {
> /* Check that parm_die already has the right attributes that
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Early LTO debug TODO
2017-08-31 15:18 ` Richard Biener
@ 2017-09-09 14:01 ` Jason Merrill
2017-09-09 22:23 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Jason Merrill @ 2017-09-09 14:01 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches List
[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]
On Thu, Aug 31, 2017 at 4:03 PM, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 31 Aug 2017, Richard Biener wrote:
>
>>
>> As suspected during review the DECL_ABSTRACT_P handling in
>> gen_formal_parameter_die is no longer necessary so the following
>> patch removes it.
>>
>> [LTO] bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>> The gdb testsuite shows no regression.
>>
>> Will apply after testing finished.
>
> Ok, so it doesn't work - it regresses for example
> gcc.dg/debug/dwarf2/inline1.c because we end up annotating the
> abstract DIE with a location. This is because in gen_subprogram_die
> (with decl set as abstract-self and thus generating a concrete
> instance subroutine die) we call
>
> else if (parm && !POINTER_BOUNDS_P (parm))
> {
> dw_die_ref parm_die = gen_decl_die (parm, NULL, NULL,
> subr_die);
>
> thus unconditionally pass NULL as origin where gen_formal_parameter_die
> just has
>
> /* If we have a previously generated DIE, use it, unless this is an
> concrete instance (origin != NULL), in which case we need a new
> DIE with a corresponding DW_AT_abstract_origin. */
> bool reusing_die;
> if (parm_die && origin == NULL)
> reusing_die = true;
> else
> {
> parm_die = new_die (DW_TAG_formal_parameter, context_die, node);
> reusing_die = false;
> }
>
> but obviously that logic is flawed with us passing origin as NULL...
>
> What saved us here is the check whether the existing param_die has
> the correct parent, if not we didn't re-use it. OTOH for die_parent
> == NULL we have the extra check that would have been dead code.
>
> Any hint as to whether we should pass in anything as origin or
> whether we should keep the existing die_parent logic somehow?
> (do we ever get a NULL context_die passed?)
The problem is that we aren't checking decl_ultimate_origin soon
enough, and thereby setting origin.
Tested x86_64-pc-linux-gnu, applying to trunk:
[-- Attachment #2: parm-debug.diff --]
[-- Type: text/plain, Size: 1952 bytes --]
commit b7702a2cbca9c50e94053cc125ff93438d377126
Author: Jason Merrill <jason@redhat.com>
Date: Sat Sep 9 11:33:14 2017 +0200
* dwarf2out.c (gen_formal_parameter_die): Remove obsolete hunk.
Check ultimate_origin before setting reusing_die.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 651dd0c..cc93db3 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -21285,30 +21285,15 @@ gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
dw_die_ref context_die)
{
tree node_or_origin = node ? node : origin;
- tree ultimate_origin;
dw_die_ref parm_die = NULL;
- if (TREE_CODE_CLASS (TREE_CODE (node_or_origin)) == tcc_declaration)
+ if (DECL_P (node_or_origin))
{
parm_die = lookup_decl_die (node);
- /* If the contexts differ, we may not be talking about the same
- thing.
- ??? When in LTO the DIE parent is the "abstract" copy and the
- context_die is the specification "copy". But this whole block
- should eventually be no longer needed. */
- if (parm_die && parm_die->die_parent != context_die && !in_lto_p)
- {
- if (!DECL_ABSTRACT_P (node))
- {
- /* This can happen when creating an inlined instance, in
- which case we need to create a new DIE that will get
- annotated with DW_AT_abstract_origin. */
- parm_die = NULL;
- }
- else
- gcc_unreachable ();
- }
+ tree ultimate_origin = decl_ultimate_origin (node_or_origin);
+ if (node || ultimate_origin)
+ origin = ultimate_origin;
if (parm_die && parm_die->die_parent == NULL)
{
@@ -21343,10 +21328,6 @@ gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
switch (TREE_CODE_CLASS (TREE_CODE (node_or_origin)))
{
case tcc_declaration:
- ultimate_origin = decl_ultimate_origin (node_or_origin);
- if (node || ultimate_origin)
- origin = ultimate_origin;
-
if (reusing_die)
goto add_location;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Early LTO debug TODO
2017-09-09 14:01 ` Jason Merrill
@ 2017-09-09 22:23 ` Jason Merrill
0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2017-09-09 22:23 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches List
On Sat, Sep 9, 2017 at 4:00 PM, Jason Merrill <jason@redhat.com> wrote:
> On Thu, Aug 31, 2017 at 4:03 PM, Richard Biener <rguenther@suse.de> wrote:
>> On Thu, 31 Aug 2017, Richard Biener wrote:
>>
>>>
>>> As suspected during review the DECL_ABSTRACT_P handling in
>>> gen_formal_parameter_die is no longer necessary so the following
>>> patch removes it.
>>>
>>> [LTO] bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>>> The gdb testsuite shows no regression.
>>>
>>> Will apply after testing finished.
>>
>> Ok, so it doesn't work - it regresses for example
>> gcc.dg/debug/dwarf2/inline1.c because we end up annotating the
>> abstract DIE with a location. This is because in gen_subprogram_die
>> (with decl set as abstract-self and thus generating a concrete
>> instance subroutine die) we call
>>
>> else if (parm && !POINTER_BOUNDS_P (parm))
>> {
>> dw_die_ref parm_die = gen_decl_die (parm, NULL, NULL,
>> subr_die);
>>
>> thus unconditionally pass NULL as origin where gen_formal_parameter_die
>> just has
>>
>> /* If we have a previously generated DIE, use it, unless this is an
>> concrete instance (origin != NULL), in which case we need a new
>> DIE with a corresponding DW_AT_abstract_origin. */
>> bool reusing_die;
>> if (parm_die && origin == NULL)
>> reusing_die = true;
>> else
>> {
>> parm_die = new_die (DW_TAG_formal_parameter, context_die, node);
>> reusing_die = false;
>> }
>>
>> but obviously that logic is flawed with us passing origin as NULL...
>>
>> What saved us here is the check whether the existing param_die has
>> the correct parent, if not we didn't re-use it. OTOH for die_parent
>> == NULL we have the extra check that would have been dead code.
>>
>> Any hint as to whether we should pass in anything as origin or
>> whether we should keep the existing die_parent logic somehow?
>> (do we ever get a NULL context_die passed?)
>
> The problem is that we aren't checking decl_ultimate_origin soon
> enough, and thereby setting origin.
>
> Tested x86_64-pc-linux-gnu, applying to trunk:
...and reverting, as it caused a bunch of GDB regressions.
> FAIL: gdb.cp/anon-struct.exp: print type of t3::~t3
> FAIL: gdb.cp/anon-struct.exp: print type of t::t
> FAIL: gdb.cp/anon-struct.exp: print type of X::t2::t2
> FAIL: gdb.cp/cpexprs.exp: print base1::base1(int)
> FAIL: gdb.cp/cpexprs.exp: print base1::base1(void)
> FAIL: gdb.cp/cpexprs.exp: print base2::base2
> FAIL: gdb.cp/cpexprs.exp: print base::~base
> FAIL: gdb.cp/cpexprs.exp: print base::base(int)
> FAIL: gdb.cp/cpexprs.exp: print base::base(void)
> FAIL: gdb.cp/cpexprs.exp: print derived::derived
> FAIL: gdb.cp/cpexprs.exp: print policy1::policy
> FAIL: gdb.cp/cpexprs.exp: print policy2::policy
> FAIL: gdb.cp/cpexprs.exp: print policy3::policy
> FAIL: gdb.cp/cpexprs.exp: print policy4::policy
> FAIL: gdb.cp/cpexprs.exp: print policyd1::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd1::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd2::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd2::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd3::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd3::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd4::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd4::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd5::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd5::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<base, operation_1<base> >::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<base, operation_1<base> >::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<char, operation_1<char> >::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<char, operation_1<char> >::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<int, operation_1<int> >::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<int, operation_1<int> >::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<long, operation_1<long> >::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<long, operation_1<long> >::~policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<tclass<int>, operation_1<tclass<int> > >::policyd
> FAIL: gdb.cp/cpexprs.exp: print policyd<tclass<int>, operation_1<tclass<int> > >::~policyd
Jason
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-09 22:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-31 14:12 [PATCH] Early LTO debug TODO Richard Biener
2017-08-31 15:18 ` Richard Biener
2017-09-09 14:01 ` Jason Merrill
2017-09-09 22:23 ` Jason Merrill
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).