[DWARF] mark partial fn versions and OMP frags as partial in dwarf2+ debug info

debug info: partial noentry functions: infra

This is the first patch of a set that addresses two different but
somewhat related issues.

On the one hand, after partial inlining, the non-inlined function
fragment is output in a way that debug info consumers can't distinguish
from the entire function: debug info lists the entire function as
abstract origin for the fragment, but nothing that indicates the
fragment does not stand for the entire function.  So, if a debugger is
asked to set a breakpoint at the entry point of the function, it might
very well set one at the entry point of the fragment, which is likely
not where users expect to stop.

On the other hand, OpenMP blocks are split out into artificial functions
that do not indicate their executable code is part of another function.
The artificial functions are nested within the original function, but
that's hardly enough: ideally, debug info consumers should be able to
tell that, if they stop within one of these functions, they're
abstractly within the original function.

This patch introduces a new DWARF attribute to indicate that a function
is a partial copy of its abstract origin, specifically, that its entry
point does not correspond to the entry point of the abstract origin.
This attribute can then be used to mark the out-of-line portion of
partial inlines, and OpenMP blocks split out into artificial functions.


This patchset was regstrapped on x86_64-linux-gnu and i686-linux-gnu.

Ok to install the first patch? (infrastructure)

Ok to install the second patch? (function versioning)

Ok to install the third patch? (OpenMP fragments)


for  include/ChangeLog

	* dwarf2.def (DW_AT_GNU_partial_noentry): New.

for  gcc/ChangeLog

	* tree-core.h (tree_function_decl): Drop unused
	tm_clone_flag.  Add partial_copy_flag.
	* tree.h (DECL_FUNCTION_PARTIAL_COPY): New.
	* dwarf2out.c (checksum_attributes): Add at_partial_noentry.
	(collect_checksum_attributes): Set it.
	(die_checksum_ordered): Checksum it.
	(gen_subprogam_die): Keep the old die if it the partial copy
	flag matches the partial noentry attribute.  Set the attribute
	as needed.
---
 gcc/dwarf2out.c    |   11 ++++++++++-
 gcc/tree-core.h    |    2 +-
 gcc/tree.h         |   10 ++++++++++
 include/dwarf2.def |    3 +++
 4 files changed, 24 insertions(+), 2 deletions(-)

On 11/15/2017 12:05 AM, Alexandre Oliva wrote:
> debug info: partial noentry functions: infra
> 
> This is the first patch of a set that addresses two different but
> somewhat related issues.
> 
> On the one hand, after partial inlining, the non-inlined function
> fragment is output in a way that debug info consumers can't distinguish
> from the entire function: debug info lists the entire function as
> abstract origin for the fragment, but nothing that indicates the
> fragment does not stand for the entire function.  So, if a debugger is
> asked to set a breakpoint at the entry point of the function, it might
> very well set one at the entry point of the fragment, which is likely
> not where users expect to stop.
> 
> On the other hand, OpenMP blocks are split out into artificial functions
> that do not indicate their executable code is part of another function.
> The artificial functions are nested within the original function, but
> that's hardly enough: ideally, debug info consumers should be able to
> tell that, if they stop within one of these functions, they're
> abstractly within the original function.
> 
> This patch introduces a new DWARF attribute to indicate that a function
> is a partial copy of its abstract origin, specifically, that its entry
> point does not correspond to the entry point of the abstract origin.
> This attribute can then be used to mark the out-of-line portion of
> partial inlines, and OpenMP blocks split out into artificial functions.
> 
> 
> This patchset was regstrapped on x86_64-linux-gnu and i686-linux-gnu.
> 
> Ok to install the first patch? (infrastructure)
> 
> Ok to install the second patch? (function versioning)
> 
> Ok to install the third patch? (OpenMP fragments)
These look generally OK to me, but I'd like Jakub to chime in -- he's
got some state on the issues around OMP debugging and how it ought to be
structured.

