This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PING^2] [PATCH] PR gold/18609
- From: Andrew Senkevich <andrew dot n dot senkevich at gmail dot com>
- To: Binutils <binutils at sourceware dot org>
- Cc: Cary Coutant <ccoutant at gmail dot com>
- Date: Wed, 3 Feb 2016 18:46:23 +0300
- Subject: Re: [PING^2] [PATCH] PR gold/18609
- Authentication-results: sourceware.org; auth=none
- References: <CAMXFM3vBLkgF71DWiA42pxg4FLq8Te5JyOCr2Nwk8XmSEhmdVg at mail dot gmail dot com> <CAMXFM3sc1zZTiBGVeJvH5Qa6fgoYbV3RW7KecsxNC17RmTcS4w at mail dot gmail dot com>
2015-12-10 14:33 GMT+03:00 Andrew Senkevich <andrew.n.senkevich@gmail.com>:
> 2015-09-08 15:27 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
>
> PING.
> Questions here were not only about unifying overflow checking but also
> how properly implemented check what branch and target are in the same
> section.
PING.
--
WBR,
Andrew