PR 25562: New binutils testsuite failures

PR 25562: New binutils testsuite failures

[Nick Clifton]

Hi Jozef,

  I am seeing some new binutils testsuite failures for various different
  targets:

Checking Binutils in: arm-wince-pe ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: i686-pc-cygwin ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: i686-pc-mingw32 ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: mcore-pe ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: mips-elf ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: mips-sgi-irix6 ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: mmix-mmixware ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: tx39-elf ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: x86_64-pc-cygwin ... BIN REGRESSION: objcopy executable (pr25662)  
Checking Binutils in: x86_64-pc-mingw64 ... BIN REGRESSION: objcopy executable (pr25662)  

  Would you mind taking a look please ?

Cheers
  Nick
  

Re: PR 25562: New binutils testsuite failures

[Jozef Lawrynowicz]

On Mon, 30 Mar 2020 10:48:32 +0100
Nick Clifton <nickc@redhat.com> wrote:

> Hi Jozef,
> 
>   I am seeing some new binutils testsuite failures for various different
>   targets:
> 
> Checking Binutils in: arm-wince-pe ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: i686-pc-cygwin ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: i686-pc-mingw32 ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: mcore-pe ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: mips-elf ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: mips-sgi-irix6 ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: mmix-mmixware ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: tx39-elf ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: x86_64-pc-cygwin ... BIN REGRESSION: objcopy executable (pr25662)  
> Checking Binutils in: x86_64-pc-mingw64 ... BIN REGRESSION: objcopy executable (pr25662)  
> 
>   Would you mind taking a look please ?

Hi Nick,

The test is working as expected, for those targets an objcopy of an executable
is not creating an exact copy of the original file.

