This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Dwarf Fission] Implement Fission Proposal (issue6305113)
- From: Cary Coutant <ccoutant at google dot com>
- To: Sterling Augustine <saugustine at google dot com>
- Cc: reply at codereview dot appspotmail dot com, gcc-patches at gcc dot gnu dot org
- Date: Mon, 5 Nov 2012 15:18:30 -0800
- Subject: Re: [Dwarf Fission] Implement Fission Proposal (issue6305113)
- References: <20121029234624.3BB57161118@sterling.mtv.corp.google.com>
> +/* %:replace-extension spec function. Replaces the extension of the
> + first argument with the second argument. */
> +
> +const char *
> +replace_extension_spec_func (int argc, const char **argv)
> +{
> + char *name;
> + char *p;
> + char *result;
> +
> + if (argc != 2)
> + fatal_error ("too few arguments to %%:replace-extension");
> +
> + name = xstrdup (argv[0]);
> + p = strrchr (name, '.');
> + if (p != NULL)
> + *p = '\0';
> +
> + result = concat (name, argv[1], NULL);
> +
> + free (name);
> + return result;
> +}
This doesn't do the right thing when there is no '.' in the last
component of the path. It should look for the last DIR_SEPARATOR,
then search for the last '.' after that.
> +/* Describe an entry into the .debug_addr section. */
> +
> +enum ate_kind {
> + ate_kind_rtx,
> + ate_kind_rtx_dtprel,
> + ate_kind_label
> +};
> +
> +typedef struct GTY(()) addr_table_entry_struct {
> + enum ate_kind kind;
> + unsigned int refcount;
> + unsigned int index;
> + union addr_table_entry_struct_union
> + {
> + rtx GTY ((tag ("ate_kind_rtx"))) rtl;
> + char * GTY ((tag ("ate_kind_label"))) label;
> + }
> + GTY ((desc ("%1.kind"))) addr;
When kind == ate_kind_rtx_dtprel, we use the rtl field. I think this needs
to be covered for GC to work. As far as I know, gengtype doesn't support
multiple tags for one union member, so I think it needs to be something
like this:
union addr_table_entry_struct_union
{
rtx GTY ((tag ("0"))) rtl;
char * GTY ((tag ("1"))) label;
}
GTY ((desc ("(%1.kind == ate_kind_label)"))) addr;
> +static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *,
> + bool);
It turns out we never call add_AT_lbl_id with force_direct == true.
I don't think it's necessary to add this parameter here.
> +/* enum for tracking thread-local variables whose address is really an offset
> + relative to the TLS pointer, which will need link-time relocation, but will
> + not need relocation by the DWARF consumer. */
> +
> +enum dtprel_bool
> + {
> + dtprel_false = 0,
> + dtprel_true = 1
> + };
Extra indentation here.
> +static inline enum dwarf_location_atom
> +dw_addr_op (enum dtprel_bool dtprel)
> +{
> + if (dtprel == dtprel_true)
> + return (dwarf_split_debug_info ? DW_OP_GNU_const_index
> + : (DWARF2_ADDR_SIZE == 4 ? DW_OP_const4u : DW_OP_const8u));
> + else
> + return (dwarf_split_debug_info ? DW_OP_GNU_addr_index : DW_OP_addr);
Unnecessary parentheses here.
> +/* Return the index for any attribute that will be referenced with a
> + DW_FORM_GNU_addr_index. Strings have their indices handled differently to
> + account for reference counting pruning. */
> +
> +static inline unsigned int
> +AT_index (dw_attr_ref a)
> +{
> + if (AT_class (a) == dw_val_class_str)
> + return a->dw_attr_val.v.val_str->index;
> + else if (a->dw_attr_val.val_entry != NULL)
> + return a->dw_attr_val.val_entry->index;
> + return NOT_INDEXED;
> +}
The comment seems out of date. DW_FORM_GNU_str_index should also be
mentioned, and it doesn't look like strings have their indices handled
differently (at least not here).
> +static void
> +remove_addr_table_entry (addr_table_entry *entry)
> +{
> + addr_table_entry *node;
> +
> + gcc_assert (dwarf_split_debug_info && addr_index_table);
> + node = (addr_table_entry *) htab_find (addr_index_table, entry);
> + node->refcount--;
> + /* After an index is assigned, the table is frozen. */
> + gcc_assert (node->refcount > 0 || node->index == NO_INDEX_ASSIGNED);
This shouldn't ever be called after we've assigned any indexes at all,
so I think it's always safe to asser that node->index == NO_INDEX_ASSIGNED.
We can also assert that the ref count should never go negative, so I think
you can rewrite this assert as:
gcc_assert (node->refcount >= 0 && node->index == NO_INDEX_ASSIGNED);
> @@ -21215,7 +22086,7 @@ prune_unused_types_update_strings (dw_die_ref die)
> *slot = s;
> }
> }
> -}
> + }
Accidental extra space?
> +static void
> +index_location_lists (dw_die_ref die)
> +{
> + dw_die_ref c;
> + dw_attr_ref a;
> + unsigned ix;
> +
> + FOR_EACH_VEC_ELT (dw_attr_node, die->die_attr, ix, a)
> + if (AT_class (a) == dw_val_class_loc_list)
> + {
> + dw_loc_list_ref list = AT_loc_list (a);
> + dw_loc_list_ref curr;
> + for (curr = list; curr != NULL; curr = curr->dw_loc_next)
> + {
> + /* Don't index an entry that has already been indexed
> + or won't be output. */
> + if (curr->begin_entry != NULL
> + || (strcmp (curr->begin, curr->end) == 0 && !curr->force))
> + continue;
Check the indentation here -- looks like these last two lines are missing
a space.
OK for trunk with these changes. Thanks!
-cary