[PATCH 3/4] pe/coff: Keep .build-id with --only-keep-debug

[PATCH 3/4] pe/coff: Keep .build-id with --only-keep-debug

[Jon TURNEY]

Keep .build-id with --only-keep-debug (STRIP_NONDEBUG)

binutils/ChangeLog:

2014-04-10  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* objcopy.c (is_nondebug_keep_contents_section): New function.
	(setup_section): Use it.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 binutils/objcopy.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 873908c..cf7da1b 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -1141,6 +1141,25 @@ is_strip_section (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
   return FALSE;
 }
 
+static bfd_boolean
+is_nondebug_keep_contents_section (bfd *ibfd, asection *isection)
+{
+  /* Always keep ELF note sections. */
+  if (ibfd->xvec->flavour == bfd_target_elf_flavour)
+    return (elf_section_type (isection) == SHT_NOTE);
+
+  /* Always keep the .build-id section for PE/COFF.
+
+     Strictly, this should be written "always keep the section storing the debug
+     directory", but that may be the .text section for objects produced by some
+     tools, which it is not sensible to keep.
+  */
+  if (ibfd->xvec->flavour == bfd_target_coff_flavour)
+    return (strcmp (bfd_get_section_name (ibfd, isection), ".build-id") == 0);
+
+  return FALSE;
+}
+
 /* Return true if SYM is a hidden symbol.  */
 
 static bfd_boolean
@@ -2695,8 +2714,7 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
     flags = p->flags | (flags & (SEC_HAS_CONTENTS | SEC_RELOC));
   else if (strip_symbols == STRIP_NONDEBUG
 	   && (flags & (SEC_ALLOC | SEC_GROUP)) != 0
-	   && !(ibfd->xvec->flavour == bfd_target_elf_flavour
-		&& elf_section_type (isection) == SHT_NOTE))
+           && !is_nondebug_keep_contents_section(ibfd, isection))
     {
       flags &= ~(SEC_HAS_CONTENTS | SEC_LOAD | SEC_GROUP);
       if (obfd->xvec->flavour == bfd_target_elf_flavour)
-- 
1.8.3.4

[PATCH 4/4] pe/coff: Don't break .build-id with objcopy/strip

[Jon TURNEY]

The file offsets in the debug directory need to be recalculated in
_bfd_XX_bfd_copy_private_bfd_data_common(), so that if an objcopy or strip adds
or removes something which causes the file offset of the CV record to change,
those offsets are correct.

(This can happen due to either sections being added or removed before the
.build-id section directly affecting it's file offset, or by the addition or
removal of sufficent section headers to change the first section's file offset
and hence all subsequent sections' file offsets.)

(In general _bfd_XX_bfd_copy_private_bfd_data_common() could be a whole lot
smarter about adjusting the DataDirectory.  If a section corresponding to a RVA
is removed or no longer has any contents, that entry should be cleared?  Offsets
should always be recomputed?)

bfd/ChangeLog:

2014-04-10  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* peXXigen.c (is_vma_in_section, find_section_by_vma): New
	functions.
	(_bfd_XX_bfd_copy_private_bfd_data_common): Recalculate file
	offsets in the debug directory.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 bfd/peXXigen.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index c00a655..837ff6c 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -2651,6 +2651,19 @@ _bfd_XX_print_private_bfd_data_common (bfd * abfd, void * vfile)
   return TRUE;
 }
 
+static bfd_boolean
+is_vma_in_section (bfd *abfd ATTRIBUTE_UNUSED, asection *sect, void *obj)
+{
+  bfd_vma addr = *(bfd_vma *)obj;
+  return (addr >= sect->vma) && (addr < (sect->vma + sect->size));
+}
+
+static asection *
+find_section_by_vma (bfd *abfd, bfd_vma addr)
+{
+  return bfd_sections_find_if (abfd, is_vma_in_section, (void *)&addr);
+}
+
 /* Copy any private info we understand from the input bfd
    to the output bfd.  */
 
@@ -2689,6 +2702,43 @@ _bfd_XX_bfd_copy_private_bfd_data_common (bfd * ibfd, bfd * obfd)
       && ! (pe_data (ibfd)->real_flags & IMAGE_FILE_RELOCS_STRIPPED))
     pe_data (obfd)->dont_strip_reloc = 1;
 
