[PATCH] Core file build-ids

[PATCH] Core file build-ids

[Keith Seitz]

This patch adds support for reading build-ids from core files.  I've
opted to maintain the current interfaces that BFD and GDB already use.

The approach taken is fairly straightforward (YMMV). When a core file
is opened, BFD loops over the various segments in the ELF file and
program headers. During this time, whenever a segment whose type is
PT_LOAD is encountered, and the build-id is unknown, I've added a call
to inspect the segment for an ELF header.  If a valid ELF file header
is found, it attempts to read in the program headers and scan for any
notes.

For those unfamiliar with build-ids, please see
https://fedoraproject.org/wiki/Releases/FeatureBuildId .

Yes, this feature has been in Fedora GDB for over a decade, but it exists
as a copy-n-paste of a lot of BFD's internals. For the brave, you can
read the original submission at
https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .

This is an attempt to clean that up.

I have run this patch through GDB's buildbot, and it has detected
no problems.

I'm a BFD novice, and the approach used here could be more conservative
(such as only reading build-id notes) -- careful review is warranted.

Keith

bfd/ChangeLog:

	* bfd-in.h (_bfd_elf_core_find_build_id): New function.
	* bfd-in2.h: Regenerate.
	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
	field.
	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
	functions.
	(elf_read_notes): Add declaration.
	* elf.c (elf_read_notes): Move elf-bfd.h.
	(_bfd_elf_core_find_build_id): New function.
	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
	build-id if none is known.
	(elf_parse_notes): For core files, scan for notes.
	* elfcore.h (elf_core_file_matches_executable_p): If both
	BFDs have identical build-ids, then they match.
	(_bfd_elf_core_find_build_id): New function.
	* elfxx-target.h (elf_backend_core_find_build_id): Define.
	(elfNN_bed): Add elf_backend_core_find_build_id.
---
 bfd/bfd-in.h       |  2 +
 bfd/bfd-in2.h      |  2 +
 bfd/elf-bfd.h      |  8 ++++
 bfd/elf.c          | 73 ++++++++++++++++++++++--------------
 bfd/elfcore.h      | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 bfd/elfxx-target.h |  4 ++
 6 files changed, 153 insertions(+), 28 deletions(-)

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index 890a79d409..868be0bedd 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -763,6 +763,8 @@ extern bfd_boolean bfd_cr16_elf32_create_embedded_relocs
   (bfd *, struct bfd_link_info *, struct bfd_section *, struct bfd_section *,
    char **);
 
+extern void _bfd_elf_core_find_build_id (bfd *, bfd_vma);
+
 /* SunOS shared library support routines for the linker.  */
 
 extern struct bfd_link_needed_list *bfd_sunos_get_needed_list
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 450c7b7fb1..c9b7171eea 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -770,6 +770,8 @@ extern bfd_boolean bfd_cr16_elf32_create_embedded_relocs
   (bfd *, struct bfd_link_info *, struct bfd_section *, struct bfd_section *,
    char **);
 
+extern void _bfd_elf_core_find_build_id (bfd *, bfd_vma);
+
 /* SunOS shared library support routines for the linker.  */
 
 extern struct bfd_link_needed_list *bfd_sunos_get_needed_list
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 0d12f4533a..4a4a47d188 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1371,6 +1371,8 @@ struct elf_backend_data
      int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
 				bfd_size_type len));
 
+  void (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
+
   /* This function is used by `_bfd_elf_get_synthetic_symtab';
      see elf.c.  */
   bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
@@ -2350,6 +2352,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
   (bfd *, bfd *);
 extern int bfd_elf32_core_file_pid
   (bfd *);
+extern void _bfd_elf32_core_find_build_id
+  (bfd *, bfd_vma);
 
 extern bfd_boolean bfd_elf32_swap_symbol_in
   (bfd *, const void *, const void *, Elf_Internal_Sym *);
@@ -2396,6 +2400,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
   (bfd *, bfd *);
 extern int bfd_elf64_core_file_pid
   (bfd *);
+extern void _bfd_elf64_core_find_build_id
+  (bfd *, bfd_vma);
 
 extern bfd_boolean bfd_elf64_swap_symbol_in
   (bfd *, const void *, const void *, Elf_Internal_Sym *);
@@ -2704,6 +2710,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
 extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
 extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
 extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
+extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
+				   size_t align) ;
 
 extern bfd_boolean _bfd_elf_parse_gnu_properties
   (bfd *, Elf_Internal_Note *);
diff --git a/bfd/elf.c b/bfd/elf.c
index b463f1df8b..1d86a0a4d6 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
 static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
 static bfd_boolean prep_headers (bfd *);
 static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
-static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
-				   size_t align) ;
 static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
 				    file_ptr offset, size_t align);
 
@@ -3008,6 +3006,13 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
   return TRUE;
 }
 
+void
+_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
+{
+  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
+    (templ, offset);
+}
+
 bfd_boolean
 bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
 {
@@ -3019,7 +3024,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
       return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
 
     case PT_LOAD:
-      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
+      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
+	return FALSE;
+      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
+	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
+      return TRUE;
 
     case PT_DYNAMIC:
       return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
@@ -11667,34 +11676,42 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
 
 	case bfd_core:
 	  {
+	    if (in.namesz == sizeof "GNU" && strcmp (in.namedata, "GNU") == 0)
+	      {
+		if (! elfobj_grok_gnu_note (abfd, &in))
+		  return FALSE;
+	      }
+	    else
+	      {
 #define GROKER_ELEMENT(S,F) {S, sizeof (S) - 1, F}
-	    struct
-	    {
-	      const char * string;
-	      size_t len;
-	      bfd_boolean (* func)(bfd *, Elf_Internal_Note *);
-	    }
-	    grokers[] =
-	    {
-	      GROKER_ELEMENT ("", elfcore_grok_note),
-	      GROKER_ELEMENT ("FreeBSD", elfcore_grok_freebsd_note),
-	      GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
-	      GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
-	      GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
-	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
-	    };
+		struct
+		{
+		  const char * string;
+		  size_t len;
+		  bfd_boolean (* func)(bfd *, Elf_Internal_Note *);
+		}
+		grokers[] =
+		  {
+		   GROKER_ELEMENT ("", elfcore_grok_note),
+		   GROKER_ELEMENT ("FreeBSD", elfcore_grok_freebsd_note),
+		   GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
+		   GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
+		   GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
+		   GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
+		  };
 #undef GROKER_ELEMENT
-	    int i;
+		int i;
 
-	    for (i = ARRAY_SIZE (grokers); i--;)
-	      {
-		if (in.namesz >= grokers[i].len
-		    && strncmp (in.namedata, grokers[i].string,
-				grokers[i].len) == 0)
+		for (i = ARRAY_SIZE (grokers); i--;)
 		  {
-		    if (! grokers[i].func (abfd, & in))
-		      return FALSE;
-		    break;
+		    if (in.namesz >= grokers[i].len
+			&& strncmp (in.namedata, grokers[i].string,
+				    grokers[i].len) == 0)
+		      {
+			if (! grokers[i].func (abfd, & in))
+			  return FALSE;
+			break;
+		      }
 		  }
 	      }
 	    break;
@@ -11721,7 +11738,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
   return TRUE;
 }
 
-static bfd_boolean
+bfd_boolean
 elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
 		size_t align)
 {
diff --git a/bfd/elfcore.h b/bfd/elfcore.h
index 395feb5ef3..395870942c 100644
--- a/bfd/elfcore.h
+++ b/bfd/elfcore.h
@@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
       return FALSE;
     }
 
+  /* If both BFDs have identical build-ids, then they match.  */
+  if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
+      && core_bfd->build_id->size == exec_bfd->build_id->size
+      && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
+		 core_bfd->build_id->size) == 0)
+    return TRUE;
+
   /* See if the name in the corefile matches the executable name.  */
   corename = elf_tdata (core_bfd)->core->program;
   if (corename != NULL)
