[RFC] Assert DECL_ABSTRACT_ORIGIN is different from the decl itself

Hi,

one of a number of symptoms of an otherwise unrelated HSA bug I've
been debugging today is gcc crashing or hanging in the C++ pretty
printer when attempting to emit a warning because dump_decl() ended up
in an infinite recursion calling itself on the DECL_ABSTRACT_ORIGIN of
the decl it was looking at, which was however the same thing.  (It was
set to itself on purpose in set_decl_origin_self as a part of final
pass, the decl was being printed because it was itself an abstract
origin of another one).

If someone ever faces a similar problem, the following (untested)
patch might save them a bit of time.  I have eventually decided not to
make it a checking-only assert because it is on a cold path and
because at release-build optimization levels, the tail-call is
optimized to a jump and thus an infinite loop if the described
situation happens, and I suppose an informative ICE is better tan that
even for users.

What do you think?  Would it be reasonable for trunk even now or
should I queue it for the next stage1?

Thanks,

Martin


gcc/cp/

2016-11-28  Martin Jambor  <mjambor@suse.cz>

	* error.c (dump_decl): Add an assert that DECL_ABSTRACT_ORIGIN
	is not the decl itself.
---
 gcc/cp/error.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On 11/28/2016 07:27 AM, Martin Jambor wrote:
> Hi,
>
> one of a number of symptoms of an otherwise unrelated HSA bug I've
> been debugging today is gcc crashing or hanging in the C++ pretty
> printer when attempting to emit a warning because dump_decl() ended up
> in an infinite recursion calling itself on the DECL_ABSTRACT_ORIGIN of
> the decl it was looking at, which was however the same thing.  (It was
> set to itself on purpose in set_decl_origin_self as a part of final
> pass, the decl was being printed because it was itself an abstract
> origin of another one).
>
> If someone ever faces a similar problem, the following (untested)
> patch might save them a bit of time.  I have eventually decided not to
> make it a checking-only assert because it is on a cold path and
> because at release-build optimization levels, the tail-call is
> optimized to a jump and thus an infinite loop if the described
> situation happens, and I suppose an informative ICE is better tan that
> even for users.
>
> What do you think?  Would it be reasonable for trunk even now or
> should I queue it for the next stage1?
>
> Thanks,
>
> Martin
>
>
> gcc/cp/
>
> 2016-11-28  Martin Jambor  <mjambor@suse.cz>
>
> 	* error.c (dump_decl): Add an assert that DECL_ABSTRACT_ORIGIN
> 	is not the decl itself.
Given it's on an error/debug path it ought to be plenty safe for now. 
What's more interesting is whether or not DECL_ABSTRACT_ORIGIN can 
legitimately point to itself and if so, how is that happening.

I don't think we have a checker for the basic tree datastructures, but 
maybe we ought to?

jeff

Hi Jeff,

On Mon, Nov 28, 2016 at 08:46:05AM -0700, Jeff Law wrote:
> On 11/28/2016 07:27 AM, Martin Jambor wrote:
> > Hi,
> > 
> > one of a number of symptoms of an otherwise unrelated HSA bug I've
> > been debugging today is gcc crashing or hanging in the C++ pretty
> > printer when attempting to emit a warning because dump_decl() ended up
> > in an infinite recursion calling itself on the DECL_ABSTRACT_ORIGIN of
> > the decl it was looking at, which was however the same thing.  (It was
> > set to itself on purpose in set_decl_origin_self as a part of final
> > pass, the decl was being printed because it was itself an abstract
> > origin of another one).
> > 
> > If someone ever faces a similar problem, the following (untested)
> > patch might save them a bit of time.  I have eventually decided not to
> > make it a checking-only assert because it is on a cold path and
> > because at release-build optimization levels, the tail-call is
> > optimized to a jump and thus an infinite loop if the described
> > situation happens, and I suppose an informative ICE is better tan that
> > even for users.
> > 
> > What do you think?  Would it be reasonable for trunk even now or
> > should I queue it for the next stage1?
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > gcc/cp/
> > 
> > 2016-11-28  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* error.c (dump_decl): Add an assert that DECL_ABSTRACT_ORIGIN
> > 	is not the decl itself.
> Given it's on an error/debug path it ought to be plenty safe for now. What's
> more interesting is whether or not DECL_ABSTRACT_ORIGIN can legitimately
> point to itself and if so, how is that happening.

