[PATCH] don't include symbols from S_ATTR_NO_TOC sections in mach-o canonical symtab output

[PATCH] don't include symbols from S_ATTR_NO_TOC sections in mach-o canonical symtab output

[Nathan Froyd]

Hi,

Generating static library archives with binutils and comparing them with
archives produced by Apple's tools reveals one significant difference:
those produced by Apple's tools are significantly smaller.  binutils
includes many ${SYMBOL_NAME}.eh symbols in the archive table of contents,
whereas Apple's tools do not.

This difference is due to the .eh symbols living in the .eh_frame section,
which is tagged with the Mach-O section flag S_ATTR_NO_TOC.  This flag
instructs the archiver to not include symbols from such a section in the
table of contents for an archive.

The patch below changes bfd_mach_o_canonicalize_symtab to respect this flag.

Tested on x86_64-apple-darwin11, no regressions.  OK to checkin?

-Nathan

	* mach-o.c (bfd_mach_o_canonicalize_symtab): Respect the
	BFD_MACH_O_S_ATTR_NO_TOC flag on symbol sections.
---
 bfd/mach-o.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 32e48ac..095afeb 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -630,7 +630,7 @@ bfd_mach_o_canonicalize_symtab (bfd *abfd, asymbol **alocation)
   bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
   long nsyms = bfd_mach_o_count_symbols (abfd);
   bfd_mach_o_symtab_command *sym = mdata->symtab;
-  unsigned long j;
+  unsigned long i, j;
 
   if (nsyms < 0)
     return nsyms;
@@ -651,12 +651,23 @@ bfd_mach_o_canonicalize_symtab (bfd *abfd, asymbol **alocation)
 
   BFD_ASSERT (sym->symbols != NULL);
 
-  for (j = 0; j < sym->nsyms; j++)
-    alocation[j] = &sym->symbols[j].symbol;
+  for (i = 0, j = 0; j < sym->nsyms; j++)
+    {
+      bfd_mach_o_asymbol *asym = &sym->symbols[j];
+      if (asym->n_sect != 0)
+	{
+	  bfd_mach_o_section *sect = mdata->sections[asym->n_sect-1];
+	  if (sect->flags & BFD_MACH_O_S_ATTR_NO_TOC)
+	    continue;
+	}
 
-  alocation[j] = NULL;
+      alocation[i] = &asym->symbol;
+      i++;
+    }
+
+  alocation[i] = NULL;
 
-  return nsyms;
+  return i;
 }
 
 /* Create synthetic symbols for indirect symbols.  */
-- 
1.7.9.5

[PATCH] implement correct alignment for Mach-O archive entries

[Nathan Froyd]

Hi,

Static archives for the Mach-O file format have an undocumented requirement:
archive entries must be aligned on four-byte boundaries.  The Mach-O file
format reference document:

https://developer.apple.com/library/mac/documentation/DeveloperTools/Conceptual/MachORuntime/Reference/reference.html

has no mention of this.  Apple's cctools sources suggest that this requirement
is only for archive entries that require long names (20 characters or a name
that contains spaces).  But Apple's linker (at least) appears to enforce this
requirement all the time, regardless of the particular characteristics of the
archive.  It is not clear to me whether this has changed over time, or whether
the requirement has always been there and is simply undocumented.

BFD already has code to align archive entries, so increasing that alignment
in certain cases is not too hard.  I considered placing this code in mach-o.c
and implementing target jump table methods for them, but it did not seem worth
it.  Suggestions on how to not call bfd_get_flavour numerous times in
archive.c welcome.  I don't feel too bad about it due to the check for
bfd_target_coff_flavour that is pre-existing.

Tested on x86_64-apple-darwin11, no regressions.  These changes also make
substantial static libraries work correctly with the Apple linker.
OK to checkin?

-Nathan

	* archive.c (pad_fileptr_for_entry): New function.
	(bfd_generic_openr_next_archived_file): Call it.
	(do_slurp_bsd_armap, do_slurp_coff_armp): Likewise.
	(bfd_write_armap): Likewise.  Align entries if we have a
	mach-o archive.
	(write_mach_o_entry_padding): New function.
	(_bfd_write_archive_contents): Call it if we have a mach-o
	archive.
---
 bfd/archive.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/bfd/archive.c b/bfd/archive.c
index 32b07a7..176fb9c 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -443,6 +443,19 @@ get_extended_arelt_filename (bfd *arch, const char *name, file_ptr *originp)
   return bfd_ardata (arch)->extended_names + table_index;
 }
 
