From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104804 invoked by alias); 16 Oct 2019 03:04:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 104571 invoked by uid 89); 16 Oct 2019 03:04:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Oct 2019 03:04:47 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id x9G34dcE015334 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 15 Oct 2019 23:04:44 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca x9G34dcE015334 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1571195085; bh=JZAk2lLxaQSmnzMfWDWi3RlzhJ/J7aGxs9Fk2M+Z00o=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=W0tPvAd+vFaKexZErczghGIGor4SqA0PD+Xs03SS42pdriKKqcP4sHv576zf3o73y nsJzT7pQtnp3/bu8zP4HqlctOLJuDdG0F7lDZVspJd9TE0nbUcvPTpHAzett1wDTSl I4ZOxdOy2aNXVZTwZKfVosQK7yVdA0R8MP2ZXSI8= Received: by simark.ca (Postfix, from userid 112) id A50E31E8D0; Tue, 15 Oct 2019 23:04:39 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id E4DFA1E575; Tue, 15 Oct 2019 23:04:37 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 16 Oct 2019 03:04:00 -0000 From: Simon Marchi To: Ali Tamur Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] DWARF 5 support: Handle line table and file indexes In-Reply-To: <20191015201927.60124-1-tamur@google.com> References: <20191015201927.60124-1-tamur@google.com> Message-ID: <99f8963b76221d747d0564919217a2eb@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.10 X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00468.txt.bz2 On 2019-10-15 16:19, Ali Tamur wrote: >> > -enum class file_name_index : unsigned int {}; >> > +typedef int file_name_index; >> >> Is there a reason you changed these?  The reason why these were of >> enum >> class type (or "strong typedef") still seems valid to me.  We don't >> want >> some code accessing the vectors blindy using these indices, so it >> encourages people to use the right accessor method instead. > I respectfully disagree. We do "index - 1" calculation in file_name_at > and > include_dir_at methods. If we are going to cast back and forth to > integers > anyways, having an enum loses its appeal. Ok, I guess that if we access the vector through these accessor functions, it should be safe. I'd still suggest adding a gdb_assert to verify that in the case of DWARF <= 4, index should be > 0. > @@ -11717,16 +11735,16 @@ dwarf2_cu::setup_type_unit_groups (struct > die_info *die) > process_full_type_unit still needs to know if this is the first > time. */ > > - tu_group->num_symtabs = line_header->file_names.size (); > + tu_group->num_symtabs = line_header->file_names_size (); > tu_group->symtabs = XNEWVEC (struct symtab *, > - line_header->file_names.size ()); > + line_header->file_names_size ()); > > - for (i = 0; i < line_header->file_names.size (); ++i) > + auto &file_names = line_header->file_names (); > + for (i = 0; i < file_names.size (); ++i) > { > - file_entry &fe = line_header->file_names[i]; > - > - dwarf2_start_subfile (this, fe.name, > - fe.include_dir (line_header)); > + file_entry *fe = &file_names[i]; In these various loops, the `fe` variable can stay a reference, unless there's a specific reason why you changed them to pointers. > @@ -20286,9 +20304,9 @@ line_header::add_include_dir (const char > *include_dir) > { > if (dwarf_line_debug >= 2) > fprintf_unfiltered (gdb_stdlog, "Adding dir %zu: %s\n", > - include_dirs.size () + 1, include_dir); > + m_include_dirs.size () + 1, include_dir); I think this debug message showing the index should be 0 based in DWARF5, and 1 based before. > > - include_dirs.push_back (include_dir); > + m_include_dirs.push_back (include_dir); > } > > void > @@ -20299,9 +20317,9 @@ line_header::add_file_name (const char *name, > { > if (dwarf_line_debug >= 2) > fprintf_unfiltered (gdb_stdlog, "Adding file %u: %s\n", > - (unsigned) file_names.size () + 1, name); > + (unsigned) file_names_size () + 1, name); Likewise. > @@ -20651,18 +20677,17 @@ dwarf_decode_line_header (sect_offset > sect_off, struct dwarf2_cu *cu) > Returns NULL if FILE_INDEX should be ignored, i.e., it is > pst->filename. */ > > static const char * > -psymtab_include_file_name (const struct line_header *lh, int > file_index, > +psymtab_include_file_name (const struct line_header *lh, const > file_entry *fe, Pass `fe` by const reference instead? > @@ -21367,13 +21392,14 @@ dwarf_decode_lines (struct line_header *lh, > const char *comp_dir, > > /* Now that we're done scanning the Line Header Program, we can > create the psymtab of each included file. */ > - for (file_index = 0; file_index < lh->file_names.size (); > file_index++) > - if (lh->file_names[file_index].included_p == 1) > + auto &file_names = lh->file_names (); > + for (file_index = 0; file_index < file_names.size (); > file_index++) > + if (file_names[file_index].included_p == 1) Whenever you can iterate using a range-for (when you don't need the iteration index), please do so: for (const file_entry &fe : lh->file_names ()) { ... } Simon