[PATCH 2/2] Gold: Enable safe ICF for shared object on x86-64

[Fangrui Song]

On 2020-10-11, H.J. Lu via Binutils wrote:
>With
>
>commit 4aebb6312eb5dcd12f2f8420028547584b708907
>Author: Rahul Chaudhry <rahulchaudhry@google.com>
>Date:   Wed Feb 15 00:37:10 2017 -0800
>
>    Improved support for --icf=safe when used with -pie.
>
>we now check opcode with R_X86_64_PC32 relocation, which tell branches
>from other instructions.  We can enable safe ICF for shared object on
>x86-64.  Also, global symbols with non-default visibility should be
>folded like local symbols.
>
>	PR gold/21452
>	* x86_64.cc (Scan::local_reloc_may_be_function_pointer): Remove
>	check for shared library.
>	(Scan::global_reloc_may_be_function_pointer): Remove check for
>	shared library and symbol visibility.
>	* testsuite/icf_safe_so_test.cc (bar_static): New function.
>	(main): Take function address of bar_static and use it.
>	* testsuite/icf_safe_so_test.sh (arch_specific_safe_fold): Also
>	check fold on x86-64.  Check bar_static isn't folded.
>---
> gold/testsuite/icf_safe_so_test.cc |  8 ++++++++
> gold/testsuite/icf_safe_so_test.sh |  2 +-
> gold/x86_64.cc                     | 18 ++----------------
> 3 files changed, 11 insertions(+), 17 deletions(-)
>
>diff --git a/gold/testsuite/icf_safe_so_test.cc b/gold/testsuite/icf_safe_so_test.cc
>index 1c593031d05..32566553276 100644
>--- a/gold/testsuite/icf_safe_so_test.cc
>+++ b/gold/testsuite/icf_safe_so_test.cc
>@@ -61,10 +61,18 @@ int bar_glob()
>   return 2;
> }
>
>+static int
>+bar_static()
>+{
>+  return 2;
>+}
>+
> int main()
> {
>   int (*p)() = foo_glob;
>   (void)p;
>+  p = bar_static;
>+  (void)p;
>   foo_static();
>   foo_prot();
>   foo_hidden();
>diff --git a/gold/testsuite/icf_safe_so_test.sh b/gold/testsuite/icf_safe_so_test.sh
>index 10f8782d1f5..4c253c55d20 100755
>--- a/gold/testsuite/icf_safe_so_test.sh
>+++ b/gold/testsuite/icf_safe_so_test.sh
>@@ -83,7 +83,7 @@ END {
>
> arch_specific_safe_fold()
> {
>-    if grep -q -e "Intel 80386" -e "ARM" -e "PowerPC" $1;
>+    if grep -q -e "Advanced Micro Devices X86-64" -e "Intel 80386" -e "ARM" -e "PowerPC" $1;
>     then
> 	shift
> 	shift
>diff --git a/gold/x86_64.cc b/gold/x86_64.cc
>index bacd89f2eff..378bac16f78 100644
>--- a/gold/x86_64.cc
>+++ b/gold/x86_64.cc
>@@ -4022,12 +4022,6 @@ Target_x86_64<size>::Scan::local_reloc_may_be_function_pointer(
>   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.
>-  if (parameters->options().shared())
>-    return true;
>-
>   return possible_function_pointer_reloc(src_obj, src_indx,
>                                          reloc.get_r_offset(), r_type);
> }
>@@ -4047,16 +4041,8 @@ Target_x86_64<size>::Scan::global_reloc_may_be_function_pointer(
>   Output_section* ,
>   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.
>-  if (parameters->options().shared()
>-      && (gsym->visibility() == elfcpp::STV_INTERNAL
>-	  || gsym->visibility() == elfcpp::STV_PROTECTED
>-	  || gsym->visibility() == elfcpp::STV_HIDDEN))
>-    return true;
>-
>+  Symbol*)
>+{
>   return possible_function_pointer_reloc(src_obj, src_indx,
>                                          reloc.get_r_offset(), r_type);
> }
>-- 
>2.26.2
>

Thanks for dropping special cases! The -shared special cases are
questionable.


I think "Improved support for --icf=safe when used with -pie." is not
needed nowadays since function calls use R_X86_64_PLT32.
(Solaris does not like R_X86_64_PLT32 and as a result they will not get
ICF benefits.)