+/* Pad PTR appropriately for ABFD.  */
+
+static file_ptr
+pad_fileptr_for_entry (bfd *abfd, file_ptr ptr)
+{
+  if (bfd_get_flavour (abfd) == bfd_target_mach_o_flavour)
+    ptr += ptr % 4;
+  else
+    ptr += ptr % 2;
+
+  return ptr;
+}
+
 /* This functions reads an arch header and returns an areltdata pointer, or
    NULL on error.
 
@@ -780,7 +793,7 @@ bfd_generic_openr_next_archived_file (bfd *archive, bfd *last_file)
       /* Pad to an even boundary...
 	 Note that last_file->origin can be odd in the case of
 	 BSD-4.4-style element with a long odd size.  */
-      filestart += filestart % 2;
+      filestart = pad_fileptr_for_entry (archive, filestart);
     }
 
   return _bfd_get_elt_at_filepos (archive, filestart);
@@ -943,9 +956,7 @@ do_slurp_bsd_armap (bfd *abfd)
       set->file_offset = H_GET_32 (abfd, rbase + BSD_SYMDEF_OFFSET_SIZE);
     }
 
-  ardata->first_file_filepos = bfd_tell (abfd);
-  /* Pad to an even boundary if you have to.  */
-  ardata->first_file_filepos += (ardata->first_file_filepos) % 2;
+  ardata->first_file_filepos = pad_fileptr_for_entry (abfd, bfd_tell (abfd));
   /* FIXME, we should provide some way to free raw_ardata when
      we are done using the strings from it.  For now, it seems
      to be allocated on an objalloc anyway...  */
@@ -1048,9 +1059,7 @@ do_slurp_coff_armap (bfd *abfd)
   *stringbase = 0;
 
   ardata->symdef_count = nsymz;
-  ardata->first_file_filepos = bfd_tell (abfd);
-  /* Pad to an even boundary if you have to.  */
-  ardata->first_file_filepos += (ardata->first_file_filepos) % 2;
+  ardata->first_file_filepos = pad_fileptr_for_entry (abfd, bfd_tell(abfd));
 
   bfd_has_map (abfd) = TRUE;
   bfd_release (abfd, raw_armap);
@@ -2099,6 +2108,20 @@ bfd_gnu_truncate_arname (bfd *abfd, const char *pathname, char *arhdr)
     (hdr->ar_name)[length] = ar_padchar (abfd);
 }
 \f
+/* Write any padding necessary for Mach-O archive entries, which must be
+   aligned on 32-bit boundaries, of ABFD.  Return true if the write was
+   successful, false otherwise.  */
+
+static bfd_boolean
+write_mach_o_entry_padding (bfd *abfd)
+{
+  static const char pad[] = "\0\0\0";
+  file_ptr fpos = bfd_tell (abfd);
+  unsigned int amt = fpos % 4;
+
+  return bfd_bwrite (&pad[0], amt, abfd) == amt;
+}
+
 /* The BFD is open for write and has its format set to bfd_archive.  */
 
 bfd_boolean
@@ -2170,6 +2193,12 @@ _bfd_write_archive_contents (bfd *arch)
 	return FALSE;
     }
 
+  if (bfd_get_flavour (arch) == bfd_target_mach_o_flavour)
+    {
+      if (!write_mach_o_entry_padding (arch))
+	return FALSE;
+    }
+
   if (elength != 0)
     {
       struct ar_hdr hdr;
@@ -2225,7 +2254,12 @@ _bfd_write_archive_contents (bfd *arch)
 	  remaining -= amt;
 	}
 