@@ -313,3 +320,88 @@ wrong:
 fail:
   return NULL;
 }
+
+/* Attempt to find a build-id in a core file from the core file BFD.
+   OFFSET is the file offset to a PT_LOAD segment that may contain
+   the build-id note.  */
+
+void
+NAME(_bfd_elf,core_find_build_id)
+  (bfd *abfd,
+   bfd_vma offset)
+{
+  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
+  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
+  Elf_Internal_Phdr *i_phdr;
+  unsigned int i;
+
+  /* Seek to the position of the segment at OFFSET.  */
+  if (bfd_seek (abfd, offset, SEEK_SET) != 0)
+    return;
+
+  /* Read in the ELF header in external format.  */
+  if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
+    return;
+
+  /* Now check to see if we have a valid ELF file, and one that BFD can
+     make use of.  The magic number must match, the address size ('class')
+     and byte-swapping must match our XVEC entry, and it must have a
+     section header table (FIXME: See comments re sections at top of this
+     file).  */
+
+  if (! elf_file_p (&x_ehdr)
+      || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
+      || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
+    return;
+
+  /* Check that file's byte order matches xvec's */
+  switch (x_ehdr.e_ident[EI_DATA])
+    {
+    case ELFDATA2MSB:		/* Big-endian */
+      if (! bfd_header_big_endian (abfd))
+	return;
+      break;
+    case ELFDATA2LSB:		/* Little-endian */
+      if (! bfd_header_little_endian (abfd))
+	return;
+      break;
+    case ELFDATANONE:		/* No data encoding specified */
+    default:			/* Unknown data encoding specified */
+      return;
+    }
+
+  elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
+#if DEBUG & 1
+  elf_debug_file (&i_ehdr);
+#endif
+
+  if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
+    return;
+
+  /* Read in program headers.  */
+  i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
+					      sizeof (*i_phdr));
+  if (i_phdr == NULL)
+    return;
+
+  if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
+    return;
+
+  /* Read in program headers and parse notes.  */
+  for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
+    {
+      Elf_External_Phdr x_phdr;
+
+      if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
+	return;
+      elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
+
+      if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
+	{
+	  elf_read_notes (abfd, offset + i_phdr->p_offset,
+			  i_phdr->p_filesz, i_phdr->p_align);
+	  if (abfd->build_id != NULL)
+	    return;
+	}
+    }
+}
diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
index e4e7546243..96b96ec62a 100644
--- a/bfd/elfxx-target.h
+++ b/bfd/elfxx-target.h
@@ -517,6 +517,9 @@
 #ifndef elf_backend_bfd_from_remote_memory
 #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
 #endif
+#ifndef elf_backend_core_find_build_id
+#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
+#endif
 #ifndef elf_backend_got_header_size
 #define elf_backend_got_header_size	0
 #endif
@@ -852,6 +855,7 @@ static struct elf_backend_data elfNN_bed =
   elf_backend_mips_rtype_to_howto,
   elf_backend_ecoff_debug_swap,
   elf_backend_bfd_from_remote_memory,
+  elf_backend_core_find_build_id,
   elf_backend_plt_sym_val,
   elf_backend_common_definition,
   elf_backend_common_section_index,
-- 
2.20.1

Re: [PATCH] Core file build-ids

[Sergio Durigan Junior]

On Wednesday, June 05 2019, Keith Seitz wrote:

> This patch adds support for reading build-ids from core files.  I've
> opted to maintain the current interfaces that BFD and GDB already use.
>
> The approach taken is fairly straightforward (YMMV). When a core file
> is opened, BFD loops over the various segments in the ELF file and
> program headers. During this time, whenever a segment whose type is
> PT_LOAD is encountered, and the build-id is unknown, I've added a call
> to inspect the segment for an ELF header.  If a valid ELF file header
> is found, it attempts to read in the program headers and scan for any
> notes.
>
> For those unfamiliar with build-ids, please see
> https://fedoraproject.org/wiki/Releases/FeatureBuildId .
>
> Yes, this feature has been in Fedora GDB for over a decade, but it exists
> as a copy-n-paste of a lot of BFD's internals. For the brave, you can
> read the original submission at
> https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .
>
> This is an attempt to clean that up.

First of all, thanks for doing this.

Just a few comments that came to my mind while looking at the patch.

