[Dwarf Fission] Implement Fission Proposal (issue6305113)

[Cary Coutant]

>> +/* Remove an entry from the addr table.  Since we have already numbered
>> +   all the entries, the best we can do here is null it out.  */
>
> This sounds like a design flaw.  Since we have everything in memory, there's
> no reason we shouldn't be able to pack the table appropriately.  Why can't
> we wait to number the entries until after we know which entries we need?
>
> Perhaps instead of having a val_index field in each attribute you should
> have the attribute point to something like an indirect_string_node for
> addresses as well.

The potential savings here didn't seem worth the effort of adding a
pass over another table to assign slots in .debug_addr. In practice,
we're seeing very few slots zeroed out here.

>> +    case DW_OP_GNU_addr_index:
>> +    case DW_OP_GNU_const_index:
>> +      {
>> +        dw_attr_node *attrx1 = VEC_index (dw_attr_node,
>> +                                          addr_index_table,
>> +                                          valx1->val_index);
>> +        dw_attr_node *attry1 = VEC_index (dw_attr_node,
>> +                                          addr_index_table,
>> +                                          valy1->val_index);
>> +        return rtx_equal_p (attrx1->dw_attr_val.v.val_addr,
>> +                            attry1->dw_attr_val.v.val_addr);
>> +      }
>
> And we shouldn't have multiple entries for the same address.  Using a hash
> table would help with this as well.

I've got a patch in the works to coalesce multiple references to .LVL
labels, which will take care of nearly all the duplicate entries we're
seeing in practice. Sterling has started working on a hash table to
solve the problem in general, but we were planning to wait to see how
much would be gained from that. Would it be OK with you if we defer
this to a follow-up patch or two?

> Deferring the choice of representation of the address until output time
> should also avoid the need for the "force_direct" parameter on various
> functions.

I'm not sure about that. Even if we build a hash table for slots in
.debug_addr, we'll still need to know when we call add_AT_addr or
add_AT_lbl_id whether or not we want to use an indirect reference. In
the cases where force_direct is true, we won't want to add the label
to the hash table.

>> +  /* This is a bit of a misnomer--the offset isn't an index, but we need
>> to
>> +     save force_direct for output.  */
>> +  attr.dw_attr_val.val_index
>> +      = (dwarf_split_debug_info && !force_direct) ? offset : -1U;
>
> How is this used for output?

I think that comment needs to be rewritten. Sterling can probably
describe it better, but I think what's happening here is that
val_index == -1U indicates that we want to output a relocated
reference to the range list entry, while any other value indicates
that we want to output the section-relative offset of the range list
entry. In this case, we're not using the val_index field as a slot
index like we do for references to .debug_addr.

>> -is_unit_die (dw_die_ref c)
> ...
>> +is_unit_die (dw_die_ref c)
>
> Why move is_unit_die?

I don't think that was intentional -- probably the result of merges
from trunk (is_unit_die was added to trunk while we were developing on
the branch).

>> +      if (dwarf_version >= 4 || dwarf_split_debug_info)
>
> Shouldn't -gsplit-debug require -gdwarf-4+ anyway?

Seems better to check explicitly than to rely on an implied relationship.

>> +enum dwarf_location_list_entry_type
>> +  {
>> +    DW_LLE_end_of_list_entry = 0,
>> +    DW_LLE_base_address_selection_entry = 1,
>> +    DW_LLE_start_end_entry = 2,
>> +    DW_LLE_start_length_entry = 3
>> +  };
>
> These should probably have GNU in the names so that it's clear at use sites
> that they're extensions.

OK with me. (I was guilty of anticipating eventual adoption into
DWARF, and since this was a new namespace, I didn't feel it was
necessary to qualify them.)

>> +      /* FIXME: needs to change for dwarf_split_debug_info.  */
>
> Please elaborate.

I think we'll want to use DW_FORM_GNU_str_index there, but we didn't
look at it closely.

>> -  add_AT_pubnames (comp_unit_die ());
>> +  if (!dwarf_split_debug_info)
>> +    add_AT_pubnames (comp_unit_die ());
>
> Why not emit AT_pubnames with DWO?

In the dwarf_split_debug_info case, the attribute gets added by
add_top_level_skeleton_die_attrs. Looks like both cases should be
handled in the same place -- Sterling, can you remove the call in
add_top_level_skeleton_die_attrs and make the one in dwarf2out_finish
unconditional (using main_comp_unit_die)?

>> +  /* The dwo_id is only for compile_units.  */
>> +  add_AT_data8 (comp_unit, DW_AT_GNU_dwo_id, dwo_id_placeholder);
>
>>...
>
>> +      /* Add a place-holder for the dwo_id to the comp-unit die.  */
>> +      add_AT_data8 (comp_unit_die (), DW_AT_GNU_dwo_id,
>> dwo_id_placeholder);
>
> These are in the skeleton CU and the DWO CU, right?  Let's add them to both
> at the same time.

I have a separate patch underway that will do exactly that, and to
compute a real dwo_id. In the meantime, let's just drop these from
this patch.

-cary