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