Iain Sandoe <develo...@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