Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 6305113: [Dwarf Patch] Implement split debug info proposal

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 10 months ago by saugustine
Modified:
12 years, 5 months ago
Reviewers:
Cary, jason
CC:
gcc-patches_gcc.gnu.org
Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/trunk/
Visibility:
Public.

Patch Set 1 #

Patch Set 2 : [Dwarf Fission] Implement Fission Proposal #

Total comments: 2

Patch Set 3 : [Dwarf Patch] Implement split debug info proposal--Updated #

Patch Set 4 : [Dwarf Fission] Implement Fission Proposal #

Patch Set 5 : [Dwarf Fission] Implement Fission Proposal #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1319 lines, -228 lines) Patch
M gcc/common.opt View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M gcc/doc/invoke.texi View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M gcc/dwarf2out.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M gcc/dwarf2out.c View 1 2 3 118 chunks +1244 lines, -224 lines 0 comments Download
M gcc/gcc.c View 1 2 3 7 chunks +37 lines, -4 lines 0 comments Download
M gcc/opts.c View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download
M include/dwarf2.h View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 20
saugustine
The enclosed patch implements the proposed dwarf extension to split debug information from the .o ...
12 years, 10 months ago (2012-06-19 22:54:25 UTC) #1
saugustine
Enclosed is a revised dwarf-fission patch. Fundamentally, it is no different than the earlier edition. ...
12 years, 9 months ago (2012-07-18 21:44:54 UTC) #2
Cary
http://codereview.appspot.com/6305113/diff/3001/gcc/dwarf2out.c File gcc/dwarf2out.c (right): http://codereview.appspot.com/6305113/diff/3001/gcc/dwarf2out.c#newcode8517 gcc/dwarf2out.c:8517: Should use SKELETON_COMP_DIE_ABBREV here instead of 1. http://codereview.appspot.com/6305113/diff/3001/gcc/dwarf2out.c#newcode8602 gcc/dwarf2out.c:8602: ...
12 years, 9 months ago (2012-07-20 00:37:15 UTC) #3
saugustine
On 2012/07/20 00:37:15, Cary wrote: > http://codereview.appspot.com/6305113/diff/3001/gcc/dwarf2out.c > File gcc/dwarf2out.c (right): > > http://codereview.appspot.com/6305113/diff/3001/gcc/dwarf2out.c#newcode8517 > ...
12 years, 9 months ago (2012-07-20 18:02:37 UTC) #4
jason_redhat.com
On 07/18/2012 05:44 PM, Sterling Augustine wrote: > +/* Remove an entry from the addr ...
12 years, 9 months ago (2012-07-25 19:24:55 UTC) #5
Cary
>> +/* Remove an entry from the addr table. Since we have already numbered >> ...
12 years, 9 months ago (2012-07-25 23:00:36 UTC) #6
saugustine
On Wed, Jul 25, 2012 at 4:00 PM, Cary Coutant <ccoutant@google.com> wrote: >>> +/* Remove ...
12 years, 9 months ago (2012-07-25 23:54:48 UTC) #7
Cary
>>>> + if (dwarf_version >= 4 || dwarf_split_debug_info) >>> >>> Shouldn't -gsplit-debug require -gdwarf-4+ anyway? ...
12 years, 9 months ago (2012-07-26 00:10:13 UTC) #8
saugustine
The enclosed patch to implment -gsplit-dwarf addresses Jason's comments as responded to by myself and ...
12 years, 8 months ago (2012-08-27 18:00:02 UTC) #9
saugustine
Hey Jason, Just wanted to be sure you saw this. I know you're busy, but ...
12 years, 7 months ago (2012-09-05 18:43:37 UTC) #10
saugustine
On Mon, Aug 27, 2012 at 11:00 AM, Sterling Augustine <saugustine@google.com> wrote: > The enclosed ...
12 years, 7 months ago (2012-09-13 22:39:00 UTC) #11
saugustine
On Mon, Aug 27, 2012 at 11:00 AM, Sterling Augustine <saugustine@google.com> wrote: > The enclosed ...
12 years, 7 months ago (2012-09-18 22:27:26 UTC) #12
saugustine
Ping? I'd love to get this in before the 4.8 stage 1 closes. thanks, Sterling ...
12 years, 7 months ago (2012-09-24 18:22:31 UTC) #13
jason_redhat.com
On 07/25/2012 07:54 PM, Sterling Augustine wrote: > On Wed, Jul 25, 2012 at 4:00 ...
12 years, 6 months ago (2012-10-08 20:33:42 UTC) #14
Cary
>>> The potential savings here didn't seem worth the effort of adding a >>> pass ...
12 years, 6 months ago (2012-10-11 00:41:07 UTC) #15
jason_redhat.com
On 10/10/2012 08:41 PM, Cary Coutant wrote: > I'm working on a follow-up patch to ...
12 years, 6 months ago (2012-10-11 14:21:44 UTC) #16
saugustine
Jason and other GCC Dwarf maintainers, Enclosed is what I hope is the final pass ...
12 years, 5 months ago (2012-10-29 23:46:26 UTC) #17
Cary
> +/* %:replace-extension spec function. Replaces the extension of the > + first argument with ...
12 years, 5 months ago (2012-11-05 23:18:34 UTC) #18
saugustine
On Mon, Nov 5, 2012 at 3:18 PM, Cary Coutant <ccoutant@google.com> wrote: >> +/* %:replace-extension ...
12 years, 5 months ago (2012-11-06 19:21:48 UTC) #19
saugustine
12 years, 5 months ago (2012-11-06 23:16:02 UTC) #20 
On Tue, Nov 6, 2012 at 11:21 AM, Sterling Augustine
<saugustine@google.com> wrote:
> On Mon, Nov 5, 2012 at 3:18 PM, Cary Coutant <ccoutant@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

Committed as attached. Thanks everyone.

Sterling
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b