> I have run this patch through GDB's buildbot, and it has detected
> no problems.
>
> I'm a BFD novice, and the approach used here could be more conservative
> (such as only reading build-id notes) -- careful review is warranted.
>
> Keith
>
> bfd/ChangeLog:
>
> 	* bfd-in.h (_bfd_elf_core_find_build_id): New function.
> 	* bfd-in2.h: Regenerate.
> 	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
> 	field.
> 	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
> 	functions.
> 	(elf_read_notes): Add declaration.
> 	* elf.c (elf_read_notes): Move elf-bfd.h.
> 	(_bfd_elf_core_find_build_id): New function.
> 	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
> 	build-id if none is known.
> 	(elf_parse_notes): For core files, scan for notes.
> 	* elfcore.h (elf_core_file_matches_executable_p): If both
> 	BFDs have identical build-ids, then they match.
> 	(_bfd_elf_core_find_build_id): New function.
> 	* elfxx-target.h (elf_backend_core_find_build_id): Define.
> 	(elfNN_bed): Add elf_backend_core_find_build_id.
> ---
>  bfd/bfd-in.h       |  2 +
>  bfd/bfd-in2.h      |  2 +
>  bfd/elf-bfd.h      |  8 ++++
>  bfd/elf.c          | 73 ++++++++++++++++++++++--------------
>  bfd/elfcore.h      | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>  bfd/elfxx-target.h |  4 ++
>  6 files changed, 153 insertions(+), 28 deletions(-)
>
> diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
> index 890a79d409..868be0bedd 100644
> --- a/bfd/bfd-in.h
> +++ b/bfd/bfd-in.h
> @@ -763,6 +763,8 @@ extern bfd_boolean bfd_cr16_elf32_create_embedded_relocs
>    (bfd *, struct bfd_link_info *, struct bfd_section *, struct bfd_section *,
>     char **);
>  
> +extern void _bfd_elf_core_find_build_id (bfd *, bfd_vma);
> +
>  /* SunOS shared library support routines for the linker.  */
>  
>  extern struct bfd_link_needed_list *bfd_sunos_get_needed_list
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 450c7b7fb1..c9b7171eea 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -770,6 +770,8 @@ extern bfd_boolean bfd_cr16_elf32_create_embedded_relocs
>    (bfd *, struct bfd_link_info *, struct bfd_section *, struct bfd_section *,
>     char **);
>  
> +extern void _bfd_elf_core_find_build_id (bfd *, bfd_vma);
> +
>  /* SunOS shared library support routines for the linker.  */
>  
>  extern struct bfd_link_needed_list *bfd_sunos_get_needed_list
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index 0d12f4533a..4a4a47d188 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -1371,6 +1371,8 @@ struct elf_backend_data
>       int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
>  				bfd_size_type len));
>  
> +  void (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
> +
>    /* This function is used by `_bfd_elf_get_synthetic_symtab';
>       see elf.c.  */
>    bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
> @@ -2350,6 +2352,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
>    (bfd *, bfd *);
>  extern int bfd_elf32_core_file_pid
>    (bfd *);
> +extern void _bfd_elf32_core_find_build_id
> +  (bfd *, bfd_vma);
>  
>  extern bfd_boolean bfd_elf32_swap_symbol_in
>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
> @@ -2396,6 +2400,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
>    (bfd *, bfd *);
>  extern int bfd_elf64_core_file_pid
>    (bfd *);
> +extern void _bfd_elf64_core_find_build_id
> +  (bfd *, bfd_vma);
>  
>  extern bfd_boolean bfd_elf64_swap_symbol_in
>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
> @@ -2704,6 +2710,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
>  extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
> +extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
> +				   size_t align) ;
>  
>  extern bfd_boolean _bfd_elf_parse_gnu_properties
>    (bfd *, Elf_Internal_Note *);
> diff --git a/bfd/elf.c b/bfd/elf.c
> index b463f1df8b..1d86a0a4d6 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
>  static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
>  static bfd_boolean prep_headers (bfd *);
>  static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
> -static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
> -				   size_t align) ;
>  static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
>  				    file_ptr offset, size_t align);
>  
> @@ -3008,6 +3006,13 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
>    return TRUE;
>  }
>  
> +void
> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
> +{
> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
> +    (templ, offset);
> +}
> +
>  bfd_boolean
>  bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>  {
> @@ -3019,7 +3024,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
>  
>      case PT_LOAD:
> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
> +	return FALSE;
> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
> +      return TRUE;
>  
>      case PT_DYNAMIC:
>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
> @@ -11667,34 +11676,42 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>  
>  	case bfd_core:
>  	  {
> +	    if (in.namesz == sizeof "GNU" && strcmp (in.namedata, "GNU") == 0)
> +	      {
> +		if (! elfobj_grok_gnu_note (abfd, &in))
> +		  return FALSE;
> +	      }
> +	    else
> +	      {

You could rewrite this to get rid of the "else" part, which would make
the patch smaller:

  if (in.namesz == sizeof "GNU" && strcmp (in.namedata, "GNU") == 0
      && ! elfobj_grok_gnu_note (abfd, &in))
    return FALSE;

>  #define GROKER_ELEMENT(S,F) {S, sizeof (S) - 1, F}
> -	    struct
> -	    {
> -	      const char * string;
> -	      size_t len;
> -	      bfd_boolean (* func)(bfd *, Elf_Internal_Note *);
> -	    }
> -	    grokers[] =
> -	    {
> -	      GROKER_ELEMENT ("", elfcore_grok_note),
> -	      GROKER_ELEMENT ("FreeBSD", elfcore_grok_freebsd_note),
> -	      GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
> -	      GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
> -	      GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
> -	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
> -	    };
> +		struct
> +		{
> +		  const char * string;
> +		  size_t len;
> +		  bfd_boolean (* func)(bfd *, Elf_Internal_Note *);
> +		}
> +		grokers[] =
> +		  {
> +		   GROKER_ELEMENT ("", elfcore_grok_note),
> +		   GROKER_ELEMENT ("FreeBSD", elfcore_grok_freebsd_note),
> +		   GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
> +		   GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
> +		   GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
> +		   GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
> +		  };
>  #undef GROKER_ELEMENT
> -	    int i;
> +		int i;

This seems unrelated to the patch.

>  
> -	    for (i = ARRAY_SIZE (grokers); i--;)
> -	      {
> -		if (in.namesz >= grokers[i].len
> -		    && strncmp (in.namedata, grokers[i].string,
> -				grokers[i].len) == 0)
> +		for (i = ARRAY_SIZE (grokers); i--;)
>  		  {
> -		    if (! grokers[i].func (abfd, & in))
> -		      return FALSE;
> -		    break;
> +		    if (in.namesz >= grokers[i].len
> +			&& strncmp (in.namedata, grokers[i].string,
> +				    grokers[i].len) == 0)
> +		      {
> +			if (! grokers[i].func (abfd, & in))
> +			  return FALSE;
> +			break;
> +		      }
>  		  }
>  	      }
>  	    break;
> @@ -11721,7 +11738,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>    return TRUE;
>  }
>  
> -static bfd_boolean
> +bfd_boolean
>  elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
>  		size_t align)
>  {
> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
> index 395feb5ef3..395870942c 100644
> --- a/bfd/elfcore.h
> +++ b/bfd/elfcore.h
> @@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
>        return FALSE;
>      }
>  
> +  /* If both BFDs have identical build-ids, then they match.  */
> +  if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
> +      && core_bfd->build_id->size == exec_bfd->build_id->size
> +      && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
> +		 core_bfd->build_id->size) == 0)
> +    return TRUE;
> +
>    /* See if the name in the corefile matches the executable name.  */
>    corename = elf_tdata (core_bfd)->core->program;
>    if (corename != NULL)
> @@ -313,3 +320,88 @@ wrong:
>  fail:
>    return NULL;
>  }
> +
> +/* Attempt to find a build-id in a core file from the core file BFD.
> +   OFFSET is the file offset to a PT_LOAD segment that may contain
> +   the build-id note.  */
> +
> +void
> +NAME(_bfd_elf,core_find_build_id)
> +  (bfd *abfd,
> +   bfd_vma offset)
> +{
> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
> +  Elf_Internal_Phdr *i_phdr;
> +  unsigned int i;
> +
> +  /* Seek to the position of the segment at OFFSET.  */
> +  if (bfd_seek (abfd, offset, SEEK_SET) != 0)
> +    return;
> +
> +  /* Read in the ELF header in external format.  */
> +  if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
> +    return;
> +
> +  /* Now check to see if we have a valid ELF file, and one that BFD can
> +     make use of.  The magic number must match, the address size ('class')
> +     and byte-swapping must match our XVEC entry, and it must have a
> +     section header table (FIXME: See comments re sections at top of this
> +     file).  */
> +
> +  if (! elf_file_p (&x_ehdr)
> +      || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
> +      || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
> +    return;
> +
> +  /* Check that file's byte order matches xvec's */
> +  switch (x_ehdr.e_ident[EI_DATA])
> +    {
> +    case ELFDATA2MSB:		/* Big-endian */
> +      if (! bfd_header_big_endian (abfd))
> +	return;
> +      break;
> +    case ELFDATA2LSB:		/* Little-endian */
> +      if (! bfd_header_little_endian (abfd))
> +	return;
> +      break;
> +    case ELFDATANONE:		/* No data encoding specified */
> +    default:			/* Unknown data encoding specified */
> +      return;
> +    }
> +
> +  elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
> +#if DEBUG & 1
> +  elf_debug_file (&i_ehdr);
> +#endif
> +
> +  if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
> +    return;
> +
> +  /* Read in program headers.  */
> +  i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
> +					      sizeof (*i_phdr));
> +  if (i_phdr == NULL)
> +    return;
> +
> +  if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
> +    return;
> +
> +  /* Read in program headers and parse notes.  */
> +  for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
> +    {
> +      Elf_External_Phdr x_phdr;
> +
> +      if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
> +	return;
> +      elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
> +
> +      if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
> +	{
> +	  elf_read_notes (abfd, offset + i_phdr->p_offset,
> +			  i_phdr->p_filesz, i_phdr->p_align);
> +	  if (abfd->build_id != NULL)
> +	    return;
> +	}
> +    }
> +}
> diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
> index e4e7546243..96b96ec62a 100644
> --- a/bfd/elfxx-target.h
> +++ b/bfd/elfxx-target.h
> @@ -517,6 +517,9 @@
>  #ifndef elf_backend_bfd_from_remote_memory
>  #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
>  #endif
> +#ifndef elf_backend_core_find_build_id
> +#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
> +#endif
>  #ifndef elf_backend_got_header_size
>  #define elf_backend_got_header_size	0
>  #endif
> @@ -852,6 +855,7 @@ static struct elf_backend_data elfNN_bed =
>    elf_backend_mips_rtype_to_howto,
>    elf_backend_ecoff_debug_swap,
>    elf_backend_bfd_from_remote_memory,
> +  elf_backend_core_find_build_id,
>    elf_backend_plt_sym_val,
>    elf_backend_common_definition,
>    elf_backend_common_section_index,
> -- 
> 2.20.1

The rest makes sense to me, but I'm not a BFD expert :-).

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Core file build-ids

[Keith Seitz]

