RE: [PATCH v4] gold: Add Mips64 support.

[Vladimir Radosavljevic <Vladimir dot Radosavljevic at imgtec 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.