PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[H.J. Lu]

When discarding space for dynamic relocations allocated for IFUNC
symbols, we should also set plt.offset to -1.  Tested on Linux/ia32
and Linux/Intel64. OK to install?

Thanks.


H.J.
---
2009-07-30  H.J. Lu  <hongjiu.lu@intel.com>

	 PR ld/10433
	 * elf-ifunc.c (_bfd_elf_allocate_ifunc_dyn_relocs): Also set
	 plt.offset to -1 when discarding space for dynamic
	 relocations.

Index: bfd/elf-ifunc.c
===================================================================
--- bfd/elf-ifunc.c	(revision 6507)
+++ bfd/elf-ifunc.c	(working copy)
@@ -193,6 +193,7 @@ _bfd_elf_allocate_ifunc_dyn_relocs (stru
 	  || h->got.refcount > 0)
 	abort ();
       h->got.offset = (bfd_vma) -1;
+      h->plt.offset = (bfd_vma) -1;
       *head = NULL;
       return TRUE;
     }

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[Alan Modra]

On Thu, Jul 30, 2009 at 09:05:04PM -0700, H.J. Lu wrote:
> 	 PR ld/10433
> 	 * elf-ifunc.c (_bfd_elf_allocate_ifunc_dyn_relocs): Also set
> 	 plt.offset to -1 when discarding space for dynamic
> 	 relocations.
> 
> Index: bfd/elf-ifunc.c
> ===================================================================
> --- bfd/elf-ifunc.c	(revision 6507)
> +++ bfd/elf-ifunc.c	(working copy)
> @@ -193,6 +193,7 @@ _bfd_elf_allocate_ifunc_dyn_relocs (stru
>  	  || h->got.refcount > 0)
>  	abort ();
>        h->got.offset = (bfd_vma) -1;
> +      h->plt.offset = (bfd_vma) -1;
>        *head = NULL;
>        return TRUE;
>      }

These should probably be

      h->got = htab->init_got_offset;
      h->plt = htab->init_plt_offset;

OK with that change.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[H.J. Lu]

On Mon, Aug 3, 2009 at 7:35 AM, Alan Modra<amodra@bigpond.net.au> wrote:
> On Thu, Jul 30, 2009 at 09:05:04PM -0700, H.J. Lu wrote:
>>        PR ld/10433
>>        * elf-ifunc.c (_bfd_elf_allocate_ifunc_dyn_relocs): Also set
>>        plt.offset to -1 when discarding space for dynamic
>>        relocations.
>>
>> Index: bfd/elf-ifunc.c
>> ===================================================================
>> --- bfd/elf-ifunc.c   (revision 6507)
>> +++ bfd/elf-ifunc.c   (working copy)
>> @@ -193,6 +193,7 @@ _bfd_elf_allocate_ifunc_dyn_relocs (stru
>>         || h->got.refcount > 0)
>>       abort ();
>>        h->got.offset = (bfd_vma) -1;
>> +      h->plt.offset = (bfd_vma) -1;
>>        *head = NULL;
>>        return TRUE;
>>      }
>
> These should probably be
>
>      h->got = htab->init_got_offset;
>      h->plt = htab->init_plt_offset;
>
> OK with that change.
>

This is what I checked in.

Thanks.

-- 
H.J.
--
2009-08-03  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/10433
	* elf-ifunc.c (_bfd_elf_allocate_ifunc_dyn_relocs): Set got
	to htab->init_got_offset and plt to htab->init_plt_offset
	when discarding space for dynamic relocations.

Index: elf-ifunc.c
===================================================================
--- elf-ifunc.c	(revision 6507)
+++ elf-ifunc.c	(working copy)
@@ -185,6 +185,8 @@ _bfd_elf_allocate_ifunc_dyn_relocs (stru
       return FALSE;
     }

+  htab = elf_hash_table (info);
+
   /* Return and discard space for dynamic relocations against it if
      it is never referenced in a non-shared object.  */
   if (!h->ref_regular)