On 6/5/19 11:57 AM, Sergio Durigan Junior wrote:
> On Wednesday, June 05 2019, Keith Seitz wrote:
>> diff --git a/bfd/elf.c b/bfd/elf.c
>> index b463f1df8b..1d86a0a4d6 100644
>> --- a/bfd/elf.c
>> +++ b/bfd/elf.c
>> @@ -3019,7 +3024,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
>>  
>>      case PT_LOAD:
>> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>> +	return FALSE;
>> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>> +      return TRUE;
>>  
>>      case PT_DYNAMIC:
>>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
>> @@ -11667,34 +11676,42 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>>  
>>  	case bfd_core:
>>  	  {
>> +	    if (in.namesz == sizeof "GNU" && strcmp (in.namedata, "GNU") == 0)
>> +	      {
>> +		if (! elfobj_grok_gnu_note (abfd, &in))
>> +		  return FALSE;
>> +	      }
>> +	    else
>> +	      {
> 
> You could rewrite this to get rid of the "else" part, which would make
> the patch smaller:
> 

Actually, I think I can make this even simpler. Not sure why I didn't do
this originally, but I'm blaming the weather.

@@ -11681,7 +11690,8 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
              GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
              GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
              GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
-             GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
+             GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note),
+             GROKER_ELEMENT ("GNU", elfobj_grok_gnu_note)
            };
 #undef GROKER_ELEMENT
            int i;

Thank you for taking a peek,
Keith

Re: [PATCH] Core file build-ids

[Nick Clifton]

Hi Keith,

  I do have a couple of comments on this patch:

> +void> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> +    (templ, offset);
I would recommend being a little bit paranoid here and checking
that templ is an elf format bfd before attempting to access the
backend data.


> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
> +	return FALSE;
> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
> +      return TRUE;

If _bfd_elf_core_find_build_id can fail (see above and below) then 
it would be worth checking the return value for an error result.


> +/* Attempt to find a build-id in a core file from the core file BFD.
> +   OFFSET is the file offset to a PT_LOAD segment that may contain
> +   the build-id note.  */
> +
> +void
> +NAME(_bfd_elf,core_find_build_id)

Given that things can wrong when attempting to find a build_id, I think
that this function should return a bfd_boolean.  I appreciate that the
caller could just check abfd->build_id != NULL, (assuming that it was NULL
to start with), but I think that it is cleaner that the function itself
should tell its caller whether ot not it succeeded.


> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */

Minor formatting nit: comments should end with a full stop and two spaces.

Cheers
  Nick

Re: [PATCH] Core file build-ids

[Keith Seitz]

Hi, Nick,

Wowser, it's been a busy quarter! I'm anxious to get back to this patch.
I appreciate your patience.

On 6/7/19 9:16 AM, Nick Clifton wrote:
>> +void
>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> +    (templ, offset);
>
> I would recommend being a little bit paranoid here and checking
> that templ is an elf format bfd before attempting to access the
> backend data.

Done.

>> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>> +	return FALSE;
>> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>> +      return TRUE;
> 
> If _bfd_elf_core_find_build_id can fail (see above and below) then 
> it would be worth checking the return value for an error result.

My concern here is what happens if we fail to find a build-id in the
core file (I/O error jumping through a header or it's just plain missing).
If _bfd_elf_core_find_build_id returns FALSE and we bail here, this will
abort constructing any more phdrs.

After implementing your recommendation, I can actually demonstrate
this scenario with one of gdb's tests: gdb.arch/i386-biarch-core.exp. This
test will FAIL with a memory read error while attempting to print the
contents of some memory location in the core file.

This location exists in a section that is internalized after we attempt
to find a (missing) build-id.

That's why I made this an optional (as in, we don't care if it fails)
step when constructing the section.

Maybe I've misunderstood what you were really asking me to do?

>> +/* Attempt to find a build-id in a core file from the core file BFD.
>> +   OFFSET is the file offset to a PT_LOAD segment that may contain
>> +   the build-id note.  */
>> +
>> +void
>> +NAME(_bfd_elf,core_find_build_id)
> 
> Given that things can wrong when attempting to find a build_id, I think
> that this function should return a bfd_boolean.  I appreciate that the
> caller could just check abfd->build_id != NULL, (assuming that it was NULL
> to start with), but I think that it is cleaner that the function itself
> should tell its caller whether ot not it succeeded.

I've made this change in this version, but at the one place it is called,
the return value is ignored (see above). Nonetheless, I could imagine a
use for this outside of my/gdb's specific use case, and I think returning
bfd_boolean is a better/safer/more-forward-thinking option, so I've left
it (updated per your request) in this version.

>> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
>> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
> 
> Minor formatting nit: comments should end with a full stop and two spaces.

Fixed.

New version below.

Keith

Core file build-ids

This patch adds support for reading build-ids from core files.  I've
opted to maintain the current interfaces that BFD and GDB already use.

The approach taken is fairly straightforward (YMMV). When a core file
is opened, BFD loops over the various segments in the ELF file and
program headers. During this time, whenever a segment whose type is
PT_LOAD is encountered, and the build-id is unknown, I've added a call
to inspect the segment for an ELF header.  If a valid ELF file header
is found, it attempts to read in the program headers and scan for any
notes.

For those unfamiliar with build-ids, please see
https://fedoraproject.org/wiki/Releases/FeatureBuildId .

Yes, this feature has been in Fedora GDB for over a decade, but it exists
as a copy-n-paste of a lot of BFD's internals. For the brave, you can
read the original submission at
https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .

This is an attempt to clean that up.

I have run this patch through GDB's buildbot, and it has detected
no problems.

bfd/ChangeLog:

	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
	field.
	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
	functions.
	(elf_read_notes): Add declaration.
	* elf.c (elf_read_notes): Move elf-bfd.h.
	(_bfd_elf_core_find_build_id): New function.
	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
	build-id if none is known.
	(elf_parse_notes): For core files, scan for notes.
	* elfcore.h (elf_core_file_matches_executable_p): If both
	BFDs have identical build-ids, then they match.
	(_bfd_elf_core_find_build_id): New function.
	* elfxx-target.h (elf_backend_core_find_build_id): Define.
	(elfNN_bed): Add elf_backend_core_find_build_id.
---
 bfd/elf-bfd.h      |   8 ++++
 bfd/elf.c          |  23 +++++++---
 bfd/elfcore.h      | 106 +++++++++++++++++++++++++++++++++++++++++++++
 bfd/elfxx-target.h |   4 ++
 4 files changed, 136 insertions(+), 5 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 0a83c17332..2d214be87f 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1377,6 +1377,8 @@ struct elf_backend_data
      int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
 				bfd_size_type len));
 
+  bfd_boolean (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
+
   /* This function is used by `_bfd_elf_get_synthetic_symtab';
      see elf.c.  */
   bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
@@ -2398,6 +2400,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
   (bfd *, bfd *);
 extern int bfd_elf32_core_file_pid
   (bfd *);
+extern bfd_boolean _bfd_elf32_core_find_build_id
+  (bfd *, bfd_vma);
 
 extern bfd_boolean bfd_elf32_swap_symbol_in
   (bfd *, const void *, const void *, Elf_Internal_Sym *);
@@ -2444,6 +2448,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
   (bfd *, bfd *);
 extern int bfd_elf64_core_file_pid
   (bfd *);
+extern bfd_boolean _bfd_elf64_core_find_build_id
+  (bfd *, bfd_vma);
 
 extern bfd_boolean bfd_elf64_swap_symbol_in
   (bfd *, const void *, const void *, Elf_Internal_Sym *);
@@ -2768,6 +2774,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
 extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
 extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
 extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
+extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
+				   size_t align) ;
 
 extern bfd_boolean _bfd_elf_parse_gnu_properties
   (bfd *, Elf_Internal_Note *);
diff --git a/bfd/elf.c b/bfd/elf.c
index 664eae5c66..2ae3ec53c8 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
 static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
 static bfd_boolean prep_headers (bfd *);
 static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
