This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] gold: Native Client target support
- From: Ian Lance Taylor <iant at google dot com>
- To: Roland McGrath <mcgrathr at google dot com>
- Cc: binutils at sourceware dot org, sehr at google dot com
- Date: Tue, 01 May 2012 14:35:01 -0700
- Subject: Re: [PATCH] gold: Native Client target support
- References: <x57jobqhxdre.fsf@frobland.mtv.corp.google.com>
Roland McGrath <mcgrathr@google.com> writes:
> 2012-04-24 Roland McGrath <mcgrathr@google.com>
>
> * configure.ac (ENABLE_GOLD): Consider *-*-nacl* targets ELF.
> * configure: Regenerate.
>
> gold/
> 2012-04-24 Roland McGrath <mcgrathr@google.com>
>
> * nacl.cc: New file.
> * nacl.h: New file.
> * Makefile.am (CCFILES, HFILES): Add them.
> * Makefile.in: Regenerate.
> * i386.cc (Output_data_plt_i386_nacl): New class.
> (Output_data_plt_i386_nacl_exec): New class.
> (Output_data_plt_i386_nacl_dyn): New class.
> (Target_i386_nacl): New class.
> (Target_selector_i386_nacl): New class.
> (target_selector_i386): Use it instead of Target_selector_i386.
> * x86_64.cc (Output_data_plt_x86_64_nacl): New class.
> (Target_x86_64_nacl): New class.
> (Target_selector_x86_64_nacl): New class.
> (target_selector_x86_64, target_selector_x32): Use it instead of
> Target_selector_x86_64.
> * arm.cc (Output_data_plt_arm_nacl): New class.
> (Target_arm_nacl): New class.
> (Target_selector_arm_nacl): New class.
> (target_selector_arm, target_selector_armbe): Use it instead of
> Target_selector_arm.
>
> * target-select.cc (select_target): Take new Input_file*
> argument, pass it on to recognize method of selector.
> * object.cc (make_elf_sized_object): Update caller.
> * parameters.cc (parameters_force_valid_target): Likewise.
> * incremental.cc (make_sized_incremental_binary): Likewise.
> * target-select.h: Update decl.
> (Target_selector::recognize): Take new Input_file* argument,
> pass it on to do_recognize.
> (Target_selector::do_recognize): Take new Input_file* argument.
> * freebsd.h (Target_selector_freebsd::do_recognize): Likewise.
> * powerpc.cc (Target_selector_powerpc::do_recognize): Likewise.
> * sparc.cc (Target_selector_sparc::do_recognize): Likewise.
> * testsuite/testfile.cc (Target_selector::do_recognize): Likewise.
>
> * target.h (Target::Target_info): New members isolate_execinstr
> and rosegment_gap.
> (Target::isolate_execinstr, Target::rosegment_gap): New methods.
> * arm.cc (Target_arm::arm_info): Update initializer.
> * i386.cc (Target_i386::i386_info): Likewise.
> * powerpc.cc (Target_powerpc::powerpc_info): Likewise.
> * sparc.cc (Target_sparc::sparc_info): Likewise.
> * x86_64.cc (Target_x86_64::x86_64_info): Likewise.
> * testsuite/testfile.cc (Target_test::test_target_info): Likewise.
> * layout.cc (Layout::attach_allocated_section_to_segment):
> Take new const Target* argument. If target->isolate_execinstr(), act
> like --rosegment.
> (Layout::find_first_load_seg): Take new const Target* argument;
> If target->isolate_execinstr(), reject PF_X segments.
> (Layout::relaxation_loop_body): Update caller.
> (Layout::set_segment_offsets): If target->isolate_execinstr(),
> reset file offset to zero when we hit LOAD_SEG, and then a second
> loop over the segments before LOAD_SEG to reassign offsets after
> addresses have been determined. Handle target->rosegment_gap().
> (Layout::attach_section_to_segment): Take new const Target* argument;
> pass it to attach_allocated_section_to_segment.
> (Layout::make_output_section): Update caller.
> (Layout::attach_sections_to_segments): Take new const Target* argument;
> pass it to attach_section_to_segment.
> * gold.cc (queue_middle_tasks): Update caller.
> * layout.h (Layout): Update method decls with new arguments.
>
> * arm.cc (Target_arm::Target_arm): Take optional argument for the
> Target_info pointer to use.
> (Target_arm::make_data_plt): New virtual method.
> (Target_arm::make_plt_entry): Use it.
> (Output_data_plt_arm::Output_data_plt_arm): Take additional argument
> for the section alignment.
> (Output_data_plt_arm::first_plt_entry_offset): Make abstract virtual.
> (Output_data_plt_arm::get_plt_entry_size): Likewise.
> (Output_data_plt_arm::fill_plt_entry): New abstract virtual method.
> (Output_data_plt_arm::fill_first_plt_entry): Likewise.
> (Output_data_plt_arm::set_final_data_size): Use get_plt_entry_size
> method instead of sizeof(plt_entry).
> (Output_data_plt_arm::add_entry): Likewise.
> Use first_plt_entry_offset method instead of sizeof(first_plt_entry).
> (Target_arm::first_plt_entry_offset): Call method on this->plt_ rather
> than static method.
> (Target_arm::plt_entry_size): Likewise.
> (Output_data_plt_arm::first_plt_entry, Output_data_plt_arm::plt_entry):
> Move to ...
> (Output_data_plt_arm_standard): ... here, new class.
> (Output_data_plt_arm::do_write): Move guts of PLT filling to...
> (Output_data_plt_arm_standard::fill_first_plt_entry): ... here ...
> (Output_data_plt_arm_standard::fill_plt_entry): ... and here.
>
> * x86_64.cc (Output_data_plt_x86_64::Output_data_plt_x86_64):
> Take additional argument for the PLT entry size.
> (Output_data_plt_x86_64::get_tlsdesc_plt_offset):
> Use get_plt_entry_size method rather than plt_entry_size variable.
> (Output_data_plt_x86_64::reserve_slot): Likewise.
> (Output_data_plt_x86_64::do_adjust_output_section): Likewise.
> (Output_data_plt_x86_64::add_entry): Likewise.
> (Output_data_plt_x86_64::add_local_ifunc_entry): Likewise.
> (Output_data_plt_x86_64::address_for_global): Likewise.
> (Output_data_plt_x86_64::address_for_local): Likewise.
> (Output_data_plt_x86_64::set_final_data_size): Likewise.
> (Output_data_plt_x86_64::first_plt_entry_offset): Likewise.
> Make method non-static.
> (Output_data_plt_x86_64::get_plt_entry_size): Make abstract virtual.
> (Output_data_plt_x86_64::add_eh_frame): New abstract virtual method.
> (Output_data_plt_x86_64::fill_first_plt_entry): Likewise.
> (Output_data_plt_x86_64::fill_plt_entry): Likewise.
> (Output_data_plt_x86_64::fill_tlsdesc_entry): Likewise.
> (Output_data_plt_x86_64::plt_entry_size)
> (Output_data_plt_x86_64::first_plt_entry)
> (Output_data_plt_x86_64::plt_entry)
> (Output_data_plt_x86_64::tlsdesc_plt_entry)
> (Output_data_plt_x86_64::plt_eh_frame_fde_size)
> (Output_data_plt_x86_64::plt_eh_frame_fde): Move to ...
> (Output_data_plt_x86_64_standard): ... here, new class.
> (Target_x86_64::Target_x86_64): Take optional argument for the
> Target_info pointer to use.
> (Target_x86_64::make_data_plt): New virtual method.
> (Target_x86_64::init_got_plt_for_update): Use it.
> Call this->plt_->add_eh_frame method here.
> (Output_data_plt_x86_64::init): Don't do add_eh_frame_for_plt here.
> (Target_x86_64::first_plt_entry_offset): Call method on this->plt_
> rather than static method.
> (Target_x86_64::plt_entry_size): Likewise.
> (Output_data_plt_x86_64::do_write): Use get_plt_entry_size method
> rather than plt_entry_size variable. Move guts of PLT filling to...
> (Output_data_plt_x86_64_standard::fill_first_plt_entry): ... here ...
> (Output_data_plt_x86_64_standard::fill_plt_entry): ... and here ...
> (Output_data_plt_x86_64_standard::fill_tlsdesc_entry): ... and here.
>
> * i386.cc (Output_data_plt_i386::Output_data_plt_i386): Take
> additional argument for the section alignment.
> Don't do add_eh_frame_for_plt here.
> (Output_data_plt_i386::first_plt_entry_offset): Make the method
> non-static. Use get_plt_entry_size method rather than plt_entry_size
> variable.
> (Output_data_plt_i386::get_plt_entry_size): Make abstract virtual.
> (Output_data_plt_i386::add_eh_frame): New abstract virtual method.
> (Output_data_plt_i386::fill_first_plt_entry): Likewise.
> (Output_data_plt_i386::fill_plt_entry): Likewise.
> (Output_data_plt_i386::set_final_data_size): Use get_plt_entry_size
> method instead of plt_entry_size.
> (Output_data_plt_i386::plt_entry_size)
> (Output_data_plt_i386::plt_eh_frame_fde_size)
> (Output_data_plt_i386::plt_eh_frame_fde): Move to ...
> (Output_data_plt_i386_standard): ... here, new class.
> (Output_data_plt_i386::exec_first_plt_entry)
> (Output_data_plt_i386::exec_plt_entry): Move to ...
> (Output_data_plt_i386_exec): ... here, new class.
> (Output_data_plt_i386::dyn_first_plt_entry): Move to ...
> (Output_data_plt_i386::dyn_plt_entry)
> (Output_data_plt_i386_dyn): ... here, new class.
> (Target_i386::Target_i386): Take optional argument for the Target_info
> pointer to use.
> (Target_i386::make_data_plt): New virtual method.
> (Target_i386::make_plt_section): Use it.
> Call this->plt_->add_eh_frame method here.
> (Output_data_plt_i386::add_entry): Use get_plt_entry_size method
> rather than plt_entry_size variable.
> (Output_data_plt_i386::add_local_ifunc_entry): Likewise.
> (Output_data_plt_i386::address_for_local): Likewise.
> (Output_data_plt_i386::do_write): Likewise.
> Move guts of PLT filling to...
> (Output_data_plt_i386_exec::fill_first_plt_entry): ... here ...
> (Output_data_plt_i386_exec::fill_plt_entry): ... and here ...
> (Output_data_plt_i386_dyn::fill_first_plt_entry): ... and here ...
> (Output_data_plt_i386_dyn::fill_plt_entry): ... and here.
> + virtual unsigned int
> + first_plt_entry_offset() const = 0;
>
> // Return the size of a PLT entry.
> - static unsigned int
> - get_plt_entry_size()
> - { return sizeof(plt_entry); }
> + virtual unsigned int
> + get_plt_entry_size() const = 0;
gold has a general pattern that you are not following here: public
functions in a class are never virtual, except for pure abstract base
classes. The idea is that we don't want to confuse the public interface
that a class presents to its users with the interface that child classes
present to the parent class. So I would be happier if these functions
were not virtual, but instead called protected virtual functions
implemented by the child classes. There are many existing examples in
gold, usually a public function NAME calling a protected virtual
function do_NAME.
> + protected:
> + virtual void
> + fill_first_plt_entry(unsigned char* pov,
> + Arm_address got_address,
> + Arm_address plt_address)
> + {
> + // Write first PLT entry. All but the last word are constants.
> + const size_t num_first_plt_words = (sizeof(first_plt_entry)
> + / sizeof(plt_entry[0]));
> + for (size_t i = 0; i < num_first_plt_words - 1; i++)
> + elfcpp::Swap<32, big_endian>::writeval(pov + i * 4, first_plt_entry[i]);
> + // Last word in first PLT entry is &GOT[0] - .
> + elfcpp::Swap<32, big_endian>::writeval(pov + 16,
> + got_address - (plt_address + 16));
> + }
> +
> + virtual void
> + fill_plt_entry(unsigned char* pov,
> + Arm_address got_address,
> + Arm_address plt_address,
> + unsigned int got_offset,
> + unsigned int plt_offset)
> + {
> + int32_t offset = ((got_address + got_offset)
> + - (plt_address + plt_offset + 8));
> +
> + gold_assert(offset >= 0 && offset < 0x0fffffff);
> + uint32_t plt_insn0 = plt_entry[0] | ((offset >> 20) & 0xff);
> + elfcpp::Swap<32, big_endian>::writeval(pov, plt_insn0);
> + uint32_t plt_insn1 = plt_entry[1] | ((offset >> 12) & 0xff);
> + elfcpp::Swap<32, big_endian>::writeval(pov + 4, plt_insn1);
> + uint32_t plt_insn2 = plt_entry[2] | (offset & 0xfff);
> + elfcpp::Swap<32, big_endian>::writeval(pov + 8, plt_insn2);
> + }
There is no reason for these large functions to be declared in the class
itself. The function implementations should be moved out.
> + protected:
> + virtual void
> + fill_first_plt_entry(unsigned char* pov,
> + Arm_address got_address,
> + Arm_address plt_address)
> + {
> + // Write first PLT entry. All but first two words are constants.
> + const size_t num_first_plt_words = (sizeof(first_plt_entry)
> + / sizeof(first_plt_entry[0]));
> +
> + int32_t got_displacement = got_address + 8 - (plt_address + 16);
> +
> + elfcpp::Swap<32, big_endian>::writeval
> + (pov + 0, first_plt_entry[0] | arm_movw_immediate (got_displacement));
> + elfcpp::Swap<32, big_endian>::writeval
> + (pov + 4, first_plt_entry[1] | arm_movt_immediate (got_displacement));
> +
> + for (size_t i = 2; i < num_first_plt_words; ++i)
> + elfcpp::Swap<32, big_endian>::writeval(pov + i * 4, first_plt_entry[i]);
> + }
> +
> + virtual void
> + fill_plt_entry(unsigned char* pov,
> + Arm_address got_address,
> + Arm_address plt_address,
> + unsigned int got_offset,
> + unsigned int plt_offset)
> + {
> + // Calculate the displacement between the PLT slot and the
> + // common tail that's part of the special initial PLT slot.
> + int32_t tail_displacement = (plt_address + (11 * sizeof(uint32_t))
> + - (plt_address + plt_offset
> + + sizeof(plt_entry) + sizeof(uint32_t)));
Add parentheses around the whole expression so that emacs indents
correctly.
> + gold_assert((tail_displacement & 3) == 0);
> + tail_displacement >>= 2;
> +
> + gold_assert ((tail_displacement & 0xff000000) == 0
> + || (-tail_displacement & 0xff000000) == 0);
> +
> + // Calculate the displacement between the PLT slot and the entry
> + // in the GOT. The offset accounts for the value produced by
> + // adding to pc in the penultimate instruction of the PLT stub.
> + const int32_t got_displacement = (got_address + got_offset
> + - (plt_address + sizeof(plt_entry)));
> +
> + elfcpp::Swap<32, big_endian>::writeval
> + (pov + 0, plt_entry[0] | arm_movw_immediate (got_displacement));
> + elfcpp::Swap<32, big_endian>::writeval
> + (pov + 4, plt_entry[1] | arm_movt_immediate (got_displacement));
> + elfcpp::Swap<32, big_endian>::writeval
> + (pov + 8, plt_entry[2]);
> + elfcpp::Swap<32, big_endian>::writeval
> + (pov + 12, plt_entry[3] | (tail_displacement & 0x00ffffff));
> + }
Move two large virtual functions above out of the class definition.
> diff --git a/gold/freebsd.h b/gold/freebsd.h
> index 175dd05..da7b85f 100644
> --- a/gold/freebsd.h
> +++ b/gold/freebsd.h
> @@ -1,6 +1,6 @@
> // freebsd.h -- FreeBSD support for gold -*- C++ -*-
>
> -// Copyright 2009, 2011 Free Software Foundation, Inc.
> +// Copyright 2009,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the copyright years.
> diff --git a/gold/gold.cc b/gold/gold.cc
> index f810bf9..e9453e6 100644
> --- a/gold/gold.cc
> +++ b/gold/gold.cc
> @@ -1,6 +1,6 @@
> // gold.cc -- main linker functions
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the copyright years. Break the line after 2012.
> @@ -90,14 +95,13 @@ class Output_data_plt_i386 : public Output_section_data
> { return this->count_ + this->irelative_count_; }
>
> // Return the offset of the first non-reserved PLT entry.
> - static unsigned int
> + unsigned int
> first_plt_entry_offset()
> - { return plt_entry_size; }
> + { return get_plt_entry_size(); }
Add explicit this: "this->get_plt_entry_size();".
> // Return the size of a PLT entry.
> - static unsigned int
> - get_plt_entry_size()
> - { return plt_entry_size; }
> + virtual unsigned int
> + get_plt_entry_size() const = 0;
Same comments as above about not having public virtual functions.
> // Set the final size.
> void
> set_final_data_size()
> {
> this->set_data_size((this->count_ + this->irelative_count_ + 1)
> - * plt_entry_size);
> + * get_plt_entry_size());
Add explicit this->
> + protected:
> + virtual void
> + fill_first_plt_entry(unsigned char* pov,
> + elfcpp::Elf_types<32>::Elf_Addr got_address)
> + {
> + memcpy(pov, first_plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2, got_address + 4);
> + elfcpp::Swap<32, false>::writeval(pov + 8, got_address + 8);
> + }
> +
> + virtual unsigned int
> + fill_plt_entry(unsigned char* pov,
> + elfcpp::Elf_types<32>::Elf_Addr got_address,
> + unsigned int got_offset,
> + unsigned int plt_offset,
> + unsigned int plt_rel_offset)
> + {
> + memcpy(pov, plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2,
> + got_address + got_offset);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 7, plt_rel_offset);
> + elfcpp::Swap<32, false>::writeval(pov + 12, - (plt_offset + 12 + 4));
> + return 6;
> + }
Move these two functions out of the class definition.
> + virtual unsigned int
> + fill_plt_entry(unsigned char* pov,
> + elfcpp::Elf_types<32>::Elf_Addr,
> + unsigned int got_offset,
> + unsigned int plt_offset,
> + unsigned int plt_rel_offset)
> + {
> + memcpy(pov, plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2, got_offset);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 7, plt_rel_offset);
> + elfcpp::Swap<32, false>::writeval(pov + 12, - (plt_offset + 12 + 4));
> + return 6;
> + }
Move this function out of the class definition.
> @@ -693,10 +839,12 @@ const Target::Target_info Target_i386::i386_info =
> true, // is_default_stack_executable
> true, // can_icf_inline_merge_sections
> '\0', // wrap_char
> - "/usr/lib/libc.so.1", // dynamic_linker
> + "/lib/ld-nacl-x86-32.so.1", // dynamic_linker
This change looks wrong. Please revert.
> @@ -861,7 +1006,7 @@ Output_data_plt_i386::add_entry(Symbol_table* symtab, Layout* layout,
> if (gsym->type() == elfcpp::STT_GNU_IFUNC
> && gsym->can_use_relative_reloc(false))
> {
> - gsym->set_plt_offset(this->irelative_count_ * plt_entry_size);
> + gsym->set_plt_offset(this->irelative_count_ * get_plt_entry_size());
Add explicit this->.
> @@ -878,7 +1023,7 @@ Output_data_plt_i386::add_entry(Symbol_table* symtab, Layout* layout,
> {
> // When setting the PLT offset we skip the initial reserved PLT
> // entry.
> - gsym->set_plt_offset((this->count_ + 1) * plt_entry_size);
> + gsym->set_plt_offset((this->count_ + 1) * get_plt_entry_size());
Add explicit this->.
> @@ -909,7 +1054,7 @@ Output_data_plt_i386::add_local_ifunc_entry(
> Sized_relobj_file<32, false>* relobj,
> unsigned int local_sym_index)
> {
> - unsigned int plt_offset = this->irelative_count_ * plt_entry_size;
> + unsigned int plt_offset = this->irelative_count_ * get_plt_entry_size();
Add explicit this->.
> @@ -998,7 +1143,7 @@ Output_data_plt_i386::address_for_global(const Symbol* gsym)
> uint64_t offset = 0;
> if (gsym->type() == elfcpp::STT_GNU_IFUNC
> && gsym->can_use_relative_reloc(false))
> - offset = (this->count_ + 1) * plt_entry_size;
> + offset = (this->count_ + 1) * get_plt_entry_size();
Add explicit this->.
> @@ -1008,12 +1153,12 @@ Output_data_plt_i386::address_for_global(const Symbol* gsym)
> uint64_t
> Output_data_plt_i386::address_for_local(const Relobj*, unsigned int)
> {
> - return this->address() + (this->count_ + 1) * plt_entry_size;
> + return this->address() + (this->count_ + 1) * get_plt_entry_size();
Add explicit this->.
> @@ -1130,15 +1275,8 @@ Output_data_plt_i386::do_write(Output_file* of)
> elfcpp::Elf_types<32>::Elf_Addr plt_address = this->address();
> elfcpp::Elf_types<32>::Elf_Addr got_address = this->got_plt_->address();
>
> - if (parameters->options().output_is_position_independent())
> - memcpy(pov, dyn_first_plt_entry, plt_entry_size);
> - else
> - {
> - memcpy(pov, exec_first_plt_entry, plt_entry_size);
> - elfcpp::Swap_unaligned<32, false>::writeval(pov + 2, got_address + 4);
> - elfcpp::Swap<32, false>::writeval(pov + 8, got_address + 8);
> - }
> - pov += plt_entry_size;
> + this->fill_first_plt_entry(pov, got_address);
> + pov += get_plt_entry_size();
Add explicit this->.
> @@ -1155,40 +1293,29 @@ Output_data_plt_i386::do_write(Output_file* of)
>
> const int rel_size = elfcpp::Elf_sizes<32>::rel_size;
>
> - unsigned int plt_offset = plt_entry_size;
> + unsigned int plt_offset = get_plt_entry_size();
Add explicit this->.
> unsigned int plt_rel_offset = 0;
> unsigned int got_offset = 12;
> const unsigned int count = this->count_ + this->irelative_count_;
> for (unsigned int i = 0;
> i < count;
> ++i,
> - pov += plt_entry_size,
> + pov += get_plt_entry_size(),
Add explicit this->.
> got_pov += 4,
> - plt_offset += plt_entry_size,
> + plt_offset += get_plt_entry_size(),
Add explicit this->.
> + protected:
> + virtual void
> + fill_first_plt_entry(unsigned char* pov,
> + elfcpp::Elf_types<32>::Elf_Addr got_address)
> + {
> + memcpy(pov, exec_first_plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2, got_address + 4);
> + elfcpp::Swap<32, false>::writeval(pov + 8, got_address + 8);
> + }
> +
> + virtual unsigned int
> + fill_plt_entry(unsigned char* pov,
> + elfcpp::Elf_types<32>::Elf_Addr got_address,
> + unsigned int got_offset,
> + unsigned int plt_offset,
> + unsigned int plt_rel_offset)
> + {
> + memcpy(pov, exec_plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2,
> + got_address + got_offset);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 33, plt_rel_offset);
> + elfcpp::Swap<32, false>::writeval(pov + 38, - (plt_offset + 38 + 4));
> + return 32;
> + }
Move these functions out of the class definition.
> + virtual unsigned int
> + fill_plt_entry(unsigned char* pov,
> + elfcpp::Elf_types<32>::Elf_Addr,
> + unsigned int got_offset,
> + unsigned int plt_offset,
> + unsigned int plt_rel_offset)
> + {
> + memcpy(pov, dyn_plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2, got_offset);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 33, plt_rel_offset);
> + elfcpp::Swap<32, false>::writeval(pov + 38, - (plt_offset + 38 + 4));
> + return 32;
> + }
move this function out of the class definition.
> diff --git a/gold/layout.cc b/gold/layout.cc
> index 65d1432..774b779 100644
> --- a/gold/layout.cc
> +++ b/gold/layout.cc
> @@ -1,6 +1,6 @@
> // layout.cc -- lay out output file sections for gold
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> diff --git a/gold/layout.h b/gold/layout.h
> index f81ea3b..ea84548 100644
> --- a/gold/layout.h
> +++ b/gold/layout.h
> @@ -1,6 +1,6 @@
> // layout.h -- lay out output file sections for gold -*- C++ -*-
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> + inline off_t offset() const
> + { return offset_; }
Add explicit this->.
> + inline section_size_type size() const
> + { return size_; }
Add explicit this->.
> + const unsigned char* data()
> + { return data_; }
Add explicit this->.
> + View view(off_t file_offset, off_t data_size)
> + {
> + return View(file_, file_offset, data_size);
Add explicit this->.
> + return View(file_, loc.offset(), loc.size());
Add explicit this->.
> + virtual Target*
> + do_instantiate_target()
> + {
> + if (is_nacl_)
Add explicit this->.
> + virtual Target*
> + do_recognize(Input_file* file, int machine, int osabi, int abiversion)
> + {
> + this->is_nacl_ = recognize_nacl_file(file);
Add explicit this->.
It looks in some cases FILE can be NULL here. You should probably skip
calling this->recognize_nacl_file in that case.
> + virtual const char*
> + do_target_bfd_name(const Target* target)
> + {
> + return (!this->is_our_target(target) ? NULL
> + : this->is_nacl_ ? this->bfd_name_
> + : base_selector::do_target_bfd_name(target));
Format this as
return (!this->is_our_target(target)
? NULL
: (this->is_nacl_
? this->bfd_name_
: base_selector::do_target_bfd_name(target)));
> + private:
> + bool
> + recognize_nacl_file(Input_file* input_file)
> + {
> + if (this->is_big_endian())
> + {
> +#if defined(HAVE_TARGET_32_BIG) || defined(HAVE_TARGET_64_BIG)
> +# ifdef HAVE_TARGET_32_BIG
> + if (this->get_size() == 32)
> + return do_recognize_nacl_file<32, true>(input_file);
> +# endif
> +# ifdef HAVE_TARGET_64_BIG
> + if (this->get_size() == 64)
> + return do_recognize_nacl_file<64, true>(input_file);
> +# endif
> +#endif
> + gold_unreachable();
> + }
> + else
> + {
> +#if defined(HAVE_TARGET_32_LITTLE) || defined(HAVE_TARGET_64_LITTLE)
> +# ifdef HAVE_TARGET_32_LITTLE
> + if (this->get_size() == 32)
> + return do_recognize_nacl_file<32, false>(input_file);
> +# endif
> +# ifdef HAVE_TARGET_64_LITTLE
> + if (this->get_size() == 64)
> + return do_recognize_nacl_file<64, false>(input_file);
> +# endif
> +#endif
> + gold_unreachable();
> + }
> + }
> +
> + template<int size, bool big_endian>
> + bool
> + do_recognize_nacl_file(Input_file* input_file)
> + {
> + Sniff_file file(input_file);
> + elfcpp::Elf_file<size, big_endian, Sniff_file> elf_file(&file);
> + const unsigned int shnum = elf_file.shnum();
> + for (unsigned int shndx = 1; shndx < shnum; ++shndx)
> + {
> + if (elf_file.section_type(shndx) == elfcpp::SHT_NOTE)
> + {
> + Sniff_file::Location loc = elf_file.section_contents(shndx);
> + if (loc.size() < (3 * 4
> + + align_address(sizeof "NaCl", 4)
> + + align_address(nacl_abi_name_.size() + 1, 4)))
> + continue;
> + Sniff_file::View view(file.view(loc));
> + const unsigned char* note_data = view.data();
> + if ((elfcpp::Swap<32, big_endian>::readval(note_data + 0)
> + == sizeof "NaCl")
> + && (elfcpp::Swap<32, big_endian>::readval(note_data + 4)
> + == nacl_abi_name_.size() + 1)
> + && (elfcpp::Swap<32, big_endian>::readval(note_data + 8)
> + == elfcpp::NT_VERSION))
> + {
> + const unsigned char* name = note_data + 12;
> + const unsigned char* desc = (name
> + + align_address(sizeof "NaCl", 4));
> + if (!memcmp(name, "NaCl", sizeof "NaCl")
> + && !memcmp(desc, nacl_abi_name_.c_str(),
> + nacl_abi_name_.size() + 1))
Don't write !memcmp(). Write memcmp() == 0.
> + return true;
> + }
> + }
> + }
> + return false;
> + }
Move these big functions out of the class definition.
> diff --git a/gold/object.cc b/gold/object.cc
> index 15e5d05..8c1359c 100644
> --- a/gold/object.cc
> +++ b/gold/object.cc
> @@ -1,6 +1,6 @@
> // object.cc -- support for an object file for linking in gold
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> diff --git a/gold/parameters.cc b/gold/parameters.cc
> index 7fc5730..a73899e 100644
> --- a/gold/parameters.cc
> +++ b/gold/parameters.cc
> @@ -1,6 +1,6 @@
> // parameters.cc -- general parameters for a link using gold
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> - Target* do_recognize(int machine, int, int)
> + virtual Target*
> + do_recognize(Input_file*, int machine, int, int)
> {
> switch (size)
> {
> @@ -2153,7 +2162,8 @@ public:
> return this->instantiate_target();
> }
>
> - Target* do_instantiate_target()
> + virtual Target*
> + do_instantiate_target()
> { return new Target_powerpc<size, big_endian>(); }
> };
Thanks for correcting the formatting here. Note that it's not necessary
to use virtual here, and I tend to omit it for functions that are
intended to be final.
> diff --git a/gold/target-select.cc b/gold/target-select.cc
> index 9370a87..90211d8 100644
> --- a/gold/target-select.cc
> +++ b/gold/target-select.cc
> @@ -1,6 +1,6 @@
> // target-select.cc -- select a target for an object file
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> diff --git a/gold/target-select.h b/gold/target-select.h
> index 310c0b9..7706077 100644
> --- a/gold/target-select.h
> +++ b/gold/target-select.h
> @@ -1,6 +1,6 @@
> // target-select.h -- select a target for an object file -*- C++ -*-
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> diff --git a/gold/target.h b/gold/target.h
> index ff97aba..0a50d06 100644
> --- a/gold/target.h
> +++ b/gold/target.h
> @@ -1,6 +1,6 @@
> // target.h -- target support for gold -*- C++ -*-
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.
Keep the spaces between the years, break the line after 2012.
> @@ -118,7 +125,7 @@ class Output_data_plt_x86_64 : public Output_section_data
> // Return the offset of the reserved TLSDESC_PLT entry.
> unsigned int
> get_tlsdesc_plt_offset() const
> - { return (this->count_ + this->irelative_count_ + 1) * plt_entry_size; }
> + { return (this->count_ + this->irelative_count_ + 1) * get_plt_entry_size(); }
Add explicit this->.
> void
> reserve_slot(unsigned int plt_index)
> {
> - this->free_list_.remove((plt_index + 1) * plt_entry_size,
> - (plt_index + 2) * plt_entry_size);
> + this->free_list_.remove((plt_index + 1) * get_plt_entry_size(),
> + (plt_index + 2) * get_plt_entry_size());
Add explicit this->.
> template<int size>
> void
> Output_data_plt_x86_64<size>::do_adjust_output_section(Output_section* os)
> {
> - os->set_entsize(plt_entry_size);
> + os->set_entsize(get_plt_entry_size());
Add explicit this->.
> @@ -1040,7 +1195,7 @@ Output_data_plt_x86_64<size>::add_entry(Symbol_table* symtab, Layout* layout,
> // Note that when setting the PLT offset for a non-IRELATIVE
> // entry we skip the initial reserved PLT entry.
> plt_index = *pcount + offset;
> - plt_offset = plt_index * plt_entry_size;
> + plt_offset = plt_index * get_plt_entry_size();
Add explicit this->.
> @@ -1057,7 +1212,8 @@ Output_data_plt_x86_64<size>::add_entry(Symbol_table* symtab, Layout* layout,
> // FIXME: This is probably not correct for IRELATIVE relocs.
>
> // For incremental updates, find an available slot.
> - plt_offset = this->free_list_.allocate(plt_entry_size, plt_entry_size, 0);
> + plt_offset = this->free_list_.allocate(get_plt_entry_size(),
> + get_plt_entry_size(), 0);
Add explicit this->.
> @@ -1065,7 +1221,7 @@ Output_data_plt_x86_64<size>::add_entry(Symbol_table* symtab, Layout* layout,
> // The GOT and PLT entries have a 1-1 correspondance, so the GOT offset
> // can be calculated from the PLT index, adjusting for the three
> // reserved entries at the beginning of the GOT.
> - plt_index = plt_offset / plt_entry_size - 1;
> + plt_index = plt_offset / get_plt_entry_size() - 1;
Add explicit this->.
> @@ -1090,7 +1246,7 @@ Output_data_plt_x86_64<size>::add_local_ifunc_entry(
> Sized_relobj_file<size, false>* relobj,
> unsigned int local_sym_index)
> {
> - unsigned int plt_offset = this->irelative_count_ * plt_entry_size;
> + unsigned int plt_offset = this->irelative_count_ * get_plt_entry_size();
Add explicit this->.
> @@ -1202,7 +1358,7 @@ Output_data_plt_x86_64<size>::address_for_global(const Symbol* gsym)
> uint64_t offset = 0;
> if (gsym->type() == elfcpp::STT_GNU_IFUNC
> && gsym->can_use_relative_reloc(false))
> - offset = (this->count_ + 1) * plt_entry_size;
> + offset = (this->count_ + 1) * get_plt_entry_size();
Add explicit this->.
> @@ -1213,7 +1369,7 @@ template<int size>
> uint64_t
> Output_data_plt_x86_64<size>::address_for_local(const Relobj*, unsigned int)
> {
> - return this->address() + (this->count_ + 1) * plt_entry_size;
> + return this->address() + (this->count_ + 1) * get_plt_entry_size();
Add explicit this->.
> @@ -1224,14 +1380,14 @@ Output_data_plt_x86_64<size>::set_final_data_size()
> unsigned int count = this->count_ + this->irelative_count_;
> if (this->has_tlsdesc_entry())
> ++count;
> - this->set_data_size((count + 1) * plt_entry_size);
> + this->set_data_size((count + 1) * get_plt_entry_size());
Add explicit this->.
> + this->fill_first_plt_entry(pov, got_address, plt_address);
> + pov += get_plt_entry_size();
Add explicit this->.
> @@ -1379,47 +1528,35 @@ Output_data_plt_x86_64<size>::do_write(Output_file* of)
> memset(got_pov, 0, 16);
> got_pov += 16;
>
> - unsigned int plt_offset = plt_entry_size;
> + unsigned int plt_offset = get_plt_entry_size();
Add explicit this->.
> unsigned int got_offset = 24;
> const unsigned int count = this->count_ + this->irelative_count_;
> for (unsigned int plt_index = 0;
> plt_index < count;
> ++plt_index,
> - pov += plt_entry_size,
> + pov += get_plt_entry_size(),
Add explicit this->.
> got_pov += 8,
> - plt_offset += plt_entry_size,
> + plt_offset += get_plt_entry_size(),
Add explicit this->.
> + protected:
> + virtual void
> + fill_first_plt_entry(unsigned char* pov,
> + typename elfcpp::Elf_types<size>::Elf_Addr got_address,
> + typename elfcpp::Elf_types<size>::Elf_Addr plt_address)
> + {
> + memcpy(pov, first_plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2,
> + (got_address + 8
> + - (plt_address + 2 + 4)));
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 9,
> + (got_address + 16
> + - (plt_address + 9 + 4)));
> + }
> +
> + virtual unsigned int
> + fill_plt_entry(unsigned char* pov,
> + typename elfcpp::Elf_types<size>::Elf_Addr got_address,
> + typename elfcpp::Elf_types<size>::Elf_Addr plt_address,
> + unsigned int got_offset,
> + unsigned int plt_offset,
> + unsigned int plt_index)
> + {
> + memcpy(pov, plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 3,
> + (got_address + got_offset
> + - (plt_address + plt_offset
> + + 3 + 4)));
> +
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 33, plt_index);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 38,
> + - (plt_offset + 38 + 4));
> +
> + return 32;
> + }
> +
> + virtual void
> + fill_tlsdesc_entry(unsigned char* pov,
> + typename elfcpp::Elf_types<size>::Elf_Addr got_address,
> + typename elfcpp::Elf_types<size>::Elf_Addr plt_address,
> + typename elfcpp::Elf_types<size>::Elf_Addr got_base,
> + unsigned int tlsdesc_got_offset,
> + unsigned int plt_offset)
> + {
> + memcpy(pov, tlsdesc_plt_entry, plt_entry_size);
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 2,
> + (got_address + 8
> + - (plt_address + plt_offset
> + + 2 + 4)));
> + elfcpp::Swap_unaligned<32, false>::writeval(pov + 9,
> + (got_base
> + + tlsdesc_got_offset
> + - (plt_address + plt_offset
> + + 9 + 4)));
> + }
Move these functions out of the class definition.
> +
> + private:
> + // The size of an entry in the PLT.
> + static const int plt_entry_size = 64;
> +
> + // The first entry in the PLT.
> + // From the AMD64 ABI: "Unlike Intel386 ABI, this ABI uses the same
> + // procedure linkage table for both programs and shared objects."
I'm not sure the comment from the AMD64 ABI is relevant here.
This patch is OK with those changes.
Thanks.
Ian