Jakub, care to chime in?

jeff

On Wed, Nov 15, 2017 at 05:05:36AM -0200, Alexandre Oliva wrote:
> debug info: partial noentry functions: infra
> 
> This is the first patch of a set that addresses two different but
> somewhat related issues.
> 
> On the one hand, after partial inlining, the non-inlined function
> fragment is output in a way that debug info consumers can't distinguish
> from the entire function: debug info lists the entire function as
> abstract origin for the fragment, but nothing that indicates the
> fragment does not stand for the entire function.  So, if a debugger is
> asked to set a breakpoint at the entry point of the function, it might
> very well set one at the entry point of the fragment, which is likely
> not where users expect to stop.
> 
> On the other hand, OpenMP blocks are split out into artificial functions
> that do not indicate their executable code is part of another function.
> The artificial functions are nested within the original function, but
> that's hardly enough: ideally, debug info consumers should be able to
> tell that, if they stop within one of these functions, they're
> abstractly within the original function.
> 
> This patch introduces a new DWARF attribute to indicate that a function
> is a partial copy of its abstract origin, specifically, that its entry
> point does not correspond to the entry point of the abstract origin.
> This attribute can then be used to mark the out-of-line portion of
> partial inlines, and OpenMP blocks split out into artificial functions.

I'm not sure I like the attribute name too much, and more importantly,
as I said before, I think the attribute should not be a flag, but a number
which tells not just that it is an outlined portion of the original
subprogram, but also what kind of outlining it is and its purpose.

For the name, I wonder if instead of
DW_AT_GNU_partial_noentry
it wouldn't be better to use e.g. one of:
DW_AT_GNU_partial
DW_AT_GNU_partial_subprogram
DW_AT_GNU_fragment
DW_AT_GNU_subprogram_fragment
As for the values I'd like to see (see e.g. DW_AT_calling_convention
and corresponding DW_CC_* values and many other examples):
1) 0 value representing a default false, that the DW_TAG_subprogram
is not any kind of subprogram fragment
2) some value for partial inlining, perhaps two if we want to mark
both fragments of the inline created by partial inlining - the
entry fragment and the outlined rest of the function
3) OpenMP outlined parallel region
4) OpenMP outlined task region
5) OpenMP outlined target region
6) OpenACC outlined kernels region
7) OpenACC outlined parallel region
Thus we would have DW_GNU_PARTIAL_* constants in some enum that we'd use
here.  Of course, the single DECL_FUNCTION_PARTIAL_COPY bit wouldn't
be enough to cover these cases, but I guess we could add an attribute
with a space in the name if this bit is set to say which of those it is
(or just use the attribute unconditionally and don't reserve a bit for
that)?

The advantage of having more details is that the debug info consumer can
then decide how to handle, say talk to OMPD to find out the parent thread,
or look it up inside of libgomp (say through infinity notes), whatever.

And we could in the future add other kinds if we start outlining for other
reasons.

	Jakub

On Nov 21, 2017, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Nov 15, 2017 at 05:05:36AM -0200, Alexandre Oliva wrote:

>> This patch introduces a new DWARF attribute to indicate that a function
>> is a partial copy of its abstract origin, specifically, that its entry
>> point does not correspond to the entry point of the abstract origin.
>> This attribute can then be used to mark the out-of-line portion of
>> partial inlines, and OpenMP blocks split out into artificial functions.

> I'm not sure I like the attribute name too much, and more importantly,
> as I said before, I think the attribute should not be a flag, but a number
> which tells not just that it is an outlined portion of the original
> subprogram, but also what kind of outlining it is and its purpose.

I suppose you don't like it because it means something other than what
you suggested.  I'd taken note of your suggestion, and even asked for
more details back then, but none of the debug info consumers seemed to
have changed their mind to the initial assessment that a single boolean
flag would suffice for the purposes of telling whether or not the entry
point of a given (implementation) subprogram should be covered by a
breakpoint at a given (source-level) subprogram.