+  /* The file offsets contained in the debug directory need rewriting. */
+  if (ope->pe_opthdr.DataDirectory[PE_DEBUG_DATA].Size != 0)
+    {
+      bfd_vma addr = ope->pe_opthdr.DataDirectory[PE_DEBUG_DATA].VirtualAddress + ope->pe_opthdr.ImageBase;
+      asection *section = find_section_by_vma (obfd, addr);
+      bfd_byte *data;
+
+      if (section && bfd_malloc_and_get_section (obfd, section, &data))
+        {
+          unsigned int i;
+          struct external_IMAGE_DEBUG_DIRECTORY *dd = (struct external_IMAGE_DEBUG_DIRECTORY *)(data + (addr - section->vma));
+
+          for (i = 0; i < ope->pe_opthdr.DataDirectory[PE_DEBUG_DATA].Size/sizeof (struct external_IMAGE_DEBUG_DIRECTORY); i++)
+            {
+              asection *ddsection;
+              struct external_IMAGE_DEBUG_DIRECTORY *edd = &(dd[i]);
+              struct internal_IMAGE_DEBUG_DIRECTORY idd;
+
+              _bfd_XXi_swap_debugdir_in (obfd, edd, &idd);
+
+              if (idd.AddressOfRawData == 0)
+                continue; /* RVA 0 means only offset is valid, not handled yet. */
+
+              ddsection = find_section_by_vma (obfd, idd.AddressOfRawData + ope->pe_opthdr.ImageBase);
+              if (!ddsection)
+                continue; /* Not in a section! */
+
+              idd.PointerToRawData = ddsection->filepos + (idd.AddressOfRawData + ope->pe_opthdr.ImageBase) - ddsection->vma;
+
+              _bfd_XXi_swap_debugdir_out (obfd, &idd, edd);
+            }
+
+          if (!bfd_set_section_contents (obfd, section, data, 0, section->size))
+            _bfd_error_handler (_("Failed to update file offsets in debug directory"));
+        }
+    }
+
   return TRUE;
 }
 
-- 
1.8.3.4

[PATCH 0/4] Fixes for support for 'ld --build-id' to pe/coff

[Jon TURNEY]

On 08/04/2014 11:04, Nicholas Clifton wrote:
>> I also posted a set of patches back in January [1].  I'd be grateful for any
>> comments on those.
>>
>> [1] https://sourceware.org/ml/binutils/2014-01/msg00296.html
> 
> Oops - sorry - that one slipped through the net.
> 
> I reviewed and applied the patches.  They were OK although there were a few
> minor problems:

I think you also fixed a bug I'd introduced where generate_build_id() could 
potentially overrun it's output buffer, so thanks for that.

Following this mail are a few patches to fix issues with pe/coff build-ids 
discovered since those original patches were posted.

Jon TURNEY (4):
  pe/coff: Display GUID build-id in the conventional way
  pe/coff: Don't interpret debug directory in section with no contents
  pe/coff: Keep .build-id with --only-keep-debug
  pe/coff: Don't break .build-id with objcopy/strip

 bfd/peXXigen.c     | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 binutils/objcopy.c | 22 +++++++++++++++--
 2 files changed, 91 insertions(+), 3 deletions(-)

-- 
1.8.3.4

[PATCH 2/4] pe/coff: Don't interpret debug directory in section with no contents

[Jon TURNEY]

For 'objdump -p', don't try to interpret the contents of a debug directory in a
section with no contents.

The meaning of a series of zeros is not very interesting.

bfd/ChangeLog:

2014-04-10 Jon TURNEY <jon.turney@dronecode.org.uk>

	* peXXigen.c (pe_print_debugdata): Don't interpret debug directory
	in a section with no contents.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 bfd/peXXigen.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index ebb80b1..c00a655 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -2388,6 +2388,13 @@ pe_print_debugdata (bfd * abfd, void * vfile)
                _("\nThere is a debug directory, but the section containing it could not be found\n"));
       return TRUE;
     }
+  else if (!(section->flags & SEC_HAS_CONTENTS))
+    {
+      fprintf (file,
+               _("\nThere is a debug directory in %s, but that section has no contents\n"),
+               section->name);
+      return TRUE;
+    }
 
   fprintf (file, _("\nThere is a debug directory in %s at 0x%lx\n\n"),
 	   section->name, (unsigned long) addr);
-- 
1.8.3.4

[PATCH 1/4] pe/coff: Display GUID build-id in the conventional way

[Jon TURNEY]

bfd/ChangeLog:

