Safe Identical Code Folding for X86-64.
Ian Lance Taylor
iant@google.com
Fri Feb 12 05:08:00 GMT 2010
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
More information about the Binutils
mailing list