Safe Identical Code Folding for X86-64.

[Sriraman Tallam]

On Fri, Feb 12, 2010 at 4:18 PM, Ian Lance Taylor <iant@google.com> wrote:
> Sriraman Tallam <tmsriram@google.com> writes:
>
>> 2010-02-12  Sriraman Tallam  <tmsriram@google.com>
>>
>>       * arm.cc (Scan::local_for_icf): New function.
>>       (Scan::global_for_icf): New function.
>>       * sparc.cc (Scan::local_for_icf): New function.
>>       (Scan::global_for_icf): New function.
>>       * powerpc.cc (Scan::local_for_icf): New function.
>>       (Scan::global_for_icf): New function.
>>       * i386.cc (Scan::local_for_icf): New function.
>>       (Scan::global_for_icf): New function.
>>       * x86_64.cc (Scan::local_for_icf): New function.
>>       (Scan::global_for_icf): New function.
>>       (Scan::possible_function_pointer_reloc): New function.
>>       (Target_x86_64::can_check_for_function_pointers): New function.
>>       * gc.h (gc_process_relocs): Scan relocation types to determine if
>>       function pointers were taken for targets that support it.
>>       * icf.cc (Icf::find_identical_sections): Include functions for
>>       folding in safe ICF whose pointer is not taken.
>>       * icf.h (Secn_fptr_taken_set): New typedef.
>>       (fptr_section_id_): New member.
>>       (section_has_function_pointers): New function.
>>       (set_section_has_function_pointers): New function.
>>       (check_section_for_function_pointers): New function.
>>       * options.h: Fix comment for safe ICF option.
>>       * target.h (can_check_for_function_pointers): New function.
>>       * testsuite/Makefile.am: Add icf_safe_so_test test case.
>>       Modify icf_safe_test for X86-64.
>>       * testsuite/Makefile.in: Regenerate.
>>       * testsuite/icf_safe_so_test.cc: New file.
>>       * testsuite/icf_safe_so_test.sh: New file.
>>       * testsuite/icf_safe_test.cc (kept_func_3): New function.
>>       (main): Change to take pointer to function kept_func_3.
>>       * testsuite/icf_safe_test.sh (arch_specific_safe_fold): Check if safe
>>       folding is done correctly for X86-64.
>
>
>> +// Safe Folding :
>> +// ------------
>> +//
>> +// ICF in safe mode folds only ctors and dtors as their function pointers can
>> +// never be taken.  Also, for X86-64, safe folding uses the relocation
>> +// type to determine if a function's pointer is taken or not and only folds
>> +// functions whose pointers are definitely not taken.
>> +//
>> +// For X86-64, it is not correct to use safe folding to build non-pie
>> +// executables using PIC/PIE objects.  This can cause the resulting binary
>> +// to have unexpected run-time behaviour in the presence of pointer
>> +// comparisons.
>
> s/as their/if their/
>
> Can you add another paragraph here or somewhere describing in detail
> the problem with PIC/PIE objects?
>
>
>> +// For safe ICF, scan a relocation for a local symbol to check if it
>> +// corresponds to a function pointer being taken.  In that case mark
>> +// the function whose pointer was taken as not foldable.
>> +
>> +inline void
>> +Target_x86_64::Scan::local_for_icf(Symbol_table* symtab,
>> +                                Layout* ,
>> +                                Target_x86_64* ,
>> +                                Sized_relobj<64, false>* object,
>> +                                unsigned int ,
>> +                                Output_section* ,
>> +                                const elfcpp::Rela<64, false>& ,
>> +                                unsigned int r_type,
>> +                                const elfcpp::Sym<64, false>& lsym)
>> +{
>> +  // When building a shared library, do not fold any local symbols as it is
>> +  // not possible to distinguish pointer taken versus a call by looking at
>> +  // the relocation types.
>> +  if (parameters->options().shared()
>> +      || possible_function_pointer_reloc(r_type))
>> +    symtab->icf()->set_section_has_function_pointers(object,
>> +                                                  lsym.get_st_shndx());
>> +
>> +}
>> +
>> +// For safe ICF, scan a relocation for a global symbol to check if it
>> +// corresponds to a function pointer being taken.  In that case mark
>> +// the function whose pointer was taken as not foldable.
>> +
>> +inline void
>> +Target_x86_64::Scan::global_for_icf(Symbol_table* symtab,
>> +                                 Layout* ,
>> +                                 Target_x86_64* ,
>> +                                 Sized_relobj<64, false>* ,
>> +                                 unsigned int ,
>> +                                 Output_section* ,
>> +                                 const elfcpp::Rela<64, false>& ,
>> +                                 unsigned int r_type,
>> +                                 Symbol* gsym)
>> +{
>> +  bool is_ordinary;
>> +  unsigned int shndx = gsym->shndx(&is_ordinary);
>> +
>> +  // When building a shared library, do not fold symbols whose visibility
>> +  // is hidden, internal or protected.
>> +  if ((parameters->options().shared()
>> +       && (gsym->visibility() == elfcpp::STV_INTERNAL
>> +        || gsym->visibility() == elfcpp::STV_PROTECTED
>> +        || gsym->visibility() == elfcpp::STV_HIDDEN))
>> +      || !is_ordinary
>> +      || possible_function_pointer_reloc(r_type))
>> +    symtab->icf()->set_section_has_function_pointers(gsym->object(), shndx);
>> +}
>
> Feel free to disagree, but I think this code would be easier to write
> if these functions returned a boolean.  Then you could call the
> functions something like global_reloc_may_be_function_pointer.  And
> you can move the set_section_has_function_pointers into
> gc_process_relocs, rather than having to duplicate it.

I agree. Also, extending it to other architectures will not add more
call-sites to set_section_has_function_pointers. I will make that
change.


>
> This is OK with those changes.
>
> Thanks.
>
> Ian
>