2014-04-10  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* peXXigen.c (pe_print_debugdata): Display GUID build-id in the
	conventional way

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 bfd/peXXigen.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index ea7846f..ebb80b1 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -2435,7 +2435,20 @@ pe_print_debugdata (bfd * abfd, void * vfile)
 					       idd.SizeOfData, cvinfo))
             continue;
 
-          for (i = 0; i < cvinfo->SignatureLength; i++)
+          /* The standard way to display a GUID seems to be as 4,2,2 bytes in
+             host order, followed by 8 single bytes, which we adopt for
+             consistency with other tools.  Display non-GUID signatures as a
+             sequence of bytes. */
+          i = 0;
+          if (cvinfo->SignatureLength >= CV_INFO_SIGNATURE_LENGTH)
+            {
+              char *guid = &(cvinfo->Signature[0]);
+              sprintf (signature, "%08x", *(uint32_t *)(guid));
+              sprintf (signature + 8, "%04x", *(uint16_t *)(guid + 4));
+              sprintf (signature + 12, "%04x", *(uint16_t *)(guid + 6));
+              i = 8;
+            }
+          for (; i < cvinfo->SignatureLength; i++)
             sprintf (&signature[i*2], "%02x", cvinfo->Signature[i] & 0xff);
 
           fprintf (file, "(format %c%c%c%c signature %s age %ld)\n",
-- 
1.8.3.4

Re: [PATCH 0/4] Fixes for support for 'ld --build-id' to pe/coff

[Christopher Faylor]

Nick,
Any comment on these? I'd like to include them in my next version of binutils
for Cygwin.

On Thu, Apr 10, 2014 at 06:45:04PM +0100, Jon TURNEY wrote:
>On 08/04/2014 11:04, Nicholas Clifton wrote:
>>> I also posted a set of patches back in January [1].  I'd be grateful for any
>>> comments on those.
>>>
>>> [1] https://sourceware.org/ml/binutils/2014-01/msg00296.html
>> 
>> Oops - sorry - that one slipped through the net.
>> 
>> I reviewed and applied the patches.  They were OK although there were a few
>> minor problems:
>
>I think you also fixed a bug I'd introduced where generate_build_id() could 
>potentially overrun it's output buffer, so thanks for that.
>
>Following this mail are a few patches to fix issues with pe/coff build-ids 
>discovered since those original patches were posted.
>
>Jon TURNEY (4):
>  pe/coff: Display GUID build-id in the conventional way
>  pe/coff: Don't interpret debug directory in section with no contents
>  pe/coff: Keep .build-id with --only-keep-debug
>  pe/coff: Don't break .build-id with objcopy/strip
>
> bfd/peXXigen.c     | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> binutils/objcopy.c | 22 +++++++++++++++--
> 2 files changed, 91 insertions(+), 3 deletions(-)
>
>-- 
>1.8.3.4
>
>

Re: [PATCH 1/4] pe/coff: Display GUID build-id in the conventional way

[Christopher Faylor]

On Thu, Apr 10, 2014 at 06:45:05PM +0100, Jon TURNEY wrote:
>bfd/ChangeLog:
>
>2014-04-10  Jon TURNEY  <jon.turney@dronecode.org.uk>
>
>	* peXXigen.c (pe_print_debugdata): Display GUID build-id in the
>	conventional way
>
>Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
>---
> bfd/peXXigen.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
>index ea7846f..ebb80b1 100644
>--- a/bfd/peXXigen.c
>+++ b/bfd/peXXigen.c
>@@ -2435,7 +2435,20 @@ pe_print_debugdata (bfd * abfd, void * vfile)
> 					       idd.SizeOfData, cvinfo))
>             continue;
> 
>-          for (i = 0; i < cvinfo->SignatureLength; i++)
>+          /* The standard way to display a GUID seems to be as 4,2,2 bytes in
>+             host order, followed by 8 single bytes, which we adopt for
>+             consistency with other tools.  Display non-GUID signatures as a
>+             sequence of bytes. */
>+          i = 0;
>+          if (cvinfo->SignatureLength >= CV_INFO_SIGNATURE_LENGTH)
>+            {
>+              char *guid = &(cvinfo->Signature[0]);
Isn't this just 'cvinfo->Signature'?

>+              sprintf (signature, "%08x", *(uint32_t *)(guid));
Can't this just be coalesced into one sprintf?

>+              sprintf (signature + 8, "%04x", *(uint16_t *)(guid + 4));
>+              sprintf (signature + 12, "%04x", *(uint16_t *)(guid + 6));
>+              i = 8;
>+            }

