Re: Safe ICF for i386.

[Sriraman Tallam <tmsriram at google dot com>]

Hi Ian,

   I checked this in with the changes you mentioned. I have attached the patch.

JFYI, Here is a simple example to show when R_386_GOTOFF is used as
reloc type for a function pointer. It is obtained when -fPIE is used.

foo.cc
------

#include <stdio.h>

int foo()
{
}

int main()
{
  int (*p)() = foo;
}

$ g++ -m32 -O0 -ffunction-sections -fPIE -c foo.cc
$ readelf -r foo.o
Relocation section '.rel.text.main' at offset 0x694 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name
00000007  00001102 R_386_PC32        00000000   __x86.get_pc_thunk.cx
0000000d  0000120a R_386_GOTPC       00000000   _GLOBAL_OFFSET_TABLE_
00000013  00000e09 R_386_GOTOFF      00000000   _Z3foov


Thanks,
-Sriraman.



On Wed, Mar 3, 2010 at 5:15 PM, Ian Lance Taylor <iant@google.com> wrote:
> 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
>

Attachment: safe_icf_i386_patch.txt
Description: Text document