Discussion:
Safe Identical Code Folding for X86-64.
(too old to reply)
Sriraman Tallam
2010-01-22 00:51:28 UTC
Permalink
Hi,

I am implementing a safe ICF option for gold that looks at
relocation types and tries to figure out if it is a function pointer
taken versus a function call. Please recall that ICF is not safe when
function pointer checks are present in the code. So, with safe ICF, I
try to fold only functions that are guaranteed to not have their
function pointer taken other than for vtable purposes. Currently, safe
ICF only folds ctors and dtors as their pointers can never be taken.

I have tried to do this for AMD X86-64. Here are the observations I
have used to write the patch. Most of this stuff should be familiar to
you all. I have also attached the patch.

Case (i) : For position dependent code (non-PIC), there is no
problem. A function call is always a PC relative relocation and a
function pointer is a direct relocation.

Here is an example :

test.cc :

int foo()
{
return 1;
}

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

$ g++ -c test.cc
$ readelf --relocs test.o

Relocation section '.rela.text._Z3barv' at offset 0x660 contains 2 entries:
Offset Info Type Sym. Value Sym. Name + Addend
00000000000c 000a0000000b R_X86_64_32S 0000000000000000 _Z3foov + 0
000000000011 000a00000002 R_X86_64_PC32 0000000000000000 _Z3foov
+ fffffffffffffffc


R_X86_64_32S is the relocation type of the pointer and R_X86_64_PC32
is the relocation type of the call. Hence, I can look at the
relocation type and exclude any function from being folded if there is
a direct relocation against it.

Case (ii) : Shared Libraries.

For shared libraries, in x86_64, they *have to be built in -fPIC mode.
Further, for shared libraries, any function whose symbol is exported
will get a PLT entry. Hence, the function pointers for these functions
will be the PLT entry's address and not the real function address.
Hence, it is always safe to fold functions whose symbols are exported
as their pointers *can never be equal*. I do not need any relocation
based disambiguation here.

Here is an example :

t.cc

int foo()
{
return 1;
}

int bar()
{
return 1;
}

$ g++ -fPIC -shared t.cc -o libt.so -Wl,--icf
$ nm t.so

000000000000025c T _Z3barv
000000000000025c T _Z3foov

Notice that foo and bar have been folded into one function.

Now,

main.cc

extern int foo();
extern int bar();

int main()
{
if (foo == bar)
printf("equal\n");
else
printf("not equal\n");
}

$ g++ main.cc -L./ -lt
$ a.out
not equal

However, for shared libraries; protected, hidden, internal symbols and
static functions cause a problem. They dont have a PLT entry and their
relocations for call and pointers are the same. So, safe ICF will
assume that these can never be folded. Fine so far.

Case (iii) Position-independent executables.

This is a monster. AFAIU, PIE is nothing but PIC code that is
executable. However, the function pointer for a function whose symbol
is exported is the address of the function, not the PLT entry. There
is no PLT entry for exported functions because they cannot be
over-ridden.

Further, It can be created in two ways as far as I know.

Sub-case 1) The individual objects can be compiled with -fPIC and then
linked into an executable.

In this case, the relocations for pointer taken versus a function call
are *still* different. The function pointer has a GOTPCREL relocation
whereas a function call has a PC relative relocation. Hence, we can
differentiate and do a safe folding, again accounting for those
special symbols (hidden, protected, internal and static functions.)


Sub-case 2) The individual objects can be compiled with -fPIE and then
linked into an executable.

In this case, the relocations for function pointer taken versus a call
are the same. From what I understood, the difference between -fPIC
objects and -fPIE objects is that objects compiled with -fPIE can
never go into a shared object hence the function pointer relocations
never have to be GOT relative.

Hence, for PIE executables, safe ICF defaults to folding only ctors and dtors.

Case (iv) Building non-PIE binaries using PIE objects.

Safe ICF should not be used in this case. I have a plan to record the
build type of each object in a separate section that can be used by
the linker to decide what kind of safety is possible. Till then,
building non-PIE binaries with PIC/PIE objects is wrong.


Here is the patch to implement really safe ICF for X86-64. Some
experiments I have conducted show that safe ICF for X86-64 is 98 % as
effective as full ICF in reducing code size with the safety guarantee
against function pointer checks.

Please let me know what you think ?


2010-01-21 Sriraman Tallam <***@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.
* 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.

