This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Safe Identical Code Folding for X86-64.


Hi,

    I made the final changes and submitted this patch.


2010-02-12  Sriraman Tallam  <tmsriram@google.com>

	* arm.cc (Scan::local_reloc_may_be_function_pointer): New function.
	(Scan::global_reloc_may_be_function_pointer): New function.
	* sparc.cc (Scan::local_reloc_may_be_function_pointer): New function.
	(Scan::global_reloc_may_be_function_pointer): New function.
	* powerpc.cc (Scan::local_reloc_may_be_function_pointer): New function.
	(Scan::global_reloc_may_be_function_pointer): New function.
	* i386.cc (Scan::local_reloc_may_be_function_pointer): New function.
	(Scan::global_reloc_may_be_function_pointer): New function.
	* x86_64.cc (Scan::local_reloc_may_be_function_pointer): New function.
	(Scan::global_reloc_may_be_function_pointer): 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.

Thanks,
-Sriraman.

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.
>
> This is OK with those changes.
>
> Thanks.
>
> Ian
>

Attachment: gold_safe_icf_2010_02_12_patch.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]