PATCH: Fix alignment of common symbol

PATCH: Fix alignment of common symbol

[H. J. Lu]

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

When there is a alignment differnce between common/normal symbols or
common symbols, ld doesn't check alignment. Here is a testcase:

gcc -O -g -c main.c
gcc -O -g -c foo.c
gcc -o foo1 -B./ main.o foo.o
/usr/bin/ld: Warning: size of symbol `foo' changed from 2 to 21 in foo.o
gcc -o foo2 -B./ foo.o main.o
for f in foo1 foo2; do echo "Running: $f"; ./$f; \
  if [ $? != 0 ]; then echo Failed; fi; done
Running: foo1
0x8049421: 1
0x8049536: 2
Running: foo2
0x8049421: 1
0x8049550: 16

I also include a patch.


H.J.

[-- Attachment #2: bfd-common.patch --]
[-- Type: text/plain, Size: 2047 bytes --]

2003-04-10  H.J. Lu <hjl@gnu.org>

	* elflink.h (elf_link_add_object_symbols): Properly handle
	alignment for common symbols.

--- bfd/elflink.h.common	2003-04-08 10:56:49.000000000 -0700
+++ bfd/elflink.h	2003-04-10 11:14:24.000000000 -0700
@@ -1999,6 +1999,8 @@ elf_link_add_object_symbols (abfd, info)
 		 is specified and no other alignments have been specified.  */
 	      || (isym->st_value == 1 && old_alignment == 0))
 	    h->root.u.c.p->alignment_power = align;
+	  else
+	    h->root.u.c.p->alignment_power = old_alignment;
 	}
 
       if (info->hash->creator->flavour == bfd_target_elf_flavour)
@@ -2007,6 +2009,42 @@ elf_link_add_object_symbols (abfd, info)
 	  bfd_boolean dynsym;
 	  int new_flag;
 
+	  /* Check the alignment when a common symbol is involved. It
+	     can happen when a common symbol is overriden by a normal
+	     definition or a common symbol is ignored due to the old
+	     normal definition. We need to make sure the maximum
+	     alignment is maintained.  */
+	  if ((old_alignment || isym->st_shndx == SHN_COMMON)
+	      && h->root.type != bfd_link_hash_common)
+	    {
+	      unsigned int common_align, normal_align, symbol_align;
+	      
+	      symbol_align = ffs (h->root.u.def.value) - 1;
+	      if ((h->root.u.def.section->owner->flags & DYNAMIC) == 0)
+		{
+		  normal_align = h->root.u.def.section->alignment_power;
+		  if (normal_align > symbol_align)
+		    normal_align = symbol_align;
+		}
+	      else
+		normal_align = symbol_align;
+
+	      if (old_alignment)
+		common_align = old_alignment;
+	      else
+		common_align = bfd_log2 (isym->st_value);
+
+	      if (normal_align < common_align)
+		{
+		  (*_bfd_error_handler)
+		    (_("alignment of symbol `%s' changed from %u to %u in %s"),
+		     name, 1 << common_align, 1 << normal_align,
+		     bfd_archive_filename (abfd));
+		  bfd_set_error (bfd_error_bad_value);
+		  goto error_free_vers;
+		}
+	    }
+
 	  /* Remember the symbol size and type.  */
 	  if (isym->st_size != 0
 	      && (definition || h->size == 0))

