Discussion:
[GOLD] PowerPC64 identical code folding
(too old to reply)
Alan Modra
2013-03-12 00:20:29 UTC
Permalink
This makes the ICF testcase, icf_test.cc, work for PowerPC64.
icf_test.cc has functions with recursive calls, which are recognised
as such in icf.cc by noticing calls back to the originating section.
On PowerPC64 a function address is the address of the descriptor in
.opd, rather than the function entry point. So icf.cc doesn't see the
calls as recursive and therefore won't fold the sections. We need to
look through the function descriptor to notice recursive calls. I
also modify the symbol value/addend pair to point at the function
entry point. That isn't strictly necessary with current ICF and gcc
generated code, but it seems The Right Thing To Do.

You might notice I didn't say the ICF testcase passes. It doesn't.
That's because the address of folded_func and kept_func are different
on PowerPC64 even though their code sections are folded. We still
have two different function descriptors.. I'll get around to folding
them too one day, along with removing unused .opd entries.

OK to apply?

* gc.h (gc_process_relocs): Look through function descriptors
to determine shndx, symvalue and addend used by ICF. Tidy
variable duplication.

Index: gold/gc.h
===================================================================
RCS file: /cvs/src/src/gold/gc.h,v
retrieving revision 1.17
diff -u -p -r1.17 gc.h
--- gold/gc.h 9 Sep 2012 03:43:51 -0000 1.17
+++ gold/gc.h 11 Mar 2013 22:25:02 -0000
@@ -235,30 +235,45 @@ gc_process_relocs(
Reloc_types<sh_type, size, big_endian>::get_reloc_addend_noerror(&reloc);
Object* dst_obj;
unsigned int dst_indx;
- typename elfcpp::Elf_types<size>::Elf_Addr dst_off;
+ typedef typename elfcpp::Elf_types<size>::Elf_Addr Address;
+ Address dst_off;

if (r_sym < local_count)
{
gold_assert(plocal_syms != NULL);
typename elfcpp::Sym<size, big_endian> lsym(plocal_syms
+ r_sym * sym_size);
- unsigned int shndx = lsym.get_st_shndx();
+ dst_indx = lsym.get_st_shndx();
bool is_ordinary;
- shndx = src_obj->adjust_sym_shndx(r_sym, shndx, &is_ordinary);
+ dst_indx = src_obj->adjust_sym_shndx(r_sym, dst_indx, &is_ordinary);
dst_obj = src_obj;
- dst_indx = shndx;
- dst_off = lsym.get_st_value();
+ dst_off = lsym.get_st_value() + addend;

if (is_icf_tracked)
{
+ Address symvalue = dst_off - addend;
if (is_ordinary)
- (*secvec).push_back(Section_id(dst_obj, dst_indx));
+ {
+ Symbol_location loc;
+ loc.object = dst_obj;
+ loc.shndx = dst_indx;
+ loc.offset = convert_types<off_t, Address>(dst_off);
+ // Look through function descriptors
+ parameters->target().function_location(&loc);
+ if (loc.shndx != dst_indx)
+ {
+ // Modify symvalue/addend to the code entry.
+ symvalue = loc.offset;
+ addend = 0;
+ }
+ (*secvec).push_back(Section_id(loc.object, loc.shndx));
+ }
else
(*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(NULL);
- long long symvalue = static_cast<long long>(lsym.get_st_value());
- (*addendvec).push_back(std::make_pair(symvalue,
- static_cast<long long>(addend)));
+ (*addendvec).push_back(std::make_pair(
+ static_cast<long long>(symvalue),
+ static_cast<long long>(addend)));
uint64_t reloc_offset =
convert_to_section_size_type(reloc.get_r_offset());
(*offsetvec).push_back(reloc_offset);
@@ -279,7 +294,7 @@ gc_process_relocs(
symtab->icf()->set_section_has_function_pointers(
src_obj, lsym.get_st_shndx());

- if (!is_ordinary || shndx == src_indx)
+ if (!is_ordinary || dst_indx == src_indx)
continue;
}
else
@@ -291,14 +306,14 @@ gc_process_relocs(

dst_obj = NULL;
dst_indx = 0;
- dst_off = 0;
bool is_ordinary = false;
if (gsym->source() == Symbol::FROM_OBJECT)
{
dst_obj = gsym->object();
dst_indx = gsym->shndx(&is_ordinary);
- dst_off = static_cast<const Sized_symbol<size>*>(gsym)->value();
}
+ dst_off = static_cast<const Sized_symbol<size>*>(gsym)->value();
+ dst_off += addend;

// When doing safe folding, check to see if this relocation is that
// of a function pointer being taken.
@@ -326,17 +341,29 @@ gc_process_relocs(
}
if (is_icf_tracked)
{
+ Address symvalue = dst_off - addend;
if (is_ordinary && gsym->source() == Symbol::FROM_OBJECT)
- (*secvec).push_back(Section_id(dst_obj, dst_indx));
+ {
+ Symbol_location loc;
+ loc.object = dst_obj;
+ loc.shndx = dst_indx;
+ loc.offset = convert_types<off_t, Address>(dst_off);
+ // Look through function descriptors
+ parameters->target().function_location(&loc);
+ if (loc.shndx != dst_indx)
+ {
+ // Modify symvalue/addend to the code entry.
+ symvalue = loc.offset;
+ addend = 0;
+ }
+ (*secvec).push_back(Section_id(loc.object, loc.shndx));
+ }
else
(*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(gsym);
- Sized_symbol<size>* sized_gsym =
- static_cast<Sized_symbol<size>* >(gsym);
- long long symvalue =
- static_cast<long long>(sized_gsym->value());
- (*addendvec).push_back(std::make_pair(symvalue,
- static_cast<long long>(addend)));
+ (*addendvec).push_back(std::make_pair(
+ static_cast<long long>(symvalue),
+ static_cast<long long>(addend)));
uint64_t reloc_offset =
convert_to_section_size_type(reloc.get_r_offset());
(*offsetvec).push_back(reloc_offset);
@@ -353,7 +380,6 @@ gc_process_relocs(
if (parameters->options().gc_sections())
{
symtab->gc()->add_reference(src_obj, src_indx, dst_obj, dst_indx);
- dst_off += addend;
parameters->sized_target<size, big_endian>()
->gc_add_reference(symtab, src_obj, src_indx,
dst_obj, dst_indx, dst_off);
--
Alan Modra
Australia Development Lab, IBM
Ian Lance Taylor
2013-03-12 00:26:44 UTC
Permalink
Post by Alan Modra
* gc.h (gc_process_relocs): Look through function descriptors
to determine shndx, symvalue and addend used by ICF. Tidy
variable duplication.
+ // Look through function descriptors
Missing period at end of sentence.

This is OK.

Thanks.

Ian
Andrew Pinski
2013-03-12 01:21:05 UTC
Permalink
Post by Alan Modra
This makes the ICF testcase, icf_test.cc, work for PowerPC64.
icf_test.cc has functions with recursive calls, which are recognised
as such in icf.cc by noticing calls back to the originating section.
On PowerPC64 a function address is the address of the descriptor in
.opd, rather than the function entry point. So icf.cc doesn't see the
calls as recursive and therefore won't fold the sections. We need to
look through the function descriptor to notice recursive calls. I
also modify the symbol value/addend pair to point at the function
entry point. That isn't strictly necessary with current ICF and gcc
generated code, but it seems The Right Thing To Do.
You might notice I didn't say the ICF testcase passes. It doesn't.
That's because the address of folded_func and kept_func are different
on PowerPC64 even though their code sections are folded. We still
have two different function descriptors.. I'll get around to folding
them too one day, along with removing unused .opd entries.
Actually keeping around different function descriptors seems better
and then for PowerPC64 you can just turn ICF by default and not
violate C/C++ rules about function pointers being different.

Thanks,
Andrew
Post by Alan Modra
OK to apply?
* gc.h (gc_process_relocs): Look through function descriptors
to determine shndx, symvalue and addend used by ICF. Tidy
variable duplication.
Index: gold/gc.h
===================================================================
RCS file: /cvs/src/src/gold/gc.h,v
retrieving revision 1.17
diff -u -p -r1.17 gc.h
--- gold/gc.h 9 Sep 2012 03:43:51 -0000 1.17
+++ gold/gc.h 11 Mar 2013 22:25:02 -0000
@@ -235,30 +235,45 @@ gc_process_relocs(
Reloc_types<sh_type, size, big_endian>::get_reloc_addend_noerror(&reloc);
Object* dst_obj;
unsigned int dst_indx;
- typename elfcpp::Elf_types<size>::Elf_Addr dst_off;
+ typedef typename elfcpp::Elf_types<size>::Elf_Addr Address;
+ Address dst_off;
if (r_sym < local_count)
{
gold_assert(plocal_syms != NULL);
typename elfcpp::Sym<size, big_endian> lsym(plocal_syms
+ r_sym * sym_size);
- unsigned int shndx = lsym.get_st_shndx();
+ dst_indx = lsym.get_st_shndx();
bool is_ordinary;
- shndx = src_obj->adjust_sym_shndx(r_sym, shndx, &is_ordinary);
+ dst_indx = src_obj->adjust_sym_shndx(r_sym, dst_indx, &is_ordinary);
dst_obj = src_obj;
- dst_indx = shndx;
- dst_off = lsym.get_st_value();
+ dst_off = lsym.get_st_value() + addend;
if (is_icf_tracked)
{
+ Address symvalue = dst_off - addend;
if (is_ordinary)
- (*secvec).push_back(Section_id(dst_obj, dst_indx));
+ {
+ Symbol_location loc;
+ loc.object = dst_obj;
+ loc.shndx = dst_indx;
+ loc.offset = convert_types<off_t, Address>(dst_off);
+ // Look through function descriptors
+ parameters->target().function_location(&loc);
+ if (loc.shndx != dst_indx)
+ {
+ // Modify symvalue/addend to the code entry.
+ symvalue = loc.offset;
+ addend = 0;
+ }
+ (*secvec).push_back(Section_id(loc.object, loc.shndx));
+ }
else
(*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(NULL);
- long long symvalue = static_cast<long long>(lsym.get_st_value());
- (*addendvec).push_back(std::make_pair(symvalue,
- static_cast<long long>(addend)));
+ (*addendvec).push_back(std::make_pair(
+ static_cast<long long>(symvalue),
+ static_cast<long long>(addend)));
uint64_t reloc_offset =
convert_to_section_size_type(reloc.get_r_offset());
(*offsetvec).push_back(reloc_offset);
@@ -279,7 +294,7 @@ gc_process_relocs(
symtab->icf()->set_section_has_function_pointers(
src_obj, lsym.get_st_shndx());
- if (!is_ordinary || shndx == src_indx)
+ if (!is_ordinary || dst_indx == src_indx)
continue;
}
else
@@ -291,14 +306,14 @@ gc_process_relocs(
dst_obj = NULL;
dst_indx = 0;
- dst_off = 0;
bool is_ordinary = false;
if (gsym->source() == Symbol::FROM_OBJECT)
{
dst_obj = gsym->object();
dst_indx = gsym->shndx(&is_ordinary);
- dst_off = static_cast<const Sized_symbol<size>*>(gsym)->value();
}
+ dst_off = static_cast<const Sized_symbol<size>*>(gsym)->value();
+ dst_off += addend;
// When doing safe folding, check to see if this relocation is that
// of a function pointer being taken.
@@ -326,17 +341,29 @@ gc_process_relocs(
}
if (is_icf_tracked)
{
+ Address symvalue = dst_off - addend;
if (is_ordinary && gsym->source() == Symbol::FROM_OBJECT)
- (*secvec).push_back(Section_id(dst_obj, dst_indx));
+ {
+ Symbol_location loc;
+ loc.object = dst_obj;
+ loc.shndx = dst_indx;
+ loc.offset = convert_types<off_t, Address>(dst_off);
+ // Look through function descriptors
+ parameters->target().function_location(&loc);
+ if (loc.shndx != dst_indx)
+ {
+ // Modify symvalue/addend to the code entry.
+ symvalue = loc.offset;
+ addend = 0;
+ }
+ (*secvec).push_back(Section_id(loc.object, loc.shndx));
+ }
else
(*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(gsym);
- Sized_symbol<size>* sized_gsym =
- static_cast<Sized_symbol<size>* >(gsym);
- long long symvalue =
- static_cast<long long>(sized_gsym->value());
- (*addendvec).push_back(std::make_pair(symvalue,
- static_cast<long long>(addend)));
+ (*addendvec).push_back(std::make_pair(
+ static_cast<long long>(symvalue),
+ static_cast<long long>(addend)));
uint64_t reloc_offset =
convert_to_section_size_type(reloc.get_r_offset());
(*offsetvec).push_back(reloc_offset);
@@ -353,7 +380,6 @@ gc_process_relocs(
if (parameters->options().gc_sections())
{
symtab->gc()->add_reference(src_obj, src_indx, dst_obj, dst_indx);
- dst_off += addend;
parameters->sized_target<size, big_endian>()
->gc_add_reference(symtab, src_obj, src_indx,
dst_obj, dst_indx, dst_off);
--
Alan Modra
Australia Development Lab, IBM
Alan Modra
2013-03-12 13:30:58 UTC
Permalink
Post by Andrew Pinski
Actually keeping around different function descriptors seems better
and then for PowerPC64 you can just turn ICF by default and not
violate C/C++ rules about function pointers being different.
Yes, good idea. --icf=safe can do the same as --icf=all on ppc64.

This patch enables --icf=safe mode for powerpc, and finally gives a
clean gold testsuite for ppc and ppc64 (unless you happen to be
affected by PR14675). I changed the icf tests that check for folding
to check sections rather than symbols. This avoids the problem that
folded functions on ppc64 remain with distinct addresses. The tests
that check for no folding continue to use symbols. OK to apply?

* powerpc.cc (is_branch_reloc): Forward declare.
(Target_powerpc::do_can_check_for_function_pointers): New predicate.
(Target_powerpc::Scan::local_reloc_may_be_function_pointer): Return
false for 64-bit, true for 32-bit non-branch relocs.
(Target_powerpc::Scan::global_reloc_may_be_function_pointer): Likewise.
* testsuite/Makefile.am (icf_test): Use linker map file instead of
nm output.
(icf_safe_test): Generate linker map file as well as nm output.
(icf_safe_so_test): Likewise.
* testsuite/Makefile.in: Regenerate.
* testsuite/icf_test.sh: Parse linker map file to determine
section folding.
* testsuite/icf_safe_test.sh: Likewise. Expect folding for PowerPC.
* testsuite/icf_safe_so_test.sh: Likewise.
(X86_32_or_ARM_specific_safe_fold): Merge into..
(arch_specific_safe_fold): ..this.
(X86_64_specific_safe_fold): Delete unused function.

Index: gold/powerpc.cc
===================================================================
RCS file: /cvs/src/src/gold/powerpc.cc,v
retrieving revision 1.86
diff -u -p -r1.86 powerpc.cc
--- gold/powerpc.cc 10 Mar 2013 23:08:18 -0000 1.86
+++ gold/powerpc.cc 12 Mar 2013 12:47:21 -0000
@@ -62,6 +62,9 @@ class Output_data_glink;
template<int size, bool big_endian>
class Stub_table;

+inline bool
+is_branch_reloc(unsigned int r_type);
+
template<int size, bool big_endian>
class Powerpc_relobj : public Sized_relobj_file<size, big_endian>
{
@@ -555,6 +558,10 @@ class Target_powerpc : public Sized_targ
void
do_function_location(Symbol_location*) const;

+ bool
+ do_can_check_for_function_pointers() const
+ { return true; }
+
// Relocate a section.
void
relocate_section(const Relocate_info<size, big_endian>*,
@@ -863,9 +870,18 @@ class Target_powerpc : public Sized_targ
unsigned int ,
Output_section* ,
const elfcpp::Rela<size, big_endian>& ,
- unsigned int ,
+ unsigned int r_type,
const elfcpp::Sym<size, big_endian>&)
- { return false; }
+ {
+ // PowerPC64 .opd is not folded, so any identical function text
+ // may be folded and we'll still keep function addresses distinct.
+ // That means no reloc is of concern here.
+ if (size == 64)
+ return false;
+ // For 32-bit, conservatively assume anything but calls to
+ // function code might be taking the address of the function.
+ return !is_branch_reloc(r_type);
+ }

inline bool
global_reloc_may_be_function_pointer(Symbol_table* , Layout* ,
@@ -873,10 +889,15 @@ class Target_powerpc : public Sized_targ
Sized_relobj_file<size, big_endian>* ,
unsigned int ,
Output_section* ,
- const elfcpp::Rela<size,
- big_endian>& ,
- unsigned int , Symbol*)
- { return false; }
+ const elfcpp::Rela<size, big_endian>& ,
+ unsigned int r_type,
+ Symbol*)
+ {
+ // As above.
+ if (size == 64)
+ return false;
+ return !is_branch_reloc(r_type);
+ }

private:
static void
Index: gold/testsuite/Makefile.am
===================================================================
RCS file: /cvs/src/src/gold/testsuite/Makefile.am,v
retrieving revision 1.208
diff -u -p -r1.208 Makefile.am
--- gold/testsuite/Makefile.am 4 Mar 2013 00:48:01 -0000 1.208
+++ gold/testsuite/Makefile.am 12 Mar 2013 12:47:22 -0000
@@ -201,14 +221,12 @@ pr14265.stdout: pr14265
$(TEST_NM) --format=bsd --numeric-sort $< > $@

check_SCRIPTS += icf_test.sh
-check_DATA += icf_test.stdout
-MOSTLYCLEANFILES += icf_test
+check_DATA += icf_test.map
+MOSTLYCLEANFILES += icf_test icf_test.map
icf_test.o: icf_test.cc
$(CXXCOMPILE) -O0 -c -ffunction-sections -g -o $@ $<
-icf_test: icf_test.o gcctestdir/ld
- $(CXXLINK) -Bgcctestdir/ -Wl,--icf=all icf_test.o
-icf_test.stdout: icf_test
- $(TEST_NM) -C icf_test > icf_test.stdout
+icf_test icf_test.map: icf_test.o gcctestdir/ld
+ $(CXXLINK) -o icf_test -Bgcctestdir/ -Wl,--icf=all,-Map,icf_test.map icf_test.o

check_SCRIPTS += icf_keep_unique_test.sh
check_DATA += icf_keep_unique_test.stdout
@@ -218,31 +236,31 @@ icf_keep_unique_test.o: icf_keep_unique_
icf_keep_unique_test: icf_keep_unique_test.o gcctestdir/ld
$(CXXLINK) -Bgcctestdir/ -Wl,--icf=all -Wl,--keep-unique,_Z11unique_funcv icf_keep_unique_test.o
icf_keep_unique_test.stdout: icf_keep_unique_test
- $(TEST_NM) -C icf_keep_unique_test > icf_keep_unique_test.stdout
+ $(TEST_NM) -C $< > $@

check_SCRIPTS += icf_safe_test.sh
-check_DATA += icf_safe_test_1.stdout icf_safe_test_2.stdout
-MOSTLYCLEANFILES += icf_safe_test
+check_DATA += icf_safe_test_1.stdout icf_safe_test_2.stdout icf_safe_test.map
+MOSTLYCLEANFILES += icf_safe_test icf_safe_test.map
icf_safe_test.o: icf_safe_test.cc
$(CXXCOMPILE) -O0 -c -ffunction-sections -g -o $@ $<
-icf_safe_test: icf_safe_test.o gcctestdir/ld
- $(CXXLINK) -Bgcctestdir/ -Wl,--icf=safe icf_safe_test.o
+icf_safe_test icf_safe_test.map: icf_safe_test.o gcctestdir/ld
+ $(CXXLINK) -o icf_safe_test -Bgcctestdir/ -Wl,--icf=safe,-Map,icf_safe_test.map icf_safe_test.o
icf_safe_test_1.stdout: icf_safe_test
- $(TEST_NM) icf_safe_test > icf_safe_test_1.stdout
+ $(TEST_NM) $< > $@
icf_safe_test_2.stdout: icf_safe_test
- $(TEST_READELF) -h icf_safe_test > icf_safe_test_2.stdout
+ $(TEST_READELF) -h $< > $@

check_SCRIPTS += icf_safe_so_test.sh
-check_DATA += icf_safe_so_test_1.stdout icf_safe_so_test_2.stdout
-MOSTLYCLEANFILES += icf_safe_so_test
+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
icf_safe_so_test.o: icf_safe_so_test.cc
$(CXXCOMPILE) -O0 -c -ffunction-sections -fPIC -g -o $@ $<
-icf_safe_so_test: icf_safe_so_test.o gcctestdir/ld
- $(CXXLINK) -Bgcctestdir/ -Wl,--icf=safe icf_safe_so_test.o -fPIC -shared
+icf_safe_so_test icf_safe_so_test.map: icf_safe_so_test.o gcctestdir/ld
+ $(CXXLINK) -o icf_safe_so_test -Bgcctestdir/ -Wl,--icf=safe,-Map,icf_safe_so_test.map icf_safe_so_test.o -fPIC -shared
icf_safe_so_test_1.stdout: icf_safe_so_test
- $(TEST_NM) icf_safe_so_test > icf_safe_so_test_1.stdout
+ $(TEST_NM) $< > $@
icf_safe_so_test_2.stdout: icf_safe_so_test
- $(TEST_READELF) -h icf_safe_so_test > icf_safe_so_test_2.stdout
+ $(TEST_READELF) -h $< > $@

check_SCRIPTS += final_layout.sh
check_DATA += final_layout.stdout
Index: gold/testsuite/icf_test.sh
===================================================================
RCS file: /cvs/src/src/gold/testsuite/icf_test.sh,v
retrieving revision 1.1
diff -u -p -r1.1 icf_test.sh
--- gold/testsuite/icf_test.sh 5 Aug 2009 20:51:56 -0000 1.1
+++ gold/testsuite/icf_test.sh 12 Mar 2013 12:47:22 -0000
@@ -28,13 +28,19 @@

check()
{
- 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 "Identical Code Folding failed to fold" $2 "and" $3
- exit 1
- fi
+ 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 \"Identical Code Folding did not fold $2 and $3\\n\"
+ exit 1;
+ }
+ }" $1
}

-check icf_test.stdout "folded_func" "kept_func"
+check icf_test.map "folded_func" "kept_func"
Index: gold/testsuite/icf_safe_test.sh
===================================================================
RCS file: /cvs/src/src/gold/testsuite/icf_safe_test.sh,v
retrieving revision 1.6
diff -u -p -r1.6 icf_safe_test.sh
--- gold/testsuite/icf_safe_test.sh 15 Sep 2012 17:11:28 -0000 1.6
+++ gold/testsuite/icf_safe_test.sh 12 Mar 2013 12:47:22 -0000
@@ -40,28 +40,34 @@ check_nofold()

check_fold()
{
- 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 did not fold " $2 "and" $3
- exit 1
- fi
+ 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()
{
- grep_x86=`grep -q -e "Advanced Micro Devices X86-64" -e "Intel 80386" -e "ARM" -e "TILE" $2`
+ grep_x86=`grep -q -e "Advanced Micro Devices X86-64" -e "Intel 80386" -e "ARM" -e "TILE" -e "PowerPC" $2`
if [ $? -eq 0 ];
then
- check_fold $1 $3 $4
+ check_fold $3 $4 $5
else
- check_nofold $1 $3 $4
+ check_nofold $1 $4 $5
fi
}

arch_specific_safe_fold icf_safe_test_1.stdout icf_safe_test_2.stdout \
- "kept_func_1" "kept_func_2"
-check_fold icf_safe_test_1.stdout "_ZN1AD1Ev" "_ZN1AC1Ev"
+ icf_safe_test.map "kept_func_1" "kept_func_2"
+check_fold icf_safe_test.map "_ZN1AD2Ev" "_ZN1AC2Ev"
check_nofold icf_safe_test_1.stdout "kept_func_3" "kept_func_1"
check_nofold icf_safe_test_1.stdout "kept_func_3" "kept_func_2"
Index: gold/testsuite/icf_safe_so_test.sh
===================================================================
RCS file: /cvs/src/src/gold/testsuite/icf_safe_so_test.sh,v
retrieving revision 1.5
diff -u -p -r1.5 icf_safe_so_test.sh
--- gold/testsuite/icf_safe_so_test.sh 21 Mar 2011 20:55:33 -0000 1.5
+++ gold/testsuite/icf_safe_so_test.sh 12 Mar 2013 12:47:22 -0000
@@ -67,41 +67,36 @@ check_fold()
return 0
fi

- 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 did not fold " $2 "and" $3
- exit 1
- fi
+ 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;
+ }
+ }" $4
}

arch_specific_safe_fold()
{
- if [ $1 -eq 0 ];
+ grep -e "Intel 80386" -e "ARM" -e "PowerPC" $1 > /dev/null 2>&1
+ if [ $? -eq 0 ];
then
- check_fold $2 $3 $4
+ check_fold $2 $4 $5 $3
else
- check_nofold $2 $3 $4
+ check_nofold $2 $4 $5
fi
}

-X86_32_or_ARM_specific_safe_fold()
-{
- grep -e "Intel 80386" -e "ARM" $1 > /dev/null 2>&1
- arch_specific_safe_fold $? $2 $3 $4
-}
-
-X86_64_specific_safe_fold ()
-{
- grep -e "Advanced Micro Devices X86-64" $1 > /dev/null 2>&1
- arch_specific_safe_fold $? $2 $3 $4
-}
-
-X86_32_or_ARM_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout "foo_prot" "foo_hidden"
-X86_32_or_ARM_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout "foo_prot" "foo_internal"
-X86_32_or_ARM_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout "foo_prot" "foo_static"
-X86_32_or_ARM_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout "foo_hidden" "foo_internal"
-X86_32_or_ARM_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout "foo_hidden" "foo_static"
-X86_32_or_ARM_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout "foo_internal" "foo_static"
+arch_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout icf_safe_so_test.map "foo_prot" "foo_hidden"
+arch_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout icf_safe_so_test.map "foo_prot" "foo_internal"
+arch_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout icf_safe_so_test.map "foo_prot" "foo_static"
+arch_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout icf_safe_so_test.map "foo_hidden" "foo_internal"
+arch_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout icf_safe_so_test.map "foo_hidden" "foo_static"
+arch_specific_safe_fold icf_safe_so_test_2.stdout icf_safe_so_test_1.stdout icf_safe_so_test.map "foo_internal" "foo_static"
check_nofold icf_safe_so_test_1.stdout "foo_glob" "bar_glob"
--
Alan Modra
Australia Development Lab, IBM
Ian Lance Taylor
2013-03-12 18:52:47 UTC
Permalink
Post by Alan Modra
* powerpc.cc (is_branch_reloc): Forward declare.
(Target_powerpc::do_can_check_for_function_pointers): New predicate.
(Target_powerpc::Scan::local_reloc_may_be_function_pointer): Return
false for 64-bit, true for 32-bit non-branch relocs.
(Target_powerpc::Scan::global_reloc_may_be_function_pointer): Likewise.
* testsuite/Makefile.am (icf_test): Use linker map file instead of
nm output.
(icf_safe_test): Generate linker map file as well as nm output.
(icf_safe_so_test): Likewise.
* testsuite/Makefile.in: Regenerate.
* testsuite/icf_test.sh: Parse linker map file to determine
section folding.
* testsuite/icf_safe_test.sh: Likewise. Expect folding for PowerPC.
* testsuite/icf_safe_so_test.sh: Likewise.
(X86_32_or_ARM_specific_safe_fold): Merge into..
(arch_specific_safe_fold): ..this.
(X86_64_specific_safe_fold): Delete unused function.
This is OK.

Thanks.

Ian
Alan Modra
2013-03-14 06:52:15 UTC
Permalink
Post by Alan Modra
* gc.h (gc_process_relocs): Look through function descriptors
to determine shndx, symvalue and addend used by ICF. Tidy
variable duplication.
On testing this with more than the gold testsuite, I discovered that
--icf on PowerPC64 is quite broken. We can't call function_location()
for a symbol defined in some object for which do_read_relocs() hasn't
been called; The .opd info for that object isn't yet set up.

It looks like I'll need to translate shndx, symvalue and addend in
icf.cc.
--
Alan Modra
Australia Development Lab, IBM
Alan Modra
2013-03-14 07:25:02 UTC
Permalink
Post by Alan Modra
Post by Alan Modra
* gc.h (gc_process_relocs): Look through function descriptors
to determine shndx, symvalue and addend used by ICF. Tidy
variable duplication.
On testing this with more than the gold testsuite, I discovered that
--icf on PowerPC64 is quite broken. We can't call function_location()
for a symbol defined in some object for which do_read_relocs() hasn't
been called; The .opd info for that object isn't yet set up.
It looks like I'll need to translate shndx, symvalue and addend in
icf.cc.
Like this. OK to apply?

* gc.h (gc_process_relocs): Don't look through function descriptors.
* icf.cc (get_section_contents): Do so here instead.

Index: gold/gc.h
===================================================================
RCS file: /cvs/src/src/gold/gc.h,v
retrieving revision 1.18
diff -u -p -r1.18 gc.h
--- gold/gc.h 12 Mar 2013 00:42:14 -0000 1.18
+++ gold/gc.h 14 Mar 2013 07:12:04 -0000
@@ -253,21 +253,7 @@ gc_process_relocs(
{
Address symvalue = dst_off - addend;
if (is_ordinary)
- {
- Symbol_location loc;
- loc.object = dst_obj;
- loc.shndx = dst_indx;
- loc.offset = convert_types<off_t, Address>(dst_off);
- // Look through function descriptors.
- parameters->target().function_location(&loc);
- if (loc.shndx != dst_indx)
- {
- // Modify symvalue/addend to the code entry.
- symvalue = loc.offset;
- addend = 0;
- }
- (*secvec).push_back(Section_id(loc.object, loc.shndx));
- }
+ (*secvec).push_back(Section_id(dst_obj, dst_indx));
else
(*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(NULL);
@@ -343,21 +329,7 @@ gc_process_relocs(
{
Address symvalue = dst_off - addend;
if (is_ordinary && gsym->source() == Symbol::FROM_OBJECT)
- {
- Symbol_location loc;
- loc.object = dst_obj;
- loc.shndx = dst_indx;
- loc.offset = convert_types<off_t, Address>(dst_off);
- // Look through function descriptors.
- parameters->target().function_location(&loc);
- if (loc.shndx != dst_indx)
- {
- // Modify symvalue/addend to the code entry.
- symvalue = loc.offset;
- addend = 0;
- }
- (*secvec).push_back(Section_id(loc.object, loc.shndx));
- }
+ (*secvec).push_back(Section_id(dst_obj, dst_indx));
else
(*secvec).push_back(Section_id(NULL, 0));
(*symvec).push_back(gsym);
Index: gold/icf.cc
===================================================================
RCS file: /cvs/src/src/gold/icf.cc,v
retrieving revision 1.20
diff -u -p -r1.20 icf.cc
--- gold/icf.cc 28 Jun 2011 21:15:42 -0000 1.20
+++ gold/icf.cc 14 Mar 2013 07:12:04 -0000
@@ -288,6 +288,25 @@ get_section_contents(bool first_iteratio

for (; it_v != v.end(); ++it_v, ++it_s, ++it_a, ++it_o, ++it_addend_size)
{
+ if (first_iteration
+ && it_v->first != NULL)
+ {
+ Symbol_location loc;
+ loc.object = it_v->first;
+ loc.shndx = it_v->second;
+ loc.offset = convert_types<off_t, long long>(it_a->first
+ + it_a->second);
+ // Look through function descriptors
+ parameters->target().function_location(&loc);
+ if (loc.shndx != it_v->second)
+ {
+ it_v->second = loc.shndx;
+ // Modify symvalue/addend to the code entry.
+ it_a->first = loc.offset;
+ it_a->second = 0;
+ }
+ }
+
// ADDEND_STR stores the symbol value and addend and offset,
// each at most 16 hex digits long. it_a points to a pair
// where first is the symbol value and second is the
--
Alan Modra
Australia Development Lab, IBM
Ian Lance Taylor
2013-03-15 05:32:31 UTC
Permalink
Post by Alan Modra
Post by Alan Modra
Post by Alan Modra
* gc.h (gc_process_relocs): Look through function descriptors
to determine shndx, symvalue and addend used by ICF. Tidy
variable duplication.
On testing this with more than the gold testsuite, I discovered that
--icf on PowerPC64 is quite broken. We can't call function_location()
for a symbol defined in some object for which do_read_relocs() hasn't
been called; The .opd info for that object isn't yet set up.
It looks like I'll need to translate shndx, symvalue and addend in
icf.cc.
Like this. OK to apply?
* gc.h (gc_process_relocs): Don't look through function descriptors.
* icf.cc (get_section_contents): Do so here instead.
This is OK.

Thanks.

Ian

Loading...