Alan Modra has already done a triage of the failures (see this thread
https://sourceware.org/pipermail/binutils/2020-March/110422.html), and has setup
XFAILs for some targets. For the remaining failures I think the maintainers of
those targets need to investigate to see if XFAILs are necessary or if there is
a real bug in the toolchain.

For the PE targets the datestamp in the executable file is not being copied by
objcopy.

For the MIPS targets, objcopy has added the names of output sections to the
string table, where in the linked executable they were not in the string table.

I can file bugzillas for these if you would like.

Thanks,
Jozef
> 
> Cheers
>   Nick
>   
> 

Re: PR 25562: New binutils testsuite failures

[Nick Clifton]

Hi Jozef,

> The test is working as expected, for those targets an objcopy of an executable
> is not creating an exact copy of the original file.

Ah, OK.

> For the PE targets the datestamp in the executable file is not being copied by
> objcopy.

The old determinism problem.  *sigh*  We should probably just xfail these
targets since we know that the binaries can never be the same.

> For the MIPS targets, objcopy has added the names of output sections to the
> string table, where in the linked executable they were not in the string table.

Where were the names stored then, if not in the string table ?  Or did the
objcopy actually add new section symbols ?

This does sound like something that the MIPS maintainers ought to investigate
and decide whether it is a real bug or not.

> I can file bugzillas for these if you would like.

No, that should not be necessary.  I will check in an update for the PE targets
once I have tested it locally.

Cheers
  Nick

Re: PR 25562: New binutils testsuite failures

[Jozef Lawrynowicz]

On Mon, 30 Mar 2020 13:26:53 +0100
Nick Clifton <nickc@redhat.com> wrote:

> Hi Jozef,
> 
> > The test is working as expected, for those targets an objcopy of an executable
> > is not creating an exact copy of the original file.  
> 
> Ah, OK.
> 
> > For the PE targets the datestamp in the executable file is not being copied by
> > objcopy.  
> 
> The old determinism problem.  *sigh*  We should probably just xfail these
> targets since we know that the binaries can never be the same.

I should clarify, the datestamp is actually just being reset completely to
the epoch, rather than being set to the time of the objcopy.

> 
> > For the MIPS targets, objcopy has added the names of output sections to the
> > string table, where in the linked executable they were not in the string table.  
> 
> Where were the names stored then, if not in the string table ?  Or did the
> objcopy actually add new section symbols ?

They're in .shstrab originally, but then the objcopy adds them to .strtab as
well. Here's the readelf output (bintest is the original file):

$ ../readelf -p .strtab -p .shstrtab bintest

String dump of section '.strtab':
  [     1]  tmpdir/bintest.o
  [    12]  a
  [    14]  c
  [    16]  _start


String dump of section '.shstrtab':
  [     1]  .symtab
  [     9]  .strtab
  [    11]  .shstrtab
  [    1b]  .data
  [    21]  .text
  [    27]  .MIPS.abiflags
  [    36]  .bss
  [    3b]  .reginfo
  [    44]  .gnu.attributes

$ ../readelf -p .strtab -p .shstrtab copy

String dump of section '.strtab':
  [     1]  .data
  [     7]  .text
  [     d]  .MIPS.abiflags
  [    1c]  .bss
  [    21]  .reginfo
  [    2a]  .gnu.attributes
  [    3a]  tmpdir/bintest.o
  [    4b]  c
  [    4d]  _start


String dump of section '.shstrtab':
  [     1]  .symtab
  [     9]  .strtab
  [    11]  .shstrtab
  [    1b]  .data
  [    21]  .text
  [    27]  .MIPS.abiflags
  [    36]  .bss
  [    3b]  .reginfo
  [    44]  .gnu.attributes

This means that in the copied executable you can see the names of the SECTION
type symbols in .symtab:

$ ../readelf -s bintest

Symbol table '.symtab' contains 11 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 00000000     0 SECTION LOCAL  DEFAULT    1 
     2: 00001204     0 SECTION LOCAL  DEFAULT    2 
     3: 00001208     0 SECTION LOCAL  DEFAULT    3 
     4: 00000201     0 SECTION LOCAL  DEFAULT    4 
     5: 00000000     0 SECTION LOCAL  DEFAULT    5 
     6: 00000000     0 SECTION LOCAL  DEFAULT    6 
............. snip ........

$ ../readelf -s copy

Symbol table '.symtab' contains 11 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 00000000     0 SECTION LOCAL  DEFAULT    1 .data
     2: 00001204     0 SECTION LOCAL  DEFAULT    2 .text
     3: 00001208     0 SECTION LOCAL  DEFAULT    3 .MIPS.abiflags
     4: 00000201     0 SECTION LOCAL  DEFAULT    4 .bss
     5: 00000000     0 SECTION LOCAL  DEFAULT    5 .reginfo
     6: 00000000     0 SECTION LOCAL  DEFAULT    6 .gnu.attributes
............... snip ............


> 
> This does sound like something that the MIPS maintainers ought to investigate
> and decide whether it is a real bug or not.
> 
> > I can file bugzillas for these if you would like.  
> 
> No, that should not be necessary.  I will check in an update for the PE targets
> once I have tested it locally.

Ok sounds good, thanks.
Jozef

> 
> Cheers
>   Nick
> 
> 

Re: PR 25562: New binutils testsuite failures

[Alan Modra]

On Mon, Mar 30, 2020 at 01:26:53PM +0100, Nick Clifton via Binutils wrote:
> Hi Jozef,
> 
> > The test is working as expected, for those targets an objcopy of an executable
> > is not creating an exact copy of the original file.
> 
> Ah, OK.
> 
> > For the PE targets the datestamp in the executable file is not being copied by
> > objcopy.
> 
> The old determinism problem.  *sigh*  We should probably just xfail these
> targets since we know that the binaries can never be the same.

Except that objcopy -p ought to preserve the time, shouldn't it?

> > For the MIPS targets, objcopy has added the names of output sections to the
> > string table, where in the linked executable they were not in the string table.
> 
> Where were the names stored then, if not in the string table ?  Or did the
> objcopy actually add new section symbols ?

ELF section symbols normally have 0 in st_name, with an implicit name
taken from their section.  MIPS for backwards compatibility with other
linkers gives them a string table entry.  objcopy decided the copy
needed that backward compat, while ld decided the original didn't.

> 
> This does sound like something that the MIPS maintainers ought to investigate
> and decide whether it is a real bug or not.
> 
> > I can file bugzillas for these if you would like.
> 
> No, that should not be necessary.  I will check in an update for the PE targets
> once I have tested it locally.
> 
> Cheers
>   Nick
> 

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR 25562: New binutils testsuite failures

[Nick Clifton]

Hi Guys,

> Alan Modra wrote:
> Except that objcopy -p ought to preserve the time, shouldn't it?

It certainly should.  I am testing a local patch now that should fix
this...

Cheers
  Nick

Re: PR 25562: New binutils testsuite failures

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

Hi Guys,

  Right, I am checking in the attached patch which makes objcopy's -p
  work with PE format files.  I had to change the insert_timestamp field 
  in the pe_data structure to an integer containing the time to put in
  the header.  The value can be 0 for a deterministic output, and it can
  also be -1, which is interpreted as 'insert the current time'.

  I have rerun the pr25662 tests with this change and all of the PE
  format targets now pass.

Cheers
  Nick

bfd/ChangeLog
2020-03-30  Nick Clifton  <nickc@redhat.com>

	PR binutils/pr25662
	* libcoff-in.h (struct pe_tdata): Rename the insert_timestamp
	field to timestamp and make it an integer.
	* libcoff.h: Regenerate.
	* peXXigen.c (_bfd_XXi_only_swap_filehdr_out): Test the timestamp
	field in the pe_data structure rather than the insert_timestamp
	field.

binutils/ChangeLog
2020-03-30  Nick Clifton  <nickc@redhat.com>

	PR binutils/25662
	* objcopy.c (copy_object): When copying PE format files set the
	timestamp field in the pe_data structure if the preserve_dates
	flag is set.
	* testsuite/binutils-all/objcopy.exp (objcopy_test) Use
	--preserve-dates in place of the -p option, in order to make its
	effect more obvious.

ld/ChangeLog
2020-03-30  Nick Clifton  <nickc@redhat.com>

	PR binutils/25662
	* emultempl/pe.em (after_open): Replace initialisation of the
	insert_timestamp field in the pe_data structure with an
	initialisation of the timestamp field.
	* emultemp/pep.em: Likewise.
	* pe-dll.c (fill_edata): Use the timestamp field in the pe_data
	structure instead of the insert_timestamp field.

[-- Attachment #2: pr25662.patch --]
[-- Type: text/x-patch, Size: 4590 bytes --]

diff --git a/bfd/libcoff-in.h b/bfd/libcoff-in.h
index 3030a65fa7..c86ffc9933 100644
--- a/bfd/libcoff-in.h
+++ b/bfd/libcoff-in.h
@@ -128,7 +128,9 @@ typedef struct pe_tdata
   int has_reloc_section;
   int dont_strip_reloc;
   int dos_message[16];
-  bfd_boolean insert_timestamp;
+  /* The timestamp to insert into the output file.
+     If the timestamp is -1 then the current time is used.  */
+  int timestamp;
   bfd_boolean (*in_reloc_p) (bfd *, reloc_howto_type *);
   flagword real_flags;
 
diff --git a/bfd/libcoff.h b/bfd/libcoff.h
index 4c7be6e935..eeb7b6b995 100644
--- a/bfd/libcoff.h
+++ b/bfd/libcoff.h
@@ -132,7 +132,9 @@ typedef struct pe_tdata
   int has_reloc_section;
   int dont_strip_reloc;
   int dos_message[16];
-  bfd_boolean insert_timestamp;
+  /* The timestamp to insert into the output file.
+     If the timestamp is -1 then the current time is used.  */
+  int timestamp;
   bfd_boolean (*in_reloc_p) (bfd *, reloc_howto_type *);
   flagword real_flags;
 
diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index e42d646552..b9eeb775d9 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -876,10 +876,10 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
 
   /* Use a real timestamp by default, unless the no-insert-timestamp
      option was chosen.  */
-  if ((pe_data (abfd)->insert_timestamp))
+  if ((pe_data (abfd)->timestamp) == -1)
     H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
   else
-    H_PUT_32 (abfd, 0, filehdr_out->f_timdat);
+    H_PUT_32 (abfd, pe_data (abfd)->timestamp, filehdr_out->f_timdat);
 
   PUT_FILEHDR_SYMPTR (abfd, filehdr_in->f_symptr,
 		      filehdr_out->f_symptr);
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index e6711a99fb..738ef4c2c9 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2774,6 +2774,11 @@ copy_object (bfd *ibfd, bfd *obfd, const bfd_arch_info_type *input_arch)
 
 		     file_alignment, section_alignment);
 	}
