This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH v4] gold: Add Mips64 support.
- From: Vladimir Radosavljevic <Vladimir dot Radosavljevic at imgtec dot com>
- To: Cary Coutant <ccoutant at gmail dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, Petar Jovanovic <Petar dot Jovanovic at imgtec dot com>
- Date: Wed, 16 Mar 2016 13:38:23 +0000
- Subject: RE: [PATCH v4] gold: Add Mips64 support.
- Authentication-results: sourceware.org; auth=none
- References: <3060420525346945A0ADBD567348A91723740827 at BADAG02 dot ba dot imgtec dot org>,<CAJimCsHMz-ZkiTRdJtf3=EF3KzZ0o5190nJpcuYfvZECVZ7JYw at mail dot gmail dot com>
Resending because previous email was blocked by SpamAssassin program.
Thank you a lot for reviews.
> + if (section_type == elfcpp::SHT_NOBITS)
> + return false;
> +
> + if (section_flags & elfcpp::SHF_WRITE)
> + return false;
>
> Just out of curiosity... Why would a NOBITS section ever not have
> SHF_WRITE set? Is there any reason to test for NOBITS?
I've added a check for NOBITS section only to follow LLD and GNU LD linkers. Do you want me to remove that?
> + // Whether this is a STT_SECTION symbol.
> + bool is_section_symbol_;
>
> I suggest moving is_section_symbol_ above shndx_ (or moving shndx_
> above tls_type_) to avoid an extra 8 bytes of padding.
Done.
> protected:
> + // The name of the options section.
> + const char* mips_elf_options_section_name()
> + { return this->is_newabi() ? ".MIPS.options" : ".options"; }
>
> This should probably be a private method.
Done.
> ++// MIPS-specific relocation writer.
> ++
> ++template<int sh_type, bool dynamic, int size, bool big_endian>
> ++struct Mips_output_reloc_writer;
> ++
> ++template<int sh_type, bool dynamic, bool big_endian>
> ++struct Mips_output_reloc_writer<sh_type, dynamic, 32, big_endian>
> ++{
> ++ typedef Output_reloc<sh_type, dynamic, 32, big_endian> Output_reloc_type;
> ++ typedef std::vector<Output_reloc_type> Relocs;
> ++
> ++ static void
> ++ write(typename Relocs::const_iterator p, unsigned char* pov)
> ++ { p->write(pov); }
> ++};
> ++
> ++template<int sh_type, bool dynamic, bool big_endian>
> ++struct Mips_output_reloc_writer<sh_type, dynamic, 64, big_endian>
> ++{
> ++ typedef Output_reloc<sh_type, dynamic, 64, big_endian> Output_reloc_type;
> ++ typedef std::vector<Output_reloc_type> Relocs;
> ++
> ++ static void
> ++ write(typename Relocs::const_iterator p, unsigned char* pov)
> ++ {
> ++ elfcpp::Mips64_rel_write<big_endian> orel(pov);
> ++ orel.put_r_offset(p->get_address());
> ++ orel.put_r_sym(p->get_symbol_index());
> ++ orel.put_r_ssym(0);
> ++ orel.put_r_type(p->type());
> ++ if (p->type() == elfcpp::R_MIPS_REL32)
> ++ orel.put_r_type2(elfcpp::R_MIPS_64);
> ++ else
> ++ orel.put_r_type2(elfcpp::R_MIPS_NONE);
> ++ orel.put_r_type3(elfcpp::R_MIPS_NONE);
> ++ }
> ++};
> ++
> ++template<int sh_type, bool dynamic, int size, bool big_endian>
> ++class Mips_output_data_reloc : public Output_data_reloc<sh_type, dynamic,
> ++ size, big_endian>
> ++{
> ++ public:
> ++ Mips_output_data_reloc(bool sort_relocs)
> ++ : Output_data_reloc<sh_type, dynamic, size, big_endian>(sort_relocs)
> ++ { }
> ++
> ++ protected:
> ++ // Write out the data.
> ++ void
> ++ do_write(Output_file* of)
> ++ {
> ++ typedef Mips_output_reloc_writer<sh_type, dynamic, size,
> big_endian> Writer;
> ++ this->template do_write_generic<Writer>(of);
> ++ }
> ++};
>
> I've committed that change along with your patch to output.h that
> moves Output_reloc::get_address() and Output_reloc::get_symbol_index()
> to the public interface (see separate email).
Thank you for doing this.
> put_r_info(Reltype_write* new_reloc, Reltype* reloc, unsigned int r_sym)
> {
> unsigned int r_type = elfcpp::elf_r_type<32>(reloc->get_r_info());
> - new_reloc->put_r_info(elfcpp::elf_r_info<64>(r_sym, r_type));
> + new_reloc->put_r_info(elfcpp::elf_r_info<32>(r_sym, r_type));
>
> Is this a bug fix to the 32-bit support? If so, it should probably go
> in separately. (That patch is preapproved.)
Done.
> + ei_class_ = size == 64 ? elfcpp::ELFCLASS64 : elfcpp::ELFCLASS32;
>
> I don't see the point of this data member -- it will always track the
> "size" template parameter for the target. Since Target_mips<32,...>
> and Target_mips<64,...> are distinct targets, you will never be called
> upon to merge them, and merge_processor_specific_flags() will never
> see an inconsistency (since a target mismatch will have already been
> detected in make_elf_sized_object).
>
I've made a patch that removes ei_class from mips.cc.
> rel16(unsigned char* view, const Mips_relobj<size, big_endian>* object,
> const Symbol_value<size>* psymval, Mips_address addend_a,
> + bool extract_addend, unsigned int r_type, bool calculate_only,
> + Valtype &calculated_value)
>
> In gold, we try to adhere to a convention where reference parameters
> are always const. If you're going to modify a reference parameter, it
> should be a pointer. (Also, space goes *after* the '&'.)
Done.
> + if (calculate_only)
> + calculated_value = x;
> + else
> + elfcpp::Swap<16, big_endian>::writeval(wv, val);
> +
> mips_reloc_shuffle(view, r_type, false);
>
> Do you really want to call mips_reloc_shuffle() if calculate_only is
> true? If so, a comment would help, but it seems to me that since you
> haven't actually written anything to the view yet, it's not really
> doing anything useful.
I've moved mips_reloc_unshuffle/mips_reloc_shuffle call before/after calculating and resolving relocations.
> - return (Bits<16>::has_overflow32(x)
> - ? This::STATUS_OVERFLOW
> - : This::STATUS_OKAY);
> + return check_overflow<16>(x);
>
> I observe that the returned status is never used if calculate_only is
> true (which makes sense). Perhaps you just want to return immediately
> after calculating the intermediate result rather than waste time
> checking for overflow.
Done.
> - 0x8df90000, // l[wd] $25, %lo(.got.plt entry)($15)
> + 0x01f90000, // l[wd] $25, %lo(.got.plt entry)($15)
>
> As above, if this is a bug fix for 32-bit, it should go in separately
> (also preapproved).
This is not a bug fix. I just removed opcode, because instruction will be selected later (in Mips_output_data_plt::do_write).
> I see the following pattern in several places:
>
> + if (sh_type == elfcpp::SHT_REL)
> + {
> + ...
> + }
> + else if (sh_type == elfcpp::SHT_RELA)
> + {
> + ...
> + }
>
> I suggest adding:
>
> ++ else
> ++ gold_unreachable();
Done.
> + // The first operation in a record which references a symbol uses the symbol
> + // implied by r_sym. The next operation in a record which references a symbol
> + // uses the special symbol value given by the r_ssym field. A third operation
> + // in a record which references a symbol will assume a NULL symbol,
> + // i.e. value zero.
>
> This comment (which agrees with what I read in the psABI) does not
> describe what the code below it does. It looks to me like the first
> relocation (r_types[0]) always uses r_sym, the second (r_types[1])
> always uses r_ssym, and the third (r_types[2]) always uses NULL.
> Perhaps a TODO comment is needed here?
Added TODO.
Regards,
Vladimir.