[-- Attachment #3: bug.tar.gz --]
[-- Type: application/x-gzip, Size: 662 bytes --]

Re: PATCH: Fix alignment of common symbol

[H. J. Lu]

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

On Thu, Apr 10, 2003 at 11:26:14AM -0700, H. J. Lu wrote:
> When there is a alignment differnce between common/normal symbols or
> common symbols, ld doesn't check alignment. Here is a testcase:
> 
> gcc -O -g -c main.c
> gcc -O -g -c foo.c
> gcc -o foo1 -B./ main.o foo.o
> /usr/bin/ld: Warning: size of symbol `foo' changed from 2 to 21 in foo.o
> gcc -o foo2 -B./ foo.o main.o
> for f in foo1 foo2; do echo "Running: $f"; ./$f; \
>   if [ $? != 0 ]; then echo Failed; fi; done
> Running: foo1
> 0x8049421: 1
> 0x8049536: 2
> Running: foo2
> 0x8049421: 1
> 0x8049550: 16
> 
> I also include a patch.
> 

I took a look at Sun's linker doc. It says alignment change will
generate a warning, not a fatal error. Here is the new patch.


H.J.

[-- Attachment #2: bfd-common.patch --]
[-- Type: text/plain, Size: 4039 bytes --]

2003-04-11  H.J. Lu <hjl@gnu.org>

	* elflink.h (elf_link_add_object_symbols): Maintain maximum
	alignment for common symbols. Warn reducing alignment for
	common symbols. Report old filename when symbol size changes.

--- bfd/elflink.h.common	2003-04-10 16:22:43.000000000 -0700
+++ bfd/elflink.h	2003-04-11 08:58:53.000000000 -0700
@@ -1714,6 +1714,7 @@ elf_link_add_object_symbols (abfd, info)
       bfd_boolean new_weakdef;
       unsigned int old_alignment;
       bfd_boolean override;
+      bfd *old_bfd;
 
       override = FALSE;
 
@@ -1818,6 +1819,7 @@ elf_link_add_object_symbols (abfd, info)
       size_change_ok = FALSE;
       type_change_ok = get_elf_backend_data (abfd)->type_change_ok;
       old_alignment = 0;
+      old_bfd = NULL;
       if (info->hash->creator->flavour == bfd_target_elf_flavour)
 	{
 	  Elf_Internal_Versym iver;
@@ -1940,9 +1942,23 @@ elf_link_add_object_symbols (abfd, info)
 	     that we don't reduce the alignment later on.  We can't
 	     check later, because _bfd_generic_link_add_one_symbol
 	     will set a default for the alignment which we want to
-	     override.  */
-	  if (h->root.type == bfd_link_hash_common)
-	    old_alignment = h->root.u.c.p->alignment_power;
+	     override. We also remember the old bfd where the existing
+	     definition comes from.  */
+	  switch (h->root.type)
+	    {
+	    default:
+	      break;
+
+	    case bfd_link_hash_defined:
+	    case bfd_link_hash_defweak:
+	      old_bfd = h->root.u.def.section->owner;
+	      break;
+	    
+	    case bfd_link_hash_common:
+	      old_bfd = h->root.u.c.p->section->owner;
+	      old_alignment = h->root.u.c.p->alignment_power;
+	      break;
+	    }
 
 	  if (elf_tdata (abfd)->verdef != NULL
 	      && ! override
@@ -1999,6 +2015,8 @@ elf_link_add_object_symbols (abfd, info)
 		 is specified and no other alignments have been specified.  */
 	      || (isym->st_value == 1 && old_alignment == 0))
 	    h->root.u.c.p->alignment_power = align;
+	  else
+	    h->root.u.c.p->alignment_power = old_alignment;
 	}
 
       if (info->hash->creator->flavour == bfd_target_elf_flavour)
@@ -2007,15 +2025,50 @@ elf_link_add_object_symbols (abfd, info)
 	  bfd_boolean dynsym;
 	  int new_flag;
 
+	  /* Check the alignment when a common symbol is involved. It
+	     can happen when a common symbol is overriden by a normal
+	     definition or a common symbol is ignored due to the old
+	     normal definition. We need to make sure the maximum
+	     alignment is maintained.  */
+	  if ((old_alignment || isym->st_shndx == SHN_COMMON)
+	      && h->root.type != bfd_link_hash_common)
+	    {
+	      unsigned int common_align, normal_align, symbol_align;
+	      
+	      symbol_align = ffs (h->root.u.def.value) - 1;
+	      if ((h->root.u.def.section->owner->flags & DYNAMIC) == 0)
+		{
+		  normal_align = h->root.u.def.section->alignment_power;
+		  if (normal_align > symbol_align)
+		    normal_align = symbol_align;
+		}
+	      else
+		normal_align = symbol_align;
+
+	      if (old_alignment)
+		common_align = old_alignment;
+	      else
+		common_align = bfd_log2 (isym->st_value);
+
+	      if (normal_align < common_align)
+		(*_bfd_error_handler)
+		  (_("Warning: alignment %u of symbol `%s' in %s is smaller than %u in %s"),
+		   1 << normal_align, name,
+		   bfd_archive_filename (old_bfd),
+		   1 << common_align, bfd_archive_filename (abfd));
+	    }
+
 	  /* Remember the symbol size and type.  */
 	  if (isym->st_size != 0
 	      && (definition || h->size == 0))
 	    {
 	      if (h->size != 0 && h->size != isym->st_size && ! size_change_ok)
 		(*_bfd_error_handler)
-		  (_("Warning: size of symbol `%s' changed from %lu to %lu in %s"),
+		  (_("Warning: size of symbol `%s' changed from %lu in %s to %lu in %s"),
 		   name, (unsigned long) h->size,
-		   (unsigned long) isym->st_size, bfd_archive_filename (abfd));
+		   bfd_archive_filename (old_bfd),
+		   (unsigned long) isym->st_size,
+		   bfd_archive_filename (abfd));
 
 	      h->size = isym->st_size;
 	    }