We could supply a lot more detail about how functions are split, and
why, but if debug info consumers have no way to make use of the info, or
have no interest in doing so, I don't see that makes sense to waste
compiler resources preserving and generating such additional
information.

This flag serves a simple purpose that doesn't require debug info
consumers to understand the implications of such relationships as a
function having multiple openmp blocks split out of it, before being
further split into inline and out-of-line, and then have the out-of-line
portion further versioned and then split into inline and out-of-line.
A debug info consumer can deal with that regardless of our providing
additional detail about the purpose of the splits elsewhere.

Now consider that we mark both inline and out-of-line functions with the
same attribute and different nonzero values, and that we assign
different values for the various kinds of split-out blocks, some of
which a debug info consumer doesn't know about.  Should it then stop at
the entry point of that fragment when a breakpoint is set on a function,
or should it not?  (if there's more than one nonzero value for which the
fragment encompasses the entry point, a conservative debug info consumer
won't know what to do)

So, you see, the additional amount of detail, along with its future
expansion, actually gets in the way of solving the one problem I've set
out to solve.

What's more, there's active interest in using the information supplied
by the patchset I posted, whereas no debug info consumer (so far, at
least) has reacted to your suggestion that we produce the additional
layer of detail.  And yet, should such interest appear at any time, we
could added it, as a separate attribute, so that it does NOT counter the
purpose of the partial_noentry attribute, providing instead just
additional detail.  The existence of the partial_noentry boolean
attribute will then not even cost anything extra: it will always go
together with the additional attribute or attributes that supply the
additional information that consumers might wish us to supply, so that
the boolean flag will just be an entry in the abbrev tables always next
to the extra-detail attribute.

> The advantage of having more details is that the debug info consumer can
> then decide how to handle, say talk to OMPD to find out the parent thread,
> or look it up inside of libgomp (say through infinity notes), whatever.

I'm afraid I still don't see what would be achieved with that, but I
suspect that if there's useful debug info consumer behavior to be
achieved, this one bit or enum won't suffice.  That being the case, I
suspect the additional information on top of the enum might very well
turn out to be enough along with the bit as well.  But this is all
speculation on my part: I'm not aware of demands from debug info users
or consumers for whatever you have in mind, and thus I certainly haven't
taken any such demands into account.  The demands I had were to provide
debug info consumers with a way to avoid setting unexpected breakpoints
at out-of-line fragments of partial inlines, and I realized the same
problem could affect openmp split-out blocks as well.  I have consulted
with debug info consumers about the presently proposed solution, and
other more detailed ones, and the simpler one is what came out ahead.

> And we could in the future add other kinds if we start outlining for other
> reasons.

Please realize that this possibility would make their jobs harder,
unless it is implemented as an add-on to the present one (i.e., another
attribute).  For this reason, I suggest we put this more detailed
suggestion aside until such time as debug info consumers display
interest in using such information.  Then, we proceed to design a proper
solution that provides them with *all* the required information to
implement the debugging plans you have in mind.  But throwing in more
detail just because we could, in the hope it might be useful, in this
case will actually fail to address the problem at hand.

Does that work for you?

Thanks,

On Wed, Nov 22, 2017 at 02:40:39AM -0200, Alexandre Oliva wrote:
> On Nov 21, 2017, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > On Wed, Nov 15, 2017 at 05:05:36AM -0200, Alexandre Oliva wrote:
> 
> >> This patch introduces a new DWARF attribute to indicate that a function
> >> is a partial copy of its abstract origin, specifically, that its entry
> >> point does not correspond to the entry point of the abstract origin.
> >> This attribute can then be used to mark the out-of-line portion of
> >> partial inlines, and OpenMP blocks split out into artificial functions.
> 
> > I'm not sure I like the attribute name too much, and more importantly,
> > as I said before, I think the attribute should not be a flag, but a number
> > which tells not just that it is an outlined portion of the original
> > subprogram, but also what kind of outlining it is and its purpose.
> 
> I suppose you don't like it because it means something other than what
> you suggested.  I'd taken note of your suggestion, and even asked for
> more details back then, but none of the debug info consumers seemed to
> have changed their mind to the initial assessment that a single boolean
> flag would suffice for the purposes of telling whether or not the entry
> point of a given (implementation) subprogram should be covered by a
> breakpoint at a given (source-level) subprogram.

Let's take them into a loop again.

> We could supply a lot more detail about how functions are split, and
> why, but if debug info consumers have no way to make use of the info, or
> have no interest in doing so, I don't see that makes sense to waste
> compiler resources preserving and generating such additional
> information.

The thing is, the different kinds of outlined regions work very differently.
Say you have:

int bar (int);
static inline int foo (int x, int y)
{
  int z = x + 9 * y;
  if (x < 20)
    return y;
  return bar (bar (bar (bar (x + y * 7))));
}
as an example of possibly partially inlined function and
void
baz (int x, int y)
{
  int z = x + 9 * y;
  #pragma omp parallel for
  for (int i = 0; i < 60; i++)
    bar (i);
}
as an example of OpenMP parallel region.  Now, if inside of the outlined
regions (bar (bar (bar (bar (x + y * 7)))); in the first case and the
#pragma omp for in the second case) you want in the debugger to query
the value of z, which isn't used in those outlined regions and thus there
is likely no DW_TAG_variable for it in the outlined region, you need to do
something quite different.  In the first case just look up the caller,
and if the innermost inlined region has the same abstract origin as the
outlined region, query the z variable there.
In the OpenMP region, it isn't a parent that you need to look up though.
In the master thread, it is a grand+ parent, where you need to skip up all
the frames that belong to the OpenMP runtime library, in other threads you
need to talk to the runtime library to find what the parent thread is and
unwind there to some frame above the frames that belong to the OpenMP
runtime library.
OpenMP task region is again quite different from this, in that the parent
might still exist but might not, the task can be invoked after the function
spawning it returned if there is no thread synchronization before that (or
might at least leave some lexical scopes).
If we have an attribute like I'm proposing, you can easily test it even as a
boolean if there is some handling common to all the outlined region kinds
(just compare the value against 0). 

> This flag serves a simple purpose that doesn't require debug info
> consumers to understand the implications of such relationships as a
> function having multiple openmp blocks split out of it, before being
> further split into inline and out-of-line, and then have the out-of-line
> portion further versioned and then split into inline and out-of-line.
> A debug info consumer can deal with that regardless of our providing
> additional detail about the purpose of the splits elsewhere.
> 
> Now consider that we mark both inline and out-of-line functions with the
> same attribute and different nonzero values, and that we assign
> different values for the various kinds of split-out blocks, some of
> which a debug info consumer doesn't know about.  Should it then stop at
> the entry point of that fragment when a breakpoint is set on a function,
> or should it not?  (if there's more than one nonzero value for which the
> fragment encompasses the entry point, a conservative debug info consumer
> won't know what to do)

If we have a value for the partial inlining inlined outer part, then yes,
the debugger would need to check for two values (if we use 1 for the inlined
outer part, then <= 1 check), it is unlikely further outlined regions of
that kind would be needed.  If we don't mark the partial inlining inlined
outer part, then just != 0.  My point is when we are adding a new attribute,
we shouldn't look just for a single consumer purpose when we actually
already now know about multiple other uses where we'll need to know more
details, and we don't want to have dozens of attributes just for this
purpose.  Furthermore, in DWARF5 one can use DW_FORM_implicit_const and can
have the enum occupy no space in the DIE like a flag would in DWARF4.
Otherwise your flag attribute would have to be always coupled with an enum
attribute telling the reason.  The cost of .debug_abbrev entries isn't
negligible.

	Jakub