Safe ICF for i386.

[Ian Lance Taylor]

Sriraman Tallam <tmsriram@google.com> writes:

> 	* i386.cc (Target_i386::can_check_for_function_pointers): New function.
> 	(Scan::possible_function_pointer_reloc): New function.
> 	(Scan::local_reloc_may_be_function_pointer): Change to call
> 	possible_function_pointer_reloc.
> 	(Scan::global_reloc_may_be_function_pointer): Ditto.
> 	* icf.h (Icf::check_section_for_function_pointers): Change to reject
> 	relocations in ".data.rel.ro._ZTV" section.
> 	* testsuite/icf_safe_so_test.sh: Change to pass i386.
> 	* testsuite/icf_safe_so_test.cc: Ditto.
> 	* testsuite/icf_safe_test.cc: Ditto.
> 	* testsuite/icf_safe_test.sh: Ditto.

> +inline bool
> +Target_i386::Scan::possible_function_pointer_reloc(unsigned int r_type)
> +{
> +  switch (r_type)
> +    {
> +    case elfcpp::R_386_32:
> +    case elfcpp::R_386_16:
> +    case elfcpp::R_386_8:
> +    case elfcpp::R_386_GOTOFF:
> +    case elfcpp::R_386_GOT32:
> +      {
> +        return true;
> +      }
> +    }
> +  return false;
> +}

Write this using a default case rather than falling through the
switch.  Otherwise you may get a warning depending on the version of
g++ you are using.

I don't really see how R_386_GOTOFF could be used to get a function
pointer.  If you're sure that it can be, then it's OK.


> +inline bool
> +Target_i386::Scan::local_reloc_may_be_function_pointer(
> +  Symbol_table* ,
> +  Layout* ,
> +  Target_i386* ,
> +  Sized_relobj<32, false>* ,
> +  unsigned int ,
> +  Output_section* ,
> +  const elfcpp::Rel<32, false>& ,
> +  unsigned int r_type,
> +  const elfcpp::Sym<32, false>&)
> +{
> +  return (possible_function_pointer_reloc(r_type));
> +}

Please remove unnecessary outer parentheses.

> +inline bool
> +Target_i386::Scan::global_reloc_may_be_function_pointer(
> +  Symbol_table* ,
> +  Layout* ,
> +  Target_i386* ,
> +  Sized_relobj<32, false>* ,
> +  unsigned int ,
> +  Output_section* ,
> +  const elfcpp::Rel<32, false>& ,
> +  unsigned int r_type,
> +  Symbol*)
> +{
> +  return (possible_function_pointer_reloc(r_type));
> +}

Please remove unnecessary outer parentheses.


> @@ -127,6 +127,7 @@ class Icf
>      return (parameters->options().icf_safe_folding()
>  	    && target->can_check_for_function_pointers()
>              && !is_prefix_of(".rodata._ZTV", section_name.c_str())
> +	    && !is_prefix_of(".data.rel.ro._ZTV", section_name.c_str())
>              && !is_prefix_of(".eh_frame", section_name.c_str()));
>    }

The indentation looks a little weird here but it may be a mail
artifact, please check that it looks OK in the source.

> +X86_32_specific_safe_fold()
> +{
> +    grep_x86_32=`grep -q -e "Intel 80386" $1`
> +    arch_specific_safe_fold $? $2 $3 $4
> +}
>  
> +X86_64_specific_safe_fold()
> +{
> +    grep_x86_64=`grep -q -e "Advanced Micro Devices X86-64" $1`
> +    arch_specific_safe_fold $? $2 $3 $4
> +}

I had to kind of puzzle over those grep statements.  Please instead
write
    grep -e ... $1 >/dev/null 2>&1
rather than assigning to a variable.


This is OK with those changes.

Thanks.

Ian