This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, Jul 25, 2012 at 4:00 PM, Cary Coutant <ccoutant@google.com> 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.
>
> 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.

It also requires a carefully watching when die sizes are measured--if
a leb128 fit inside a byte and then grows to need two bytes, all the
size and die_offset calculations will need to be redone. Right now
there is no attempt at deferring size calculations or ordering them.
It's a pretty big change for only marginal benefit.

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

Right. We would have to track it even with a 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.

Exactly. I'll fix up the comment.

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

True, and I'll fix that.

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

I think Jason is right here. Although they are theoretically separate,
there has been no testing of -gsplit-dwarf without dwarf4, and I don't
think we plan to test it that way.

I think the best thing to do is make -gspit-debug-info imply dwarf4.
I'll fix it up.

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

Yes. I have a local fix for this--so there won't be any fixme at all--
I'll roll into the updated version.

>
>>> -  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)?

AT_pubnames needs to go in all the skeleton dies, including the TUs,
so moving the call from add_top_level_skeleton_die_attrs would cause a
problem. Adding it here also would make me need to special case the
comp_unit_die there. So that's pretty confusing.

Still, you are absolutely right that it is confusing here. I'll add a
comment to sort it all out.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]