[PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[H.J. Lu]

Linker should never generate dynamic relocations for relocations in
non-SEC_ALLOC section which has no impact on run-time behavior.  Such
relocations should be resolved to 0.

	PR ld/26688
	* elf32-bfin.c (bfinfdpic_relocate_section): Skip non SEC_ALLOC
	section for R_BFIN_FUNCDESC.
---
 bfd/elf32-bfin.c | 235 ++++++++++++++++++++++++-----------------------
 1 file changed, 118 insertions(+), 117 deletions(-)

diff --git a/bfd/elf32-bfin.c b/bfd/elf32-bfin.c
index 7c926b8a96e..401683c5367 100644
--- a/bfd/elf32-bfin.c
+++ b/bfd/elf32-bfin.c
@@ -2727,129 +2727,130 @@ bfinfdpic_relocate_section (bfd * output_bfd,
 	  break;
 
 	case R_BFIN_FUNCDESC:
-	  {
-	    int dynindx;
-	    bfd_vma addend = rel->r_addend;
-
-	    if (! (h && h->root.type == bfd_link_hash_undefweak
-		   && BFINFDPIC_SYM_LOCAL (info, h)))
-	      {
-		/* If the symbol is dynamic and there may be dynamic
-		   symbol resolution because we are or are linked with a
-		   shared library, emit a FUNCDESC relocation such that
-		   the dynamic linker will allocate the function
-		   descriptor.  If the symbol needs a non-local function
-		   descriptor but binds locally (e.g., its visibility is
-		   protected, emit a dynamic relocation decayed to
-		   section+offset.  */
-		if (h && ! BFINFDPIC_FUNCDESC_LOCAL (info, h)
-		    && BFINFDPIC_SYM_LOCAL (info, h)
-		    && !bfd_link_pde (info))
-		  {
-		    dynindx = elf_section_data (h->root.u.def.section
-						->output_section)->dynindx;
-		    addend += h->root.u.def.section->output_offset
-		      + h->root.u.def.value;
-		  }
-		else if (h && ! BFINFDPIC_FUNCDESC_LOCAL (info, h))
-		  {
-		    if (addend)
-		      {
-			info->callbacks->warning
-			  (info, _("R_BFIN_FUNCDESC references dynamic symbol with nonzero addend"),
-			   name, input_bfd, input_section, rel->r_offset);
-			return FALSE;
-		      }
-		    dynindx = h->dynindx;
-		  }
-		else
-		  {
-		    /* Otherwise, we know we have a private function
-		       descriptor, so reference it directly.  */
-		    BFD_ASSERT (picrel->privfd);
-		    r_type = R_BFIN_BYTE4_DATA;
-		    dynindx = elf_section_data (bfinfdpic_got_section (info)
-						->output_section)->dynindx;
-		    addend = bfinfdpic_got_section (info)->output_offset
-		      + bfinfdpic_got_initial_offset (info)
-		      + picrel->fd_entry;
-		  }
-
-		/* If there is room for dynamic symbol resolution, emit
-		   the dynamic relocation.  However, if we're linking an
-		   executable at a fixed location, we won't have emitted a
-		   dynamic symbol entry for the got section, so idx will
-		   be zero, which means we can and should compute the
-		   address of the private descriptor ourselves.  */
-		if (bfd_link_pde (info)
-		    && (!h || BFINFDPIC_FUNCDESC_LOCAL (info, h)))
-		  {
-		    bfd_vma offset;
-
-		    addend += bfinfdpic_got_section (info)->output_section->vma;
-		    if ((bfd_section_flags (input_section->output_section)
-			 & (SEC_ALLOC | SEC_LOAD)) == (SEC_ALLOC | SEC_LOAD))
-		      {
-			if (_bfinfdpic_osec_readonly_p (output_bfd,
-						       input_section
-						       ->output_section))
-			  {
-			    info->callbacks->warning
-			      (info,
-			       _("cannot emit fixups in read-only section"),
-			       name, input_bfd, input_section, rel->r_offset);
-			    return FALSE;
-			  }
+	  if ((input_section->flags & SEC_ALLOC) != 0)
+	    {
+	      int dynindx;
+	      bfd_vma addend = rel->r_addend;
 
-			offset = _bfd_elf_section_offset
-			  (output_bfd, info,
-			   input_section, rel->r_offset);
+	      if (! (h && h->root.type == bfd_link_hash_undefweak
+		     && BFINFDPIC_SYM_LOCAL (info, h)))
+		{
+		  /* If the symbol is dynamic and there may be dynamic
+		     symbol resolution because we are or are linked with a
+		     shared library, emit a FUNCDESC relocation such that
+		     the dynamic linker will allocate the function
+		     descriptor.  If the symbol needs a non-local function
+		     descriptor but binds locally (e.g., its visibility is
+		     protected, emit a dynamic relocation decayed to
+		     section+offset.  */
+		  if (h && ! BFINFDPIC_FUNCDESC_LOCAL (info, h)
+		      && BFINFDPIC_SYM_LOCAL (info, h)
+		      && !bfd_link_pde (info))
+		    {
+		      dynindx = elf_section_data (h->root.u.def.section
+						  ->output_section)->dynindx;
+		      addend += h->root.u.def.section->output_offset
+			+ h->root.u.def.value;
+		    }
+		  else if (h && ! BFINFDPIC_FUNCDESC_LOCAL (info, h))
+		    {
+		      if (addend)
+			{
+			  info->callbacks->warning
+			    (info, _("R_BFIN_FUNCDESC references dynamic symbol with nonzero addend"),
+			     name, input_bfd, input_section, rel->r_offset);
+			  return FALSE;
+			}
+		      dynindx = h->dynindx;
+		    }
+		  else
+		    {
+		      /* Otherwise, we know we have a private function
+			 descriptor, so reference it directly.  */
+		      BFD_ASSERT (picrel->privfd);
+		      r_type = R_BFIN_BYTE4_DATA;
+		      dynindx = elf_section_data (bfinfdpic_got_section (info)
+						  ->output_section)->dynindx;
+		      addend = bfinfdpic_got_section (info)->output_offset
+			+ bfinfdpic_got_initial_offset (info)
+			+ picrel->fd_entry;
+		    }
 
-			if (offset != (bfd_vma)-1)
-			  _bfinfdpic_add_rofixup (output_bfd,
-						  bfinfdpic_gotfixup_section
-						  (info),
+		  /* If there is room for dynamic symbol resolution, emit
+		     the dynamic relocation.  However, if we're linking an
+		     executable at a fixed location, we won't have emitted a
+		     dynamic symbol entry for the got section, so idx will
+		     be zero, which means we can and should compute the
+		     address of the private descriptor ourselves.  */
+		  if (bfd_link_pde (info)
+		      && (!h || BFINFDPIC_FUNCDESC_LOCAL (info, h)))
+		    {
+		      bfd_vma offset;
+
+		      addend += bfinfdpic_got_section (info)->output_section->vma;
+		      if ((bfd_section_flags (input_section->output_section)
+			   & (SEC_ALLOC | SEC_LOAD)) == (SEC_ALLOC | SEC_LOAD))
+			{
+			  if (_bfinfdpic_osec_readonly_p (output_bfd,
+							  input_section
+							  ->output_section))
+			    {
+			      info->callbacks->warning
+				(info,
+				 _("cannot emit fixups in read-only section"),
+				 name, input_bfd, input_section, rel->r_offset);
+			      return FALSE;
+			    }
+
+			  offset = _bfd_elf_section_offset
+			    (output_bfd, info,
+			     input_section, rel->r_offset);
+
+			  if (offset != (bfd_vma)-1)
+			    _bfinfdpic_add_rofixup (output_bfd,
+						    bfinfdpic_gotfixup_section
+						    (info),
+						    offset + input_section
+						    ->output_section->vma
+						    + input_section->output_offset,
+						    picrel);
+			}
+		    }
+		  else if ((bfd_section_flags (input_section->output_section)
+			    & (SEC_ALLOC | SEC_LOAD)) == (SEC_ALLOC | SEC_LOAD))
+		    {
+		      bfd_vma offset;
+
+		      if (_bfinfdpic_osec_readonly_p (output_bfd,
+						      input_section
+						      ->output_section))
+			{
+			  info->callbacks->warning
+			    (info,
+			     _("cannot emit dynamic relocations in read-only section"),
+			     name, input_bfd, input_section, rel->r_offset);
+			  return FALSE;
+			}
+		      offset = _bfd_elf_section_offset (output_bfd, info,
+							input_section, rel->r_offset);
+
+		      if (offset != (bfd_vma)-1)
+			_bfinfdpic_add_dyn_reloc (output_bfd,
+						  bfinfdpic_gotrel_section (info),
 						  offset + input_section
 						  ->output_section->vma
 						  + input_section->output_offset,
-						  picrel);
-		      }
-		  }
-		else if ((bfd_section_flags (input_section->output_section)
-			  & (SEC_ALLOC | SEC_LOAD)) == (SEC_ALLOC | SEC_LOAD))
-		  {
-		    bfd_vma offset;
-
-		    if (_bfinfdpic_osec_readonly_p (output_bfd,
-						   input_section
-						   ->output_section))
-		      {
-			info->callbacks->warning
-			  (info,
-			   _("cannot emit dynamic relocations in read-only section"),
-			   name, input_bfd, input_section, rel->r_offset);
-			return FALSE;
-		      }
-		    offset = _bfd_elf_section_offset (output_bfd, info,
-						      input_section, rel->r_offset);
-
-		    if (offset != (bfd_vma)-1)
-		      _bfinfdpic_add_dyn_reloc (output_bfd,
-						bfinfdpic_gotrel_section (info),
-						offset + input_section
-						->output_section->vma
-						+ input_section->output_offset,
-						r_type,
-						dynindx, addend, picrel);
-		  }
-		else
-		  addend += bfinfdpic_got_section (info)->output_section->vma;
-	      }
+						  r_type,
+						  dynindx, addend, picrel);
+		    }
+		  else
+		    addend += bfinfdpic_got_section (info)->output_section->vma;
+		}
 
-	    /* We want the addend in-place because dynamic
-	       relocations are REL.  Setting relocation to it should
-	       arrange for it to be installed.  */
-	    relocation = addend - rel->r_addend;
+	      /* We want the addend in-place because dynamic
+		 relocations are REL.  Setting relocation to it should
+		 arrange for it to be installed.  */
+	      relocation = addend - rel->r_addend;
 	  }
 	  check_segment[0] = check_segment[1] = got_segment;
 	  break;
-- 
2.29.2

Re: [PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[Mike Frysinger]

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

On 13 Jan 2021 04:41, H.J. Lu via Binutils wrote:
> Linker should never generate dynamic relocations for relocations in
> non-SEC_ALLOC section which has no impact on run-time behavior.  Such
> relocations should be resolved to 0.

does elf32-frv.c need the same fix ?  kind of looks like it.

otherwise, lgtm, thanks
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[H.J. Lu]

On Wed, Jan 13, 2021 at 10:00 PM Mike Frysinger <vapier@gentoo.org> wrote:
>
> On 13 Jan 2021 04:41, H.J. Lu via Binutils wrote:
> > Linker should never generate dynamic relocations for relocations in
> > non-SEC_ALLOC section which has no impact on run-time behavior.  Such
> > relocations should be resolved to 0.
>
> does elf32-frv.c need the same fix ?  kind of looks like it.

Yes.  FRV maintainers need to take a look.   There are many code/bug
duplications in FDPIC support.   They should be merged.

> otherwise, lgtm, thanks
> -mike

Fixed on master.  Nick, is it OK for 2.36 branch?

-- 
H.J.

Re: [PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[Nick Clifton]

Hi H.J.

> Fixed on master.  Nick, is it OK for 2.36 branch?

Yes - please apply.

Cheers
   Nick

Update my email address (was: Re: [PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC)

[Alexandre Oliva]

On Jan 14, 2021, "H.J. Lu" <hjl.tools@gmail.com> wrote:

> On Wed, Jan 13, 2021 at 10:00 PM Mike Frysinger <vapier@gentoo.org> wrote:
>> 
>> On 13 Jan 2021 04:41, H.J. Lu via Binutils wrote:
>> > Linker should never generate dynamic relocations for relocations in
>> > non-SEC_ALLOC section which has no impact on run-time behavior.  Such
>> > relocations should be resolved to 0.
>> 
>> does elf32-frv.c need the same fix ?  kind of looks like it.

> Yes.  FRV maintainers need to take a look.

I haven't had access to FR-V (or SH, or MN10300) hardware for a while,
but I think I can still take care of this one.  Thanks,

This message made me realize I had failed to update my email address in
binutils/MAINTAINERS.  I'm checking this in now.


Update my email address (long overdue!)

From: Alexandre Oliva <oliva@gnu.org>

for  binutils/ChangeLog

	* MAINTAINERS: Update my email address.
---
 binutils/MAINTAINERS |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/binutils/MAINTAINERS b/binutils/MAINTAINERS
index af9a08e5720c5..b7a3376b714f4 100644
--- a/binutils/MAINTAINERS
+++ b/binutils/MAINTAINERS
@@ -84,7 +84,7 @@ responsibility among the other maintainers.
   EPIPHANY	   Joern Rennecke <joern.rennecke@embecosm.com>
   FR30		   Nick Clifton <nickc@redhat.com>
   FRV		   Nick Clifton <nickc@redhat.com>
-  FRV		   Alexandre Oliva <aoliva@redhat.com>
+  FRV		   Alexandre Oliva <aoliva@sourceware.org>
   GOLD		   Ian Lance Taylor <iant@google.com>
   GOLD		   Cary Coutant <ccoutant@gmail.com>
   H8300		   Prafulla Thakare <prafulla.thakare@kpitcummins.com>
@@ -109,7 +109,7 @@ responsibility among the other maintainers.
   MIPS		   Chenghua Xu <paul.hua.gm@gmail.com>
   MIPS I-IV	   Maciej W. Rozycki <macro@linux-mips.org>
   MMIX		   Hans-Peter Nilsson <hp@bitrange.com>
-  MN10300	   Alexandre Oliva <aoliva@redhat.com>
+  MN10300	   Alexandre Oliva <aoliva@sourceware.org>
   Moxie		   Anthony Green <green@moxielogic.com>
   MSP430	   Dmitry Diky <diwil@spec.ru>
   NDS32		   Kuan-Lin Chen <kuanlinchentw@gmail.com>
@@ -133,7 +133,7 @@ responsibility among the other maintainers.
   S12Z		   John Darrington <john@darrington.wattle.id.au>
   s390, s390x	   Martin Schwidefsky <schwidefsky@de.ibm.com>
   s390, s390x	   Andreas Krebbel <krebbel@linux.vnet.ibm.com>
-  SH		   Alexandre Oliva <aoliva@redhat.com>
+  SH		   Alexandre Oliva <aoliva@sourceware.org>
   SPARC		   David S. Miller <davem@davemloft.net>
   SPARC		   Jose E. Marchesi <jose.marchesi@oracle.com>
   SPU		   Alan Modra <amodra@gmail.com>


-- 
Alexandre Oliva, happy hacker  https://FSFLA.org/blogs/lxo/
   Free Software Activist         GNU Toolchain Engineer
        Vim, Vi, Voltei pro Emacs -- GNUlius Caesar

Re: [PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[Mike Frysinger]

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

On 14 Jan 2021 05:28, H.J. Lu via Binutils wrote:
> On Wed, Jan 13, 2021 at 10:00 PM Mike Frysinger wrote:
> > On 13 Jan 2021 04:41, H.J. Lu via Binutils wrote:
> > > Linker should never generate dynamic relocations for relocations in
> > > non-SEC_ALLOC section which has no impact on run-time behavior.  Such
> > > relocations should be resolved to 0.
> >
> > does elf32-frv.c need the same fix ?  kind of looks like it.
> 
> Yes.  FRV maintainers need to take a look.

well, didn't you merge this ? :)
	frv: Don't generate dynamic relocation for non SEC_ALLOC sections

> There are many code/bug duplications in FDPIC support.
> They should be merged.

to be fair, Blackfin copied FRV :).  we owe a ton to FRV wrt FDPIC in the
toolchain & kernel.

i took a look a while ago about trying to factor things out at least in
gdb, but it got so invasive, and i didn't have FRV to verify against,
that i ended up shelving it.

i'd feel similarly about such large refactors in bfd.  not sure how to
even structure it.  would we create files like fdpic.c/fdpic32.c ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] bfin: Skip non SEC_ALLOC section for R_BFIN_FUNCDESC

[H.J. Lu]

On Thu, Jan 14, 2021 at 12:27 PM Mike Frysinger <vapier@gentoo.org> wrote:
>
> On 14 Jan 2021 05:28, H.J. Lu via Binutils wrote:
> > On Wed, Jan 13, 2021 at 10:00 PM Mike Frysinger wrote:
> > > On 13 Jan 2021 04:41, H.J. Lu via Binutils wrote:
> > > > Linker should never generate dynamic relocations for relocations in
> > > > non-SEC_ALLOC section which has no impact on run-time behavior.  Such
> > > > relocations should be resolved to 0.
> > >
> > > does elf32-frv.c need the same fix ?  kind of looks like it.
> >
> > Yes.  FRV maintainers need to take a look.
>
> well, didn't you merge this ? :)
>         frv: Don't generate dynamic relocation for non SEC_ALLOC sections
>
> > There are many code/bug duplications in FDPIC support.
> > They should be merged.
>
> to be fair, Blackfin copied FRV :).  we owe a ton to FRV wrt FDPIC in the
> toolchain & kernel.
>
> i took a look a while ago about trying to factor things out at least in
> gdb, but it got so invasive, and i didn't have FRV to verify against,
> that i ended up shelving it.
>
> i'd feel similarly about such large refactors in bfd.  not sure how to
> even structure it.  would we create files like fdpic.c/fdpic32.c ?
> -mike

Take a look at elfxx-x86.[ch] to see how I merged common parts
of i386 and x86-64.

-- 
H.J.