From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16464 invoked by alias); 10 Aug 2012 00:56:05 -0000 Received: (qmail 16456 invoked by uid 22791); 10 Aug 2012 00:56:04 -0000 X-SWARE-Spam-Status: No, hits=-3.6 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KAM_STOCKGEN,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_YM,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-lb0-f169.google.com (HELO mail-lb0-f169.google.com) (209.85.217.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 10 Aug 2012 00:55:48 +0000 Received: by lbon3 with SMTP id n3so625174lbo.0 for ; Thu, 09 Aug 2012 17:55:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:x-system-of-record:x-gm-message-state; bh=UFxCKtjmuCgTgid7Nm/YwlHirS+upE2gIkFv6eSZKkg=; b=ZBzUvsjX6lYvRrSrjfviku99ysjUN8pqmE8uGjcuqWIsnIpN4OEaZweTW+zgJ6H2Hj LXvGi1YRvasgKkpuSgA280gQIxNBCodcGDWdnJIpzxq09nudxtZVPF3q9YsVgH7ERFlm asHHqaqPcetwHsKE0448NluYqdlWIFwJgaRdNsAc7ovK/zulAHbyuRsIk8XG6OK6/zC1 C19QXPIa0XGdR8saUS8ObVWAfKUHIvUtQmJGSp+I2kddr++ZRIe6E5D5qKwAJ7BtHmv5 hv/1I9mtLURbTYJBiz3iQ299Yo1rR7TJ1EkJ804uwJGfX1oysQDj7oP6r0tGw5gCxTnq DmsA== Received: by 10.152.125.116 with SMTP id mp20mr1130231lab.19.1344560146082; Thu, 09 Aug 2012 17:55:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.152.125.116 with SMTP id mp20mr1130227lab.19.1344560145887; Thu, 09 Aug 2012 17:55:45 -0700 (PDT) Received: by 10.112.102.10 with HTTP; Thu, 9 Aug 2012 17:55:45 -0700 (PDT) In-Reply-To: <20120809132716.GB30412@bubble.grove.modra.org> References: <20120809132716.GB30412@bubble.grove.modra.org> Date: Fri, 10 Aug 2012 02:11:00 -0000 Message-ID: Subject: Re: powerpc gold work in progress From: Ian Lance Taylor To: binutils@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-Gm-Message-State: ALoCoQkowh21KpJfNNNtYC1q7C4DW2IOLLAiA2XPffuYRZNfn5ahj9RP/HEFiSe8BtFz7vrqFJgR43T0uLUv6H6MqnwpPjtRCYj6MmdiLjMk3ZfcFvo1LgeC5fQyJvUYLZuUy3zilHmPhpOSTxduJ23ZFQe8JrFZpGSsl0JVtV6zL+jPMLrFzKS9o0B7jA5XHPTERdmg6hrH X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2012-08/txt/msg00183.txt.bz2 On Thu, Aug 9, 2012 at 6:27 AM, Alan Modra wrote: > I wrote a lot of this well over a year ago, but > never found the time to test (or enthusiasm to wrestle with C++ - I'd > almost be more comfortable if this was fortran). ;-) But FORTRAN doesn't have templates. > * object.h (Sized_relobj_file::find_shdr): Two off new functions. > (Sized_relobj_file::find_special_sections): New function. > * object.cc (Sized_relobj_file::find_shdr): Two off new functions. > (Sized_relobj_file::find_eh_frame): Use find_shdr. > (Sized_relobj_file::find_special_sections): New function, split out.. > (Sized_relobj_file::do_read_symbols): ..from here. > * output.h (Output_data_got::num_entries): New function. > (Output_data_got::last_got_offset,set_got_size): Use it. > (class Got_entry): No longer private. > * powerpc.cc (class Powerpc_relobj): New. > (class Powerpc_relocate_functions): Delete all psymval variants or > convert to value,addend type. Delete pcrela, pcrela_unaligned. > Implement _ha functions using corresponding _hi function. > (Powerpc_relobj::find_special_sections): New function. > (Target_powerpc::do_make_elf_object): New function. > (class Output_data_got_powerpc): New. > (class Output_data_glink): New. > (class Powerpc_scan_relocatable_reloc): New. > Many more changes througout file. > --- gold/object.h 26 Apr 2012 00:07:17 -0000 1.117 > +++ gold/object.h 9 Aug 2012 12:12:54 -0000 > map_to_kept_section(unsigned int shndx, bool* found) const; > > + // Find the section header with the given SH_NAME. > + const unsigned char* > + find_shdr(const unsigned char* pshdrs, elfcpp::Elf_Word sh_name) const; I can't see any reason to make this method public. It should probably be private. > + // Find the section header with the given NAME. Ignore headers that > + // have sh_name less than NAMES + UOFF. If section header is > + // found, updates UOFF so that a subsequent call will find the next > + // match. > + const unsigned char* > + find_shdr(const unsigned char* pshdrs, const char *name, > + const char* names, section_size_type names_size, > + size_t& uoff) const; s/const char *name,/const char* name,/ gold doesn't use non-const references as function parameters. Change uoff to be size_t*. gold generally tries to avoid overloading methods that are not essentially identical, and these two versions of find_shdr are not essentially identical. One should be renamed. I guess I don't understand how this can do what you want, assuming I understand what you want. It seems to assume that, if there is more than one section with the same name, they will have different sh_name fields. But that assumption does not seem correct to me. An ELF file can certainly have multiple section headers with the same sh_name value. > + // Stash away info for a number of special sections. > + // Return true if any of the sections found require local symbols to be read. > + virtual bool > + find_special_sections(Read_symbols_data* sd); gold uses a specific pattern for virtual methods, which is intended to clearly separate the interface of users of the class and the interface between the class and child classes. This method should be part of the public interface of the class. It should be in the list of protected methods, and it should start with do_, as in do_find_special_sections. > +// Find the section header with the given name. > + > +template > +const unsigned char* > +Sized_relobj_file::find_shdr( > + const unsigned char* pshdrs, > + const char *name, s/const char */const char* / > +{ > + size_t len = strlen(name) + 1; > + for (const char *p = names + uoff; > + (p = reinterpret_cast(memmem(p, names_size - (p - names), > + name, len))) != NULL; > + p += len) I don't really like putting assignments in a for loop conditional. I find it hard to follow. I would write this as a while loop instead. But I admit this is pure personal style. > @@ -520,24 +567,15 @@ Sized_relobj_file::fin > const char* names, > section_size_type names_size) const > { > - const unsigned int shnum = this->shnum(); > - const unsigned char* p = pshdrs + This::shdr_size; > - for (unsigned int i = 1; i < shnum; ++i, p += This::shdr_size) > + const unsigned char* s; > + size_t off = 0; > + > + while ((s = this->find_shdr(pshdrs, ".eh_frame", > + names, names_size, off)) != NULL) > { > - typename This::Shdr shdr(p); > + typename This::Shdr shdr(s); > if (this->check_eh_frame_flags(&shdr)) > - { > - if (shdr.get_sh_name() >= names_size) > - { > - this->error(_("bad section name offset for section %u: %lu"), > - i, static_cast(shdr.get_sh_name())); > - continue; > - } > - > - const char* name = names + shdr.get_sh_name(); > - if (strcmp(name, ".eh_frame") == 0) > - return true; > - } > + return true; > } > return false; > } I'm not sure it's really worth worrying about the possibility of multiple sections named .eh_frame. The chances of us handling that correctly are low. In the current code the reason that check_eh_frame_flags is called first is an optimization: it saves the call to strcmp. In your code, where you find the section name first, I think it would be fine to skip the call to check_eh_frame_flags, and probably just remove that function entirely. > @@ -2318,7 +2323,6 @@ class Output_data_got : public Output_da > void > write(unsigned char* pov) const; > > - private: > enum > { > GSYM_CODE = 0x7fffffff, Please don't just drop the private here. It's OK to make GSYM_CODE, etc., public or protected, but the data members should still be private. > +protected: > + // Post constructor setup. > + void > + do_setup() > + { > + // Call parent's setup method. > + Sized_relobj_file::do_setup(); > + > + } Why override the parent method if all you are going to do is call it? > + unsigned int got2_section; Data members have names ending with an underscore. I ran out of time to look at all the changes in powerpc.cc, and I wanted to get this out. I'll take another look after revisions. Is it really true that the only way to find the .got2 section in an input file is by name? Ian