| Message ID | orfu5ix62w.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PR84620] output symbolic entry_view as data2, not addr | expand |
On Mar 2, 2018, Alexandre Oliva <aoliva@redhat.com> wrote: > Mark Wielaard is implementing support for LVU and IEPM in elfutils, and > he was surprised by the encoding of DW_AT_GNU_entry_view; so was I! > When GCC computes and outputs views internally (broken without internal > view resets), it outputs entry_view attributes as data: it knows the > view numbers. However, when it uses the assembler to compute the views > symbolically, it has to output the view symbolically. > I'd chosen lbl_id to that end, without giving it much thought, but that > was no good: it's not just space-inefficient, it uses an addr encoding, > indirect in some cases, for something that's not a real label, but > rather a a small assembler data constant. Oops. Jakub wrote (in the BZ): > I meant to say: > The char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; line > should come at the and of the union, not before the other classes. I've moved the union field down in the revised patch below, but I don't see the point, and I thought it would be better to keep it close to logically-similar entries. If the point is just to make it parallel to the order of the enum (which manh other entries don't seem to have cared about), maybe moving the enum would be better? > What guarantees the symview symbols always fit into 16 bits? Does > something punt if it needs more than 65536 views? There's no guarantee it will fit in 16 bits; the assembler will warn if it truncates a view number. There's no real upper limit, so uleb128 would be ideal, but since that's not viable ATM, I had to pick something, and 16 bits would cover all really useful cases than 32 or even 64 bits would, while being more compact. I was even tempted to go with 8 bits, but thought that was pushing it. I can make it 32 if you consider it better. Or maybe a param? > The FIXMEs don't really look helpful, we are not going to change the > offset computation from compile time to assemble time, that would be > terribly expensive. Why do you say it would be terribly expensive to let the assembler compute offsets in .debug_info? [IEPM] [PR debug/84620] use constant form for DW_AT_GNU_entry_view When outputting entry views in symbolic mode, we used to use a lbl_id, but that outputs the view as an addr, perhaps even in an indirect one, which is all excessive and undesirable for a small assembler-computed constant. Introduce a new value class for symbolic views, so that we can output the labels as constant data, using small forms that should suffice for any symbolic views. Ideally, we'd use uleb128, but then the compiler would have to defer .debug_info offset computation to the assembler. I'm not going there for now, so a symbolic uleb128 assembler constant in an attribute is not something GCC can deal with ATM. for gcc/ChangeLog PR debug/84620 * dwarf2out.h (dw_val_class): Add dw_val_class_symview. (dw_val_node): Add val_symbolic_view. * dwarf2out.c (dw_val_equal_p): Handle symview. (add_AT_symview): New. (print_dw_val): Handle symview. (attr_checksum, attr_checksum_ordered): Likewise. (same_dw_val_p, size_of_die): Likewise. (value_format, output_die): Likewise. (add_high_low_attributes): Use add_AT_symview for entry_view. --- gcc/dwarf2out.c | 40 +++++++++++++++++++++++++++++++++++++++- gcc/dwarf2out.h | 4 +++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 41bb11558f69..b52edee845f2 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -1434,6 +1434,8 @@ dw_val_equal_p (dw_val_node *a, dw_val_node *b) return a->v.val_die_ref.die == b->v.val_die_ref.die; case dw_val_class_fde_ref: return a->v.val_fde_index == b->v.val_fde_index; + case dw_val_class_symview: + return strcmp (a->v.val_symbolic_view, b->v.val_symbolic_view) == 0; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -3600,6 +3602,7 @@ static addr_table_entry *add_addr_table_entry (void *, enum ate_kind); static void remove_addr_table_entry (addr_table_entry *); static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool); static inline rtx AT_addr (dw_attr_node *); +static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *); @@ -5114,6 +5117,21 @@ add_AT_vms_delta (dw_die_ref die, enum dwarf_attribute attr_kind, add_dwarf_attr (die, &attr); } +/* Add a symbolic view identifier attribute value to a DIE. */ + +static inline void +add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind, + const char *view_label) +{ + dw_attr_node attr; + + attr.dw_attr = attr_kind; + attr.dw_attr_val.val_class = dw_val_class_symview; + attr.dw_attr_val.val_entry = NULL; + attr.dw_attr_val.v.val_symbolic_view = xstrdup (view_label); + add_dwarf_attr (die, &attr); +} + /* Add a label identifier attribute value to a DIE. */ static inline void @@ -6457,6 +6475,9 @@ print_dw_val (dw_val_node *val, bool recurse, FILE *outfile) fprintf (outfile, "delta: @slotcount(%s-%s)", val->v.val_vms_delta.lbl2, val->v.val_vms_delta.lbl1); break; + case dw_val_class_symview: + fprintf (outfile, "view: %s", val->v.val_symbolic_view); + break; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -6828,6 +6849,7 @@ attr_checksum (dw_attr_node *at, struct md5_ctx *ctx, int *mark) case dw_val_class_fde_ref: case dw_val_class_vms_delta: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7124,6 +7146,7 @@ attr_checksum_ordered (enum dwarf_tag tag, dw_attr_node *at, break; case dw_val_class_fde_ref: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7624,6 +7647,9 @@ same_dw_val_p (const dw_val_node *v1, const dw_val_node *v2, int *mark) case dw_val_class_die_ref: return same_die_p (v1->v.val_die_ref.die, v2->v.val_die_ref.die, mark); + case dw_val_class_symview: + return strcmp (v1->v.val_symbolic_view, v2->v.val_symbolic_view) == 0; + case dw_val_class_fde_ref: case dw_val_class_vms_delta: case dw_val_class_lbl_id: @@ -9284,6 +9310,10 @@ size_of_die (dw_die_ref die) size += csize; } break; + case dw_val_class_symview: + /* FIXME: Use uleb128 rather than data2. */ + size += 2; + break; case dw_val_class_const_implicit: case dw_val_class_unsigned_const_implicit: case dw_val_class_file_implicit: @@ -9732,6 +9762,9 @@ value_format (dw_attr_node *a) default: return DW_FORM_block1; } + case dw_val_class_symview: + /* FIXME: Use uleb128 rather than data2. */ + return DW_FORM_data2; case dw_val_class_vec: switch (constant_size (a->dw_attr_val.v.val_vec.length * a->dw_attr_val.v.val_vec.elt_size)) @@ -10497,6 +10530,11 @@ output_die (dw_die_ref die) } break; + case dw_val_class_symview: + /* FIXME: Use uleb128 rather than data2. */ + dw2_asm_output_addr (2, a->dw_attr_val.v.val_symbolic_view, "%s", name); + break; + case dw_val_class_const_implicit: if (flag_debug_asm) fprintf (asm_out_file, "\t\t\t%s %s (" @@ -23809,7 +23847,7 @@ add_high_low_attributes (tree stmt, dw_die_ref die) indirecting them through a table might make things easier, but even that would be more wasteful, space-wise, than what we have now. */ - add_AT_lbl_id (die, DW_AT_GNU_entry_view, label); + add_AT_symview (die, DW_AT_GNU_entry_view, label); } } diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h index a1856a5185e2..a0ba414014d4 100644 --- a/gcc/dwarf2out.h +++ b/gcc/dwarf2out.h @@ -161,7 +161,8 @@ enum dw_val_class dw_val_class_const_implicit, dw_val_class_unsigned_const_implicit, dw_val_class_file_implicit, - dw_val_class_view_list + dw_val_class_view_list, + dw_val_class_symview }; /* Describe a floating point constant value, or a vector constant value. */ @@ -233,6 +234,7 @@ struct GTY(()) dw_val_node { } GTY ((tag ("dw_val_class_vms_delta"))) val_vms_delta; dw_discr_value GTY ((tag ("dw_val_class_discr_value"))) val_discr_value; dw_discr_list_ref GTY ((tag ("dw_val_class_discr_list"))) val_discr_list; + char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; } GTY ((desc ("%1.val_class"))) v; };
On Tue, Mar 06, 2018 at 03:13:11AM -0300, Alexandre Oliva wrote: > On Mar 2, 2018, Alexandre Oliva <aoliva@redhat.com> wrote: > > > Mark Wielaard is implementing support for LVU and IEPM in elfutils, and > > he was surprised by the encoding of DW_AT_GNU_entry_view; so was I! > > When GCC computes and outputs views internally (broken without internal > > view resets), it outputs entry_view attributes as data: it knows the > > view numbers. However, when it uses the assembler to compute the views > > symbolically, it has to output the view symbolically. > > > I'd chosen lbl_id to that end, without giving it much thought, but that > > was no good: it's not just space-inefficient, it uses an addr encoding, > > indirect in some cases, for something that's not a real label, but > > rather a a small assembler data constant. Oops. > > Jakub wrote (in the BZ): > > > I meant to say: > > The char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; line > > should come at the and of the union, not before the other classes. > > I've moved the union field down in the revised patch below, but I don't > see the point, and I thought it would be better to keep it close to > logically-similar entries. If the point is just to make it parallel to > the order of the enum (which manh other entries don't seem to have cared > about), maybe moving the enum would be better? I think the order should match the order of the dw_val_class entries and should be sorted from the most commonly used ones (ones used by most different attributes etc.), so that somebody trying to learn dwarf2out stuff can learn it more easily (say the dw_val_class_const, dw_val_class_const_unsigned, dw_val_class_flag first etc.), but apparently it is completely random already, I'll defer to Jason what he wants. > > What guarantees the symview symbols always fit into 16 bits? Does > > something punt if it needs more than 65536 views? > > There's no guarantee it will fit in 16 bits; the assembler will warn if > it truncates a view number. There's no real upper limit, so uleb128 > would be ideal, but since that's not viable ATM, I had to pick > something, and 16 bits would cover all really useful cases than 32 or > even 64 bits would, while being more compact. I was even tempted to go > with 8 bits, but thought that was pushing it. I can make it 32 if you > consider it better. Or maybe a param? I'm afraid I haven't understood yet why we need labels instead of compiler assigned numbers for the views, so not really sure I can answer this. Can the compiler count how many different view labels it has produced or will produce, or at least compute easily an upper bound, and decide on the form based on that? Or what will exactly happen if you use too small form? Silent wrong-debug, assembler error, something else? > > The FIXMEs don't really look helpful, we are not going to change the > > offset computation from compile time to assemble time, that would be > > terribly expensive. > > Why do you say it would be terribly expensive to let the assembler compute > offsets in .debug_info? Because people are already complaining about slow assembly times of debug info, and offsets are used pretty much everywhere in .debug_info. With some assemblers that don't allow .uleb128/.sleb128 that is impossible to do, with others you'd need to emit a new label before every DIE and all DW_AT_type and many others would need to use .LD123456-.LD1092456 kind of expressions which assembler would need to parse and resolve (DW_FORM_ref{1,2,4,8,_udata}). Jakub
On Tue, Mar 6, 2018 at 2:28 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Mar 06, 2018 at 03:13:11AM -0300, Alexandre Oliva wrote: >> Jakub wrote (in the BZ): >> >> > I meant to say: >> > The char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; line >> > should come at the and of the union, not before the other classes. >> >> I've moved the union field down in the revised patch below, but I don't >> see the point, and I thought it would be better to keep it close to >> logically-similar entries. If the point is just to make it parallel to >> the order of the enum (which manh other entries don't seem to have cared >> about), maybe moving the enum would be better? > > I think the order should match the order of the dw_val_class entries and > should be sorted from the most commonly used ones (ones used by most > different attributes etc.), so that somebody trying to learn dwarf2out > stuff can learn it more easily (say the dw_val_class_const, > dw_val_class_const_unsigned, dw_val_class_flag first etc.), but apparently > it is completely random already, I'll defer to Jason what he wants. I don't think the order of the GTY tags matters much, it's just boilerplate. Jason
On Mar 6, 2018, Jakub Jelinek <jakub@redhat.com> wrote: > I'm afraid I haven't understood yet why we need labels instead of compiler > assigned numbers for the views, The compiler can't determine points at which the insn address changes reliably (or at all). It's impossible in the general case, considering that alignment requests might be fulfilled by an assembler without any address change, if the current address was just right, and only the assembler would know that. > Can the compiler count how many different view labels it has produced or > will produce It can do that, yeah. We could even count the max estimated view number, which, in the absence of view reset points (the compiler has that disabled except on ports that define a target hook to enable it, and none do), is equivalent to the number of views. I guess this is a reasonable upper bound to use, in the absence of a closer estimate. I'll give it a shot. > Or what will exactly happen if you use too small form? > Silent wrong-debug, assembler error, something else? >> the assembler will warn if it truncates a view number Wrong-debug, but not silent. Not an error either. >> Why do you say it would be terribly expensive to let the assembler compute >> offsets in .debug_info? > Because people are already complaining about slow assembly times of debug > info, and offsets are used pretty much everywhere in .debug_info. They're used pretty much everywhere in code as well; I suspect we have a lot more labels in code than we'd have within debug info. As to complaints... are those recent, possibly related with https://sourceware.org/bugzilla/show_bug.cgi?id=22870 or were the complains already in place before LVU went in? > With some assemblers that don't allow .uleb128/.sleb128 that is impossible > to do, with others you'd need to emit a new label before every DIE and all > DW_AT_type and many others would need to use .LD123456-.LD1092456 kind of > expressions which assembler would need to parse and resolve > (DW_FORM_ref{1,2,4,8,_udata}). Assemblers determine addresses for labels all the time, and surely computing offsets between them, when they're in the same section, can't be that expensive. Well, with relaxable frags whose sizes depend on the offsets, it could in theory get expensive indeed, but would it in practice? I can see that mandatory [us]leb128 fields could be a problem, but are there any fields that involve offsets into .debug_info, and that must be represented as [us]leb128? I don't think so.
On Thu, Mar 08, 2018 at 06:05:04AM -0300, Alexandre Oliva wrote: > On Mar 6, 2018, Jakub Jelinek <jakub@redhat.com> wrote: > > > I'm afraid I haven't understood yet why we need labels instead of compiler > > assigned numbers for the views, > > The compiler can't determine points at which the insn address changes > reliably (or at all). It's impossible in the general case, considering > that alignment requests might be fulfilled by an assembler without any > address change, if the current address was just right, and only the > assembler would know that. An upper bound would be fine, count any potential spot where you introduce a view and if the upper bound fits into 8-bit value, use data1, if into 16-bit value, use data2, otherwise use data4. Jakub
On Mar 8, 2018, Jakub Jelinek <jakub@redhat.com> wrote: > An upper bound would be fine, count any potential spot where you introduce a > view and if the upper bound fits into 8-bit value, use data1, if into 16-bit > value, use data2, otherwise use data4. [IEPM] [PR debug/84620] use constant form for DW_AT_GNU_entry_view When outputting entry views in symbolic mode, we used to use a lbl_id, but that outputs the view as an addr, perhaps even in an indirect one, which is all excessive and undesirable for a small assembler-computed constant. Introduce a new value class for symbolic views, so that we can output the labels as constant data, using as narrow forms as possible, but wide enough for any symbolic views output in the compilation. We don't know exactly where the assembler will reset views, but we count the symbolic views since known reset points and use that as an upper bound for view numbers. Ideally, we'd use uleb128, but then the compiler would have to defer .debug_info offset computation to the assembler. I'm not going there for now, so a symbolic uleb128 assembler constant in an attribute is not something GCC can deal with ATM. Regstrapped on i686- and x86_64-linux-gnu, with binutils 2.30. Through visual inspection, I've confirmed there are consistent uses of data1 and data2 for DW_AT_GNU_entry_view in libgcc and libstdc++; wider const forms were not needed. Ok to install? for gcc/ChangeLog PR debug/84620 * dwarf2out.h (dw_val_class): Add dw_val_class_symview. (dw_val_node): Add val_symbolic_view. * dwarf2out.c (dw_line_info_table): Add symviews_since_reset. (symview_upper_bound): New. (new_line_info_table): Initialize symviews_since_reset. (dwarf2out_source_line): Count symviews_since_reset and set symview_upper_bound. (dw_val_equal_p): Handle symview. (add_AT_symview): New. (print_dw_val): Handle symview. (attr_checksum, attr_checksum_ordered): Likewise. (same_dw_val_p, size_of_die): Likewise. (value_format, output_die): Likewise. (add_high_low_attributes): Use add_AT_symview for entry_view. --- gcc/dwarf2out.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- gcc/dwarf2out.h | 4 ++ 2 files changed, 94 insertions(+), 7 deletions(-) diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 0348f4460e98..fbf87378ac79 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -1434,6 +1434,8 @@ dw_val_equal_p (dw_val_node *a, dw_val_node *b) return a->v.val_die_ref.die == b->v.val_die_ref.die; case dw_val_class_fde_ref: return a->v.val_fde_index == b->v.val_fde_index; + case dw_val_class_symview: + return strcmp (a->v.val_symbolic_view, b->v.val_symbolic_view) == 0; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -2951,6 +2953,11 @@ struct GTY(()) dw_line_info_table { going to ask the assembler to assign. */ var_loc_view view; + /* This counts the number of symbolic views emitted in this table + since the latest view reset. Its max value, over all tables, + sets symview_upper_bound. */ + var_loc_view symviews_since_reset; + #define FORCE_RESET_NEXT_VIEW(x) ((x) = (var_loc_view)-1) #define RESET_NEXT_VIEW(x) ((x) = (var_loc_view)0) #define FORCE_RESETTING_VIEW_P(x) ((x) == (var_loc_view)-1) @@ -2959,6 +2966,13 @@ struct GTY(()) dw_line_info_table { vec<dw_line_info_entry, va_gc> *entries; }; +/* This is an upper bound for view numbers that the assembler may + assign to symbolic views output in this translation. It is used to + decide how big a field to use to represent view numbers in + symview-classed attributes. */ + +static var_loc_view symview_upper_bound; + /* If we're keep track of location views and their reset points, and INSN is a reset point (i.e., it necessarily advances the PC), mark the next view in TABLE as reset. */ @@ -3603,6 +3617,7 @@ static addr_table_entry *add_addr_table_entry (void *, enum ate_kind); static void remove_addr_table_entry (addr_table_entry *); static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool); static inline rtx AT_addr (dw_attr_node *); +static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *); @@ -5117,6 +5132,21 @@ add_AT_vms_delta (dw_die_ref die, enum dwarf_attribute attr_kind, add_dwarf_attr (die, &attr); } +/* Add a symbolic view identifier attribute value to a DIE. */ + +static inline void +add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind, + const char *view_label) +{ + dw_attr_node attr; + + attr.dw_attr = attr_kind; + attr.dw_attr_val.val_class = dw_val_class_symview; + attr.dw_attr_val.val_entry = NULL; + attr.dw_attr_val.v.val_symbolic_view = xstrdup (view_label); + add_dwarf_attr (die, &attr); +} + /* Add a label identifier attribute value to a DIE. */ static inline void @@ -6460,6 +6490,9 @@ print_dw_val (dw_val_node *val, bool recurse, FILE *outfile) fprintf (outfile, "delta: @slotcount(%s-%s)", val->v.val_vms_delta.lbl2, val->v.val_vms_delta.lbl1); break; + case dw_val_class_symview: + fprintf (outfile, "view: %s", val->v.val_symbolic_view); + break; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -6831,6 +6864,7 @@ attr_checksum (dw_attr_node *at, struct md5_ctx *ctx, int *mark) case dw_val_class_fde_ref: case dw_val_class_vms_delta: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7127,6 +7161,7 @@ attr_checksum_ordered (enum dwarf_tag tag, dw_attr_node *at, break; case dw_val_class_fde_ref: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7627,6 +7662,9 @@ same_dw_val_p (const dw_val_node *v1, const dw_val_node *v2, int *mark) case dw_val_class_die_ref: return same_die_p (v1->v.val_die_ref.die, v2->v.val_die_ref.die, mark); + case dw_val_class_symview: + return strcmp (v1->v.val_symbolic_view, v2->v.val_symbolic_view) == 0; + case dw_val_class_fde_ref: case dw_val_class_vms_delta: case dw_val_class_lbl_id: @@ -9287,6 +9325,16 @@ size_of_die (dw_die_ref die) size += csize; } break; + case dw_val_class_symview: + if (symview_upper_bound <= 0xff) + size += 1; + else if (symview_upper_bound <= 0xffff) + size += 2; + else if (symview_upper_bound <= 0xffffffff) + size += 4; + else + size += 8; + break; case dw_val_class_const_implicit: case dw_val_class_unsigned_const_implicit: case dw_val_class_file_implicit: @@ -9735,6 +9783,17 @@ value_format (dw_attr_node *a) default: return DW_FORM_block1; } + case dw_val_class_symview: + /* ??? We might use uleb128, but then we'd have to compute + .debug_info offsets in the assembler. */ + if (symview_upper_bound <= 0xff) + return DW_FORM_data1; + else if (symview_upper_bound <= 0xffff) + return DW_FORM_data2; + else if (symview_upper_bound <= 0xffffffff) + return DW_FORM_data4; + else + return DW_FORM_data8; case dw_val_class_vec: switch (constant_size (a->dw_attr_val.v.val_vec.length * a->dw_attr_val.v.val_vec.elt_size)) @@ -10500,6 +10559,22 @@ output_die (dw_die_ref die) } break; + case dw_val_class_symview: + { + int vsize; + if (symview_upper_bound <= 0xff) + vsize = 1; + else if (symview_upper_bound <= 0xffff) + vsize = 2; + else if (symview_upper_bound <= 0xffffffff) + vsize = 4; + else + vsize = 8; + dw2_asm_output_addr (vsize, a->dw_attr_val.v.val_symbolic_view, + "%s", name); + } + break; + case dw_val_class_const_implicit: if (flag_debug_asm) fprintf (asm_out_file, "\t\t\t%s %s (" @@ -23812,7 +23887,7 @@ add_high_low_attributes (tree stmt, dw_die_ref die) indirecting them through a table might make things easier, but even that would be more wasteful, space-wise, than what we have now. */ - add_AT_lbl_id (die, DW_AT_GNU_entry_view, label); + add_AT_symview (die, DW_AT_GNU_entry_view, label); } } @@ -27409,6 +27484,7 @@ new_line_info_table (void) table->line_num = 1; table->is_stmt = DWARF_LINE_DEFAULT_IS_STMT_START; FORCE_RESET_NEXT_VIEW (table->view); + table->symviews_since_reset = 0; return table; } @@ -27606,11 +27682,16 @@ dwarf2out_source_line (unsigned int line, unsigned int column, /* If we're using the assembler to compute view numbers, we can't issue a .loc directive for line zero, so we can't get a view number at this point. We might attempt to - compute it from the previous view, but since we're - omitting the line number entry, we might as well omit the - view number as well. That means pretending it's a view - number zero, which might very well turn out to be - correct. */ + compute it from the previous view, or equate it to a + subsequent view (though it might not be there!), but + since we're omitting the line number entry, we might as + well omit the view number as well. That means pretending + it's a view number zero, which might very well turn out + to be correct. ??? Extend the assembler so that the + compiler could emit e.g. ".locview .LVU#", to output a + view without changing line number information. We'd then + have to count it in symviews_since_reset; when it's omitted, + it doesn't count. */ if (!zero_view_p) zero_view_p = BITMAP_GGC_ALLOC (); bitmap_set_bit (zero_view_p, table->view); @@ -27699,6 +27780,9 @@ dwarf2out_source_line (unsigned int line, unsigned int column, { if (!RESETTING_VIEW_P (table->view)) { + table->symviews_since_reset++; + if (table->symviews_since_reset > symview_upper_bound) + symview_upper_bound = table->symviews_since_reset; /* When we're using the assembler to compute view numbers, we output symbolic labels after "view" in .loc directives, and the assembler will set them for @@ -27717,6 +27801,7 @@ dwarf2out_source_line (unsigned int line, unsigned int column, } else { + table->symviews_since_reset = 0; if (FORCE_RESETTING_VIEW_P (table->view)) fputs (" view -0", asm_out_file); else diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h index a1856a5185e2..a0ba414014d4 100644 --- a/gcc/dwarf2out.h +++ b/gcc/dwarf2out.h @@ -161,7 +161,8 @@ enum dw_val_class dw_val_class_const_implicit, dw_val_class_unsigned_const_implicit, dw_val_class_file_implicit, - dw_val_class_view_list + dw_val_class_view_list, + dw_val_class_symview }; /* Describe a floating point constant value, or a vector constant value. */ @@ -233,6 +234,7 @@ struct GTY(()) dw_val_node { } GTY ((tag ("dw_val_class_vms_delta"))) val_vms_delta; dw_discr_value GTY ((tag ("dw_val_class_discr_value"))) val_discr_value; dw_discr_list_ref GTY ((tag ("dw_val_class_discr_list"))) val_discr_list; + char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; } GTY ((desc ("%1.val_class"))) v; };
On Fri, Mar 09, 2018 at 05:49:34AM -0300, Alexandre Oliva wrote: > @@ -27699,6 +27780,9 @@ dwarf2out_source_line (unsigned int line, unsigned int column, > { > if (!RESETTING_VIEW_P (table->view)) > { > + table->symviews_since_reset++; > + if (table->symviews_since_reset > symview_upper_bound) > + symview_upper_bound = table->symviews_since_reset; You could have used symview_upper_bound = MAX (symview_upper_bound, table->symviews_since_reset); Also, I'd say there should be symview_upper_bound = 0; in dwarf2out_c_finalize for better behavior of GCC JIT. Otherwise LGTM, but please give Jason a day to comment on. Jakub
On Fri, Mar 9, 2018 at 4:02 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Mar 09, 2018 at 05:49:34AM -0300, Alexandre Oliva wrote: >> @@ -27699,6 +27780,9 @@ dwarf2out_source_line (unsigned int line, unsigned int column, >> { >> if (!RESETTING_VIEW_P (table->view)) >> { >> + table->symviews_since_reset++; >> + if (table->symviews_since_reset > symview_upper_bound) >> + symview_upper_bound = table->symviews_since_reset; > > You could have used > symview_upper_bound > = MAX (symview_upper_bound, table->symviews_since_reset); > > Also, I'd say there should be > symview_upper_bound = 0; > in dwarf2out_c_finalize for better behavior of GCC JIT. > > Otherwise LGTM, but please give Jason a day to comment on. OK. Jason
On Mar 9, 2018, Jakub Jelinek <jakub@redhat.com> wrote: > Also, I'd say there should be > symview_upper_bound = 0; > in dwarf2out_c_finalize for better behavior of GCC JIT. Thanks. I added code to clear zero_view_p too. Here's what I'm checking in. [IEPM] [PR debug/84620] use constant form for DW_AT_GNU_entry_view When outputting entry views in symbolic mode, we used to use a lbl_id, but that outputs the view as an addr, perhaps even in an indirect one, which is all excessive and undesirable for a small assembler-computed constant. Introduce a new value class for symbolic views, so that we can output the labels as constant data, using as narrow forms as possible, but wide enough for any symbolic views output in the compilation. We don't know exactly where the assembler will reset views, but we count the symbolic views since known reset points and use that as an upper bound for view numbers. Ideally, we'd use uleb128, but then the compiler would have to defer .debug_info offset computation to the assembler. I'm not going there for now, so a symbolic uleb128 assembler constant in an attribute is not something GCC can deal with ATM. for gcc/ChangeLog PR debug/84620 * dwarf2out.h (dw_val_class): Add dw_val_class_symview. (dw_val_node): Add val_symbolic_view. * dwarf2out.c (dw_line_info_table): Add symviews_since_reset. (symview_upper_bound): New. (new_line_info_table): Initialize symviews_since_reset. (dwarf2out_source_line): Count symviews_since_reset and set symview_upper_bound. (dw_val_equal_p): Handle symview. (add_AT_symview): New. (print_dw_val): Handle symview. (attr_checksum, attr_checksum_ordered): Likewise. (same_dw_val_p, size_of_die): Likewise. (value_format, output_die): Likewise. (add_high_low_attributes): Use add_AT_symview for entry_view. (dwarf2out_finish): Reset symview_upper_bound, clear zero_view_p. --- gcc/dwarf2out.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- gcc/dwarf2out.h | 4 ++ 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index c39910f26cb3..4e6ee5e8f82e 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -1434,6 +1434,8 @@ dw_val_equal_p (dw_val_node *a, dw_val_node *b) return a->v.val_die_ref.die == b->v.val_die_ref.die; case dw_val_class_fde_ref: return a->v.val_fde_index == b->v.val_fde_index; + case dw_val_class_symview: + return strcmp (a->v.val_symbolic_view, b->v.val_symbolic_view) == 0; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -2951,6 +2953,11 @@ struct GTY(()) dw_line_info_table { going to ask the assembler to assign. */ var_loc_view view; + /* This counts the number of symbolic views emitted in this table + since the latest view reset. Its max value, over all tables, + sets symview_upper_bound. */ + var_loc_view symviews_since_reset; + #define FORCE_RESET_NEXT_VIEW(x) ((x) = (var_loc_view)-1) #define RESET_NEXT_VIEW(x) ((x) = (var_loc_view)0) #define FORCE_RESETTING_VIEW_P(x) ((x) == (var_loc_view)-1) @@ -2959,6 +2966,13 @@ struct GTY(()) dw_line_info_table { vec<dw_line_info_entry, va_gc> *entries; }; +/* This is an upper bound for view numbers that the assembler may + assign to symbolic views output in this translation. It is used to + decide how big a field to use to represent view numbers in + symview-classed attributes. */ + +static var_loc_view symview_upper_bound; + /* If we're keep track of location views and their reset points, and INSN is a reset point (i.e., it necessarily advances the PC), mark the next view in TABLE as reset. */ @@ -3603,6 +3617,7 @@ static addr_table_entry *add_addr_table_entry (void *, enum ate_kind); static void remove_addr_table_entry (addr_table_entry *); static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool); static inline rtx AT_addr (dw_attr_node *); +static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *); @@ -5117,6 +5132,21 @@ add_AT_vms_delta (dw_die_ref die, enum dwarf_attribute attr_kind, add_dwarf_attr (die, &attr); } +/* Add a symbolic view identifier attribute value to a DIE. */ + +static inline void +add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind, + const char *view_label) +{ + dw_attr_node attr; + + attr.dw_attr = attr_kind; + attr.dw_attr_val.val_class = dw_val_class_symview; + attr.dw_attr_val.val_entry = NULL; + attr.dw_attr_val.v.val_symbolic_view = xstrdup (view_label); + add_dwarf_attr (die, &attr); +} + /* Add a label identifier attribute value to a DIE. */ static inline void @@ -6460,6 +6490,9 @@ print_dw_val (dw_val_node *val, bool recurse, FILE *outfile) fprintf (outfile, "delta: @slotcount(%s-%s)", val->v.val_vms_delta.lbl2, val->v.val_vms_delta.lbl1); break; + case dw_val_class_symview: + fprintf (outfile, "view: %s", val->v.val_symbolic_view); + break; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -6831,6 +6864,7 @@ attr_checksum (dw_attr_node *at, struct md5_ctx *ctx, int *mark) case dw_val_class_fde_ref: case dw_val_class_vms_delta: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7127,6 +7161,7 @@ attr_checksum_ordered (enum dwarf_tag tag, dw_attr_node *at, break; case dw_val_class_fde_ref: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7627,6 +7662,9 @@ same_dw_val_p (const dw_val_node *v1, const dw_val_node *v2, int *mark) case dw_val_class_die_ref: return same_die_p (v1->v.val_die_ref.die, v2->v.val_die_ref.die, mark); + case dw_val_class_symview: + return strcmp (v1->v.val_symbolic_view, v2->v.val_symbolic_view) == 0; + case dw_val_class_fde_ref: case dw_val_class_vms_delta: case dw_val_class_lbl_id: @@ -9287,6 +9325,16 @@ size_of_die (dw_die_ref die) size += csize; } break; + case dw_val_class_symview: + if (symview_upper_bound <= 0xff) + size += 1; + else if (symview_upper_bound <= 0xffff) + size += 2; + else if (symview_upper_bound <= 0xffffffff) + size += 4; + else + size += 8; + break; case dw_val_class_const_implicit: case dw_val_class_unsigned_const_implicit: case dw_val_class_file_implicit: @@ -9735,6 +9783,17 @@ value_format (dw_attr_node *a) default: return DW_FORM_block1; } + case dw_val_class_symview: + /* ??? We might use uleb128, but then we'd have to compute + .debug_info offsets in the assembler. */ + if (symview_upper_bound <= 0xff) + return DW_FORM_data1; + else if (symview_upper_bound <= 0xffff) + return DW_FORM_data2; + else if (symview_upper_bound <= 0xffffffff) + return DW_FORM_data4; + else + return DW_FORM_data8; case dw_val_class_vec: switch (constant_size (a->dw_attr_val.v.val_vec.length * a->dw_attr_val.v.val_vec.elt_size)) @@ -10500,6 +10559,22 @@ output_die (dw_die_ref die) } break; + case dw_val_class_symview: + { + int vsize; + if (symview_upper_bound <= 0xff) + vsize = 1; + else if (symview_upper_bound <= 0xffff) + vsize = 2; + else if (symview_upper_bound <= 0xffffffff) + vsize = 4; + else + vsize = 8; + dw2_asm_output_addr (vsize, a->dw_attr_val.v.val_symbolic_view, + "%s", name); + } + break; + case dw_val_class_const_implicit: if (flag_debug_asm) fprintf (asm_out_file, "\t\t\t%s %s (" @@ -23815,7 +23890,7 @@ add_high_low_attributes (tree stmt, dw_die_ref die) indirecting them through a table might make things easier, but even that would be more wasteful, space-wise, than what we have now. */ - add_AT_lbl_id (die, DW_AT_GNU_entry_view, label); + add_AT_symview (die, DW_AT_GNU_entry_view, label); } } @@ -27412,6 +27487,7 @@ new_line_info_table (void) table->line_num = 1; table->is_stmt = DWARF_LINE_DEFAULT_IS_STMT_START; FORCE_RESET_NEXT_VIEW (table->view); + table->symviews_since_reset = 0; return table; } @@ -27609,11 +27685,16 @@ dwarf2out_source_line (unsigned int line, unsigned int column, /* If we're using the assembler to compute view numbers, we can't issue a .loc directive for line zero, so we can't get a view number at this point. We might attempt to - compute it from the previous view, but since we're - omitting the line number entry, we might as well omit the - view number as well. That means pretending it's a view - number zero, which might very well turn out to be - correct. */ + compute it from the previous view, or equate it to a + subsequent view (though it might not be there!), but + since we're omitting the line number entry, we might as + well omit the view number as well. That means pretending + it's a view number zero, which might very well turn out + to be correct. ??? Extend the assembler so that the + compiler could emit e.g. ".locview .LVU#", to output a + view without changing line number information. We'd then + have to count it in symviews_since_reset; when it's omitted, + it doesn't count. */ if (!zero_view_p) zero_view_p = BITMAP_GGC_ALLOC (); bitmap_set_bit (zero_view_p, table->view); @@ -27702,6 +27783,9 @@ dwarf2out_source_line (unsigned int line, unsigned int column, { if (!RESETTING_VIEW_P (table->view)) { + table->symviews_since_reset++; + if (table->symviews_since_reset > symview_upper_bound) + symview_upper_bound = table->symviews_since_reset; /* When we're using the assembler to compute view numbers, we output symbolic labels after "view" in .loc directives, and the assembler will set them for @@ -27720,6 +27804,7 @@ dwarf2out_source_line (unsigned int line, unsigned int column, } else { + table->symviews_since_reset = 0; if (FORCE_RESETTING_VIEW_P (table->view)) fputs (" view -0", asm_out_file); else @@ -31295,6 +31380,11 @@ dwarf2out_finish (const char *) debug_line_str_hash->traverse<enum dwarf_form, output_indirect_string> (form); } + + /* ??? Move lvugid out of dwarf2out_source_line and reset it too? */ + symview_upper_bound = 0; + if (zero_view_p) + bitmap_clear (zero_view_p); } /* Returns a hash value for X (which really is a variable_value_struct). */ diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h index a1856a5185e2..a0ba414014d4 100644 --- a/gcc/dwarf2out.h +++ b/gcc/dwarf2out.h @@ -161,7 +161,8 @@ enum dw_val_class dw_val_class_const_implicit, dw_val_class_unsigned_const_implicit, dw_val_class_file_implicit, - dw_val_class_view_list + dw_val_class_view_list, + dw_val_class_symview }; /* Describe a floating point constant value, or a vector constant value. */ @@ -233,6 +234,7 @@ struct GTY(()) dw_val_node { } GTY ((tag ("dw_val_class_vms_delta"))) val_vms_delta; dw_discr_value GTY ((tag ("dw_val_class_discr_value"))) val_discr_value; dw_discr_list_ref GTY ((tag ("dw_val_class_discr_list"))) val_discr_list; + char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; } GTY ((desc ("%1.val_class"))) v; };
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 41bb11558f69..b52edee845f2 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -1434,6 +1434,8 @@ dw_val_equal_p (dw_val_node *a, dw_val_node *b) return a->v.val_die_ref.die == b->v.val_die_ref.die; case dw_val_class_fde_ref: return a->v.val_fde_index == b->v.val_fde_index; + case dw_val_class_symview: + return strcmp (a->v.val_symbolic_view, b->v.val_symbolic_view) == 0; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -3600,6 +3602,7 @@ static addr_table_entry *add_addr_table_entry (void *, enum ate_kind); static void remove_addr_table_entry (addr_table_entry *); static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool); static inline rtx AT_addr (dw_attr_node *); +static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *); static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *); @@ -5114,6 +5117,21 @@ add_AT_vms_delta (dw_die_ref die, enum dwarf_attribute attr_kind, add_dwarf_attr (die, &attr); } +/* Add a symbolic view identifier attribute value to a DIE. */ + +static inline void +add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind, + const char *view_label) +{ + dw_attr_node attr; + + attr.dw_attr = attr_kind; + attr.dw_attr_val.val_class = dw_val_class_symview; + attr.dw_attr_val.val_entry = NULL; + attr.dw_attr_val.v.val_symbolic_view = xstrdup (view_label); + add_dwarf_attr (die, &attr); +} + /* Add a label identifier attribute value to a DIE. */ static inline void @@ -6457,6 +6475,9 @@ print_dw_val (dw_val_node *val, bool recurse, FILE *outfile) fprintf (outfile, "delta: @slotcount(%s-%s)", val->v.val_vms_delta.lbl2, val->v.val_vms_delta.lbl1); break; + case dw_val_class_symview: + fprintf (outfile, "view: %s", val->v.val_symbolic_view); + break; case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -6828,6 +6849,7 @@ attr_checksum (dw_attr_node *at, struct md5_ctx *ctx, int *mark) case dw_val_class_fde_ref: case dw_val_class_vms_delta: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7124,6 +7146,7 @@ attr_checksum_ordered (enum dwarf_tag tag, dw_attr_node *at, break; case dw_val_class_fde_ref: + case dw_val_class_symview: case dw_val_class_lbl_id: case dw_val_class_lineptr: case dw_val_class_macptr: @@ -7624,6 +7647,9 @@ same_dw_val_p (const dw_val_node *v1, const dw_val_node *v2, int *mark) case dw_val_class_die_ref: return same_die_p (v1->v.val_die_ref.die, v2->v.val_die_ref.die, mark); + case dw_val_class_symview: + return strcmp (v1->v.val_symbolic_view, v2->v.val_symbolic_view) == 0; + case dw_val_class_fde_ref: case dw_val_class_vms_delta: case dw_val_class_lbl_id: @@ -9284,6 +9310,10 @@ size_of_die (dw_die_ref die) size += csize; } break; + case dw_val_class_symview: + /* FIXME: Use uleb128 rather than data2. */ + size += 2; + break; case dw_val_class_const_implicit: case dw_val_class_unsigned_const_implicit: case dw_val_class_file_implicit: @@ -9732,6 +9762,9 @@ value_format (dw_attr_node *a) default: return DW_FORM_block1; } + case dw_val_class_symview: + /* FIXME: Use uleb128 rather than data2. */ + return DW_FORM_data2; case dw_val_class_vec: switch (constant_size (a->dw_attr_val.v.val_vec.length * a->dw_attr_val.v.val_vec.elt_size)) @@ -10497,6 +10530,11 @@ output_die (dw_die_ref die) } break; + case dw_val_class_symview: + /* FIXME: Use uleb128 rather than data2. */ + dw2_asm_output_addr (2, a->dw_attr_val.v.val_symbolic_view, "%s", name); + break; + case dw_val_class_const_implicit: if (flag_debug_asm) fprintf (asm_out_file, "\t\t\t%s %s (" @@ -23809,7 +23847,7 @@ add_high_low_attributes (tree stmt, dw_die_ref die) indirecting them through a table might make things easier, but even that would be more wasteful, space-wise, than what we have now. */ - add_AT_lbl_id (die, DW_AT_GNU_entry_view, label); + add_AT_symview (die, DW_AT_GNU_entry_view, label); } } diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h index a1856a5185e2..f2240af85375 100644 --- a/gcc/dwarf2out.h +++ b/gcc/dwarf2out.h @@ -161,7 +161,8 @@ enum dw_val_class dw_val_class_const_implicit, dw_val_class_unsigned_const_implicit, dw_val_class_file_implicit, - dw_val_class_view_list + dw_val_class_view_list, + dw_val_class_symview }; /* Describe a floating point constant value, or a vector constant value. */ @@ -209,6 +210,7 @@ struct GTY(()) dw_val_node { HOST_WIDE_INT GTY ((default)) val_int; unsigned HOST_WIDE_INT GTY ((tag ("dw_val_class_unsigned_const"))) val_unsigned; + char * GTY ((tag ("dw_val_class_symview"))) val_symbolic_view; double_int GTY ((tag ("dw_val_class_const_double"))) val_double; wide_int_ptr GTY ((tag ("dw_val_class_wide_int"))) val_wide; dw_vec_const GTY ((tag ("dw_val_class_vec"))) val_vec;