Re: [Patch mach-o/gas] support .previous

[Tristan Gingold <gingold at adacore dot com>]

On Jan 1, 2012, at 5:11 PM, Iain Sandoe wrote:

> .previous is a nice feature and will help us with compatibility with elf-style code.
> 
> OK?

I have a few questions, see below.

> Iain
> 
> gas:
> 
> 	* config/obj-macho.c (previous_section): New file-scope var.
> 	(obj_mach_o_parse_section): Split from obj_mach_o_section.
> 	(obj_mach_o_section): Use obj_mach_o_parse_section, update previous.
> 	(obj_mach_o_known_section): Update previous.
> 	(obj_mach_o_objc_section): Likewise.
> 	(obj_mach_o_debug_section): Likewise.
> 	(obj_mach_o_opt_tgt_section): Likewise.
> 	(obj_mach_o_get_base_section): Split from obj_mach_o_base_section.
> 	(obj_mach_o_base_section): Use obj_mach_o_get_base_section, update previous.
> 	(obj_mach_o_previous): New.
> 	(mach_o_pseudo_table): Add previous.
> 
> gas/testsuite:
> 
> 	* gas/mach-o/previous-1.d: New.
> 	* gas/mach-o/previous-1.s: New.
> 	* gas/mach-o/warn-prev-1.s: New.
> 
> ==
> 
> gas/config/obj-macho.c                 |  185 +++++++++++++++++++++++---------
> gas/testsuite/gas/mach-o/previous-1.d  |    8 ++
> gas/testsuite/gas/mach-o/previous-1.s  |   20 ++++
> gas/testsuite/gas/mach-o/warn-prev-1.s |    5 +
> 4 files changed, 167 insertions(+), 51 deletions(-)
> 
> diff --git a/gas/config/obj-macho.c b/gas/config/obj-macho.c
> index 74fb0c9..abea09b 100644
> --- a/gas/config/obj-macho.c
> +++ b/gas/config/obj-macho.c
> @@ -1,5 +1,5 @@
> /* Mach-O object file format
> -   Copyright 2009, 2011 Free Software Foundation, Inc.
> +   Copyright 2009, 2011, 2012 Free Software Foundation, Inc.
> 
>    This file is part of GAS, the GNU Assembler.
> 
> @@ -48,6 +48,9 @@
> /* Forward decl.  */
> static segT obj_mach_o_segT_from_bfd_name (const char *nam, int must_succeed);
> 
> +/* Support for .previous for improved compatibility with elf-style code.  */
> +static segT previous_section;
> +
> /* TODO: Implement "-dynamic"/"-static" command line options.  */
> 
> static int obj_mach_o_is_static;
> @@ -175,8 +178,8 @@ collect_16char_name (char *dest, const char *msg, int require_comma)
>    Not all section types and attributes are accepted by the Darwin system
>    assemblers as user-specifiable - although, at present, we do here.  */
> 
> -static void
> -obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
> +static segT
> +obj_mach_o_parse_section (void)
> {
>   char *p;
>   char c;
> @@ -196,17 +199,13 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
>   char segname[17];
>   char sectname[17];
> 
> -#ifdef md_flush_pending_output
> -  md_flush_pending_output ();
> -#endif
> -
>   /* Zero-length segment and section names are allowed.  */
>   /* Parse segment name.  */
>   memset (segname, 0, sizeof(segname));
>   if (collect_16char_name (segname, "segment", 1))
>     {
>       ignore_rest_of_line ();
> -      return;
> +      return NULL;
>     }
>   input_line_pointer++; /* Skip the terminating ',' */
> 
> @@ -240,7 +239,7 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
>           as_bad (_("unknown or invalid section type '%s'"), p);
> 	  p[len] = tmpc;
> 	  ignore_rest_of_line ();
> -	  return;
> +	  return NULL;
>         }
>       else
> 	sectype_given = 1;
> @@ -278,7 +277,7 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
>                   as_bad (_("unknown or invalid section attribute '%s'"), p);
> 		  p[len] = tmpc;
> 		  ignore_rest_of_line ();
> -		  return;
> +		  return NULL;
>                 }
>               else
> 		{
> @@ -297,7 +296,7 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
>                 {
> 		  as_bad (_("unexpected section size information"));
> 		  ignore_rest_of_line ();
> -		  return;
> +		  return NULL;
> 		}
> 
> 	      input_line_pointer++;
> @@ -307,7 +306,7 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
>             {
>               as_bad (_("missing sizeof_stub expression"));
> 	      ignore_rest_of_line ();
> -	      return;
> +	      return NULL;
>             }
>         }
>     }
> @@ -341,8 +340,8 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
>       name = n;
>     }
> 
> -  /* Sub-segments don't exists as is on Mach-O.  */
> -  sec = subseg_new (name, 0);
> +  /* Get or create the section.  */
> +  sec = subseg_get (name, 0);
> 
>   oldflags = bfd_get_section_flags (stdoutput, sec);
>   msect = bfd_mach_o_get_mach_o_section (sec);
> @@ -375,7 +374,30 @@ obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
> 	  || msect->flags != (secattr | sectype))
> 	as_warn (_("Ignoring changed section attributes for %s"), name);
>     }
> -  demand_empty_rest_of_line ();
> +
> +  return sec;
> +}
> +
> +static void
> +obj_mach_o_section (int ignore ATTRIBUTE_UNUSED)
> +{
> +  segT new_seg;
> +  segT old_seg = now_seg;
> +
> +#ifdef md_flush_pending_output
> +  md_flush_pending_output ();
> +#endif
> +
> +  new_seg = obj_mach_o_parse_section ();
> +
> +  if (new_seg != NULL)
> +    {
> +      demand_empty_rest_of_line ();
> +
> +      subseg_set (new_seg, 0);
> +      if (now_seg != old_seg)
> +	previous_section = old_seg;

What is the reason of this guard ? It seems to be that it prevents user to write code such as:

asm ("
  .section x
  […]
  .previous
");

as it would make .previous behavior undefined for the user

> +    }
> }
> 
> static segT
> @@ -443,17 +465,23 @@ static const char * const known_sections[] =
> static void
> obj_mach_o_known_section (int sect_index)
> {
> -  segT section;
> +  segT new_seg;
> +  segT old_seg = now_seg;
> 
> #ifdef md_flush_pending_output
>   md_flush_pending_output ();
> #endif
> 
> -  section = obj_mach_o_segT_from_bfd_name (known_sections[sect_index], 1);
> -  if (section != NULL)
> -    subseg_set (section, 0);
> +  new_seg = obj_mach_o_segT_from_bfd_name (known_sections[sect_index], 1);
> 
> +  if (new_seg != NULL)
> +    {
> +      subseg_set (new_seg, 0);
> +      if (now_seg != old_seg)
> +	previous_section = old_seg;
> +    }
>   /* else, we leave the section as it was; there was a fatal error anyway.  */
> +  demand_empty_rest_of_line ();
> }
> 
> static const char * const objc_sections[] =
> @@ -491,22 +519,25 @@ static const char * const objc_sections[] =
> static void
> obj_mach_o_objc_section (int sect_index)
> {
> -  segT section;
> +  segT new_seg;
> +  segT old_seg = now_seg;
> 
> #ifdef md_flush_pending_output
>   md_flush_pending_output ();
> #endif
> 
> -  section = obj_mach_o_segT_from_bfd_name (objc_sections[sect_index], 1);
> -  if (section != NULL)
> +  new_seg = obj_mach_o_segT_from_bfd_name (objc_sections[sect_index], 1);
> +  if (new_seg != NULL)
>     {
>       obj_mach_o_seen_objc_section = 1; /* We need to ensure that certain
> 					   sections are present and in the
> 					   right order.  */
> -      subseg_set (section, 0);
> +      subseg_set (new_seg, 0);
> +      if (now_seg != old_seg)
> +	previous_section = old_seg;
>     }
> -
>   /* else, we leave the section as it was; there was a fatal error anyway.  */
> +  demand_empty_rest_of_line ();
> }

BTW, obj_mach_o_objc_section looks furiously like obj_mach_o_known_section.  Maybe we can reserve a range for objc sections and merge both implementations.

> 
> /* Debug section directives.  */
> @@ -536,17 +567,22 @@ static const char * const debug_sections[] =
> static void
> obj_mach_o_debug_section (int sect_index)
> {
> -  segT section;
> +  segT new_seg;
> +  segT old_seg = now_seg;
> 
> #ifdef md_flush_pending_output
>   md_flush_pending_output ();
> #endif
> 
> -  section = obj_mach_o_segT_from_bfd_name (debug_sections[sect_index], 1);
> -  if (section != NULL)
> -    subseg_set (section, 0);
> -
> +  new_seg = obj_mach_o_segT_from_bfd_name (debug_sections[sect_index], 1);
> +  if (new_seg != NULL)
> +    {
> +      subseg_set (new_seg, 0);
> +      if (now_seg != old_seg)
> +	previous_section = old_seg;
> +    }
>   /* else, we leave the section as it was; there was a fatal error anyway.  */
> +  demand_empty_rest_of_line ();
> }

Ditto for the merge.

> 
> /* This could be moved to the tc-xx files, but there is so little dependency
> @@ -584,22 +620,25 @@ static void
> obj_mach_o_opt_tgt_section (int sect_index)
> {
>   const struct opt_tgt_sect *tgtsct = &tgt_sections[sect_index];
> -  segT section;
> +  segT new_seg;
> +  segT old_seg = now_seg;
> 
> #ifdef md_flush_pending_output
>   md_flush_pending_output ();
> #endif
> 
> -  section = obj_mach_o_segT_from_bfd_name (tgtsct->name, 0);
> -  if (section == NULL)
> +  new_seg = obj_mach_o_segT_from_bfd_name (tgtsct->name, 0);
> +  if (new_seg == NULL)
>     {
>       as_bad (_("%s is not used for the selected target"), tgtsct->name);
>       /* Leave the section as it is.  */
>     }
>   else
>     {
> -      bfd_mach_o_section *mo_sec = bfd_mach_o_get_mach_o_section (section);
> -      subseg_set (section, 0);
> +      bfd_mach_o_section *mo_sec = bfd_mach_o_get_mach_o_section (new_seg);
> +      subseg_set (new_seg, 0);
> +      if (now_seg != old_seg)
> +	previous_section = old_seg;
> #if defined (TC_I386)
>       mo_sec->reserved2 = tgtsct->x86_val;
> #elif defined (TC_PPC)
> @@ -608,26 +647,14 @@ obj_mach_o_opt_tgt_section (int sect_index)
>       mo_sec->reserved2 = 0;
> #endif
>     }
> +  demand_empty_rest_of_line ();
> }
> 
> -/* We don't necessarily have the three 'base' sections on mach-o.
> -   Normally, we would start up with only the 'text' section defined.
> -   However, even that can be suppressed with (TODO) c/l option "-n".
> -   Thus, we have to be able to create all three sections on-demand.  */
> -
> -static void
> -obj_mach_o_base_section (int sect_index)
> +static segT
> +obj_mach_o_get_base_section (int sect_index)
> {
>   segT section;
> 
> -#ifdef md_flush_pending_output
> -  md_flush_pending_output ();
> -#endif
> -
> -  /* We don't support numeric (or any other) qualifications on the
> -     well-known section shorthands.  */
> -  demand_empty_rest_of_line ();
> -
>   switch (sect_index)
>     {
>       /* Handle the three sections that are globally known within GAS.
> @@ -659,10 +686,39 @@ obj_mach_o_base_section (int sect_index)
> 	break;
>       default:
>         as_fatal (_("internal error: base section index out of range"));
> -        return;
> +        return NULL;
> 	break;
>     }
> -  subseg_set (section, 0);
> +  return section;
> +}
> +
> +/* We don't necessarily have the three 'base' sections on mach-o.
> +   Normally, we would start up with only the 'text' section defined.
> +   However, even that can be suppressed with (TODO) c/l option "-n".
> +   Thus, we have to be able to create all three sections on-demand.  */
> +
> +static void
> +obj_mach_o_base_section (int sect_index)
> +{
> +  segT new_seg;
> +  segT old_seg = now_seg;
> +
> +#ifdef md_flush_pending_output
> +  md_flush_pending_output ();
> +#endif
> +
> +  new_seg = obj_mach_o_get_base_section (sect_index);
> +  if (new_seg != NULL)
> +    {
> +      subseg_set (new_seg, 0);
> +      if (now_seg != old_seg)
> +	previous_section = old_seg;
> +    }
> +  /* else, we leave the section as it was; there was a fatal error anyway.  */
> +
> +  /* We don't support numeric (or any other) qualifications on canonical
> +    section names.  */
> +  demand_empty_rest_of_line ();
> }
> 
> /* This finishes off parsing a .comm or .lcomm statement, which both can have
> @@ -718,7 +774,31 @@ obj_mach_o_common_parse (int is_local, symbolS *symbolP,
> static void
> obj_mach_o_comm (int is_local)
> {
> +  segT old_seg = now_seg;
>   s_comm_internal (is_local, obj_mach_o_common_parse);
> +  if (now_seg != old_seg)
> +    previous_section = old_seg;
> +}

Humm, why does it set previous_section ?

> +
> +static void
> +obj_mach_o_previous (int ignore ATTRIBUTE_UNUSED)
> +{
> +  segT old_prev;
> +
> +#ifdef md_flush_pending_output
> +  md_flush_pending_output ();
> +#endif
> +
> +  if (previous_section == 0)
> +    {
> +      as_warn (_(".previous, but no section switch seen; ignored"));
> +      return;
> +    }
> +
> +  old_prev = previous_section;
> +  previous_section = now_seg;
> +  subseg_set (old_prev, 0);
> +  demand_empty_rest_of_line ();
> }
> 
> /* Set properties that apply to the whole file.  At present, the only
> @@ -838,6 +918,9 @@ const pseudo_typeS mach_o_pseudo_table[] =
> 
>   { "section", obj_mach_o_section, 0},
> 
> +  /* Support the elf-style previous.  */
> +  { "previous", obj_mach_o_previous, 0},
> +
>   /* Symbol-related.  */
>   { "indirect_symbol", obj_mach_o_placeholder, 0},
>   { "weak_definition", obj_mach_o_placeholder, 0},
> diff --git a/gas/testsuite/gas/mach-o/previous-1.d b/gas/testsuite/gas/mach-o/previous-1.d
> new file mode 100644
> index 0000000..64d4ee5
> --- /dev/null
> +++ b/gas/testsuite/gas/mach-o/previous-1.d
> @@ -0,0 +1,8 @@
> +#objdump: -t
> +.*: +file format mach-o.*
> +#...
> +SYMBOL TABLE:
> +(00000000)?00000000 g.*0f SECT.*01 0000 \[.text\] a
> +(00000000)?00000000 g.*0f SECT.*02 0000 \[.data\] a1
> +(00000000)?00000001 g.*0f SECT.*01 0000 \[.text\] a2
> +(00000000)?00000001 g.*0f SECT.*02 0000 \[.data\] a3
> \ No newline at end of file
> diff --git a/gas/testsuite/gas/mach-o/previous-1.s b/gas/testsuite/gas/mach-o/previous-1.s
> new file mode 100644
> index 0000000..5522db7
> --- /dev/null
> +++ b/gas/testsuite/gas/mach-o/previous-1.s
> @@ -0,0 +1,20 @@
> +	.text
> +
> +	.globl a
> +a:	.space 1
> +	
> +	.data
> +
> +	.globl a1
> +a1:	.space 1
> +
> +	.previous
> +
> +	.globl a2
> +a2:	.space 1
> +	
> +	.previous
> +
> +	.globl a3
> +a3:	.space 1
> +
> diff --git a/gas/testsuite/gas/mach-o/warn-prev-1.s b/gas/testsuite/gas/mach-o/warn-prev-1.s
> new file mode 100644
> index 0000000..fd0c0b5
> --- /dev/null
> +++ b/gas/testsuite/gas/mach-o/warn-prev-1.s
> @@ -0,0 +1,5 @@
> +# { dg-do assemble }
> +
> +	.previous
> +
> +# { dg-warning ".previous, but no section switch seen; ignored" "" { target *-*-darwin*} 3 }
> 

Overall, it looks ok.

Thanks,
Tristan.