Re: [Dwarf Fission] Implement Fission Proposal (issue6305113)

[Jason Merrill <jason at redhat dot com>]

On 07/18/2012 05:44 PM, Sterling Augustine wrote:
> +/* 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.

> +    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.

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

> +static inline enum dwarf_location_atom dw_addr_op (bool dtprel)

Function name should be on a new line.

> +		      bool *added, bool force_direct)

force_direct needs to be documented.

> +  /* 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?

> -is_unit_die (dw_die_ref c)
...
> +is_unit_die (dw_die_ref c)

Why move is_unit_die?

> +      if (dwarf_version >= 4 || dwarf_split_debug_info)

Shouldn't -gsplit-debug require -gdwarf-4+ anyway?

> +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.

> +  /* For split_debug_sections with use_type info, all type units int the
> +     skeleton sections have identical dies (but different headers).  This single
> +     die will be output many times.  */

s/int/in/
The second line goes past 80 chars.

> +  /* Produce the skeleton compilation-unit header.  This one differs enough from

> +                             (!dwarf_split_debug_info ? debug_line_section_label

These lines are too long as well.


> +          temp = new_addr_loc_descr (rtl, true);
> +      mem_loc_result = new_addr_loc_descr (rtl, 0);

Let's use true/false consistently when passing a boolean value, or 
better yet an enum.

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

Please elaborate.

> +static void
> +output_index_strings (void)

Needs a comment.

> -  add_AT_pubnames (comp_unit_die ());
> +  if (!dwarf_split_debug_info)
> +    add_AT_pubnames (comp_unit_die ());

Why not emit AT_pubnames with DWO?

> +  /* 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.

Jason