Re: Safe Identical Code Folding for X86-64.

[Sriraman Tallam <tmsriram at google dot com>]

Hi Ian,


    Thanks for the review. I have made all the changes. Please take
another look.


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.


Thanks,
-Sriraman.

On Thu, Feb 11, 2010 at 9:08 PM, Ian Lance Taylor <iant@google.com> wrote:
> Sriraman Tallam <tmsriram@google.com> writes:
>
>> 2010-01-21 ?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::is_non_pic_reloc_type_func_ptr): New function.
>> ? ? ? * gc.h (gc_process_relocs): Scan relocation types to determine if
>> ? ? ? ? function pointers were taken for targets that support it.
>
> No extra indentation here.
>
>> ? ? ? * 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.
>> ? ? ? (is_section_fptr_taken): New function.
>> ? ? ? (set_section_fptr_taken): New function.
>> ? ? ? (check_section_for_fptr_taken): New function.
>> ? ? ? * options.h: Fix comment for safe ICF option.
>> ? ? ? * target.h (is_fptr_taken_checked): 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.
>
>> Index: arm.cc
>> ===================================================================
>> RCS file: /cvs/src/src/gold/arm.cc,v
>> retrieving revision 1.59
>> diff -u -u -p -r1.59 arm.cc
>> --- arm.cc ? ?20 Jan 2010 17:29:52 -0000 ? ? ?1.59
>> +++ arm.cc ? ?22 Jan 2010 00:18:11 -0000
>> @@ -1873,6 +1873,24 @@ class Target_arm : public Sized_target<3
>> ? ? ? ? ?const elfcpp::Rel<32, big_endian>& reloc, unsigned int r_type,
>> ? ? ? ? ?Symbol* gsym);
>>
>> + ? ?inline void
>> + ? ?local_for_icf(Symbol_table* , Layout* , Target_arm* ,
>> + ? ? ? ? ? ? ? Sized_relobj<32, big_endian>* ,
>> + ? ? ? ? ? ? ? unsigned int ,
>> + ? ? ? ? ? ? ? Output_section* ,
>> + ? ? ? ? ? ? ? const elfcpp::Rel<32, big_endian>& , unsigned int ,
>> + ? ? ? ? ? ? ? const elfcpp::Sym<32, big_endian>& )
>> + ? ?{ }
>
> Please remove the extra spaces before commas and parenthesis.
>
>> +
>> + ? ?inline void
>> + ? ?global_for_icf(Symbol_table* , Layout* , Target_arm* ,
>> + ? ? ? ? ? ? ? ?Sized_relobj<32, big_endian>* ,
>> + ? ? ? ? ? ? ? ?unsigned int ,
>> + ? ? ? ? ? ? ? ?Output_section* ,
>> + ? ? ? ? ? ? ? ?const elfcpp::Rel<32, big_endian>& , unsigned int ,
>> + ? ? ? ? ? ? ? ?Symbol* )
>> + ? ?{ }
>
> Same here.
>
>
>> Index: gc.h
>> ===================================================================
>> RCS file: /cvs/src/src/gold/gc.h,v
>> retrieving revision 1.9
>> diff -u -u -p -r1.9 gc.h
>> --- gc.h ? ? ?20 Jan 2010 17:29:52 -0000 ? ? ?1.9
>> +++ gc.h ? ? ?22 Jan 2010 00:18:11 -0000
>> @@ -162,19 +162,20 @@ template<int size, bool big_endian, type
>> ?inline void
>> ?gc_process_relocs(
>> ? ? ?Symbol_table* symtab,
>> - ? ?Layout*,
>> - ? ?Target_type* ,
>> + ? ?Layout* ,
>> + ? ?Target_type* target,
>
> No extra space before comma.
>
>> ? ? ?Sized_relobj<size, big_endian>* src_obj,
>> ? ? ?unsigned int src_indx,
>> ? ? ?const unsigned char* prelocs,
>> ? ? ?size_t reloc_count,
>> - ? ?Output_section*,
>> + ? ?Output_section* ,
>
> Same here.
>
>
>> + ?if (parameters->options().icf_enabled()
>> + ? ? ?&& parameters->options().icf_safe_folding()
>> + ? ? ?&& target->is_fptr_taken_checked())
>> + ? ?{
>> + ? ? ?src_section_name = src_obj->section_name(src_indx);
>> + ? ?}
>
> Getting the name of a section is slow, and you just did it a few lines
> above. ?Don't do it twice.
>
>
>> + ? ? ? // When doing safe folding, check to see if this relocation is that
>> + ? ? ? // of a function pointer being taken.
>> + ? ? ? if (parameters->options().icf_enabled()
>> + ? ? ? ? ? ? ?&& symtab->icf()->check_section_for_fptr_taken(src_section_name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? target))
>> + ? ? ? ? ? ?{
>> + ? ? ? ? ? scan.local_for_icf(symtab, NULL, NULL, src_obj, src_indx,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL, reloc, r_type, lsym);
>> + ? ? ? ? ? ?}
>
> How about renaming check_section_for_fptr_taken to
> check_section_for_function_pointers, here and elsewhere.
>
>
>> +// Safe Folding :
>> +// ------------
>> +//
>> +// ICF in safe mode, folds only ctors and dtors as their function pointers can
>> +// never be taken. ?Also, for AMD X86-64, safe folding uses the relocation
>> +// type to determine if a function's pointer was 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/mode,/mode/
> s/AMD //
>
>
>> -static bool
>> +bool
>> ?is_function_ctor_or_dtor(const char* mangled_func_name)
>
> Keep this static, no reason to change it.
>
>> @@ -577,14 +591,18 @@ Icf::find_identical_sections(const Input
>> ? ? ? ? ? ?if (parameters->options().gc_sections()
>> ? ? ? ? ? ? ? ?&& symtab->gc()->is_section_garbage(*p, i))
>> ? ? ? ? ? ? ? ?continue;
>> + ? ? ? const char* mangled_func_name = strrchr(section_name, '.');
>> + ? ? ? gold_assert(mangled_func_name != NULL);
>> ? ? ? ? // With --icf=safe, check if the mangled function name is a ctor
>> ? ? ? ? // or a dtor. ?The mangled function name can be obtained from the
>> ? ? ? ? // section name by stripping the section prefix.
>> - ? ? ? const char* mangled_func_name = strrchr(section_name, '.');
>> - ? ? ? gold_assert(mangled_func_name != NULL);
>> ? ? ? ? if (parameters->options().icf_safe_folding()
>> - ? ? ? ? ? && !is_function_ctor_or_dtor(mangled_func_name + 1))
>> - ? ? ? ? continue;
>> + ? ? ? ? ? ? ?&& !is_function_ctor_or_dtor(mangled_func_name + 1)
>> + ? ? ? ? ? && (target.is_fptr_taken_checked() == false
>> + ? ? ? ? ? ? ? ? ?|| is_section_fptr_taken(*p, i)))
>> + ? ? ? ? ? ?{
>> + ? ? ? ? ? continue;
>> + ? ? ? ? ? ?}
>
> Don't write == false, write !target.is_fptr_taken_checked().
>
> Actually I don't like that name either. ?How about
> can_check_for_function_pointers. ?And how about changing
> is_section_fptr_taken to section_has_function_pointers.
>
>
>> ? ?DEFINE_enum(icf, options::TWO_DASHES, '\0', "none",
>> ? ? ? ? ? ? ? ?N_("Identical Code Folding. "
>> - ? ? ? ? ? ? ? ? "\'--icf=safe\' folds only ctors and dtors."),
>> + ? ? ? ? ? ? ? ? "\'--icf=safe\' folds only ctors and dtors. ?"
>> + ? ? ? ? ? ? ? ? "For X86-64, also folds functions whose pointers are "
>> + ? ? ? ? ? ? ? ? "definitely not taken. ?For X86-64, do not use safe "
>> + ? ? ? ? ? ? ? ? "ICF to build non-pie executables with pic objects."),
>> ? ? ? ? ? ? ("[none,all,safe]"),
>> ? ? ? ? ? ? ? ?{"none", "all", "safe"});
>
> Doesn't that make the --help output look kind of ridiculous? ?Not sure
> what to do here, but I don't think we should do that.
>
>> @@ -1364,6 +1393,94 @@ Target_x86_64::Scan::unsupported_reloc_g
>> ? ? ? ? ? ?object->name().c_str(), r_type, gsym->demangled_name().c_str());
>> ?}
>>
>> +// Returns true if this relocation type could be that of a function pointer
>> +// only if the target is not position-independent code.
>> +inline bool
>> +Target_x86_64::Scan::is_non_pic_reloc_type_func_ptr(unsigned int r_type)
>
> Let's call this possible_function_pointer_reloc. ?The non_pic part
> doesn't need to be in the name, I think.
>
>> + ?// 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())
>> + ? ?{
>> + ? ? ?symtab->icf()->set_section_fptr_taken(object,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? lsym.get_st_shndx());
>> + ? ? ?return;
>> + ? ?}
>> +
>> + ?if (is_non_pic_reloc_type_func_ptr(r_type))
>> + ? ?symtab->icf()->set_section_fptr_taken(object, lsym.get_st_shndx());
>> +}
>
> The then-block is the same here; combine the conditions with ||.
>
> Why don't you check the symbol type? ?If the type is STT_OBJECT, you
> know it is not a function.
>
>
>> +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)
>> +{
>> + ?if (parameters->options().shared())
>> + ? ?{
>> + ? ? ?// When building a shared library, do not fold symbols whose visibility
>> + ? ? ?// is hidden, internal or protected.
>> + ? ? ?if (gsym->visibility() == elfcpp::STV_INTERNAL
>> + ? ? ? || gsym->visibility() == elfcpp::STV_PROTECTED
>> + ? ? ? || gsym->visibility() == elfcpp::STV_HIDDEN)
>> + ? ? ? ?{
>> + ? ? ? bool is_ordinary;
>> + ? ? ? ? ?symtab->icf()->set_section_fptr_taken(gsym->object(),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gsym->shndx(&is_ordinary));
>> + ? ? ? ?}
>> + ? ? ?return;
>> + ? ?}
>> +
>> + ?if (is_non_pic_reloc_type_func_ptr(r_type))
>> + ? ?{
>> + ? ? ?bool is_ordinary;
>> + ? ? ?symtab->icf()->set_section_fptr_taken(gsym->object(),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gsym->shndx(&is_ordinary));
>> + ? ?}
>> +}
>
> These conditions can also be combined.
>
> Here too it seems like you should check the symbol type.
>
> You can't ignore is_ordinary here. ?If is_ordinary is false, you must
> not treat the the symbol index as usual. ?I believe that could happen
> here for a common symbol. ?It's probably simplest to punt if
> is_ordinary is false--to treat it as a taking a function pointer.
>
>> +// not be folded into kept_func_2 other than for AMD X86-64 which can
>> +// use relocation types to determine if function pointers are taken.
>> +// kept_func_3 should never be folded as its pointer is taken. The ctor
>> +// and dtor of class A must be folded.
>
> s/AMD // ?Intel makes them too, after all.
>
>
> Please fix and resend.
>
> Thanks.
>
> Ian
>

Attachment: gold_safe_icf_2010_02_12_patch.txt
Description: Text document