-      if ((arelt_size (current) % 2) == 1)
+      if (bfd_get_flavour (arch) == bfd_target_mach_o_flavour)
+	{
+	  if (!write_mach_o_entry_padding (arch))
+	    return FALSE;
+	}
+      else if ((arelt_size (current) % 2) == 1)
 	{
 	  if (bfd_bwrite (&ARFMAG[1], 1, arch) != 1)
 	    return FALSE;
@@ -2465,6 +2499,10 @@ bsd_write_armap (bfd *arch,
   if (bfd_bwrite (temp, sizeof (temp), arch) != sizeof (temp))
     return FALSE;
 
+  /* Mach-O requires 32-bit aligned entries.  */
+  if (bfd_get_flavour (arch) == bfd_target_mach_o_flavour)
+    firstreal += firstreal % 4;
+
   for (count = 0; count < orl_count; count++)
     {
       unsigned int offset;
@@ -2478,7 +2516,7 @@ bsd_write_armap (bfd *arch,
 
 	      firstreal += (ared->parsed_size + ared->extra_size
 			    + sizeof (struct ar_hdr));
-	      firstreal += firstreal % 2;
+	      firstreal = pad_fileptr_for_entry (arch, firstreal);
 	      current = current->archive_next;
 	    }
 	  while (current != map[count].u.abfd);
-- 
1.7.9.5

Re: [PATCH] implement correct alignment for Mach-O archive entries

[Matt Thomas]

On Oct 29, 2013, at 10:54 AM, Nathan Froyd <froydnj@mozilla.com> wrote:

> +  if (bfd_get_flavour (abfd) == bfd_target_mach_o_flavour)
> +    ptr += ptr % 4;
> +  else

That does not pad to 4.

ptr = (ptr + 3) & ~3;

or

ptr += 4 - (ptr % 4);

Re: [PATCH] implement correct alignment for Mach-O archive entries

[Nathan Froyd]

----- Original Message -----
> On Oct 29, 2013, at 10:54 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
> 
> > +  if (bfd_get_flavour (abfd) == bfd_target_mach_o_flavour)
> > +    ptr += ptr % 4;
> > +  else
> 
> That does not pad to 4.

Ah, it doesn't quite.  We probably get lucky in that PTR is always even,
where that formula does work.

> ptr += 4 - (ptr % 4);

This pads unnecessarily when ptr is a multiple of 4 already.

-Nathan

Re: [PATCH] don't include symbols from S_ATTR_NO_TOC sections in mach-o canonical symtab output

[Tristan Gingold]

On Oct 29, 2013, at 6:54 PM, Nathan Froyd <froydnj@mozilla.com> wrote:

> Hi,
> 
> Generating static library archives with binutils and comparing them with
> archives produced by Apple's tools reveals one significant difference:
> those produced by Apple's tools are significantly smaller.  binutils
> includes many ${SYMBOL_NAME}.eh symbols in the archive table of contents,
> whereas Apple's tools do not.
> 
> This difference is due to the .eh symbols living in the .eh_frame section,
> which is tagged with the Mach-O section flag S_ATTR_NO_TOC.  This flag
> instructs the archiver to not include symbols from such a section in the
> table of contents for an archive.
> 
> The patch below changes bfd_mach_o_canonicalize_symtab to respect this flag.
> 
> Tested on x86_64-apple-darwin11, no regressions.  OK to checkin?

Yes, this is OK.

(Interesting to know that GNU ar is used on Darwin :-)

Tristan.

> 
> -Nathan
> 
> 	* mach-o.c (bfd_mach_o_canonicalize_symtab): Respect the
> 	BFD_MACH_O_S_ATTR_NO_TOC flag on symbol sections.
> ---
> bfd/mach-o.c |   21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/bfd/mach-o.c b/bfd/mach-o.c
> index 32e48ac..095afeb 100644
> --- a/bfd/mach-o.c
> +++ b/bfd/mach-o.c
> @@ -630,7 +630,7 @@ bfd_mach_o_canonicalize_symtab (bfd *abfd, asymbol **alocation)
>   bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
>   long nsyms = bfd_mach_o_count_symbols (abfd);
>   bfd_mach_o_symtab_command *sym = mdata->symtab;
> -  unsigned long j;
> +  unsigned long i, j;
> 
>   if (nsyms < 0)
>     return nsyms;
> @@ -651,12 +651,23 @@ bfd_mach_o_canonicalize_symtab (bfd *abfd, asymbol **alocation)
> 
>   BFD_ASSERT (sym->symbols != NULL);
> 
> -  for (j = 0; j < sym->nsyms; j++)
> -    alocation[j] = &sym->symbols[j].symbol;
> +  for (i = 0, j = 0; j < sym->nsyms; j++)
> +    {
> +      bfd_mach_o_asymbol *asym = &sym->symbols[j];
> +      if (asym->n_sect != 0)
> +	{
> +	  bfd_mach_o_section *sect = mdata->sections[asym->n_sect-1];
> +	  if (sect->flags & BFD_MACH_O_S_ATTR_NO_TOC)
> +	    continue;
> +	}
> 
> -  alocation[j] = NULL;
> +      alocation[i] = &asym->symbol;
> +      i++;
> +    }
> +
> +  alocation[i] = NULL;
> 
> -  return nsyms;
> +  return i;
> }
> 
> /* Create synthetic symbols for indirect symbols.  */
> -- 
> 1.7.9.5
> 

Re: [PATCH] implement correct alignment for Mach-O archive entries

[Tristan Gingold]

On Oct 29, 2013, at 7:15 PM, Nathan Froyd <froydnj@mozilla.com> wrote:

> ----- Original Message -----
>> On Oct 29, 2013, at 10:54 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
>> 
>>> +  if (bfd_get_flavour (abfd) == bfd_target_mach_o_flavour)
>>> +    ptr += ptr % 4;
>>> +  else
>> 
>> That does not pad to 4.
> 
> Ah, it doesn't quite.  We probably get lucky in that PTR is always even,
> where that formula does work.

This is OK for the darwin part, but you need a global maintainer OK for archive.c

Thanks,
Tristan.