Re: [Patch mach-o] correct vmsize calculation for zerofill cases.

[Tristan Gingold <gingold at adacore dot com>]

On Feb 9, 2012, at 4:01 PM, Iain Sandoe wrote:

> We have to place zerofill sections at the end of segments (in vm terms).
> However, they appear in MH_OBJECTS in source-specification order.
> 
> When one counts the vmsize (during building the segment command) with the zerofills in the file position, it is possible to end up with a difference of a few bytes owing to alignment rounding.  This shows up pretty rarely, but Darwin 10's ld barfs on it.
> 
> So, unfortunately, we need to compute the segment vmsize in three loops (two for zerofill sections and zerofill GB sections). ... if only the MH_OBJECT specified actually placing the sections at the end ...
> 
> OK?

Ok, although I have a suggestion below in the patch.

[ Maybe we should have a linked list of section per segment ]

Thanks,
Tristan.

> Iain
> 
> bfd:
> 
> 	* mach-o.c (bfd_mach_o_build_seg_command): Count zerofill section
> 	vma additions in their logical, rather than physical order.
> 
> Index: bfd/mach-o.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/mach-o.c,v
> retrieving revision 1.99
> diff -u -p -r1.99 mach-o.c
> --- bfd/mach-o.c	2 Feb 2012 11:55:42 -0000	1.99
> +++ bfd/mach-o.c	9 Feb 2012 14:54:16 -0000
> @@ -2026,7 +2026,12 @@ bfd_mach_o_build_seg_command (const char
>   seg->sect_head = NULL;
>   seg->sect_tail = NULL;
> 
> -  /*  Append sections to the segment.  */
> +  /*  Append sections to the segment.
> +
> +      This is a little tedious, we have to honor the need to account zerofill
> +      sections after all the rest.  This forces us to do the calculation of
> +      total vmsize in three passes so that any alignment increments are
> +      properly accounted.  */
> 
>   for (i = 0; i < mdata->nsects; ++i)
>     {
> @@ -2039,14 +2044,10 @@ bfd_mach_o_build_seg_command (const char
> 	  && strncmp (segment, s->segname, BFD_MACH_O_SEGNAME_SIZE) != 0)
> 	continue;
> 
> +      /* Although we account for zerofill section sizes in vm order, they are
> +	 placed in the file in source sequence.  */
>       bfd_mach_o_append_section_to_segment (seg, sec);
> -
>       s->offset = 0;
> -      if (s->size > 0)
> -       {
> -          seg->vmsize = FILE_ALIGN (seg->vmsize, s->align);
> -	  seg->vmsize += s->size;
> -        }
> 
>       /* Zerofill sections have zero file size & offset,
> 	 and are not written.  */
> @@ -2057,19 +2058,59 @@ bfd_mach_o_build_seg_command (const char
> 
>       if (s->size > 0)
>        {
> +	  seg->vmsize = FILE_ALIGN (seg->vmsize, s->align);
> +	  seg->vmsize += s->size;
> +
> +	  seg->filesize = FILE_ALIGN (seg->filesize, s->align);
> +	  seg->filesize += s->size;
> +
>           mdata->filelen = FILE_ALIGN (mdata->filelen, s->align);
>           s->offset = mdata->filelen;
>         }
> 
>       sec->filepos = s->offset;
> -
>       mdata->filelen += s->size;
>     }
> 
> -  seg->filesize = mdata->filelen - seg->fileoff;
> -  seg->filesize = FILE_ALIGN(seg->filesize, 2);
> +  /* Now pass through again, for zerofill, only now we just update the vmsize.  */
> +  for (i = 0; i < mdata->nsects; ++i)
> +    {
> +      bfd_mach_o_section *s = mdata->sections[i];
> +
> +      if (! is_mho
> +	  && strncmp (segment, s->segname, BFD_MACH_O_SEGNAME_SIZE) != 0)
> +	continue;
> +
> +      if ((s->flags & BFD_MACH_O_SECTION_TYPE_MASK) != BFD_MACH_O_S_ZEROFILL)
> +        continue;

I simply propose to move the test on flags before the segment name comparison, as a micro-optimization.

> +
> +      if (s->size > 0)
> +	{
> +	  seg->vmsize = FILE_ALIGN (seg->vmsize, s->align);
> +	  seg->vmsize += s->size;
> +	}
> +    }
> +
> +  /* Now pass through again, for zerofill_GB.  */
> +  for (i = 0; i < mdata->nsects; ++i)
> +    {
> +      bfd_mach_o_section *s = mdata->sections[i];
> +
> +      if (! is_mho
> +	  && strncmp (segment, s->segname, BFD_MACH_O_SEGNAME_SIZE) != 0)
> +	continue;
> +
> +      if ((s->flags & BFD_MACH_O_SECTION_TYPE_MASK) != BFD_MACH_O_S_GB_ZEROFILL)
> +        continue;

Same here.

> +
> +      if (s->size > 0)
> +	{
> +	  seg->vmsize = FILE_ALIGN (seg->vmsize, s->align);
> +	  seg->vmsize += s->size;
> +	}
> +    }
> 
> -  /* Allocate relocation room.  */
> +  /* Allocate space for the relocations.  */
>   mdata->filelen = FILE_ALIGN(mdata->filelen, 2);
> 
>   for (i = 0; i < mdata->nsects; ++i)
>