Re: [Patch, Darwin/libiberty] fix target/48108 by adding a section wrapper scheme to the darwin port.

[Ian Lance Taylor <ian at airs dot com>]

Iain Sandoe <developer@sandoe-acoustics.co.uk> writes:


This patch looks basically OK but it seems to have a number of
extraneous changes which make it harder to review.


> libiberty:
>
> 	PR target/48108
> 	* simple-object-mach-o.c (GNU_SECTION_NAMES): Remove.
> 	(GNU_WRAPPER_SECTS, GNU_WRAPPER_INDEX,
> 	GNU_WRAPPER_NAMES): New macros.
> 	(simple_object_mach_o_match): Amend comment and error message.
> 	Do not require that a segment name is specified.
> 	(simple_object_mach_o_segment): Handle wrapper scheme, if a
> segment name is
> 	specified.  (simple_object_mach_o_write_section_header): Allow
> the segment name
> 	to be supplied. (simple_object_mach_o_write_segment): Handle
> wrapper scheme, if
> 	a segment name is specified.  Ensure that the top-level
> segment name in the load
> 	command is empty.   (simple_object_mach_o_write_to_file): Determine the
> 	number of sections during segment output, use that in writing
> the header.
>
> gcc:
>
> 	PR target/48108
> 	* config/darwin.c (top level): Amend comments concerning LTO output.
> 	(lto_section_num): New variable.  (darwin_lto_section_e): New GTY.
> 	(LTO_SECTS_SECTION, LTO_INDEX_SECTION): New.
> 	(LTO_NAMES_SECTION): Rename.
> 	(darwin_asm_named_section): Record LTO section counts and switches
> 	in a vec of darwin_lto_section_e.
> 	(darwin_file_start): Remove unused code.
> 	(darwin_file_end): Put an LTO section termination label.  Handle output
> 	of the wrapped LTO sections, index and names table.
> 	
> include:
>
> 	PR target/48108
> 	*simple-object.h: Amend comments for simple read/write to note the
> 	wrapper scheme for Mach-o.

Space after '*'.


> -#define GNU_SECTION_NAMES "__section_names"

Are we sure it is OK to drop support for __section_names?  Won't that
mean that any old LTO files will not work with new gcc?  Is that OK?


