This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [GOLD] Support --icf=safe with -pie for x86_64
- From: "Rahul Chaudhry via binutils" <binutils at sourceware dot org>
- To: Cary Coutant <ccoutant at gmail dot com>
- Cc: Alan Modra <amodra at gmail dot com>, Binutils <binutils at sourceware dot org>, Sriraman Tallam <tmsriram at google dot com>
- Date: Fri, 3 Feb 2017 12:17:45 -0800
- Subject: Re: [GOLD] Support --icf=safe with -pie for x86_64
- Authentication-results: sourceware.org; auth=none
- References: <CAJRD=oqcd2y03pjosB6ifygwGv1wO0VgPFFqvTiSOvFhaqisJA@mail.gmail.com> <20170113012324.GO32333@bubble.grove.modra.org> <CAJRD=opJc+d+RENAfdndtyB2mjSHLTgXfhua2KF=R46dtkA-4Q@mail.gmail.com> <CAJimCsE=L3ZqLCxSn=V_HA_+BWL3uwjp5uoPtuQjk1sEXX1bhg@mail.gmail.com> <CAJRD=oq+MyiTzpLjWHTCG5Oz57zjBCmLQUHEptdK3pJNhb0Lyg@mail.gmail.com>
- Reply-to: Rahul Chaudhry <rahulchaudhry at google dot com>
>> I looked (maybe not carefully enough), but I didn't find anything in
>> this call chain that restricts the check to text sections:
>>
>> gc_process_relocs
>> -> scan.[global|local]_reloc_may_be_function_pointer
>> -> possible_function_pointer_reloc
>>
>> You need to check the SHF_EXECINST flag before assuming you're looking
>> at an opcode. (Look in x86_64.cc for "is_executable".)
>
> I'll take a closer look at this, add a check, and send an updated patch.
>
>
>> I think you should also be checking that the target symbol is
>> STT_FUNC; that should rule out most jump table cases. (I see a check
>> for != STT_OBJECT in the local symbol path, but nothing in the global
>> symbol path. It may be the case that STT_NOTYPE is used for some
>> extern function symbols, but you want to be conservative, right?)
>
> Ditto for this one.
Please see the updated patch below. There are two updates:
* Added a check for SHF_EXECINSTR flag before checking the call/jump opcodes.
* Added a check for STT_FUNC in the global symbol path.
Also, to add some background behind this patch, we're linking a large C++
binary (chrome-browser) with -pie.
* With --icf=none, the binary size is 177M.
* With --icf=safe, before this patch, the binary size is 176M.
ICF is able to fold 16730 sections (only ctors and dtors, since
Target_x86_64::do_can_check_for_function_pointers returns
false when using -pie).
* After applying this patch, with --icf=safe, the binary size is 169M.
ICF is able to fold 55623 sections.
* x86_64.cc (Target_x86_64::do_can_check_for_function_pointers):
Return true even when building pie binaries.
(Target_x86_64::possible_function_pointer_reloc): Check opcode
for R_X86_64_PC32 relocations.
(Target_x86_64::local_reloc_may_be_function_pointer): Pass
extra arguments to local_reloc_may_be_function_pointer.
(Target_x86_64::global_reloc_may_be_function_pointer): Likewise.
* gc.h (gc_process_relocs): Add check for STT_FUNC.
* testsuite/Makefile.am (icf_safe_pie_test): New test case.
* testsuite/Makefile.in: Regenerate.
* testsuite/icf_safe_pie_test.sh: New shell script.
--
Rahul
diff --git gold/ChangeLog gold/ChangeLog
index 56fe9669db..e811b8ea6d 100644
--- gold/ChangeLog
+++ gold/ChangeLog
@@ -1,3 +1,17 @@
+2017-02-03 Rahul Chaudhry <rahulchaudhry@google.com>
+
+ * x86_64.cc (Target_x86_64::do_can_check_for_function_pointers):
+ Return true even when building pie binaries.
+ (Target_x86_64::possible_function_pointer_reloc): Check opcode
+ for R_X86_64_PC32 relocations.
+ (Target_x86_64::local_reloc_may_be_function_pointer): Pass
+ extra arguments to local_reloc_may_be_function_pointer.
+ (Target_x86_64::global_reloc_may_be_function_pointer): Likewise.
+ * gc.h (gc_process_relocs): Add check for STT_FUNC.
+ * testsuite/Makefile.am (icf_safe_pie_test): New test case.
+ * testsuite/Makefile.in: Regenerate.
+ * testsuite/icf_safe_pie_test.sh: New shell script.
+
2017-02-03 Alan Modra <amodra@gmail.com>
* powerpc.cc (Powerpc_relobj::make_toc_relative): Don't crash
diff --git gold/gc.h gold/gc.h
index 45f8d2af9c..327efc25bf 100644
--- gold/gc.h
+++ gold/gc.h
@@ -295,6 +295,7 @@ gc_process_relocs(
// When doing safe folding, check to see if this relocation is that
// of a function pointer being taken.
if (gsym->source() == Symbol::FROM_OBJECT
+ && gsym->type() == elfcpp::STT_FUNC
&& check_section_for_function_pointers
&& dst_obj != NULL
&& (!is_ordinary
diff --git gold/testsuite/Makefile.am gold/testsuite/Makefile.am
index d9480abb42..d0803d251a 100644
--- gold/testsuite/Makefile.am
+++ gold/testsuite/Makefile.am
@@ -274,6 +274,20 @@ icf_safe_test_1.stdout: icf_safe_test
icf_safe_test_2.stdout: icf_safe_test
$(TEST_READELF) -h $< > $@
+check_SCRIPTS += icf_safe_pie_test.sh
+check_DATA += icf_safe_pie_test_1.stdout icf_safe_pie_test_2.stdout icf_safe_pie_test.map
+MOSTLYCLEANFILES += icf_safe_pie_test icf_safe_pie_test.map
+icf_safe_pie_test.o: icf_safe_test.cc
+ $(CXXCOMPILE) -O0 -c -ffunction-sections -fPIE -g -o $@ $<
+icf_safe_pie_test: icf_safe_pie_test.o gcctestdir/ld
+ $(CXXLINK) -o icf_safe_pie_test -Bgcctestdir/ -Wl,--icf=safe,-Map,icf_safe_pie_test.map icf_safe_pie_test.o -pie
+icf_safe_pie_test.map: icf_safe_pie_test
+ @touch icf_safe_pie_test.map
+icf_safe_pie_test_1.stdout: icf_safe_pie_test
+ $(TEST_NM) $< > $@
+icf_safe_pie_test_2.stdout: icf_safe_pie_test
+ $(TEST_READELF) -h $< > $@
+
check_SCRIPTS += icf_safe_so_test.sh
check_DATA += icf_safe_so_test_1.stdout icf_safe_so_test_2.stdout icf_safe_so_test.map
MOSTLYCLEANFILES += icf_safe_so_test icf_safe_so_test.map
diff --git gold/testsuite/Makefile.in gold/testsuite/Makefile.in
index 2c67bf5fe2..133e733cf1 100644
--- gold/testsuite/Makefile.in
+++ gold/testsuite/Makefile.in
@@ -82,6 +82,7 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_test.sh \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_keep_unique_test.sh \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_test.sh \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_pie_test.sh \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_so_test.sh \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ final_layout.sh \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ text_section_grouping.sh \
@@ -103,6 +104,9 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_test_1.stdout \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_test_2.stdout \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_test.map \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_pie_test_1.stdout \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_pie_test_2.stdout \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_pie_test.map \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_so_test_1.stdout \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_so_test_2.stdout \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_so_test.map \
@@ -125,6 +129,8 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_test icf_test.map \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_keep_unique_test \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_test icf_safe_test.map \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_pie_test \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_pie_test.map \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_so_test \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ icf_safe_so_test.map \
@GCC_TRUE@@NATIVE_LINKER_TRUE@ final_layout \
@@ -5093,6 +5099,8 @@ icf_keep_unique_test.sh.log: icf_keep_unique_test.sh
@p='icf_keep_unique_test.sh'; $(am__check_pre) $(LOG_COMPILE) "$$tst" $(am__check_post)
icf_safe_test.sh.log: icf_safe_test.sh
@p='icf_safe_test.sh'; $(am__check_pre) $(LOG_COMPILE) "$$tst" $(am__check_post)
+icf_safe_pie_test.sh.log: icf_safe_pie_test.sh
+ @p='icf_safe_pie_test.sh'; $(am__check_pre) $(LOG_COMPILE) "$$tst" $(am__check_post)
icf_safe_so_test.sh.log: icf_safe_so_test.sh
@p='icf_safe_so_test.sh'; $(am__check_pre) $(LOG_COMPILE) "$$tst" $(am__check_post)
final_layout.sh.log: final_layout.sh
@@ -5910,6 +5918,16 @@ uninstall-am:
@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_NM) $< > $@
@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_test_2.stdout: icf_safe_test
@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_READELF) -h $< > $@
+@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_pie_test.o: icf_safe_test.cc
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXCOMPILE) -O0 -c -ffunction-sections -fPIE -g -o $@ $<
+@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_pie_test: icf_safe_pie_test.o gcctestdir/ld
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXLINK) -o icf_safe_pie_test -Bgcctestdir/ -Wl,--icf=safe,-Map,icf_safe_pie_test.map icf_safe_pie_test.o -pie
+@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_pie_test.map: icf_safe_pie_test
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ @touch icf_safe_pie_test.map
+@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_pie_test_1.stdout: icf_safe_pie_test
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_NM) $< > $@
+@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_pie_test_2.stdout: icf_safe_pie_test
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_READELF) -h $< > $@
@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_so_test.o: icf_safe_so_test.cc
@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXCOMPILE) -O0 -c -ffunction-sections -fPIC -g -o $@ $<
@GCC_TRUE@@NATIVE_LINKER_TRUE@icf_safe_so_test: icf_safe_so_test.o gcctestdir/ld
diff --git gold/testsuite/icf_safe_pie_test.sh gold/testsuite/icf_safe_pie_test.sh
new file mode 100755
index 0000000000..f91a80c0b9
--- /dev/null
+++ gold/testsuite/icf_safe_pie_test.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+
+# icf_safe_pie_test.sh -- test --icf=safe -pie
+
+# Copyright (C) 2009-2017 Free Software Foundation, Inc.
+# Written by Sriraman Tallam <tmsriram@google.com>.
+# Modified by Rahul Chaudhry <rahulchaudhry@google.com>.
+
+# This file is part of gold.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+# The goal of this program is to verify if --icf=safe works with
+# -pie as expected. File icf_safe_test.cc is in this test. This
+# program checks if only ctors and dtors are folded, except for
+# the architectures which use relocation types and instruction
+# opcodes to detect if function pointers are taken.
+
+set -e
+
+check_nofold()
+{
+ func_addr_1=`grep $2 $1 | awk '{print $1}'`
+ func_addr_2=`grep $3 $1 | awk '{print $1}'`
+ if [ $func_addr_1 = $func_addr_2 ]
+ then
+ echo "Safe Identical Code Folding folded" $2 "and" $3
+ exit 1
+ fi
+}
+
+check_fold()
+{
+ awk "
+BEGIN { discard = 0; }
+/^Discarded input/ { discard = 1; }
+/^Memory map/ { discard = 0; }
+/.*\\.text\\..*($2|$3).*/ { act[discard] = act[discard] \" \" \$0; }
+END {
+ # printf \"kept\" act[0] \"\\nfolded\" act[1] \"\\n\";
+ if (length(act[0]) == 0 || length(act[1]) == 0)
+ {
+ printf \"Safe Identical Code Folding did not fold $2 and $3\\n\"
+ exit 1;
+ }
+ }" $1
+}
+
+arch_specific_safe_fold()
+{
+ if grep -q -e "Advanced Micro Devices X86-64" -e "Intel 80386" -e "ARM" -e "TILE" -e "PowerPC" -e "AArch64" -e "IBM S/390" $2;
+ then
+ check_fold $3 $4 $5
+ else
+ check_nofold $1 $4 $5
+ fi
+}
+
+arch_specific_safe_fold icf_safe_pie_test_1.stdout icf_safe_pie_test_2.stdout \
+ icf_safe_pie_test.map "kept_func_1" "kept_func_2"
+check_fold icf_safe_pie_test.map "_ZN1AD2Ev" "_ZN1AC2Ev"
+check_nofold icf_safe_pie_test_1.stdout "kept_func_3" "kept_func_1"
+check_nofold icf_safe_pie_test_1.stdout "kept_func_3" "kept_func_2"
diff --git gold/x86_64.cc gold/x86_64.cc
index d21c26813c..7f1742dd5f 100644
--- gold/x86_64.cc
+++ gold/x86_64.cc
@@ -729,10 +729,13 @@ class Target_x86_64 : public Sized_target<size, false>
// and global_reloc_may_be_function_pointer)
// if a function's pointer is taken. ICF uses this in safe mode to only
// fold those functions whose pointer is defintely not taken. For x86_64
- // pie binaries, safe ICF cannot be done by looking at relocation types.
+ // pie binaries, safe ICF cannot be done by looking at only relocation
+ // types, and for certain cases (e.g. R_X86_64_PC32), the instruction
+ // opcode is checked as well to distinguish a function call from taking
+ // a function's pointer.
bool
do_can_check_for_function_pointers() const
- { return !parameters->options().pie(); }
+ { return true; }
// Return the base for a DW_EH_PE_datarel encoding.
uint64_t
@@ -924,7 +927,10 @@ class Target_x86_64 : public Sized_target<size, false>
check_non_pic(Relobj*, unsigned int r_type, Symbol*);
inline bool
- possible_function_pointer_reloc(unsigned int r_type);
+ possible_function_pointer_reloc(Sized_relobj_file<size, false>* src_obj,
+ unsigned int src_indx,
+ unsigned int r_offset,
+ unsigned int r_type);
bool
reloc_needs_plt_for_ifunc(Sized_relobj_file<size, false>*,
@@ -3277,7 +3283,11 @@ Target_x86_64<size>::Scan::unsupported_reloc_global(
// Returns true if this relocation type could be that of a function pointer.
template<int size>
inline bool
-Target_x86_64<size>::Scan::possible_function_pointer_reloc(unsigned int r_type)
+Target_x86_64<size>::Scan::possible_function_pointer_reloc(
+ Sized_relobj_file<size, false>* src_obj,
+ unsigned int src_indx,
+ unsigned int r_offset,
+ unsigned int r_type)
{
switch (r_type)
{
@@ -3296,6 +3306,41 @@ Target_x86_64<size>::Scan::possible_function_pointer_reloc(unsigned int r_type)
{
return true;
}
+ case elfcpp::R_X86_64_PC32:
+ {
+ // This relocation may be used both for function calls and
+ // for taking address of a function. We distinguish between
+ // them by checking the opcodes.
+ uint64_t sh_flags = src_obj->section_flags(src_indx);
+ bool is_executable = (sh_flags & elfcpp::SHF_EXECINSTR) != 0;
+ if (is_executable)
+ {
+ section_size_type stype;
+ const unsigned char* view = src_obj->section_contents(src_indx,
+ &stype,
+ true);
+
+ // call
+ if (r_offset >= 1
+ && view[r_offset - 1] == 0xe8)
+ return false;
+
+ // jmp
+ if (r_offset >= 1
+ && view[r_offset - 1] == 0xe9)
+ return false;
+
+ // jo/jno/jb/jnb/je/jne/jna/ja/js/jns/jp/jnp/jl/jge/jle/jg
+ if (r_offset >= 2
+ && view[r_offset - 2] == 0x0f
+ && view[r_offset - 1] >= 0x80
+ && view[r_offset - 1] <= 0x8f)
+ return false;
+ }
+
+ // Be conservative and treat all others as function pointers.
+ return true;
+ }
}
return false;
}
@@ -3310,18 +3355,21 @@ Target_x86_64<size>::Scan::local_reloc_may_be_function_pointer(
Symbol_table* ,
Layout* ,
Target_x86_64<size>* ,
- Sized_relobj_file<size, false>* ,
- unsigned int ,
+ Sized_relobj_file<size, false>* src_obj,
+ unsigned int src_indx,
Output_section* ,
- const elfcpp::Rela<size, false>& ,
+ const elfcpp::Rela<size, false>& reloc,
unsigned int r_type,
const elfcpp::Sym<size, false>&)
{
// 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.
- return (parameters->options().shared()
- || possible_function_pointer_reloc(r_type));
+ if (parameters->options().shared())
+ return true;
+
+ return possible_function_pointer_reloc(src_obj, src_indx,
+ reloc.get_r_offset(), r_type);
}
// For safe ICF, scan a relocation for a global symbol to check if it
@@ -3334,20 +3382,23 @@ Target_x86_64<size>::Scan::global_reloc_may_be_function_pointer(
Symbol_table*,
Layout* ,
Target_x86_64<size>* ,
- Sized_relobj_file<size, false>* ,
- unsigned int ,
+ Sized_relobj_file<size, false>* src_obj,
+ unsigned int src_indx,
Output_section* ,
- const elfcpp::Rela<size, false>& ,
+ const elfcpp::Rela<size, false>& reloc,
unsigned int r_type,
Symbol* gsym)
{
// When building a shared library, do not fold symbols whose visibility
// is hidden, internal or protected.
- return ((parameters->options().shared()
- && (gsym->visibility() == elfcpp::STV_INTERNAL
- || gsym->visibility() == elfcpp::STV_PROTECTED
- || gsym->visibility() == elfcpp::STV_HIDDEN))
- || possible_function_pointer_reloc(r_type));
+ if (parameters->options().shared()
+ && (gsym->visibility() == elfcpp::STV_INTERNAL
+ || gsym->visibility() == elfcpp::STV_PROTECTED
+ || gsym->visibility() == elfcpp::STV_HIDDEN))
+ return true;
+
+ return possible_function_pointer_reloc(src_obj, src_indx,
+ reloc.get_r_offset(), r_type);
}
// Scan a relocation for a global symbol.