+
+      if (preserve_dates
+	  && bfd_get_flavour (ibfd) == bfd_target_coff_flavour
+	  && bfd_pei_p (ibfd))
+	pe->timestamp = pe_data (ibfd)->coff.timestamp;
     }
 
   if (isympp)
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index e4eb53cf84..56a7db8199 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -76,7 +76,7 @@ proc objcopy_test {testname srcfile type asflags ldflags} {
 	    unresolved "objcopy $type ($testname)"
 	    return
 	}
-	set xflags "-p"
+	set xflags "--preserve-dates"
     }
 
     set got [binutils_run $OBJCOPY "$OBJCOPYFLAGS $xflags $t_tempfile $t_copyfile"]
diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index db23b221d6..4fe195ec32 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -1375,7 +1375,10 @@ gld_${EMULATION_NAME}_after_open (void)
   pe_data (link_info.output_bfd)->pe_opthdr = pe;
   pe_data (link_info.output_bfd)->dll = init[DLLOFF].value;
   pe_data (link_info.output_bfd)->real_flags |= real_flags;
-  pe_data (link_info.output_bfd)->insert_timestamp = insert_timestamp;
+  if (insert_timestamp)
+    pe_data (link_info.output_bfd)->timestamp = -1;
+  else
+    pe_data (link_info.output_bfd)->timestamp = 0;
 
   /* At this point we must decide whether to use long section names
      in the output or not.  If the user hasn't explicitly specified
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 3d09a0a6b1..3e03eb3a6e 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -1364,7 +1364,10 @@ gld_${EMULATION_NAME}_after_open (void)
   pe_data (link_info.output_bfd)->pe_opthdr = pep;
   pe_data (link_info.output_bfd)->dll = init[DLLOFF].value;
   pe_data (link_info.output_bfd)->real_flags |= real_flags;
-  pe_data (link_info.output_bfd)->insert_timestamp = insert_timestamp;
+  if (insert_timestamp)
+    pe_data (link_info.output_bfd)->timestamp = -1;
+  else
+    pe_data (link_info.output_bfd)->timestamp = 0;
 
   /* At this point we must decide whether to use long section names
      in the output or not.  If the user hasn't explicitly specified
diff --git a/ld/pe-dll.c b/ld/pe-dll.c
index 397af8780e..0addde2318 100644
--- a/ld/pe-dll.c
+++ b/ld/pe-dll.c
@@ -1211,8 +1211,10 @@ fill_edata (bfd *abfd, struct bfd_link_info *info ATTRIBUTE_UNUSED)
 
   memset (edata_d, 0, edata_sz);
 
-  if (pe_data (abfd)->insert_timestamp)
+  if (pe_data (abfd)->timestamp == -1)
     H_PUT_32 (abfd, time (0), edata_d + 4);
+  else
+    H_PUT_32 (abfd, pe_data (abfd)->timestamp, edata_d + 4);
 
   if (pe_def_file->version_major != -1)
     {

Re: PR 25562: New binutils testsuite failures

[Maciej W. Rozycki]

On Mon, 30 Mar 2020, Alan Modra via Binutils wrote:

> > Where were the names stored then, if not in the string table ?  Or did the
> > objcopy actually add new section symbols ?
> 
> ELF section symbols normally have 0 in st_name, with an implicit name
> taken from their section.  MIPS for backwards compatibility with other
> linkers gives them a string table entry.  objcopy decided the copy
> needed that backward compat, while ld decided the original didn't.

 This came from commit 174fd7f95561 ("New bfd elf hook: force naming of 
local section symbols"), 
<https://sourceware.org/ml/binutils/2004-02/msg00072.html>; I guess the 
hook only needs to return TRUE iff (elf_elfheader (abfd)->e_type == 
ET_REL).  Like below.

 However this is actually not enough to fix the `pr25662' test case, 
because apparently we have another MIPS IRIX-compatibility bug, which 
makes `sh_info' set differently for `.symtab' by `objcopy' -- it's 10 in 
input and 7 in output; of course the underlying cause is IRIX's different 
interpretation of symbol table splitting.  Which happens regardless of 
this change and is possibly more serious than just an inconsistency with 
section symbol names.

 So I don't feel like committing this as it stands: I think we need the 
other bug tracked down and then all this mess covered with proper 
testsuite cases before I feel comfortable with making such changes.

  Maciej
---
 bfd/elfxx-mips.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

binutils-mips-bfd-local-section-symbols-et-rel.diff
Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c
+++ binutils/bfd/elfxx-mips.c
@@ -7284,7 +7284,7 @@ _bfd_mips_elf_eh_frame_address_size (bfd
 bfd_boolean
 _bfd_mips_elf_name_local_section_symbols (bfd *abfd)
 {
-  return SGI_COMPAT (abfd);
+  return elf_elfheader (abfd)->e_type == ET_REL && SGI_COMPAT (abfd);
 }
 \f
 /* Work over a section just before writing it out.  This routine is

Re: PR 25562: New binutils testsuite failures

[Maciej W. Rozycki]

On Tue, 31 Mar 2020, Maciej W. Rozycki wrote:

> > > Where were the names stored then, if not in the string table ?  Or did the
> > > objcopy actually add new section symbols ?
> > 
> > ELF section symbols normally have 0 in st_name, with an implicit name
> > taken from their section.  MIPS for backwards compatibility with other
> > linkers gives them a string table entry.  objcopy decided the copy
> > needed that backward compat, while ld decided the original didn't.
> 
>  This came from commit 174fd7f95561 ("New bfd elf hook: force naming of 
> local section symbols"), 
> <https://sourceware.org/ml/binutils/2004-02/msg00072.html>; I guess the 
> hook only needs to return TRUE iff (elf_elfheader (abfd)->e_type == 
> ET_REL).  Like below.
> 
>  However this is actually not enough to fix the `pr25662' test case, 
> because apparently we have another MIPS IRIX-compatibility bug, which 
> makes `sh_info' set differently for `.symtab' by `objcopy' -- it's 10 in 
> input and 7 in output; of course the underlying cause is IRIX's different 
> interpretation of symbol table splitting.  Which happens regardless of 
> this change and is possibly more serious than just an inconsistency with 
> section symbol names.
> 
>  So I don't feel like committing this as it stands: I think we need the 
> other bug tracked down and then all this mess covered with proper 
> testsuite cases before I feel comfortable with making such changes.

 For the record; I meant to post it earlier.

 I have found some time and incentive to look into it, and actually we 
have three bugs here (with targets using the IRIX aka non-traditional ELF 
format):

1. `objcopy' will give names to local section symbols in copies of 
   non-ET_REL files (according to commit 174fd7f95561 ("New bfd elf hook:  
   force naming of local section symbols"), the peculiarity is a 
   workaround for a MIPSpro static linker bug); fixed with my proposal 
   previously posted.

2. LD will *not* give names to local section symbols in relocatable links, 
   which obviously makes its output as useless as GAS output would be in 
   the absence of commit 174fd7f95561.

3. LD will *not* set the `sh_info' field of the static symbol table 
   according to IRIX rules (i.e. to split the table between section and 
   other symbols), unlike GAS or `objcopy'.

I have fixed the three issues, folding #1 and #2 together.  Posted as: 
<https://sourceware.org/pipermail/binutils/2020-July/112545.html>, fixing 
the `pr25662' test case as well, and committed as commit 3f1b17bbf022 
("MIPS/LD: Set symtab's `sh_info' correctly for IRIX emulations") and 
commit c77cb2a09c55 ("MIPS: Make the IRIX naming of local section symbols 
consistent").

  Maciej