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

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

[Iain Sandoe]

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?

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;
+
+      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;
+
+      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)

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

[Tristan Gingold]

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)
> 

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

[Iain Sandoe]

On 9 Feb 2012, at 15:06, Tristan Gingold wrote:

>>
>> OK?
>
> Ok, although I have a suggestion below in the patch.
done & applied.

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

once we start to do read/write/update and other filetypes, it would  
seem that something like that would be useful/required.
  - for MH_OBJECTs only there's not much we can change -- unless....
.. we could see if ordering the sections from GAS actually causes any  
onward tool-chain problems... but maybe leave until we have a basic  
system working?

Iain