Well, I tried to explain it in my original email but I also wanted to
be as brief as possible, so perhaps it is necessary to elaborate a bit:

There is a function set_decl_origin_self() in dwarf2out.c that does
just that, sets DECL_ABSTRACT_ORIGIN to the decl itself, and its
comment makes it clear that is intended (according to git blame, the
whole comment and much of the implementation come from 1992, though ;-)
The function is called from the "final" pass through dwarf2out_decl(),
and gen_decl_die().

So, for one reason or another, this is the intended behavior.
Apparently, after that one is not supposed to be printing the decl
name of such a "finished" a function.  It is too bad however that this
can happen if a "finished" function is itself an abstract origin of a
different one, which is optimized and expanded only afterwards and you
attempt to print its decl name, because it triggers printing the decl
name of the finished function, in turn triggering the infinite
recursion/loop.  I am quite surprised that we have not hit this
earlier (e.g. with warnings in IPA-CP clones) but perhaps there is a
reason.

I will append the patch to some bootstrap and testing run and commit
it afterwards if it passes.

Thanks,

Martin

> 
> I don't think we have a checker for the basic tree datastructures, but maybe
> we ought to?
> 
> jeff
>

On Mon, Nov 28, 2016 at 6:28 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi Jeff,
>
> On Mon, Nov 28, 2016 at 08:46:05AM -0700, Jeff Law wrote:
>> On 11/28/2016 07:27 AM, Martin Jambor wrote:
>> > Hi,
>> >
>> > one of a number of symptoms of an otherwise unrelated HSA bug I've
>> > been debugging today is gcc crashing or hanging in the C++ pretty
>> > printer when attempting to emit a warning because dump_decl() ended up
>> > in an infinite recursion calling itself on the DECL_ABSTRACT_ORIGIN of
>> > the decl it was looking at, which was however the same thing.  (It was
>> > set to itself on purpose in set_decl_origin_self as a part of final
>> > pass, the decl was being printed because it was itself an abstract
>> > origin of another one).
>> >
>> > If someone ever faces a similar problem, the following (untested)
>> > patch might save them a bit of time.  I have eventually decided not to
>> > make it a checking-only assert because it is on a cold path and
>> > because at release-build optimization levels, the tail-call is
>> > optimized to a jump and thus an infinite loop if the described
>> > situation happens, and I suppose an informative ICE is better tan that
>> > even for users.
>> >
>> > What do you think?  Would it be reasonable for trunk even now or
>> > should I queue it for the next stage1?
>> >
>> > Thanks,
>> >
>> > Martin
>> >
>> >
>> > gcc/cp/
>> >
>> > 2016-11-28  Martin Jambor  <mjambor@suse.cz>
>> >
>> >     * error.c (dump_decl): Add an assert that DECL_ABSTRACT_ORIGIN
>> >     is not the decl itself.
>> Given it's on an error/debug path it ought to be plenty safe for now. What's
>> more interesting is whether or not DECL_ABSTRACT_ORIGIN can legitimately
>> point to itself and if so, how is that happening.
>
> Well, I tried to explain it in my original email but I also wanted to
> be as brief as possible, so perhaps it is necessary to elaborate a bit:
>
> There is a function set_decl_origin_self() in dwarf2out.c that does
> just that, sets DECL_ABSTRACT_ORIGIN to the decl itself, and its
> comment makes it clear that is intended (according to git blame, the
> whole comment and much of the implementation come from 1992, though ;-)
> The function is called from the "final" pass through dwarf2out_decl(),
> and gen_decl_die().
>
> So, for one reason or another, this is the intended behavior.
> Apparently, after that one is not supposed to be printing the decl
> name of such a "finished" a function.  It is too bad however that this
> can happen if a "finished" function is itself an abstract origin of a
> different one, which is optimized and expanded only afterwards and you
> attempt to print its decl name, because it triggers printing the decl
> name of the finished function, in turn triggering the infinite
> recursion/loop.  I am quite surprised that we have not hit this
> earlier (e.g. with warnings in IPA-CP clones) but perhaps there is a
> reason.
>
> I will append the patch to some bootstrap and testing run and commit
> it afterwards if it passes.

Other users explicitely check for the self-reference when walking origins.

Richard.

