Re: Patch to recent ELF linker enhancement

Re: Patch to recent ELF linker enhancement

[Nick Clifton]

Hi Ian,

:    1999-12-13  Nick Clifton  <nickc@cygnus.com>
: 
: 	   * elflink.h (elf_link_add_archive_symbols): Do not bother
: 	   checking for defined commons if the element has already been
: 	   linked in.
: 
: I think this patch will force the linker to read in archive elements
: which it does not need to read in.

Err, presumably you mean the original patch, not my little bugfix
patch represneted by the ChangeLog entry above ?

: I also don't understand why it makes any difference.  Why is the
: included array not being set correctly in this case?  Why do we even
: reach the code that you are changing if the archive element has
: already been linked in?  What is the test case?

The test case is as follows:

  % cat foo1.c
extern void libcall ();
void _start (void) { libcall (); }

  % cat foo2a.c
extern int var;
void libcall (void) { var = 1; }

  % cat foo2b.c
int var;
void extra (void) { var = 2; }

  % gcc -c foo1.c foo2a.c foo2b.c
  % ar cr libfoo2.a foo2a.o foo2b.o
  % ld foo1.o -L. -\( -lfoo2 -\)
./libfoo2.a: could not read symbols: Bad value


However, you are right about my patch being unnecessary - it is the
elf_link_is_defined_archive_sytmbol() itself that is wrong.  I was
forgetting to check to see if the symbol was allocated to the common
section.  The patch below fixes this and prevents the above test case
from generating an error.

Cheers
	Nick

1999-12-13  Nick Clifton  <nickc@cygnus.com>

	* elflink.h (elf_link_is_defined_archive_symbol): Check to see
        if the symbol is in the common section.

Index: elflink.h
===================================================================
RCS file: /cvs/binutils/binutils/bfd/elflink.h,v
retrieving revision 1.39
diff -p -r1.39 elflink.h
*** elflink.h	1999/12/10 20:17:28	1.39
--- elflink.h	1999/12/14 01:54:14
*************** elf_link_is_defined_archive_symbol (abfd
*** 161,167 ****
  	{
  	  result =
  	    (ELF_ST_BIND (sym.st_info) == STB_GLOBAL)
! 	    && (sym.st_shndx != SHN_UNDEF);
  	  break;
  	}
      }
--- 161,169 ----
  	{
  	  result =
  	    (ELF_ST_BIND (sym.st_info) == STB_GLOBAL)
! 	    && (sym.st_shndx != SHN_UNDEF)
! 	    && (sym.st_shndx != SHN_COMMON)
! 	    ;
  	  break;
  	}
      }

Re: Patch to recent ELF linker enhancement

[Ian Lance Taylor]

   Date: Mon, 13 Dec 1999 17:56:38 -0800
   From: Nick Clifton <nickc@cygnus.com>

   :    1999-12-13  Nick Clifton  <nickc@cygnus.com>
   : 
   : 	   * elflink.h (elf_link_add_archive_symbols): Do not bother
   : 	   checking for defined commons if the element has already been
   : 	   linked in.
   : 
   : I think this patch will force the linker to read in archive elements
   : which it does not need to read in.

   Err, presumably you mean the original patch, not my little bugfix
   patch represneted by the ChangeLog entry above ?

No, I didn't.  If h->root.type is, say, bfd_link_hash_defined, then
your recent small patch would call _bfd_get_elt_at_filepos, unlike the
original code.

   However, you are right about my patch being unnecessary - it is the
   elf_link_is_defined_archive_sytmbol() itself that is wrong.  I was
   forgetting to check to see if the symbol was allocated to the common
   section.  The patch below fixes this and prevents the above test case
   from generating an error.

   1999-12-13  Nick Clifton  <nickc@cygnus.com>

	   * elflink.h (elf_link_is_defined_archive_symbol): Check to see
	   if the symbol is in the common section.

Looks reasonable.

Some day we will have to handle processor specific STB_* and SHN_*
values.  For example, for MIPS ELF a symbol in section
SHN_MIPS_SCOMMON is a common symbol.  Perhaps we can use the
backend_symbol_processing hook somehow.

Ian

Re: Patch to recent ELF linker enhancement

[Ian Lance Taylor]

   Date: Mon, 13 Dec 1999 17:09:46 -0800
   From: Nick Clifton <nickc@cygnus.com>

     Below is a patch to my recent change to the ELF linker to cope with
     commons defined in archives.  This patch prevents the linker from
     trying to link in the same archive element twice if it happens to
     contain the definition of a common symbol.

     OK to apply ?

   1999-12-13  Nick Clifton  <nickc@cygnus.com>

	   * elflink.h (elf_link_add_archive_symbols): Do not bother
	   checking for defined commons if the element has already been
	   linked in.

I think this patch will force the linker to read in archive elements
which it does not need to read in.

I also don't understand why it makes any difference.  Why is the
included array not being set correctly in this case?  Why do we even
reach the code that you are changing if the archive element has
already been linked in?  What is the test case?

Ian

Patch to recent ELF linker enhancement

[Nick Clifton]

Hi Guys,

  Below is a patch to my recent change to the ELF linker to cope with
  commons defined in archives.  This patch prevents the linker from
  trying to link in the same archive element twice if it happens to
  contain the definition of a common symbol.

  OK to apply ?

Cheers
	Nick


1999-12-13  Nick Clifton  <nickc@cygnus.com>

	* elflink.h (elf_link_add_archive_symbols): Do not bother
	checking for defined commons if the element has already been
	linked in.

Index: elflink.h
===================================================================
RCS file: /cvs/cvsfiles/devo/bfd/elflink.h,v
retrieving revision 1.140
diff -p -r1.140 elflink.h
*** elflink.h	1999/12/10 12:18:52	1.140
- --- elflink.h	1999/12/14 00:54:06
*************** elf_link_add_archive_symbols (abfd, info
*** 291,296 ****
- --- 291,298 ----
  	  if (h == NULL)
  	    continue;
  
+ 	  element = _bfd_get_elt_at_filepos (abfd, symdef->file_offset);
+ 	  
  	  if (h->root.type == bfd_link_hash_common)
  	    {
  	      /* We currently have a common symbol.  The archive map contains
*************** elf_link_add_archive_symbols (abfd, info
*** 305,311 ****
  		 map alone.  Instead we must read in the element's symbol
  		 table and check that to see what kind of symbol definition
  		 this is.  */
! 	      if (! elf_link_is_defined_archive_symbol (abfd, symdef))
  		continue;
  	    }
  	  else if (h->root.type != bfd_link_hash_undefined)
- --- 307,314 ----
  		 map alone.  Instead we must read in the element's symbol
  		 table and check that to see what kind of symbol definition
  		 this is.  */
! 	      if ((element && element->archive_pass != 0)
! 		  || ! elf_link_is_defined_archive_symbol (abfd, symdef))
  		continue;
  	    }
  	  else if (h->root.type != bfd_link_hash_undefined)
*************** elf_link_add_archive_symbols (abfd, info
*** 316,323 ****
  	    }
  
  	  /* We need to include this archive member.  */
- - 
- - 	  element = _bfd_get_elt_at_filepos (abfd, symdef->file_offset);
  	  if (element == (bfd *) NULL)
  	    goto error_return;
  
- --- 319,324 ----