Re: Safe Identical Code Folding for X86-64.

[Ian Lance Taylor <iant at google dot com>]

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