-static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
-				   size_t align) ;
 static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
 				    file_ptr offset, size_t align);
 
@@ -3060,6 +3058,16 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
   return TRUE;
 }
 
+static bfd_boolean
+_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
+{
+  /* The return value is ignored.  Build-ids are considered optional.  */
+  if (templ->xvec->flavour == bfd_target_elf_flavour)
+    return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
+      (templ, offset);
+  return FALSE;
+}
+
 bfd_boolean
 bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
 {
@@ -3071,7 +3079,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
       return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
 
     case PT_LOAD:
-      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
+      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
+	return FALSE;
+      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
+	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
+      return TRUE;
 
     case PT_DYNAMIC:
       return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
@@ -11790,7 +11802,8 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
 	      GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
 	      GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
 	      GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
-	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
+	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note),
+	      GROKER_ELEMENT ("GNU", elfobj_grok_gnu_note)
 	    };
 #undef GROKER_ELEMENT
 	    int i;
@@ -11830,7 +11843,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
   return TRUE;
 }
 
-static bfd_boolean
+bfd_boolean
 elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
 		size_t align)
 {
diff --git a/bfd/elfcore.h b/bfd/elfcore.h
index 3550eaac27..751e831914 100644
--- a/bfd/elfcore.h
+++ b/bfd/elfcore.h
@@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
       return FALSE;
     }
 
+  /* If both BFDs have identical build-ids, then they match.  */
+  if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
+      && core_bfd->build_id->size == exec_bfd->build_id->size
+      && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
+		 core_bfd->build_id->size) == 0)
+    return TRUE;
+
   /* See if the name in the corefile matches the executable name.  */
   corename = elf_tdata (core_bfd)->core->program;
   if (corename != NULL)
@@ -313,3 +320,102 @@ wrong:
 fail:
   return NULL;
 }
+
+/* Attempt to find a build-id in a core file from the core file BFD.
+   OFFSET is the file offset to a PT_LOAD segment that may contain
+   the build-id note.  Returns TRUE upon success, FALSE otherwise.  */
+
+bfd_boolean
+NAME(_bfd_elf,core_find_build_id)
+  (bfd *abfd,
+   bfd_vma offset)
+{
+  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form.   */
+  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form.   */
+  Elf_Internal_Phdr *i_phdr;
+  unsigned int i;
+
+  /* Seek to the position of the segment at OFFSET.  */
+  if (bfd_seek (abfd, offset, SEEK_SET) != 0)
+    goto fail;
+
+  /* Read in the ELF header in external format.  */
+  if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
+    {
+      if (bfd_get_error () != bfd_error_system_call)
+	goto wrong;
+      else
+	goto fail;
+    }
+
+  /* Now check to see if we have a valid ELF file, and one that BFD can
+     make use of.  The magic number must match, the address size ('class')
+     and byte-swapping must match our XVEC entry, and it must have a
+     section header table (FIXME: See comments re sections at top of this
+     file).  */
+
+  if (! elf_file_p (&x_ehdr)
+      || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
+      || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
+    goto wrong;
+
+  /* Check that file's byte order matches xvec's */
+  switch (x_ehdr.e_ident[EI_DATA])
+    {
+    case ELFDATA2MSB:		/* Big-endian */
+      if (! bfd_header_big_endian (abfd))
+	goto wrong;
+      break;
+    case ELFDATA2LSB:		/* Little-endian */
+      if (! bfd_header_little_endian (abfd))
+	goto wrong;
+      break;
+    case ELFDATANONE:		/* No data encoding specified */
+    default:			/* Unknown data encoding specified */
+      goto wrong;
+    }
+
+  elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
+#if DEBUG & 1
+  elf_debug_file (&i_ehdr);
+#endif
+
+  if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
+    goto fail;
+
+  /* Read in program headers.  */
+  i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
+					      sizeof (*i_phdr));
+  if (i_phdr == NULL)
+    goto fail;
+
+  if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
+    goto fail;
+
+  /* Read in program headers and parse notes.  */
+  for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
+    {
+      Elf_External_Phdr x_phdr;
+
+      if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
+	goto fail;
+      elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
+
+      if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
+	{
+	  elf_read_notes (abfd, offset + i_phdr->p_offset,
+			  i_phdr->p_filesz, i_phdr->p_align);
+	  if (abfd->build_id != NULL)
+	    return TRUE;
+	}
+    }
+
+  /* Having gotten this far, we have a valid ELF section, but no
+     build-id was found.  */
+  goto fail;
+
+wrong:
+  bfd_set_error (bfd_error_wrong_format);
+fail:
+  return FALSE;
+}
diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
index 78a1f6314d..29ad2e8481 100644
--- a/bfd/elfxx-target.h
+++ b/bfd/elfxx-target.h
@@ -521,6 +521,9 @@
 #ifndef elf_backend_bfd_from_remote_memory
 #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
 #endif
+#ifndef elf_backend_core_find_build_id
+#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
+#endif
 #ifndef elf_backend_got_header_size
 #define elf_backend_got_header_size	0
 #endif
@@ -860,6 +863,7 @@ static struct elf_backend_data elfNN_bed =
   elf_backend_mips_rtype_to_howto,
   elf_backend_ecoff_debug_swap,
   elf_backend_bfd_from_remote_memory,
+  elf_backend_core_find_build_id,
   elf_backend_plt_sym_val,
   elf_backend_common_definition,
   elf_backend_common_section_index,
-- 
2.21.0

Re: [PATCH] Core file build-ids

[Keith Seitz]

Hi,

Has anyone had an opportunity to (or plan to) review?

Keith