Re: PATCH: Fix alignment of common symbol

[Nick Clifton]

Hi H.J.

  Thanks for catching this.  Do you think that it is worth developing
  a linker test case to check for this ?

> 2003-04-11  H.J. Lu <hjl@gnu.org>
> 
> 	* elflink.h (elf_link_add_object_symbols): Maintain maximum
> 	alignment for common symbols. Warn reducing alignment for
> 	common symbols. Report old filename when symbol size changes.

  Approved and applied.

Cheers
        Nick

Re: PATCH: Fix alignment of common symbol

[H. J. Lu]

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

On Mon, Apr 14, 2003 at 12:06:56PM +0100, Nick Clifton wrote:
> Hi H.J.
> 
>   Thanks for catching this.  Do you think that it is worth developing
>   a linker test case to check for this ?

That is a good idea. Here it is. It caught a bug in my patch :-).


H.J.

[-- Attachment #2: binutils-common-test.patch --]
[-- Type: text/plain, Size: 5528 bytes --]

bfd/

2003-04-14  H.J. Lu <hjl@gnu.org>

	* elflink.h (elf_link_add_object_symbols): Properly report
	filename for alignment reduction.

ld/testsuite/

	* ld-elfcom/elfcom.exp: New file.
	* ld-elfcom/common1a.c: New file.
	* ld-elfcom/common1b.c: New file.

2003-04-14  H.J. Lu <hjl@gnu.org>


--- binutils/bfd/elflink.h.common-test	2003-04-14 08:17:52.000000000 -0700
+++ binutils/bfd/elflink.h	2003-04-14 12:22:05.000000000 -0700
@@ -2036,6 +2036,7 @@ elf_link_add_object_symbols (abfd, info)
 	      && h->root.type != bfd_link_hash_common)
 	    {
 	      unsigned int common_align, normal_align, symbol_align;
+	      bfd *normal_bfd, *common_bfd;
 
 	      symbol_align = ffs (h->root.u.def.value) - 1;
 	      if ((h->root.u.def.section->owner->flags & DYNAMIC) == 0)
@@ -2048,16 +2049,25 @@ elf_link_add_object_symbols (abfd, info)
 		normal_align = symbol_align;
 
 	      if (old_alignment)
-		common_align = old_alignment;
+		{
+		  common_align = old_alignment;
+		  common_bfd = old_bfd;
+		  normal_bfd = abfd;
+		}
 	      else
-		common_align = bfd_log2 (isym->st_value);
+		{
+		  common_align = bfd_log2 (isym->st_value);
+		  common_bfd = abfd;
+		  normal_bfd = old_bfd;
+		}
 
 	      if (normal_align < common_align)
 		(*_bfd_error_handler)
 		  (_("Warning: alignment %u of symbol `%s' in %s is smaller than %u in %s"),
 		   1 << normal_align, name,
-		   bfd_archive_filename (old_bfd),
-		   1 << common_align, bfd_archive_filename (abfd));
+		   bfd_archive_filename (normal_bfd),
+		   1 << common_align,
+		   bfd_archive_filename (common_bfd));
 	    }
 
 	  /* Remember the symbol size and type.  */