@@ -192,7 +194,8 @@ _bfd_elf_allocate_ifunc_dyn_relocs (stru
       if (h->plt.refcount > 0
 	  || h->got.refcount > 0)
 	abort ();
-      h->got.offset = (bfd_vma) -1;
+      h->got = htab->init_got_offset;
+      h->plt = htab->init_plt_offset;
       *head = NULL;
       return TRUE;
     }
@@ -203,8 +206,6 @@ _bfd_elf_allocate_ifunc_dyn_relocs (stru
   else
     sizeof_reloc = bed->s->sizeof_rel;

-  htab = elf_hash_table (info);
-
   /* When building a static executable, use .iplt, .igot.plt and
      .rel[a].iplt sections for STT_GNU_IFUNC symbols.  */
   if (htab->splt != NULL)

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[Matthias Klose]

On 22.07.2009 23:11, Mike Frysinger wrote:
> On Wednesday 22 July 2009 20:41:23 David Daney wrote:
>> H.J. Lu wrote:
>>> On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra wrote:
>>>> On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
>>>>> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
>>>>> I checked in this patch to fix it.
>>>>
>>>> You don't have global authority to commit patches.  I'd normally let
>>>> this sort of patch pass, but I quite clearly explained why I removed
>>>> your test of ref_regular before.  Please revert this patch and instead
>>>> fix it in your target finish_dynamic_symbol.
>>>
>>> I disagree. If ref_regular isn't set properly, IFUNC may not work
>>> properly at all. I will revert my patch and move it to the Linux
>>> binutils instead.
>>
>> Granted all GNU/Linux systems use your binutils fork, but
>> could you explain why the issue is important enough to fix in your
>> binutils fork, but doesn't merit fixing in the sourceware.org version?
>
> that's not really true.  we've switched Gentoo to the GNU releases by default
> (while still making the HJL versions available).  mips tended to be screwed up
> in the non-GNU releases, and havent found any real pressing need to use the
> HJL versions in general.  plus, it's a lot easier to stabilize on a stable
> release than one made against the latest development tree.
> -mike

I'm using the FSF releases/branches for Debian as well; currently using trunk 
updated to the same base that it used for the HJL releases until the next FSF 
release. This works well except for mips, which shoes regressions in 2.19 and 
trunk (PR10144), and in trunk (-fPIE broken, http://bugs.debian.org/532821).

Ubuntu does use a subset of the patches, which H.J. includes separately in the 
patches directory. AFAICS Fedora does do the same (at least reverting some patches).

   Matthias

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[Nick Clifton]

Hi H.J.

   First of all Alan is right.  You do not have the authority to commit 
patches to generic code without first gaining approval.  So please stop 
doing it.  Even if it is for a critical Linux system bug.

> Any backends which support IFUNC should set the ref_regular field
> properly for IFUNC symbols. Otherwise IFUNC symbols in static
> executables may not work properly. I don't see why the generic ELF
> linker shouldn't test the ref_regular field for IFUNC symbols.

Your argument is reasonable.  But, the decision has been made that 
ref_regular should be tested in the target backend code and not the 
generic code.  You may not like it, but you can live with it.  All 
projects involve compromises in various places and this is one of them.

Refusing to fix the GNU Binutils sources in the manner suggested however 
does not help the long term goal of providing free, effective binary 
tools to anyone who wants them.  You are a very active and helpful 
contributor the Binutils project so please do not let this one bug 
become a major issue.

Cheers
   Nick

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On Wednesday 22 July 2009 20:41:23 David Daney wrote:
> H.J. Lu wrote:
> > On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra wrote:
> >> On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
> >>> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
> >>> I checked in this patch to fix it.
> >>
> >> You don't have global authority to commit patches.  I'd normally let
> >> this sort of patch pass, but I quite clearly explained why I removed
> >> your test of ref_regular before.  Please revert this patch and instead
> >> fix it in your target finish_dynamic_symbol.
> >
> > I disagree. If ref_regular isn't set properly, IFUNC may not work
> > properly at all. I will revert my patch and move it to the Linux
> > binutils instead.
>
> Granted all GNU/Linux systems use your binutils fork, but
> could you explain why the issue is important enough to fix in your
> binutils fork, but doesn't merit fixing in the sourceware.org version?

that's not really true.  we've switched Gentoo to the GNU releases by default 
(while still making the HJL versions available).  mips tended to be screwed up 
in the non-GNU releases, and havent found any real pressing need to use the 
HJL versions in general.  plus, it's a lot easier to stabilize on a stable 
release than one made against the latest development tree.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[H.J. Lu]

On Wed, Jul 22, 2009 at 5:41 PM, David Daney<ddaney@caviumnetworks.com> wrote:
> H.J. Lu wrote:
>>
>> On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra<amodra@bigpond.net.au> wrote:
>>>
>>> On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
>>>>
>>>> Hi,
>>>>
>>>> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
>>>> I checked in this patch to fix it.
>>>
>>> You don't have global authority to commit patches.  I'd normally let
>>> this sort of patch pass, but I quite clearly explained why I removed
>>> your test of ref_regular before.  Please revert this patch and instead
>>> fix it in your target finish_dynamic_symbol.
>>>
>>
>> I disagree. If ref_regular isn't set properly, IFUNC may not work
>> properly at all. I will revert my patch and move it to the Linux
>> binutils instead.
>
> Granted all GNU/Linux systems use your binutils fork, but
> could you explain why the issue is important enough to fix in your binutils
> fork, but doesn't merit fixing in the sourceware.org version?
>

There is a disagreement on what the correct fix is. Anyone can apply
my fix on he GNU binutils if they want IFUNC symbols to work
properly in static executables.

-- 
H.J.

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[H.J. Lu]

On Wed, Jul 22, 2009 at 5:37 PM, Alan Modra<amodra@bigpond.net.au> wrote:
> On Wed, Jul 22, 2009 at 04:51:57PM -0700, H.J. Lu wrote:
>> On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra<amodra@bigpond.net.au> wrote:
>> > On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
>> >> Hi,
>> >>
>> >> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
>> >> I checked in this patch to fix it.
>> >
>> > You don't have global authority to commit patches.  I'd normally let
>> > this sort of patch pass, but I quite clearly explained why I removed
>> > your test of ref_regular before.  Please revert this patch and instead
>> > fix it in your target finish_dynamic_symbol.
>> >
>>
>> I disagree. If ref_regular isn't set properly, IFUNC may not work
>> properly at all.
>
> I don't entirely object to your use of ref_regular.  You ensure
> that ref_regular is set for all references in your backend code.
> However, most backends *don't* set ref_regular as you seem to expect.
> So you shouldn't use the flag in generic code.  Just use the flag in
> your backend code.

Any backends which support IFUNC should set the ref_regular field
properly for IFUNC symbols. Otherwise IFUNC symbols in static
executables may not work properly. I don't see why the generic ELF
linker shouldn't test the ref_regular field for IFUNC symbols.


-- 
H.J.

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[David Daney]

H.J. Lu wrote:
> On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra<amodra@bigpond.net.au> wrote:
>> On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
>>> Hi,
>>>
>>> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
>>> I checked in this patch to fix it.
>> You don't have global authority to commit patches.  I'd normally let
>> this sort of patch pass, but I quite clearly explained why I removed
>> your test of ref_regular before.  Please revert this patch and instead
>> fix it in your target finish_dynamic_symbol.
>>
> 
> I disagree. If ref_regular isn't set properly, IFUNC may not work
> properly at all. I will revert my patch and move it to the Linux
> binutils instead.

Granted all GNU/Linux systems use your binutils fork, but
could you explain why the issue is important enough to fix in your 
binutils fork, but doesn't merit fixing in the sourceware.org version?

Thanks,
David Daney

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[Alan Modra]

On Wed, Jul 22, 2009 at 04:51:57PM -0700, H.J. Lu wrote:
> On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra<amodra@bigpond.net.au> wrote:
> > On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
> >> Hi,
> >>
> >> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
> >> I checked in this patch to fix it.
> >
> > You don't have global authority to commit patches.  I'd normally let
> > this sort of patch pass, but I quite clearly explained why I removed
> > your test of ref_regular before.  Please revert this patch and instead
> > fix it in your target finish_dynamic_symbol.
> >
> 
> I disagree. If ref_regular isn't set properly, IFUNC may not work
> properly at all.

I don't entirely object to your use of ref_regular.  You ensure
that ref_regular is set for all references in your backend code.
However, most backends *don't* set ref_regular as you seem to expect.
So you shouldn't use the flag in generic code.  Just use the flag in
your backend code.

> I will revert my patch and move it to the Linux
> binutils instead.

If you are going to contribute to mainline binutils you also need to
fix the problem there too.  It's dead easy to do so.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[H.J. Lu]

On Wed, Jul 22, 2009 at 4:34 PM, Alan Modra<amodra@bigpond.net.au> wrote:
> On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
>> Hi,
>>
>> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
>> I checked in this patch to fix it.
>
> You don't have global authority to commit patches.  I'd normally let
> this sort of patch pass, but I quite clearly explained why I removed
> your test of ref_regular before.  Please revert this patch and instead
> fix it in your target finish_dynamic_symbol.
>

I disagree. If ref_regular isn't set properly, IFUNC may not work
properly at all. I will revert my patch and move it to the Linux
binutils instead.


-- 
H.J.

Re: PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[Alan Modra]

On Wed, Jul 22, 2009 at 02:32:27PM -0700, H.J. Lu wrote:
> Hi,
> 
> A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
> I checked in this patch to fix it.

You don't have global authority to commit patches.  I'd normally let
this sort of patch pass, but I quite clearly explained why I removed
your test of ref_regular before.  Please revert this patch and instead
fix it in your target finish_dynamic_symbol.

> +2009-07-22  H.J. Lu  <hongjiu.lu@intel.com>
> +
> +	 PR ld/10433
> +	 * elflink.c (elf_link_output_extsym): Special case ifunc syms
> +	 when ref_regular, not def_regular.
> +

-- 
Alan Modra
Australia Development Lab, IBM

PATCH: PR ld/10433: Latest ld fails to link ldconfig properly

[H.J. Lu]

Hi,

A STT_GNU_IFUNC symbol goes through PLT only if it is ever referenced.
I checked in this patch to fix it.


H.J.
--
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 6448)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2009-07-22  H.J. Lu  <hongjiu.lu@intel.com>
+
+	 PR ld/10433
+	 * elflink.c (elf_link_output_extsym): Special case ifunc syms
+	 when ref_regular, not def_regular.
+
 2009-07-21  H.J. Lu  <hongjiu.lu@intel.com>
 
 	 PR ld/10426
Index: elflink.c
===================================================================
--- elflink.c	(revision 6448)
+++ elflink.c	(working copy)
@@ -8660,9 +8660,10 @@ elf_link_output_extsym (struct elf_link_
      and also to finish up anything that needs to be done for this
      symbol.  FIXME: Not calling elf_backend_finish_dynamic_symbol for
      forced local syms when non-shared is due to a historical quirk.
-     STT_GNU_IFUNC symbol must go through PLT.  */
+     STT_GNU_IFUNC symbol must go through PLT only if it is ever
+     referenced.  */
   if ((h->type == STT_GNU_IFUNC
-       && h->def_regular
+       && h->ref_regular
        && !finfo->info->relocatable)
       || ((h->dynindx != -1
 	   || h->forced_local)