Thanks,
-Sriraman.
John Reiser
2010-01-22 01:16:29 UTC
Permalink
I am implementing a safe ICF option for gold ... for AMD X86-64. ...
Case (i) : For position dependent code (non-PIC), there is no
problem. A function call is always a PC relative relocation and a
function pointer is a direct relocation.
That depends on the compiler. I have a compiler that uses no relocation
at all for a CALL if the target is visible in the same compilation unit.
The displacement is computed at compile time, and used as a constant.
Also, in some cases a function pointer can be created by %rip-relative
LEA using a constant displacement with no relocation at all.

Is the proposed ICF for gcc only? What are the assumptions
about the properties of compiler-generated code?

--
Sriraman Tallam
2010-01-22 01:25:53 UTC
Permalink
     I am implementing a safe ICF option for gold ... for AMD X86-64. ...
Case (i) : For position dependent code (non-PIC),  there is no
problem. A function call is always a PC relative relocation and a
function pointer is a direct relocation.
That depends on the compiler.  I have a compiler that uses no relocation
at all for a CALL if the target is visible in the same compilation unit.
The displacement is computed at compile time, and used as a constant.
Also, in some cases a function pointer can be created by %rip-relative
LEA using a constant displacement with no relocation at all.
Is it true even if you use -ffunction-sections ?
Is the proposed ICF for gcc only?  What are the assumptions
about the properties of compiler-generated code?
--
Sriraman Tallam
2010-01-22 01:42:55 UTC
Permalink
     I am implementing a safe ICF option for gold ... for AMD X86-64. ...
Case (i) : For position dependent code (non-PIC),  there is no
problem. A function call is always a PC relative relocation and a
function pointer is a direct relocation.
That depends on the compiler.  I have a compiler that uses no relocation
at all for a CALL if the target is visible in the same compilation unit.
The displacement is computed at compile time, and used as a constant.
Also, in some cases a function pointer can be created by %rip-relative
LEA using a constant displacement with no relocation at all.
If bar calls foo and does not use a relocation for foo and uses a
displacement instead, it is somehow guaranteed that foo and bar will
be linked with that displacement. That, in effect, it rules out any
kind of folding, not just safe, right ?
Is the proposed ICF for gcc only?  What are the assumptions
about the properties of compiler-generated code?
--
John Reiser
2010-01-22 05:51:27 UTC
Permalink
Post by Sriraman Tallam
If bar calls foo and does not use a relocation for foo and uses a
displacement instead, it is somehow guaranteed that foo and bar will
be linked with that displacement.
Correct. The compiler translates directly from source (.c or .cc, etc.)
to object (.o), without going through any text-based assembly language.
The compiler says, "If I can see a suitable target for a CALL, then I
will address the target directly via pc-relative addressing with a
numerical constant displacement and no relocation record."
Post by Sriraman Tallam
That, in effect, rules out any kind of folding, not just safe, right ?
Folding still is possible. However it requires an analysis that understands
the meaning of compiled instructions, and not just 'join' operations over
tables of Elf64_Rela, Elf64_Sym, etc. Inter-linking of code compiled
by different compilers can be an adventure, too. The specifications
such as an ABI (Application Binary Interface) often do not mention
all of the properties that a simple implementation of code folding
might want to assume.

--
Ian Lance Taylor
2010-01-22 15:22:55 UTC
Permalink
Post by John Reiser
Post by Sriraman Tallam
If bar calls foo and does not use a relocation for foo and uses a
displacement instead, it is somehow guaranteed that foo and bar will
be linked with that displacement.
Correct. The compiler translates directly from source (.c or .cc, etc.)
to object (.o), without going through any text-based assembly language.
The compiler says, "If I can see a suitable target for a CALL, then I
will address the target directly via pc-relative addressing with a
numerical constant displacement and no relocation record."
Post by Sriraman Tallam
That, in effect, rules out any kind of folding, not just safe, right ?
Folding still is possible. However it requires an analysis that understands
the meaning of compiled instructions, and not just 'join' operations over
tables of Elf64_Rela, Elf64_Sym, etc. Inter-linking of code compiled
by different compilers can be an adventure, too. The specifications
such as an ABI (Application Binary Interface) often do not mention
all of the properties that a simple implementation of code folding
might want to assume.
That is all true but irrelevant to the way that gold implements code
folding, which is to fold separate sections. The compiler can not use
direct PC-relative addressing between functions in different sections.
The kind of folding you describe would be far less efficient at link
time.