--- binutils/ld/testsuite/ld-elfcom/common1a.c.common-test	2003-04-14 12:28:20.000000000 -0700
+++ binutils/ld/testsuite/ld-elfcom/common1a.c	2003-04-14 11:24:27.000000000 -0700
@@ -0,0 +1,2 @@
+char foo1 [2] __attribute__((aligned(64)));
+char foo2 [2] __attribute__((aligned(128)));
--- binutils/ld/testsuite/ld-elfcom/common1b.c.common-test	2003-04-14 12:28:20.000000000 -0700
+++ binutils/ld/testsuite/ld-elfcom/common1b.c	2003-04-14 11:24:33.000000000 -0700
@@ -0,0 +1,3 @@
+static char dummy1 = 'X';
+char foo1 [] = "Aligned at odd byte.";
+char foo2 [4];
--- binutils/ld/testsuite/ld-elfcom/elfcom.exp.common-test	2003-04-14 12:28:20.000000000 -0700
+++ binutils/ld/testsuite/ld-elfcom/elfcom.exp	2003-04-14 12:27:37.000000000 -0700
@@ -0,0 +1,92 @@
+# Expect script for commom symbol tests
+#   Copyright 2003 Free Software Foundation, Inc.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+#
+# Written by H.J. Lu (hjl@gnu.org)
+#
+
+# Make sure that ld correctly handles common symbols in ELF.
+
+# This test can only be run on ELF platforms.
+# Square bracket expressions seem to confuse istarget.
+if { ![istarget hppa*64*-*-hpux*] \
+     && ![istarget *-*-gnu] \
+     && ![istarget *-*-linux*] \
+     && ![istarget *-*-elf] } {
+    return
+}
+
+if { [istarget *-*-linux*aout*] \
+     || [istarget *-*-linux*oldld*] } {
+    return
+}
+
+proc dump_common1 { testname } {
+    global exec_output
+
+    send_log "readelf -s tmpdir/common1.o | grep foo\n"
+    catch "exec readelf -s tmpdir/common1.o | grep foo" exec_output
+    if { ![regexp "(\[ 	\]*)(\[0-9\]+):(\[ 	\]*)(\[0\]*)80(\[ 	\]+)4(\[ 	\]+)OBJECT(\[ 	\]+)GLOBAL(\[ 	\]+)DEFAULT(\[ 	\]+)COM(\[ 	\]+)foo2" $exec_output]
+ 	 || ![regexp "(\[ 	\]*)(\[0-9\]+):(\[ 	\]*)(\[0\]*)1(\[ 	\]+)21(\[ 	\]+)OBJECT(\[ 	\]+)GLOBAL(\[ 	\]+)DEFAULT(\[ 	\]+)(\[0-9\]+)(\[ 	\]+)foo1" $exec_output] } {
+	send_log "$exec_output\n"
+	verbose $exec_output
+	fail $testname
+	return 0
+    }
+
+    return 1
+}
+
+set test1 "size/aligment change of commom symbols"
+if { ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/common1a.c tmpdir/common1a.o]
+    || ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/common1b.c tmpdir/common1b.o] } {
+    unresolved $test1
+    return
+}
+
+global ld
+global link_output
+
+if { [ld_simple_link $ld tmpdir/common1.o "-r tmpdir/common1a.o tmpdir/common1b.o"] } {
+    unresolved $test1
+    return
+}
+
+if { ![regexp "Warning: alignment 1 of symbol \`foo1\' in tmpdir/common1b.o is smaller than 64 in tmpdir/common1a.o" $link_output]
+     || ![regexp "Warning: size of symbol \`foo1\' changed from 2 in tmpdir/common1a.o to 21 in tmpdir/common1b.o" $link_output] } {
+    fail "$test1 (warning 1)"
+} else {
+    pass "$test1 (warning 1)"
+}
+
+if { [dump_common1 "$test1 (change 1)"] } {
+    pass "$test1 (change 1)"
+}
+
+if { [ld_simple_link $ld tmpdir/common1.o "-r tmpdir/common1b.o tmpdir/common1a.o"] } {
+    unresolved $test1
+    return
+}
+
+if { ![regexp "Warning: alignment 1 of symbol \`foo1\' in tmpdir/common1b.o is smaller than 64 in tmpdir/common1a.o" $link_output] } {
+    fail "$test1 (warning 2)"
+} else {
+    pass "$test1 (warningi 2)"
+}
+
+if { [dump_common1 "$test1 (change 2)"] } {
+    pass "$test1 (change 2)"
+}

Re: PATCH: Fix alignment of common symbol

[Nick Clifton]

Hi H.J.

> bfd/
> 
> 2003-04-14  H.J. Lu <hjl@gnu.org>
> 
> 	* elflink.h (elf_link_add_object_symbols): Properly report
> 	filename for alignment reduction.
> 
> ld/testsuite/
> 
> 	* ld-elfcom/elfcom.exp: New file.
> 	* ld-elfcom/common1a.c: New file.
> 	* ld-elfcom/common1b.c: New file.

Thanks!  Approved and applied.

Cheers
        Nick