On Mon, Nov 5, 2012 at 3:18 PM, Cary Coutant <ccout...@google.com> wrote:
>> +/* %: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.
Good catch. Fixed.
>> +/* 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;
>
Done.
>> +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.
Done--this actually cleans up the patch quite a bit.
>> +/* 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.
Fixed.
>> +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.
Removed.
>> +/* 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).
Updated.
>> +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);
>
Done.
>> @@ -21215,7 +22086,7 @@ prune_unused_types_update_strings (dw_die_ref die)
>> *slot = s;
>> }
>> }
>> -}
>> + }
>
> Accidental extra space?
Yes. Fixed.
>> +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.
>
Fixed.
>
> OK for trunk with these changes. Thanks!
>
> -cary
And with that, unless anyone else (Jason?) has any more comments, I
will check this in later today.
Sterling