cgf

RE: [PATCH 1/4] pe/coff: Display GUID build-id in the conventional way

[Pierre Muller]

Hi everyone,


> > bfd/peXXigen.c | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> >diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
> >index ea7846f..ebb80b1 100644
> >--- a/bfd/peXXigen.c
> >+++ b/bfd/peXXigen.c
> >@@ -2435,7 +2435,20 @@ pe_print_debugdata (bfd * abfd, void * vfile)
> > 					       idd.SizeOfData, cvinfo))
> >             continue;
> >
> >-          for (i = 0; i < cvinfo->SignatureLength; i++)
> >+          /* The standard way to display a GUID seems to be as 4,2,2
> bytes in
> >+             host order, followed by 8 single bytes, which we adopt
> for
> >+             consistency with other tools.  Display non-GUID
> signatures as a
> >+             sequence of bytes. */
> >+          i = 0;
> >+          if (cvinfo->SignatureLength >= CV_INFO_SIGNATURE_LENGTH)
> >+            {
> >+              char *guid = &(cvinfo->Signature[0]);
> Isn't this just 'cvinfo->Signature'?
> 
> >+              sprintf (signature, "%08x", *(uint32_t *)(guid));
> Can't this just be coalesced into one sprintf?
> 
> >+              sprintf (signature + 8, "%04x", *(uint16_t *)(guid +
> 4));
> >+              sprintf (signature + 12, "%04x", *(uint16_t *)(guid +
 
  I was also wondering if this would not require some
byte swapping to write the 32 and 16 bit integers correctly on
a big endian machine.

Pierre Muller

Re: [PATCH 1/4] pe/coff: Display GUID build-id in the conventional way

[Jon TURNEY]

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

On 21/04/2014 18:25, Pierre Muller wrote:
>   I was also wondering if this would not require some
> byte swapping to write the 32 and 16 bit integers correctly on
> a big endian machine.

Yes, I don't think I've got this right.

I think you are quite correct that with this patch I also need to do some
swapping on write to make a GUID set with --buildid=0xhexdigits be reported in
a consistent way.

I think I've been confused by the fact that these components of the GUID are
said to be in native byte-order, but that really means little-endian since (I
think) these files could only sensibly exist for a little-endian target.

Replacement patch attached.

[-- Attachment #2: 0001-pe-coff-Swap-GUID-build-id-on-read-and-write.patch --]
[-- Type: text/plain, Size: 2620 bytes --]

From 8b14872e2954c6dc3a43ce5eec0fc72753176cdf Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Mon, 21 Apr 2014 21:11:06 +0100
Subject: [PATCH] pe/coff: Swap GUID build-id on read and write

Byte-swap GUID build-id on read and write so it is consistently displayed in the
conventional way.

bfd/ChangeLog:

2014-04-21  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* peXXigen.c (_bfd_XXi_slurp_codeview_record)
	(_bfd_XXi_write_codeview_record): Byte-swap GUID from
	little-endian to big-endian order for consistent and conventional
	display.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 bfd/peXXigen.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index ea7846f..2de64a2 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -1088,7 +1088,15 @@ _bfd_XXi_slurp_codeview_record (bfd * abfd, file_ptr where, unsigned long length
       CV_INFO_PDB70 *cvinfo70 = (CV_INFO_PDB70 *)(buffer);
 
       cvinfo->Age = H_GET_32(abfd, cvinfo70->Age);
-      memcpy (cvinfo->Signature, cvinfo70->Signature, CV_INFO_SIGNATURE_LENGTH);
+
+      /* A GUID consists of 4,2,2 byte values in little-endian order, followed
+         by 8 single bytes. Byte swap then so we can conveniently treat the GUID
+         as 16 bytes in big-endian order. */
+      bfd_putb32 (bfd_getl32 (cvinfo70->Signature), cvinfo->Signature);
+      bfd_putb16 (bfd_getl16 (&(cvinfo70->Signature[4])), &(cvinfo->Signature[4]));
+      bfd_putb16 (bfd_getl16 (&(cvinfo70->Signature[6])), &(cvinfo->Signature[6]));
+      memcpy (&(cvinfo->Signature[8]), &(cvinfo70->Signature[8]), 8);
+
       cvinfo->SignatureLength = CV_INFO_SIGNATURE_LENGTH;
       // cvinfo->PdbFileName = cvinfo70->PdbFileName;
 
@@ -1121,7 +1129,14 @@ _bfd_XXi_write_codeview_record (bfd * abfd, file_ptr where, CODEVIEW_INFO *cvinf
 
   cvinfo70 = (CV_INFO_PDB70 *) buffer;
   H_PUT_32 (abfd, CVINFO_PDB70_CVSIGNATURE, cvinfo70->CvSignature);
-  memcpy (&(cvinfo70->Signature), cvinfo->Signature, CV_INFO_SIGNATURE_LENGTH);
+
+  /* Byte swap the GUID from 16 bytes in big-endian order to 4,2,2 byte values
+     in little-endian order, followed by 8 single bytes. */
+  bfd_putl32 (bfd_getb32 (cvinfo->Signature), cvinfo70->Signature);
+  bfd_putl16 (bfd_getb16 (&(cvinfo->Signature[4])), &(cvinfo70->Signature[4]));
+  bfd_putl16 (bfd_getb16 (&(cvinfo->Signature[6])), &(cvinfo70->Signature[6]));
+  memcpy (&(cvinfo70->Signature[8]), &(cvinfo->Signature[8]), 8);
+
   H_PUT_32 (abfd, cvinfo->Age, cvinfo70->Age);
   cvinfo70->PdbFileName[0] = '\0';
 
-- 
1.8.5.5

Re: [PATCH 0/4] Fixes for support for 'ld --build-id' to pe/coff

[Jon TURNEY]

On 16/04/2014 16:39, Christopher Faylor wrote:
> Nick,
> Any comment on these? I'd like to include them in my next version of binutils
> for Cygwin.

Ping?

> On Thu, Apr 10, 2014 at 06:45:04PM +0100, Jon TURNEY wrote:
>>
>> Following this mail are a few patches to fix issues with pe/coff build-ids
>> discovered since those original patches were posted.
>>
>> Jon TURNEY (4):
>>   pe/coff: Display GUID build-id in the conventional way
>>   pe/coff: Don't interpret debug directory in section with no contents
>>   pe/coff: Keep .build-id with --only-keep-debug
>>   pe/coff: Don't break .build-id with objcopy/strip
>>
>> bfd/peXXigen.c     | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> binutils/objcopy.c | 22 +++++++++++++++--
>> 2 files changed, 91 insertions(+), 3 deletions(-)
>>
>> --
>> 1.8.3.4

Re: [PATCH 0/4] Fixes for support for 'ld --build-id' to pe/coff

[Nicholas Clifton]

Hi Jon,

> Ping?

Oops - very sorry for dropping the ball on these patches.  I have now 
checked them in (all 4).

Cheers
   Nick

Re: [PATCH 0/4] Fixes for support for 'ld --build-id' to pe/coff

[Jan Kratochvil]

Hello Nick,

On Fri, 16 May 2014 16:36:59 +0200, Nicholas Clifton wrote:
> Oops - very sorry for dropping the ball on these patches.  I have now
> checked them in (all 4).

since this patch on i686 host (that is without --enable-64-bit-bfd):
	./configure --enable-targets=all;make
will fail with:
	ei386pep.c:(.text+0x1fe1): undefined reference to `_bfd_pex64i_swap_debugdir_out'
	ei386pep.c:(.text+0x20a7): undefined reference to `_bfd_pex64i_write_codeview_record'
	collect2: error: ld returned 1 exit status
	Makefile:1031: recipe for target 'ld-new' failed

It happens since:

61e2488cd8497d158303a78563ad40f51f5c3f8e is the first bad commit
commit 61e2488cd8497d158303a78563ad40f51f5c3f8e
Author: Jon TURNEY <jon.turney@dronecode.org.uk>
Date:   Tue Apr 8 10:59:43 2014 +0100
    Add support for generating and inserting build IDs into COFF binaries.

Similar failure for different targets has been fixed recently by:
	[PATCH] bfd/targets: create elf64 selvecs only if 64-bit bfd support is enabled.
	https://sourceware.org/ml/binutils/2014-06/msg00176.html
	https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=886aba9eff0eba428e634f579e8999b67da01b57


Jan

Re: [PATCH 0/4] Fixes for support for 'ld --build-id' to pe/coff

[Nicholas Clifton]

Hi Jan,

> 	ei386pep.c:(.text+0x1fe1): undefined reference to `_bfd_pex64i_swap_debugdir_out'
> 	ei386pep.c:(.text+0x20a7): undefined reference to `_bfd_pex64i_write_codeview_record'

This should be fixed now.

Cheers
   Nick