From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27779 invoked by alias); 27 Aug 2008 18:31:32 -0000 Received: (qmail 27771 invoked by uid 22791); 27 Aug 2008 18:31:31 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.33.17) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 27 Aug 2008 18:30:49 +0000 Received: from zps36.corp.google.com (zps36.corp.google.com [172.25.146.36]) by smtp-out3.google.com with ESMTP id m7RIUcEh010773 for ; Wed, 27 Aug 2008 19:30:39 +0100 Received: from wf-out-1314.google.com (wfd26.prod.google.com [10.142.4.26]) by zps36.corp.google.com with ESMTP id m7RIUGw8010420 for ; Wed, 27 Aug 2008 11:30:37 -0700 Received: by wf-out-1314.google.com with SMTP id 26so2874024wfd.1 for ; Wed, 27 Aug 2008 11:30:37 -0700 (PDT) Received: by 10.142.154.20 with SMTP id b20mr98863wfe.222.1219861837090; Wed, 27 Aug 2008 11:30:37 -0700 (PDT) Received: from dhcp-172-29-127-178.ame.corp.google.com ( [72.14.228.1]) by mx.google.com with ESMTPS id 30sm6559979wff.18.2008.08.27.11.30.34 (version=TLSv1/SSLv3 cipher=RC4-MD5); Wed, 27 Aug 2008 11:30:35 -0700 (PDT) Message-ID: <48B59D48.9060105@google.com> Date: Thu, 28 Aug 2008 17:36:00 -0000 From: Diego Novillo User-Agent: Thunderbird 2.0.0.16 (Macintosh/20080707) MIME-Version: 1.0 To: Rafael Espindola CC: gcc-patches , Cary Coutant Subject: Re: [lto][patch] Add initial gold plugin References: <38a0d8450808270339p3054cacbq9265a0168e1afd78@mail.gmail.com> In-Reply-To: <38a0d8450808270339p3054cacbq9265a0168e1afd78@mail.gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2008-08/txt/msg02173.txt.bz2 Rafael Espindola wrote: > +it under the terms of the GNU General Public License as published by > +the Free Software Foundation; either version 3 of the License, or > +(at your option) any later version. > + > +This program is distributed in the hope that it will be useful, > +but WITHOUT ANY WARRANTY; without even the implied warranty of > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +GNU General Public License for more details. > + > +You should have received a copy of the GNU General Public License > +along with this program; if not, write to the Free Software > +Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ > + Could you add a file level comment explaining at a high level how the plugin operates? Some of it will need to refer to the documentation in gold, which for now is mostly what we have in the LTO wiki, I suspect. > +/* Returns the IL symbol table of file ELF. */ > + > +Elf_Data * > +get_symtab (Elf *elf) > +{ > + Elf_Scn *section = get_section (elf, ".gnu.lto_.symtab"); > + assert (section); Do we really want to assert here? Maybe return an error code so the caller can emit a diagnostic? > + Elf_Data *data = 0; > + data = elf_getdata (section, data); > + assert (data); > + return data; > +} > + > +static struct ld_plugin_symbol * > +translate (Elf_Data *symtab, int *num) Needs documentation. Applies to most new functions in the file. > + > +static enum ld_plugin_status > +claim_file_handler(const struct ld_plugin_input_file *file, int *claimed) > +{ > + enum ld_plugin_status status; > + Elf *elf = elf_begin(file->fd, ELF_C_READ, NULL); Space before '('. There are a few occurrences of this throughout. > > -Elf_Scn * > -get_section (Elf *elf, const char *name) > +static enum ld_plugin_status > +add_symbols (const void *handle, int nsyms, > + const struct ld_plugin_symbol *syms) > { > - const char *string_table = get_string_table (elf); > - Elf_Scn *section = 0; > - while ((section = elf_nextscn(elf, section)) != 0) > + const char *kind_str[] = {"DEF", "WEAKDEF", "UNDEF", > + "WEAKUNDEF", "COMMON"}; > + const char *visibility_str[] = {"DEFAULT", "PROTECTED", > + "INTERNAL", "HIDDEN"}; > + const char *resolution_str[] = {"UNKNOWN", "UNDEF", > + "PREVAILING_DEF", > + "PREVAILING_DEF_IRONLY", > + "PREEMPTED_REG", > + "PREEMPTED_IR", > + "RESOLVED_IR", > + "RESOLVED_EXEC", > + "RESOLVED_DYN"}; Move to a header file? The rest looks fine. Thanks. Diego.