> Thanks,
>
> Martin
>
>>
>> I don't think we have a checker for the basic tree datastructures, but maybe
>> we ought to?
>>
>> jeff
>>

On 11/29/2016 03:13 AM, Richard Biener wrote:
> On Mon, Nov 28, 2016 at 6:28 PM, Martin Jambor <mjambor@suse.cz> wrote:
>> Hi Jeff,
>>
>> On Mon, Nov 28, 2016 at 08:46:05AM -0700, Jeff Law wrote:
>>> On 11/28/2016 07:27 AM, Martin Jambor wrote:
>>>> Hi,
>>>>
>>>> one of a number of symptoms of an otherwise unrelated HSA bug I've
>>>> been debugging today is gcc crashing or hanging in the C++ pretty
>>>> printer when attempting to emit a warning because dump_decl() ended up
>>>> in an infinite recursion calling itself on the DECL_ABSTRACT_ORIGIN of
>>>> the decl it was looking at, which was however the same thing.  (It was
>>>> set to itself on purpose in set_decl_origin_self as a part of final
>>>> pass, the decl was being printed because it was itself an abstract
>>>> origin of another one).
>>>>
>>>> If someone ever faces a similar problem, the following (untested)
>>>> patch might save them a bit of time.  I have eventually decided not to
>>>> make it a checking-only assert because it is on a cold path and
>>>> because at release-build optimization levels, the tail-call is
>>>> optimized to a jump and thus an infinite loop if the described
>>>> situation happens, and I suppose an informative ICE is better tan that
>>>> even for users.
>>>>
>>>> What do you think?  Would it be reasonable for trunk even now or
>>>> should I queue it for the next stage1?
>>>>
>>>> Thanks,
>>>>
>>>> Martin
>>>>
>>>>
>>>> gcc/cp/
>>>>
>>>> 2016-11-28  Martin Jambor  <mjambor@suse.cz>
>>>>
>>>>     * error.c (dump_decl): Add an assert that DECL_ABSTRACT_ORIGIN
>>>>     is not the decl itself.
>>> Given it's on an error/debug path it ought to be plenty safe for now. What's
>>> more interesting is whether or not DECL_ABSTRACT_ORIGIN can legitimately
>>> point to itself and if so, how is that happening.
>>
>> Well, I tried to explain it in my original email but I also wanted to
>> be as brief as possible, so perhaps it is necessary to elaborate a bit:
>>
>> There is a function set_decl_origin_self() in dwarf2out.c that does
>> just that, sets DECL_ABSTRACT_ORIGIN to the decl itself, and its
>> comment makes it clear that is intended (according to git blame, the
>> whole comment and much of the implementation come from 1992, though ;-)
>> The function is called from the "final" pass through dwarf2out_decl(),
>> and gen_decl_die().
>>
>> So, for one reason or another, this is the intended behavior.
>> Apparently, after that one is not supposed to be printing the decl
>> name of such a "finished" a function.  It is too bad however that this
>> can happen if a "finished" function is itself an abstract origin of a
>> different one, which is optimized and expanded only afterwards and you
>> attempt to print its decl name, because it triggers printing the decl
>> name of the finished function, in turn triggering the infinite
>> recursion/loop.  I am quite surprised that we have not hit this
>> earlier (e.g. with warnings in IPA-CP clones) but perhaps there is a
>> reason.
>>
>> I will append the patch to some bootstrap and testing run and commit
>> it afterwards if it passes.
>
> Other users explicitely check for the self-reference when walking origins.
I think that makes it pretty clear that we have to handle 
self-reference.  So it seems that rather than an assert that we should 
just not walk down a self-referencing DECL_ABSTRACT_ORIGIN.

jeff

Hi,

