Message ID | ord14kyqbj.fsf@lxoliva.fsfla.org |
---|---|
State | New |
Headers | show |
Series | [DWARF] mark partial fn versions and OMP frags as partial in dwarf2+ debug info | expand |
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
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 76a538f1ff97..db0bc12a1f46 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -6944,6 +6944,7 @@ struct checksum_attributes dw_attr_node *at_virtuality; dw_attr_node *at_visibility; dw_attr_node *at_vtable_elem_location; + dw_attr_node *at_partial_noentry; }; /* Collect the attributes that we will want to use for the checksum. */ @@ -7108,6 +7109,9 @@ collect_checksum_attributes (struct checksum_attributes *attrs, dw_die_ref die) case DW_AT_vtable_elem_location: attrs->at_vtable_elem_location = a; break; + case DW_AT_GNU_partial_noentry: + attrs->at_partial_noentry = a; + break; default: break; } @@ -7183,6 +7187,7 @@ die_checksum_ordered (dw_die_ref die, struct md5_ctx *ctx, int *mark) CHECKSUM_ATTR (attrs.at_type); CHECKSUM_ATTR (attrs.at_friend); CHECKSUM_ATTR (attrs.at_alignment); + CHECKSUM_ATTR (attrs.at_partial_noentry); /* Checksum the child DIEs. */ c = die->die_child; @@ -21933,7 +21938,9 @@ gen_subprogram_die (tree decl, dw_die_ref context_die) if (old_die && old_die->die_parent == NULL) add_child_die (context_die, old_die); - if (old_die && get_AT_ref (old_die, DW_AT_abstract_origin)) + if (old_die && get_AT_ref (old_die, DW_AT_abstract_origin) + && (DECL_FUNCTION_PARTIAL_COPY (decl) + == get_AT_flag (old_die, DW_AT_GNU_partial_noentry))) { /* If we have a DW_AT_abstract_origin we have a working cached version. */ @@ -21943,6 +21950,8 @@ gen_subprogram_die (tree decl, dw_die_ref context_die) { subr_die = new_die (DW_TAG_subprogram, context_die, decl); add_abstract_origin_attribute (subr_die, origin); + if (DECL_FUNCTION_PARTIAL_COPY (decl)) + add_AT_flag (subr_die, DW_AT_GNU_partial_noentry, true); /* This is where the actual code for a cloned function is. Let's emit linkage name attribute for it. This helps debuggers to e.g, set breakpoints into diff --git a/gcc/tree-core.h b/gcc/tree-core.h index f74f1453de6d..507016db23e9 100644 --- a/gcc/tree-core.h +++ b/gcc/tree-core.h @@ -1784,7 +1784,7 @@ struct GTY(()) tree_function_decl { unsigned pure_flag : 1; unsigned looping_const_or_pure_flag : 1; unsigned has_debug_args_flag : 1; - unsigned tm_clone_flag : 1; + unsigned partial_copy_flag : 1; unsigned versioned_function : 1; /* No bits left. */ }; diff --git a/gcc/tree.h b/gcc/tree.h index 39acffe52662..12c0b3835d9f 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3002,6 +3002,16 @@ extern vec<tree, va_gc> **decl_debug_args_insert (tree); #define DECL_FUNCTION_VERSIONED(NODE)\ (FUNCTION_DECL_CHECK (NODE)->function_decl.versioned_function) +/* In FUNCTION_DECL, this is set if this function is only a partial + clone/version/copy of its DECL_ABSTRACT_ORIGIN. It should be used + for the non-inlined portion of a partial inline, for openmp blocks + turned into separate functions, and for any other cases in which we + clone only select parts of a function, presumably omitting its + entry point, that is presumed to remain in a separate, controlling + version of the function. */ +#define DECL_FUNCTION_PARTIAL_COPY(NODE) \ + (FUNCTION_DECL_CHECK (NODE)->function_decl.partial_copy_flag) + /* In FUNCTION_DECL, this is set if this function is a C++ constructor. Devirtualization machinery uses this knowledge for determing type of the object constructed. Also we assume that constructor address is not diff --git a/include/dwarf2.def b/include/dwarf2.def index 2a3b23fef873..f2dd9196b039 100644 --- a/include/dwarf2.def +++ b/include/dwarf2.def @@ -433,6 +433,9 @@ DW_AT (DW_AT_GNU_all_source_call_sites, 0x2118) DW_AT (DW_AT_GNU_macros, 0x2119) /* Attribute for C++ deleted special member functions (= delete;). */ DW_AT (DW_AT_GNU_deleted, 0x211a) +/* This flag indicates a partial copy of a function, whose entry point + does not correspond to that of the abstract origin. */ +DW_AT (DW_AT_GNU_partial_noentry, 0x211b) /* Extensions for Fission. See http://gcc.gnu.org/wiki/DebugFission. */ DW_AT (DW_AT_GNU_dwo_name, 0x2130) DW_AT (DW_AT_GNU_dwo_id, 0x2131) debug info: partial noentry functions: partial inlines When we version a function without copying all of its blocks, particularly when we omit or modify the entry point, mark the function as a partial copy. This should enable debug info consumers to avoid setting a breakpoint in the partial copy when the user requests a breakpoint at the function's entry point. An alternate entry point is specified when versioning a function only as we inline part of it; the non-inlined portion is what goes in the new version. Such partial copies used to refer back to the entire function as their abstract origin. Without the partial copy marker, debug info consumers would set a breakpoint for the function at the inlined entry point, and also at the partial copy's entry point, although the latter is not the actual entry point for the function. for gcc/ChangeLog * tree-inline.c (tree_function_versioning): Mark a version as a partial copy when an alternate entry point is given, and when versioning a partial copy. --- gcc/tree-inline.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index d4aa5bed7398..df0f8cabccc1 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -5829,6 +5829,9 @@ tree_function_versioning (tree old_decl, tree new_decl, old_version_node->used_as_abstract_origin = true; DECL_FUNCTION_PERSONALITY (new_decl) = DECL_FUNCTION_PERSONALITY (old_decl); + DECL_FUNCTION_PARTIAL_COPY (new_decl) = new_entry + || DECL_FUNCTION_PARTIAL_COPY (old_decl); + /* Prepare the data structures for the tree copy. */ memset (&id, 0, sizeof (id)); debug info: partial noentry functions: omp split-out blocks We emit some OMP blocks as separate functions that are nested in, but not otherwise related with the function they were originally part of. This patch makes such newly-created artificial functions refer back to the original function as their abstract origin, but as partial copies, so that they're not mistaken as the whole function. This enables line numbers in the split-out block to still be recognized as part of the original function. It may, however, cause debug information consumers that do not support the partial copy attribute to mistake the nested functions as the whole function. for gcc/ChangeLog * omp-low.c (create_omp_child_function): Mark newly-created function as a partial copy of the original one. Mark the original function's cgraph node as used as abstract origin. --- gcc/omp-low.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gcc/omp-low.c b/gcc/omp-low.c index 33e633cd627b..40166f9bfb3a 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -1633,6 +1633,10 @@ create_omp_child_function (omp_context *ctx, bool task_copy) = DECL_FUNCTION_SPECIFIC_TARGET (current_function_decl); DECL_FUNCTION_VERSIONED (decl) = DECL_FUNCTION_VERSIONED (current_function_decl); + DECL_FUNCTION_PARTIAL_COPY (decl) = 1; + DECL_ABSTRACT_ORIGIN (decl) = DECL_ORIGIN (current_function_decl); + if (DECL_ORIGIN (current_function_decl) == current_function_decl) + cgraph_node::get_create (current_function_decl)->used_as_abstract_origin = true; if (omp_maybe_offloaded_ctx (ctx)) {