Ian
Eric Botcazou
2010-01-22 09:13:01 UTC
Permalink
Post by Sriraman Tallam
int foo()
{
return 1;
}
int bar()
{
int (*p)() = foo;
p();
}
$ g++ -c test.cc
$ readelf --relocs test.o
Offset Info Type Sym. Value Sym. Name +
Addend 00000000000c 000a0000000b R_X86_64_32S 0000000000000000
_Z3foov + 0 000000000011 000a00000002 R_X86_64_PC32 0000000000000000
_Z3foov + fffffffffffffffc
You probably meant:

int foo()
{
return 1;
}

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

$ g++ -c test.cc -ffunction-sections
--
Eric Botcazou
Sriraman Tallam
2010-01-22 18:44:20 UTC
Permalink
Post by Sriraman Tallam
Post by Sriraman Tallam
int foo()
{
 return 1;
}
int bar()
{
 int (*p)() = foo;
 p();
}
$ g++ -c test.cc
$ readelf --relocs test.o
 Offset          Info           Type           Sym. Value    Sym. Name +
Addend 00000000000c  000a0000000b R_X86_64_32S      0000000000000000
_Z3foov + 0 000000000011  000a00000002 R_X86_64_PC32     0000000000000000
_Z3foov + fffffffffffffffc
int foo()
{
 return 1;
}
int bar()
{
 int (*p)() = foo;
 foo ();
}
$ g++ -c test.cc -ffunction-sections
Yes !, thats what I meant. Should have been a copy-paste error on my
part. Thanks for pointing it out.
Post by Sriraman Tallam
--
Eric Botcazou
Ian Lance Taylor
2010-02-12 05:08:05 UTC
Permalink
Post by Sriraman Tallam
* 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.
Post by Sriraman Tallam
* 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.
Post by Sriraman Tallam
+
+ 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.
Post by Sriraman Tallam
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.
Post by Sriraman Tallam
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.
Post by Sriraman Tallam
+ 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.
Post by Sriraman Tallam
+ // 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.
Post by Sriraman Tallam
+// ------------
+//
+// 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 //
Post by Sriraman Tallam
-static bool
+bool
is_function_ctor_or_dtor(const char* mangled_func_name)
Keep this static, no reason to change it.
Post by Sriraman Tallam
@@ -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.
Post by Sriraman Tallam
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.
Post by Sriraman Tallam
@@ -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.
Post by Sriraman Tallam
+ // 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.
Post by Sriraman Tallam
+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.
Post by Sriraman Tallam
+// 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
Sriraman Tallam
2010-02-12 23:46:33 UTC
Permalink
Hi Ian,


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


2010-02-12 Sriraman Tallam <***@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.
Post by Ian Lance Taylor
      * 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.
+// ------------
+//
+// 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
Ian Lance Taylor
2010-02-13 00:18:31 UTC
Permalink
Post by Sriraman Tallam
* 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.
+// ------------
+//
+// 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?
Post by Sriraman Tallam
+// 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
Sriraman Tallam
2010-02-13 00:27:00 UTC
Permalink
Post by Ian Lance Taylor
      * 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.
+// ------------
+//
+// 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.
I agree. Also, extending it to other architectures will not add more
call-sites to set_section_has_function_pointers. I will make that
change.
Post by Ian Lance Taylor
This is OK with those changes.
Thanks.
Ian
Sriraman Tallam
2010-02-13 02:05:17 UTC
Permalink
Hi,

I made the final changes and submitted this patch.


2010-02-12 Sriraman Tallam <***@google.com>

* arm.cc (Scan::local_reloc_may_be_function_pointer): New function.
(Scan::global_reloc_may_be_function_pointer): New function.
* sparc.cc (Scan::local_reloc_may_be_function_pointer): New function.
(Scan::global_reloc_may_be_function_pointer): New function.
* powerpc.cc (Scan::local_reloc_may_be_function_pointer): New function.
(Scan::global_reloc_may_be_function_pointer): New function.
* i386.cc (Scan::local_reloc_may_be_function_pointer): New function.
(Scan::global_reloc_may_be_function_pointer): New function.
* x86_64.cc (Scan::local_reloc_may_be_function_pointer): New function.
(Scan::global_reloc_may_be_function_pointer): 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.
Post by Ian Lance Taylor
      * 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.
+// ------------
+//
+// 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
Continue reading on narkive:
Loading...