On 10/1/19 6:07 AM, Keith Seitz wrote:
> Hi, Nick,
> 
> Wowser, it's been a busy quarter! I'm anxious to get back to this patch.
> I appreciate your patience.
> 
> On 6/7/19 9:16 AM, Nick Clifton wrote:
>>> +void
>>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> +    (templ, offset);
>>
>> I would recommend being a little bit paranoid here and checking
>> that templ is an elf format bfd before attempting to access the
>> backend data.
> 
> Done.
> 
>>> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>>> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>>> +	return FALSE;
>>> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>>> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>>> +      return TRUE;
>>
>> If _bfd_elf_core_find_build_id can fail (see above and below) then 
>> it would be worth checking the return value for an error result.
> 
> My concern here is what happens if we fail to find a build-id in the
> core file (I/O error jumping through a header or it's just plain missing).
> If _bfd_elf_core_find_build_id returns FALSE and we bail here, this will
> abort constructing any more phdrs.
> 
> After implementing your recommendation, I can actually demonstrate
> this scenario with one of gdb's tests: gdb.arch/i386-biarch-core.exp. This
> test will FAIL with a memory read error while attempting to print the
> contents of some memory location in the core file.
> 
> This location exists in a section that is internalized after we attempt
> to find a (missing) build-id.
> 
> That's why I made this an optional (as in, we don't care if it fails)
> step when constructing the section.
> 
> Maybe I've misunderstood what you were really asking me to do?
> 
>>> +/* Attempt to find a build-id in a core file from the core file BFD.
>>> +   OFFSET is the file offset to a PT_LOAD segment that may contain
>>> +   the build-id note.  */
>>> +
>>> +void
>>> +NAME(_bfd_elf,core_find_build_id)
>>
>> Given that things can wrong when attempting to find a build_id, I think
>> that this function should return a bfd_boolean.  I appreciate that the
>> caller could just check abfd->build_id != NULL, (assuming that it was NULL
>> to start with), but I think that it is cleaner that the function itself
>> should tell its caller whether ot not it succeeded.
> 
> I've made this change in this version, but at the one place it is called,
> the return value is ignored (see above). Nonetheless, I could imagine a
> use for this outside of my/gdb's specific use case, and I think returning
> bfd_boolean is a better/safer/more-forward-thinking option, so I've left
> it (updated per your request) in this version.
> 
>>> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
>>> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
>>
>> Minor formatting nit: comments should end with a full stop and two spaces.
> 
> Fixed.
> 
> New version below.
> 
> Keith
> 
> Core file build-ids
> 
> This patch adds support for reading build-ids from core files.  I've
> opted to maintain the current interfaces that BFD and GDB already use.
> 
> The approach taken is fairly straightforward (YMMV). When a core file
> is opened, BFD loops over the various segments in the ELF file and
> program headers. During this time, whenever a segment whose type is
> PT_LOAD is encountered, and the build-id is unknown, I've added a call
> to inspect the segment for an ELF header.  If a valid ELF file header
> is found, it attempts to read in the program headers and scan for any
> notes.
> 
> For those unfamiliar with build-ids, please see
> https://fedoraproject.org/wiki/Releases/FeatureBuildId .
> 
> Yes, this feature has been in Fedora GDB for over a decade, but it exists
> as a copy-n-paste of a lot of BFD's internals. For the brave, you can
> read the original submission at
> https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .
> 
> This is an attempt to clean that up.
> 
> I have run this patch through GDB's buildbot, and it has detected
> no problems.
> 
> bfd/ChangeLog:
> 
> 	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
> 	field.
> 	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
> 	functions.
> 	(elf_read_notes): Add declaration.
> 	* elf.c (elf_read_notes): Move elf-bfd.h.
> 	(_bfd_elf_core_find_build_id): New function.
> 	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
> 	build-id if none is known.
> 	(elf_parse_notes): For core files, scan for notes.
> 	* elfcore.h (elf_core_file_matches_executable_p): If both
> 	BFDs have identical build-ids, then they match.
> 	(_bfd_elf_core_find_build_id): New function.
> 	* elfxx-target.h (elf_backend_core_find_build_id): Define.
> 	(elfNN_bed): Add elf_backend_core_find_build_id.
> ---
>  bfd/elf-bfd.h      |   8 ++++
>  bfd/elf.c          |  23 +++++++---
>  bfd/elfcore.h      | 106 +++++++++++++++++++++++++++++++++++++++++++++
>  bfd/elfxx-target.h |   4 ++
>  4 files changed, 136 insertions(+), 5 deletions(-)
> 
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index 0a83c17332..2d214be87f 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -1377,6 +1377,8 @@ struct elf_backend_data
>       int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
>  				bfd_size_type len));
>  
> +  bfd_boolean (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
> +
>    /* This function is used by `_bfd_elf_get_synthetic_symtab';
>       see elf.c.  */
>    bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
> @@ -2398,6 +2400,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
>    (bfd *, bfd *);
>  extern int bfd_elf32_core_file_pid
>    (bfd *);
> +extern bfd_boolean _bfd_elf32_core_find_build_id
> +  (bfd *, bfd_vma);
>  
>  extern bfd_boolean bfd_elf32_swap_symbol_in
>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
> @@ -2444,6 +2448,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
>    (bfd *, bfd *);
>  extern int bfd_elf64_core_file_pid
>    (bfd *);
> +extern bfd_boolean _bfd_elf64_core_find_build_id
> +  (bfd *, bfd_vma);
>  
>  extern bfd_boolean bfd_elf64_swap_symbol_in
>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
> @@ -2768,6 +2774,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
>  extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
> +extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
> +				   size_t align) ;
>  
>  extern bfd_boolean _bfd_elf_parse_gnu_properties
>    (bfd *, Elf_Internal_Note *);
> diff --git a/bfd/elf.c b/bfd/elf.c
> index 664eae5c66..2ae3ec53c8 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
>  static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
>  static bfd_boolean prep_headers (bfd *);
>  static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
> -static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
> -				   size_t align) ;
>  static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
>  				    file_ptr offset, size_t align);
>  
> @@ -3060,6 +3058,16 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
>    return TRUE;
>  }
>  
> +static bfd_boolean
> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
> +{
> +  /* The return value is ignored.  Build-ids are considered optional.  */
> +  if (templ->xvec->flavour == bfd_target_elf_flavour)
> +    return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
> +      (templ, offset);
> +  return FALSE;
> +}
> +
>  bfd_boolean
>  bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>  {
> @@ -3071,7 +3079,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
>  
>      case PT_LOAD:
> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
> +	return FALSE;
> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
> +      return TRUE;
>  
>      case PT_DYNAMIC:
>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
> @@ -11790,7 +11802,8 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>  	      GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
>  	      GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
>  	      GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
> -	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
> +	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note),
> +	      GROKER_ELEMENT ("GNU", elfobj_grok_gnu_note)
>  	    };
>  #undef GROKER_ELEMENT
>  	    int i;
> @@ -11830,7 +11843,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>    return TRUE;
>  }
>  
> -static bfd_boolean
> +bfd_boolean
>  elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
>  		size_t align)
>  {
> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
> index 3550eaac27..751e831914 100644
> --- a/bfd/elfcore.h
> +++ b/bfd/elfcore.h
> @@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
>        return FALSE;
>      }
>  
> +  /* If both BFDs have identical build-ids, then they match.  */
> +  if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
> +      && core_bfd->build_id->size == exec_bfd->build_id->size
> +      && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
> +		 core_bfd->build_id->size) == 0)
> +    return TRUE;
> +
>    /* See if the name in the corefile matches the executable name.  */
>    corename = elf_tdata (core_bfd)->core->program;
>    if (corename != NULL)
> @@ -313,3 +320,102 @@ wrong:
>  fail:
>    return NULL;
>  }
> +
> +/* Attempt to find a build-id in a core file from the core file BFD.
> +   OFFSET is the file offset to a PT_LOAD segment that may contain
> +   the build-id note.  Returns TRUE upon success, FALSE otherwise.  */
> +
> +bfd_boolean
> +NAME(_bfd_elf,core_find_build_id)
> +  (bfd *abfd,
> +   bfd_vma offset)
> +{
> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form.   */
> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form.   */
> +  Elf_Internal_Phdr *i_phdr;
> +  unsigned int i;
> +
> +  /* Seek to the position of the segment at OFFSET.  */
> +  if (bfd_seek (abfd, offset, SEEK_SET) != 0)
> +    goto fail;
> +
> +  /* Read in the ELF header in external format.  */
> +  if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
> +    {
> +      if (bfd_get_error () != bfd_error_system_call)
> +	goto wrong;
> +      else
> +	goto fail;
> +    }
> +
> +  /* Now check to see if we have a valid ELF file, and one that BFD can
> +     make use of.  The magic number must match, the address size ('class')
> +     and byte-swapping must match our XVEC entry, and it must have a
> +     section header table (FIXME: See comments re sections at top of this
> +     file).  */
> +
> +  if (! elf_file_p (&x_ehdr)
> +      || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
> +      || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
> +    goto wrong;
> +
> +  /* Check that file's byte order matches xvec's */
> +  switch (x_ehdr.e_ident[EI_DATA])
> +    {
> +    case ELFDATA2MSB:		/* Big-endian */
> +      if (! bfd_header_big_endian (abfd))
> +	goto wrong;
> +      break;
> +    case ELFDATA2LSB:		/* Little-endian */
> +      if (! bfd_header_little_endian (abfd))
> +	goto wrong;
> +      break;
> +    case ELFDATANONE:		/* No data encoding specified */
> +    default:			/* Unknown data encoding specified */
> +      goto wrong;
> +    }
> +
> +  elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
> +#if DEBUG & 1
> +  elf_debug_file (&i_ehdr);
> +#endif
> +
> +  if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
> +    goto fail;
> +
> +  /* Read in program headers.  */
> +  i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
> +					      sizeof (*i_phdr));
> +  if (i_phdr == NULL)
> +    goto fail;
> +
> +  if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
> +    goto fail;
> +
> +  /* Read in program headers and parse notes.  */
> +  for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
> +    {
> +      Elf_External_Phdr x_phdr;
> +
> +      if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
> +	goto fail;
> +      elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
> +
> +      if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
> +	{
> +	  elf_read_notes (abfd, offset + i_phdr->p_offset,
> +			  i_phdr->p_filesz, i_phdr->p_align);
> +	  if (abfd->build_id != NULL)
> +	    return TRUE;
> +	}
> +    }
> +
> +  /* Having gotten this far, we have a valid ELF section, but no
> +     build-id was found.  */
> +  goto fail;
> +
> +wrong:
> +  bfd_set_error (bfd_error_wrong_format);
> +fail:
> +  return FALSE;
> +}
> diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
> index 78a1f6314d..29ad2e8481 100644
> --- a/bfd/elfxx-target.h
> +++ b/bfd/elfxx-target.h
> @@ -521,6 +521,9 @@
>  #ifndef elf_backend_bfd_from_remote_memory
>  #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
>  #endif
> +#ifndef elf_backend_core_find_build_id
> +#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
> +#endif
>  #ifndef elf_backend_got_header_size
>  #define elf_backend_got_header_size	0
>  #endif
> @@ -860,6 +863,7 @@ static struct elf_backend_data elfNN_bed =
>    elf_backend_mips_rtype_to_howto,
>    elf_backend_ecoff_debug_swap,
>    elf_backend_bfd_from_remote_memory,
> +  elf_backend_core_find_build_id,
>    elf_backend_plt_sym_val,
>    elf_backend_common_definition,
>    elf_backend_common_section_index,
> 