On Tue, Nov 29, 2016 at 10:17:02AM -0700, Jeff Law wrote:
> On 11/29/2016 03:13 AM, Richard Biener wrote:
> > On Mon, Nov 28, 2016 at 6:28 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > > Hi Jeff,
> > > 
> > > On Mon, Nov 28, 2016 at 08:46:05AM -0700, Jeff Law wrote:
> > > > On 11/28/2016 07:27 AM, Martin Jambor wrote:
> > > > > Hi,
> > > > > 
> > > > > one of a number of symptoms of an otherwise unrelated HSA bug I've
> > > > > been debugging today is gcc crashing or hanging in the C++ pretty
> > > > > printer when attempting to emit a warning because dump_decl() ended up
> > > > > in an infinite recursion calling itself on the DECL_ABSTRACT_ORIGIN of
> > > > > the decl it was looking at, which was however the same thing.  (It was
> > > > > set to itself on purpose in set_decl_origin_self as a part of final
> > > > > pass, the decl was being printed because it was itself an abstract
> > > > > origin of another one).
> > > > > 
> > > > > If someone ever faces a similar problem, the following (untested)
> > > > > patch might save them a bit of time.  I have eventually decided not to
> > > > > make it a checking-only assert because it is on a cold path and
> > > > > because at release-build optimization levels, the tail-call is
> > > > > optimized to a jump and thus an infinite loop if the described
> > > > > situation happens, and I suppose an informative ICE is better tan that
> > > > > even for users.
> > > > > 
> > > > > What do you think?  Would it be reasonable for trunk even now or
> > > > > should I queue it for the next stage1?
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Martin
> > > > > 
> > > > > 
> > > > > gcc/cp/
> > > > > 
> > > > > 2016-11-28  Martin Jambor  <mjambor@suse.cz>
> > > > > 
> > > > >     * error.c (dump_decl): Add an assert that DECL_ABSTRACT_ORIGIN
> > > > >     is not the decl itself.
> > > > Given it's on an error/debug path it ought to be plenty safe for now. What's
> > > > more interesting is whether or not DECL_ABSTRACT_ORIGIN can legitimately
> > > > point to itself and if so, how is that happening.
> > > 
> > > Well, I tried to explain it in my original email but I also wanted to
> > > be as brief as possible, so perhaps it is necessary to elaborate a bit:
> > > 
> > > There is a function set_decl_origin_self() in dwarf2out.c that does
> > > just that, sets DECL_ABSTRACT_ORIGIN to the decl itself, and its
> > > comment makes it clear that is intended (according to git blame, the
> > > whole comment and much of the implementation come from 1992, though ;-)
> > > The function is called from the "final" pass through dwarf2out_decl(),
> > > and gen_decl_die().
> > > 
> > > So, for one reason or another, this is the intended behavior.
> > > Apparently, after that one is not supposed to be printing the decl
> > > name of such a "finished" a function.  It is too bad however that this
> > > can happen if a "finished" function is itself an abstract origin of a
> > > different one, which is optimized and expanded only afterwards and you
> > > attempt to print its decl name, because it triggers printing the decl
> > > name of the finished function, in turn triggering the infinite
> > > recursion/loop.  I am quite surprised that we have not hit this
> > > earlier (e.g. with warnings in IPA-CP clones) but perhaps there is a
> > > reason.
> > > 
> > > I will append the patch to some bootstrap and testing run and commit
> > > it afterwards if it passes.
> > 
> > Other users explicitely check for the self-reference when walking origins.
> I think that makes it pretty clear that we have to handle self-reference.
> So it seems that rather than an assert that we should just not walk down a
> self-referencing DECL_ABSTRACT_ORIGIN.
> 

I'm not sure what you mean by "walk down."  The code in dump_decl()
that deals with function decls is:

    case FUNCTION_DECL:
      if (! DECL_LANG_SPECIFIC (t))
	{
	  if (DECL_ABSTRACT_ORIGIN (t))
	    dump_decl (pp, DECL_ABSTRACT_ORIGIN (t), flags);
	  else
	    pp_string (pp, M_("<built-in>"));
	}
      else if (DECL_GLOBAL_CTOR_P (t) || DECL_GLOBAL_DTOR_P (t))
	dump_global_iord (pp, t);
      else
	dump_function_decl (pp, t, flags);
      break;

I suppose that there are good reasons for treating
!DECL_LANG_SPECIFIC(t) specially and not pass it down to
dump_function_decl, even if DECL_ABSTRACT_ORIGIN (t) == t.  But
printing <built-in>, though perhaps better than an ICE or hang, feels
also wrong and we already print it when we shouldn't (see PR 78589).

So I wonder what the options are... perhaps it seems that we can call
dump_function_name which starts with code handling
!DECL_LANG_SPECIFIC(t) cases, even instead of the weird <built-in>
thing?

I guess I'll give it a try later this week.

Thanks,

Martin