Re: [Patch mach-o/gas] set section vma.

[Tristan Gingold <gingold at adacore dot com>]

On Jan 12, 2012, at 3:42 PM, Iain Sandoe wrote:

> It would be good to sort this out soon - because otherwise we're going to end up with a *lot* of tests to change.
> 
> There's a constraint that zerofill sections must appear after all others in their segment.
> Likewise GB zerofill must appear last.

An alternative implementation would be to reorder the sections, but I am ok with your implementation.

> ===
> 
> If there were no external model, I would have chosen to sort the sections as per ld's requirements - however, that's not what the native 'as' does, and the implementation follows the latter.
> 
> (there are a bunch of tests to adjust when this is applied - but nothing exciting to review - just filling in numbers where there is currently 000000)
> 
> OK?

See a few comments.

> Iain
> 
> gas:
> 
> 	* config/obj-macho.c (obj_mach_o_set_vma_data): New type.
> 	(obj_mach_o_set_section_vma): New.
> 	(obj_mach_o_post_relax_hook): New.
> 	* config/obj-macho.h (md_post_relax_hook): Define.
> 	(obj_mach_o_post_relax_hook): Declare.
> 
> gas/config/obj-macho.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
> gas/config/obj-macho.h |    3 ++
> 2 files changed, 69 insertions(+), 0 deletions(-)
> 
> diff --git a/gas/config/obj-macho.c b/gas/config/obj-macho.c
> index 61e6771..46fb9b4 100644
> --- a/gas/config/obj-macho.c
> +++ b/gas/config/obj-macho.c
> @@ -1471,6 +1471,72 @@ obj_macho_frob_symbol (struct symbol *sp)
>   return 0;
> }
> 
> +/* This is a case where, if starting from scratch, I'd do things differently.
> +
> +   IMO the object file would be tidier if sorted into the same order that ld
> +   requires.  However, the native tool doesn't do that - it leaves the sections
> +   in the order they appear in the source, and fudges the section vma so that
> +   the constraints of zerofill sections are met.
> +
> +   We follow this for now - if nothing else, it makes comparison easier, but
> +   one day we might wish to re-order.  */
> +
> +typedef struct obj_mach_o_set_vma_data
> +{
> +  bfd_vma vma;
> +  unsigned zerofill;
> +} obj_mach_o_set_vma_data;

I would rename zerofill to zerofill_pass to make it clear this is not a boolean.

> +
> +/* We do three passes through to set the vma, so that:
> +   zerofill sections appear after all others in their segment
> +   GB zerofill appear last.  */
> +
> +static void
> +obj_mach_o_set_section_vma (bfd *abfd ATTRIBUTE_UNUSED, asection *sec, void *xxx)

Please rename xxx to v_p, as XXX is often used as a FIXME marker.

> +{
> +  bfd_mach_o_section *ms = bfd_mach_o_get_mach_o_section (sec);
> +  unsigned bfd_align = bfd_get_section_alignment (abfd, sec);
> +  obj_mach_o_set_vma_data *p = (struct obj_mach_o_set_vma_data *)xxx;
> +  unsigned flags = (ms->flags & BFD_MACH_O_SECTION_TYPE_MASK);
> +  unsigned zf;
> +
> +  zf = 0;
> +  if (flags == BFD_MACH_O_S_ZEROFILL)
> +    zf = 1;
> +  else if (flags == BFD_MACH_O_S_GB_ZEROFILL)
> +    zf = 2;
> +
> +  if (p->zerofill != zf)
> +    return;
> +
> +  /* We know the section size now - so make a vma for the section just
> +     based on order.  */
> +  ms->size = bfd_get_section_size (sec);
> +
> +  /* Make sure that the align agrees, and set to the largest value chosen.  */
> +  ms->align = ms->align > bfd_align ? ms->align : bfd_align;
> +  bfd_set_section_alignment (abfd, sec, ms->align);
> +
> +  p->vma += (1 << ms->align) - 1;
> +  p->vma &= ~((1 << ms->align) - 1);
> +  ms->addr = p->vma;
> +  bfd_set_section_vma (abfd, sec, p->vma);
> +  p->vma += ms->size;
> +}
> +
> +void obj_mach_o_post_relax_hook (void)
> +{
> +  obj_mach_o_set_vma_data d;
> +  d.vma = 0;
> +  d.zerofill = 0;
> +
> +  bfd_map_over_sections (stdoutput, obj_mach_o_set_section_vma, (char *) &d);
> +  d.zerofill = 1;
> +  bfd_map_over_sections (stdoutput, obj_mach_o_set_section_vma, (char *) &d);
> +  d.zerofill = 2;
> +  bfd_map_over_sections (stdoutput, obj_mach_o_set_section_vma, (char *) &d);

Looks like a loop from 0 to 2!

> +}
> +
> static void
> obj_mach_o_set_indirect_symbols (bfd *abfd, asection *sec,
> 				 void *xxx ATTRIBUTE_UNUSED)
> diff --git a/gas/config/obj-macho.h b/gas/config/obj-macho.h
> index 9f1f3db..2fc5cb6 100644
> --- a/gas/config/obj-macho.h
> +++ b/gas/config/obj-macho.h
> @@ -62,6 +62,9 @@ extern void obj_macho_frob_label (struct symbol *);
> #define obj_frob_symbol(s, punt) punt = obj_macho_frob_symbol(s)
> extern int obj_macho_frob_symbol (struct symbol *);
> 
> +#define md_post_relax_hook obj_mach_o_post_relax_hook()
> +void obj_mach_o_post_relax_hook (void);
> +
> #define obj_frob_file_after_relocs obj_mach_o_frob_file_after_relocs
> extern void obj_mach_o_frob_file_after_relocs (void);

Ok with the suggested changes.  No need to resubmit.

Thanks,
Tristan.