> @@ -258,18 +274,10 @@ simple_object_mach_o_match (
>      }
>  #endif
>  
> -  /* We require the user to provide a segment name.  This is
> -     unfortunate but I don't see any good choices here.  */
> -
> -  if (segment_name == NULL)
> +  /* Although a standard MH_OBJECT has a single anonymous segment, we allow
> +     a segment name to be specified - but don't act on it at this stage.  */
> +  if (segment_name && strlen (segment_name) > MACH_O_NAME_LEN)
>      {
> -      *errmsg = "Mach-O file found but no segment name specified";
> -      *err = 0;
> -      return NULL;
> -    }
> -
> -  if (strlen (segment_name) > MACH_O_NAME_LEN)
> -    {
>        *errmsg = "Mach-O segment name too long";
>        *err = 0;
>        return NULL;

Please write "segment_name != NULL &&", not just "segment_name &&".



> @@ -294,13 +302,14 @@ simple_object_mach_o_match (
>    filetype = (*fetch_32) (b + offsetof (struct mach_o_header_32, filetype));
>    if (filetype != MACH_O_MH_OBJECT)
>      {
> -      *errmsg = "Mach-O file is not object file";
> +      *errmsg = "Mach-O file is not an MH_OBJECT file";
>        *err = 0;
>        return NULL;
>      }

I'm not sure this error message change is an improvement.  This error
message may be seen by end users, not just developers.  I think more end
users will understand "object file."  Why do you want to make this
change?


>    omr = XNEW (struct simple_object_mach_o_read);
> -  omr->segment_name = xstrdup (segment_name);
> +  if (segment_name)
> +    omr->segment_name = xstrdup (segment_name);
>    omr->magic = magic;
>    omr->is_big_endian = is_big_endian;
>    omr->cputype = (*fetch_32) (b + offsetof (struct mach_o_header_32, cputype));

Please write "if (segment_name != NULL)".


> @@ -356,9 +365,25 @@ simple_object_mach_o_section_info (int is_big_endi
>      }
>  }
>  
> -/* Handle a segment in a Mach-O file.  Return 1 if we should continue,
> -   0 if the caller should return.  */
> +/* Handle a segment in a Mach-O Object file.
>  
> +   This will callback to the function pfn for each "section found" the meaning
> +   of which depends on a gnu extension to mach-o:
> +   
> +   When omr->segment_name is specified, we implement a wrapper scheme (which
> +   whilst it will, in principle, work with any segment name, is likely to be
> +   meaningless current for anything except __GNU_LTO).
> +   
> +   If we find mach-o sections (with the segment name as specified) which also
> +   contain: a 'sects' wrapper, an index, and a  name table, we expand this into
> +   as many sections as are specified in the index.  In this case, there will
> +   be a callback for each of these.
> +   
> +   When the omr->segment_name is not specified, then we would call-back for
> +   every mach-o section specified in the file.
> +
> +   Return 1 if we should continue, 0 if the caller should return.  */

Is there a reason that you are changing the code to support not
specifying a segment?  Should that be a separate change?


> @@ -375,12 +400,19 @@ simple_object_mach_o_segment (simple_object_read *
>    size_t sechdrsize;
>    size_t segname_offset;
>    size_t sectname_offset;
> -  unsigned int nsects;
> -  unsigned char *secdata;
> -  unsigned int i;
> -  unsigned int strtab_index;
> -  char *strtab;
> -  size_t strtab_size;
> +  int nsects;

Why change the type?

> +  unsigned char *secdata = NULL;

Why initialize?

> +  int i;

Why change the type?

> +  int strtab_index, index_index, sections_index;

Please put all variables in separate lines, like the existing code.

> +  char *strtab = NULL;
> +  size_t strtab_size = 0;
> +  unsigned char *index = NULL;
> +  size_t index_size;
> +  unsigned int n_wrapped_sects = 0;
> +  size_t wrapper_sect_size = 0;
> +  off_t wrapper_sect_offset = (off_t)0;
> +  int wrapping = (omr->segment_name && strlen (omr->segment_name) > 0);
> +  strtab_index = index_index = sections_index = -1;

Please initialize variables where they are used, not at the start of the
function.

Please write "omr->segment_name != NULL &&".  Please don't test whether
omr->segment_name is the empty string.  The description of the function
does not make a special case for an empty segment name, so I don't see
why this code should.  If you do need to for the empty string for some
reason, please write "omr->segment_name[0] != '\0'", not "strlen
(omr->segment_name) > 0".


>    /* Process the sections.  */
> -
>    for (i = 0; i < nsects; ++i)
>      {
>        const unsigned char *sechdr;

Please leave the blank line.


> -      char namebuf[MACH_O_NAME_LEN + 1];
> +      char namebuf[MACH_O_NAME_LEN*2 + 2]; /* space for segment,section\0.  */

Please put spaces around '*'.


> +      /* Otherwise, make a name like __segment,__section as per the convention
> +         in mach-o asm.  */
> +      memset (namebuf, 0, MACH_O_NAME_LEN*2+2);

Please put spaces around '*' and '+'.


> +      memcpy (namebuf, (char *) sechdr + segname_offset, MACH_O_NAME_LEN);
> +      l = strlen (namebuf);
> +      namebuf[l] = ',';
> +      memcpy (namebuf, (char *) sechdr + sectname_offset, MACH_O_NAME_LEN);

This is clearly wrong.  The last line should be namebuf + l.  But as
noted above I'm not sure why you need this functionality at all.



> +/* Write out the single (anonymous) segment containing the sections of a Mach-O
> +   Object file.
> +   
> +   As a GNU extension to mach-o, when the caller specifies a segment name in
> +   sobj->segment_name, all the sections passed will be output under a single
> +   mach-o section header.  The caller's sections are indexed within this 
> +   'wrapper' section by a table stored in a second mach-o section.  Finally,
> +   arbitrary length section names are permitted by the extension and these are
> +   stored in a table in a third mach-o section.
> +   
> +   Note that this is only likely to make any sense for the __GNU_LTO segment
> +   at present.
> +   
> +   If the segment_name is not specified (or zero-length) we just write out the 
> +   sections as we find them.  In that case we assume that the section name is 
> +   in the form __SEGMENT_NAME,__section_name as per Mach-O asm.  */

Why implement the case of segment_name == NULL?  Why make a zero-length
segment name a special case?  If you need to handle a zero-length
segment name for some reason, you need to document that in
libiberty/simple-object.h.


> +  int wrapping = (sobj->segment_name && strlen (sobj->segment_name) > 0);
> +  size_t nsects_in;
> +  uint32_t *index = NULL;
> +  char *snames = NULL;
> +  unsigned int sect;

If you need to test the length, test using [0], not strlen, as per
above.

Please initialize variables where you need them.

The libiberty library is meant to be highly portable, which means that
types like "uint32_t" are not available.  Just use "unsigned int".



> +  /* Count the number of sections we start with.  */
> +  for (section = sobj->sections; section != NULL;section = section->next)
> +    nsects_in++;

Please add a space after the second semicolon.


> +      for (section = sobj->sections, sect = 0; section != NULL;
> +	   section = section->next, sect++)

Please break the line after the first semicolon.


> +      else
> +	{
> +	  char namebuf[MACH_O_NAME_LEN + 1];
> +	  char segnbuf[MACH_O_NAME_LEN + 1];
> +	  char *comma;
> +	  /* Try to extract segment,section from the input name.  */
> +	  memset (namebuf, 0, sizeof namebuf);
> +	  memset (segnbuf, 0, sizeof segnbuf);
> +	  comma = strchr (section->name, ',');

Please add an empty blank line after the variable declarations.

> +	  if (comma)

Please write "comma != NULL".

> +	    {
> +	      int len = comma-section->name;

Please put spaces around the '-'.

> +	      len = len > MACH_O_NAME_LEN ? MACH_O_NAME_LEN : len;
> +	      if (len)
> +	        strncpy (namebuf, section->name, len);

Please write "len > 0".  But really you don't have to test this as
strncpy will do the right thing.

> +	      strncpy (segnbuf, comma+1, MACH_O_NAME_LEN);

Please put spaces around the '+'.

> +  if (wrapping)
> +    {
> +      size_t secsize;
> +      int i;
> +      /* Write the section header for the wrapper.  */
> +      /* Account for any initial aligment - which becomes the alignment for this
> +	 created section.  */
> +      secsize = (offset - index[0]); 

Please put a blank line after the variable declarations.

> +      /* Subtract the wrapper section start from the begining of each sub
> +	 section.  */
> +      for (i=1; i<nsects_in;i++)

Please put spaces around '=', around '<', and a space after the second
semicolon.

> +      /* Now do the index, we'll align this to 4 bytes although the read code
> +	 will handle unaligned.  */
> +      offset += 3; offset &= ~0x03;

Please break the line after the first semicolon.

Ian