Re: [PATCH] Core file build-ids

[Keith Seitz]

Ping?

On 10/16/19 8:22 AM, Keith Seitz wrote:
> Hi,
> 
> Has anyone had an opportunity to (or plan to) review?
> 
> Keith
> 
> On 10/1/19 6:07 AM, Keith Seitz wrote:
>> Hi, Nick,
>>
>> Wowser, it's been a busy quarter! I'm anxious to get back to this patch.
>> I appreciate your patience.
>>
>> On 6/7/19 9:16 AM, Nick Clifton wrote:
>>>> +void
>>>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> +  return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> +    (templ, offset);
>>>
>>> I would recommend being a little bit paranoid here and checking
>>> that templ is an elf format bfd before attempting to access the
>>> backend data.
>>
>> Done.
>>
>>>> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>>>> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>>>> +	return FALSE;
>>>> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>>>> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>>>> +      return TRUE;
>>>
>>> If _bfd_elf_core_find_build_id can fail (see above and below) then 
>>> it would be worth checking the return value for an error result.
>>
>> My concern here is what happens if we fail to find a build-id in the
>> core file (I/O error jumping through a header or it's just plain missing).
>> If _bfd_elf_core_find_build_id returns FALSE and we bail here, this will
>> abort constructing any more phdrs.
>>
>> After implementing your recommendation, I can actually demonstrate
>> this scenario with one of gdb's tests: gdb.arch/i386-biarch-core.exp. This
>> test will FAIL with a memory read error while attempting to print the
>> contents of some memory location in the core file.
>>
>> This location exists in a section that is internalized after we attempt
>> to find a (missing) build-id.
>>
>> That's why I made this an optional (as in, we don't care if it fails)
>> step when constructing the section.
>>
>> Maybe I've misunderstood what you were really asking me to do?
>>
>>>> +/* Attempt to find a build-id in a core file from the core file BFD.
>>>> +   OFFSET is the file offset to a PT_LOAD segment that may contain
>>>> +   the build-id note.  */
>>>> +
>>>> +void
>>>> +NAME(_bfd_elf,core_find_build_id)
>>>
>>> Given that things can wrong when attempting to find a build_id, I think
>>> that this function should return a bfd_boolean.  I appreciate that the
>>> caller could just check abfd->build_id != NULL, (assuming that it was NULL
>>> to start with), but I think that it is cleaner that the function itself
>>> should tell its caller whether ot not it succeeded.
>>
>> I've made this change in this version, but at the one place it is called,
>> the return value is ignored (see above). Nonetheless, I could imagine a
>> use for this outside of my/gdb's specific use case, and I think returning
>> bfd_boolean is a better/safer/more-forward-thinking option, so I've left
>> it (updated per your request) in this version.
>>
>>>> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
>>>> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
>>>
>>> Minor formatting nit: comments should end with a full stop and two spaces.
>>
>> Fixed.
>>
>> New version below.
>>
>> Keith
>>
>> Core file build-ids
>>
>> This patch adds support for reading build-ids from core files.  I've
>> opted to maintain the current interfaces that BFD and GDB already use.
>>
>> The approach taken is fairly straightforward (YMMV). When a core file
>> is opened, BFD loops over the various segments in the ELF file and
>> program headers. During this time, whenever a segment whose type is
>> PT_LOAD is encountered, and the build-id is unknown, I've added a call
>> to inspect the segment for an ELF header.  If a valid ELF file header
>> is found, it attempts to read in the program headers and scan for any
>> notes.
>>
>> For those unfamiliar with build-ids, please see
>> https://fedoraproject.org/wiki/Releases/FeatureBuildId .
>>
>> Yes, this feature has been in Fedora GDB for over a decade, but it exists
>> as a copy-n-paste of a lot of BFD's internals. For the brave, you can
>> read the original submission at
>> https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .
>>
>> This is an attempt to clean that up.
>>
>> I have run this patch through GDB's buildbot, and it has detected
>> no problems.
>>
>> bfd/ChangeLog:
>>
>> 	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
>> 	field.
>> 	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
>> 	functions.
>> 	(elf_read_notes): Add declaration.
>> 	* elf.c (elf_read_notes): Move elf-bfd.h.
>> 	(_bfd_elf_core_find_build_id): New function.
>> 	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
>> 	build-id if none is known.
>> 	(elf_parse_notes): For core files, scan for notes.
>> 	* elfcore.h (elf_core_file_matches_executable_p): If both
>> 	BFDs have identical build-ids, then they match.
>> 	(_bfd_elf_core_find_build_id): New function.
>> 	* elfxx-target.h (elf_backend_core_find_build_id): Define.
>> 	(elfNN_bed): Add elf_backend_core_find_build_id.
>> ---
>>  bfd/elf-bfd.h      |   8 ++++
>>  bfd/elf.c          |  23 +++++++---
>>  bfd/elfcore.h      | 106 +++++++++++++++++++++++++++++++++++++++++++++
>>  bfd/elfxx-target.h |   4 ++
>>  4 files changed, 136 insertions(+), 5 deletions(-)
>>
>> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
>> index 0a83c17332..2d214be87f 100644
>> --- a/bfd/elf-bfd.h
>> +++ b/bfd/elf-bfd.h
>> @@ -1377,6 +1377,8 @@ struct elf_backend_data
>>       int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
>>  				bfd_size_type len));
>>  
>> +  bfd_boolean (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
>> +
>>    /* This function is used by `_bfd_elf_get_synthetic_symtab';
>>       see elf.c.  */
>>    bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
>> @@ -2398,6 +2400,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
>>    (bfd *, bfd *);
>>  extern int bfd_elf32_core_file_pid
>>    (bfd *);
>> +extern bfd_boolean _bfd_elf32_core_find_build_id
>> +  (bfd *, bfd_vma);
>>  
>>  extern bfd_boolean bfd_elf32_swap_symbol_in
>>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
>> @@ -2444,6 +2448,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
>>    (bfd *, bfd *);
>>  extern int bfd_elf64_core_file_pid
>>    (bfd *);
>> +extern bfd_boolean _bfd_elf64_core_find_build_id
>> +  (bfd *, bfd_vma);
>>  
>>  extern bfd_boolean bfd_elf64_swap_symbol_in
>>    (bfd *, const void *, const void *, Elf_Internal_Sym *);
>> @@ -2768,6 +2774,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
>>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
>>  extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
>>  extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
>> +extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
>> +				   size_t align) ;
>>  
>>  extern bfd_boolean _bfd_elf_parse_gnu_properties
>>    (bfd *, Elf_Internal_Note *);
>> diff --git a/bfd/elf.c b/bfd/elf.c
>> index 664eae5c66..2ae3ec53c8 100644
>> --- a/bfd/elf.c
>> +++ b/bfd/elf.c
>> @@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
>>  static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
>>  static bfd_boolean prep_headers (bfd *);
>>  static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
>> -static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
>> -				   size_t align) ;
>>  static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
>>  				    file_ptr offset, size_t align);
>>  
>> @@ -3060,6 +3058,16 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
>>    return TRUE;
>>  }
>>  
>> +static bfd_boolean
>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
>> +{
>> +  /* The return value is ignored.  Build-ids are considered optional.  */
>> +  if (templ->xvec->flavour == bfd_target_elf_flavour)
>> +    return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
>> +      (templ, offset);
>> +  return FALSE;
>> +}
>> +
>>  bfd_boolean
>>  bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>>  {
>> @@ -3071,7 +3079,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
>>  
>>      case PT_LOAD:
>> -      return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>> +      if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>> +	return FALSE;
>> +      if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>> +	_bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>> +      return TRUE;
>>  
>>      case PT_DYNAMIC:
>>        return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
>> @@ -11790,7 +11802,8 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>>  	      GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
>>  	      GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
>>  	      GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
>> -	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
>> +	      GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note),
>> +	      GROKER_ELEMENT ("GNU", elfobj_grok_gnu_note)
>>  	    };
>>  #undef GROKER_ELEMENT
>>  	    int i;
>> @@ -11830,7 +11843,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>>    return TRUE;
>>  }
>>  
>> -static bfd_boolean
>> +bfd_boolean
>>  elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
>>  		size_t align)
>>  {
>> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
>> index 3550eaac27..751e831914 100644
>> --- a/bfd/elfcore.h
>> +++ b/bfd/elfcore.h
>> @@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
>>        return FALSE;
>>      }
>>  
>> +  /* If both BFDs have identical build-ids, then they match.  */
>> +  if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
>> +      && core_bfd->build_id->size == exec_bfd->build_id->size
>> +      && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
>> +		 core_bfd->build_id->size) == 0)
>> +    return TRUE;
>> +
>>    /* See if the name in the corefile matches the executable name.  */
>>    corename = elf_tdata (core_bfd)->core->program;
>>    if (corename != NULL)
>> @@ -313,3 +320,102 @@ wrong:
>>  fail:
>>    return NULL;
>>  }
>> +
>> +/* Attempt to find a build-id in a core file from the core file BFD.
>> +   OFFSET is the file offset to a PT_LOAD segment that may contain
>> +   the build-id note.  Returns TRUE upon success, FALSE otherwise.  */
>> +
>> +bfd_boolean
>> +NAME(_bfd_elf,core_find_build_id)
>> +  (bfd *abfd,
>> +   bfd_vma offset)
>> +{
>> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form.   */
>> +  Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form.   */
>> +  Elf_Internal_Phdr *i_phdr;
>> +  unsigned int i;
>> +
>> +  /* Seek to the position of the segment at OFFSET.  */
>> +  if (bfd_seek (abfd, offset, SEEK_SET) != 0)
>> +    goto fail;
>> +
>> +  /* Read in the ELF header in external format.  */
>> +  if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
>> +    {
>> +      if (bfd_get_error () != bfd_error_system_call)
>> +	goto wrong;
>> +      else
>> +	goto fail;
>> +    }
>> +
>> +  /* Now check to see if we have a valid ELF file, and one that BFD can
>> +     make use of.  The magic number must match, the address size ('class')
>> +     and byte-swapping must match our XVEC entry, and it must have a
>> +     section header table (FIXME: See comments re sections at top of this
>> +     file).  */
>> +
>> +  if (! elf_file_p (&x_ehdr)
>> +      || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
>> +      || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
>> +    goto wrong;
>> +
>> +  /* Check that file's byte order matches xvec's */
>> +  switch (x_ehdr.e_ident[EI_DATA])
>> +    {
>> +    case ELFDATA2MSB:		/* Big-endian */
>> +      if (! bfd_header_big_endian (abfd))
>> +	goto wrong;
>> +      break;
>> +    case ELFDATA2LSB:		/* Little-endian */
>> +      if (! bfd_header_little_endian (abfd))
>> +	goto wrong;
>> +      break;
>> +    case ELFDATANONE:		/* No data encoding specified */
>> +    default:			/* Unknown data encoding specified */
>> +      goto wrong;
>> +    }
>> +
>> +  elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
>> +#if DEBUG & 1
>> +  elf_debug_file (&i_ehdr);
>> +#endif
>> +
>> +  if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
>> +    goto fail;
>> +
>> +  /* Read in program headers.  */
>> +  i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
>> +					      sizeof (*i_phdr));
>> +  if (i_phdr == NULL)
>> +    goto fail;
>> +
>> +  if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
>> +    goto fail;
>> +
>> +  /* Read in program headers and parse notes.  */
>> +  for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
>> +    {
>> +      Elf_External_Phdr x_phdr;
>> +
>> +      if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
>> +	goto fail;
>> +      elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
>> +
>> +      if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
>> +	{
>> +	  elf_read_notes (abfd, offset + i_phdr->p_offset,
>> +			  i_phdr->p_filesz, i_phdr->p_align);
>> +	  if (abfd->build_id != NULL)
>> +	    return TRUE;
>> +	}
>> +    }
>> +
>> +  /* Having gotten this far, we have a valid ELF section, but no
>> +     build-id was found.  */
>> +  goto fail;
>> +
>> +wrong:
>> +  bfd_set_error (bfd_error_wrong_format);
>> +fail:
>> +  return FALSE;
>> +}
>> diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
>> index 78a1f6314d..29ad2e8481 100644
>> --- a/bfd/elfxx-target.h
>> +++ b/bfd/elfxx-target.h
>> @@ -521,6 +521,9 @@
>>  #ifndef elf_backend_bfd_from_remote_memory
>>  #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
>>  #endif
>> +#ifndef elf_backend_core_find_build_id
>> +#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
>> +#endif
>>  #ifndef elf_backend_got_header_size
>>  #define elf_backend_got_header_size	0
>>  #endif
>> @@ -860,6 +863,7 @@ static struct elf_backend_data elfNN_bed =
>>    elf_backend_mips_rtype_to_howto,
>>    elf_backend_ecoff_debug_swap,
>>    elf_backend_bfd_from_remote_memory,
>> +  elf_backend_core_find_build_id,
>>    elf_backend_plt_sym_val,
>>    elf_backend_common_definition,
>>    elf_backend_common_section_index,
>>
> 
> 

Re: [PATCH] Core file build-ids

[Nick Clifton]

Hi Keith,

> Ping?

Oops - sorry.

>>> 	* elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
>>> 	field.
>>> 	(_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
>>> 	functions.
>>> 	(elf_read_notes): Add declaration.
>>> 	* elf.c (elf_read_notes): Move elf-bfd.h.
>>> 	(_bfd_elf_core_find_build_id): New function.
>>> 	(bfd_section_from_phdr): Scan core file PT_LOAD segments for
>>> 	build-id if none is known.
>>> 	(elf_parse_notes): For core files, scan for notes.
>>> 	* elfcore.h (elf_core_file_matches_executable_p): If both
>>> 	BFDs have identical build-ids, then they match.
>>> 	(_bfd_elf_core_find_build_id): New function.
>>> 	* elfxx-target.h (elf_backend_core_find_build_id): Define.
>>> 	(elfNN_bed): Add elf_backend_core_find_build_id.

Approved and applied.

Cheers
  Nick