Re: [PATCH] PR gold/18609

[Andrew Senkevich <andrew dot n dot senkevich at gmail dot com>]

2015-08-27 15:12 GMT+03:00 Andrew Senkevich <andrew.n.senkevich@gmail.com>:
>
> 2015-07-22 4:06 GMT+03:00 Cary Coutant <ccoutant@gmail.com>:
> >>>>>> If you remove those changes, won't it generate an unused GOT slot
> >>>>>> when GOTPCREL relocation is converted to PC-relative relocation?
> >>>>>
> >>>>> Yes, it can generate unused GOT slots.
> >>>>
> >>>> I think we should add a testcase to check for the unused GOT slot.
> >>>> Please check if you can implement similar heuristic in gold:
> >>>>
> >>>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=59cab532835904f368b0aa99267afba5fda5ded2
> >>>
> >>> No addresses available at the time of Target_x86_64<size>::Scan::local
> >>> and *::global work, so not clear how to use some heuristics here...
> >>
> >> There are no addresses available in ld.bfd neither.  An estimate was
> >> used in ld.bfd.
> >
> > In gold, I think the best you can do is check to see if the branch and
> > its target are in the same output section, then use
> > output_section_offset() plus offset within the input section to check
> > distance between the two. At the time you're scanning relocations, we
> > haven't even done a tentative layout of output sections, so you'll
> > have to be pessimistic if the two aren't in the same output section.
>
> Do you mean uint64_t Object::output_section_offset(unsigned int shndx)?
> Why we should use it if checked what branch and target are in the same
> section so distance depends on offsets in that section only?
> I propose the following patch, comments are welcome.
>
> 2015-08-27  Andrew Senkevich  <andrew.senkevich@intel.com>
>
> gold/
>         PR gold/18609
>         * x86_64.cc (Target_x86_64::Relocate::relocate): Added overflow check
>         for convert mov foo@GOTPCREL(%rip), %reg to lea foo(%rip), %reg.
>         (Target_x86_64::Scan::global, Target_x86_64::Scan::local): Added check
>         to avoid unused PLT slots.
>
> diff --git a/gold/x86_64.cc b/gold/x86_64.cc
> index 007af1d..db17443 100644
> --- a/gold/x86_64.cc
> +++ b/gold/x86_64.cc
> @@ -2344,6 +2344,32 @@ Target_x86_64<size>::Scan::reloc_needs_plt_for_ifunc(
>    return flags != 0;
>  }
>
> +template<int size, int valsize>
> +class x86_64_overflow_check
> +{
> +public:
> +  typedef typename elfcpp::Elf_types<size>::Elf_Addr Address;
> +
> +  static inline bool
> +  has_overflow_signed(Address value)
> +  {
> +    // limit = 1 << (valsize - 1) without shift count exceeding size of type
> +    Address limit = static_cast<Address>(1) << ((valsize - 1) >> 1);
> +    limit <<= ((valsize - 1) >> 1);
> +    limit <<= ((valsize - 1) - 2 * ((valsize - 1) >> 1));
> +    return value + limit > (limit << 1) - 1;
> +  }
> +
> +  static inline bool
> +  has_overflow_unsigned(Address value)
> +  {
> +    Address limit = static_cast<Address>(1) << ((valsize - 1) >> 1);
> +    limit <<= ((valsize - 1) >> 1);
> +    limit <<= ((valsize - 1) - 2 * ((valsize - 1) >> 1));
> +    return value > (limit << 1) - 1;
> +  }
> +};
> +
>  // Scan a relocation for a local symbol.
>
>  template<int size>
> @@ -2480,14 +2506,21 @@ Target_x86_64<size>::Scan::local(Symbol_table* symtab,
>         // The symbol requires a GOT section.
>         Output_data_got<64, false>* got = target->got_section(symtab, layout);
>
> -       // If the relocation symbol isn't IFUNC,
> -       // and is local, then we will convert
> +       typename elfcpp::Elf_types<size>::Elf_Addr value
> +           = reloc.get_r_offset() + reloc.get_r_addend() - lsym.get_st_value();
> +
> +       // If the relocation symbol isn't IFUNC, and is local,
> +       // and branch and its target are in the same output section,
> +       // and distance isn't overflow, then we will convert
>         // mov foo@GOTPCREL(%rip), %reg
>         // to lea foo(%rip), %reg.
>         // in Relocate::relocate.
>         if (r_type == elfcpp::R_X86_64_GOTPCREL
>             && reloc.get_r_offset() >= 2
> -           && !is_ifunc)
> +           && !is_ifunc
> +           && strcmp(object->section_name(lsym.get_st_shndx()).c_str(),
> +                     ".text") == 0
> +           && !x86_64_overflow_check<size, 32>::has_overflow_signed(value))
>           {
>             section_size_type stype;
>             const unsigned char* view = object->section_contents(data_shndx,
> @@ -2496,7 +2529,6 @@ Target_x86_64<size>::Scan::local(Symbol_table* symtab,
>               break;
>           }
>
> -
>         // The symbol requires a GOT entry.
>         unsigned int r_sym = elfcpp::elf_r_sym<size>(reloc.get_r_info());
>
> @@ -2905,14 +2937,21 @@ Target_x86_64<size>::Scan::global(Symbol_table* symtab,
>        {
>         // The symbol requires a GOT entry.
>         Output_data_got<64, false>* got = target->got_section(symtab, layout);
> +       Sized_symbol<size>* ssym = static_cast<Sized_symbol<size>*>(gsym);
> +       typename elfcpp::Elf_types<size>::Elf_Addr value
> +           = reloc.get_r_offset() + reloc.get_r_addend() - ssym->value();
>
>         // If we convert this from
>         // mov foo@GOTPCREL(%rip), %reg
>         // to lea foo(%rip), %reg.
> -       // in Relocate::relocate, then there is nothing to do here.
> +       // in Relocate::relocate and
> +       // branch and its target are in the same output section and
> +       // distance isn't overflow, then there is nothing to do here.
>         if (r_type == elfcpp::R_X86_64_GOTPCREL
>             && reloc.get_r_offset() >= 2
> -           && Target_x86_64<size>::can_convert_mov_to_lea(gsym))
> +           && Target_x86_64<size>::can_convert_mov_to_lea(gsym)
> +           && gsym->output_section() == output_section
> +           && !x86_64_overflow_check<size, 32>::has_overflow_signed(value))
>           {
>             section_size_type stype;
>             const unsigned char* view = object->section_contents(data_shndx,
> @@ -3543,7 +3582,10 @@ Target_x86_64<size>::Relocate::relocate(
>        // mov foo@GOTPCREL(%rip), %reg
>        // to lea foo(%rip), %reg.
>        // if possible.
> -      if (rela.get_r_offset() >= 2
> +      typename elfcpp::Elf_types<size>::Elf_Addr value;
> +      value = psymval->value(object, addend) - address;
> +      if (!x86_64_overflow_check<size, 32>::has_overflow_signed(value)
> +         && rela.get_r_offset() >= 2
>           && view[-2] == 0x8b
>           && ((gsym == NULL && !psymval->is_ifunc_symbol())
>               || (gsym != NULL
>
>
> --
> WBR,
> Andrew

Ping.


--
WBR,
Andrew