[patchv3 0/8] Validate binary before use

[patchv3 0/8] Validate binary before use

[Jan Kratochvil]

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

Hi,

git://sourceware.org/git/archer.git
jankratochvil/gdbserverbuildid
 - although it is not properly separated into 1..8 there as in this series

the is a follow-up to never checked in series:
	Message-ID: <1366127096-5744-1-git-send-email-ARistovski@qnx.com>
	https://sourceware.org/ml/gdb-patches/2013-04/msg00472.html

The changes [attached] are:

 * Implemented the review comments I made.
   (Except for comments for the testcase.)

 * Removed fetching build-id in solib-svr4.c for NAT run:
   /* There is no way to safely fetch build-id from running inferior without OS
      specific code.  The code from get_hex_build_id from gdbserver/linux-low.c
      could be used for GNU/Linux NAT target.  */

   As former code regressed break-interp.exp with 32-bit targets for:
   egrep '(BINprelinkNO.*pieATTACH.*relinkYES|BINprelinkYES.*pieATTACH.*relinkNO).*attach main bt'

   Implementing new feature into gdbserver and not linux-nat seems correct to
   me according to:
   https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity
   I will list there this local/remote difference after check-in but that
   should be OK after the final switch to gdbserver.

I left there authorship intact except for added mine to [patchv3 7/8].
I do not mind to change it any way.


Jan

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

diff --git a/gdb/common/common-target.c b/gdb/common/common-target.c
index 4bd2731..d6e5d60 100644
--- a/gdb/common/common-target.c
+++ b/gdb/common/common-target.c
@@ -1,6 +1,6 @@
 /* Utility target functions for GDB, and GDBserver.
 
-   Copyright (C) 2013 Free Software Foundation, Inc.
+   Copyright (C) 2014 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
diff --git a/gdb/common/common-target.h b/gdb/common/common-target.h
index b252c00..9aedc12 100644
--- a/gdb/common/common-target.h
+++ b/gdb/common/common-target.h
@@ -1,6 +1,6 @@
 /* Utility target functions for GDB, and GDBserver.
 
-   Copyright (C) 2013 Free Software Foundation, Inc.
+   Copyright (C) 2014 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
diff --git a/gdb/common/linux-maps.c b/gdb/common/linux-maps.c
index 6432b8f..bb587df 100644
--- a/gdb/common/linux-maps.c
+++ b/gdb/common/linux-maps.c
@@ -1,5 +1,5 @@
 /* Linux-specific memory maps manipulation routines.
-   Copyright (C) 2013 Free Software Foundation, Inc.
+   Copyright (C) 2014 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
diff --git a/gdb/common/linux-maps.h b/gdb/common/linux-maps.h
index e989376..03b14d3 100644
--- a/gdb/common/linux-maps.h
+++ b/gdb/common/linux-maps.h
@@ -1,5 +1,5 @@
 /* Linux-specific memory maps manipulation routines.
-   Copyright (C) 2013 Free Software Foundation, Inc.
+   Copyright (C) 2014 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index d14b215..6f05bd4 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -163,10 +163,7 @@ typedef union ElfXX_Nhdr
 #define ELFXX_SIZEOF(elf64, hdr) ((elf64) ? sizeof ((hdr)._64) \
 					  : sizeof ((hdr)._32))
 /* Round up to next 4 byte boundary.  */
-#define ELFXX_ROUNDUP_4(elf64, what) ((elf64) ? (((what) + 3)		\
-						 & ~((Elf64_Word)3U))	\
-					      : (((what) + 3)		\
-						 & ~((Elf32_Word) 3U)))
+#define ELFXX_ROUNDUP_4(elf64, what) (((what) + 3) & ~(ULONGEST) 3)
 #define BUILD_ID_INVALID "?"
 
 /* A list of all unknown processes which receive stop signals.  Some
@@ -5764,7 +5761,7 @@ free_mapping_entry_vec (VEC (mapping_entry_s) *lst)
 static int
 compare_mapping_entry_range (const void *const k, const void *const b)
 {
-  const ULONGEST key = *(CORE_ADDR *) k;
+  const ULONGEST key = *(const CORE_ADDR *) k;
   const mapping_entry_s *const p = b;
 
   if (key < p->vaddr)
@@ -5812,7 +5809,8 @@ read_build_id (struct find_memory_region_callback_data *const p,
 	  || e_phentsize != ELFXX_SIZEOF (is_elf64, *phdr))
 	{
 	  /* Basic sanity check failed.  */
-	  warning ("Could not read program header.");
+	  warning (_("Could not identify program header at %s."),
+		   paddress (load_addr));
 	  return;
 	}
 
@@ -5823,7 +5821,8 @@ read_build_id (struct find_memory_region_callback_data *const p,
 			     ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize)
 	  != 0)
 	{
-	  warning ("Could not read program header.");
+	  warning (_("Could not read program header at %s."),
+		   paddress (load_addr));
 	  return;
 	}
 
@@ -5847,14 +5846,14 @@ read_build_id (struct find_memory_region_callback_data *const p,
 				 ELFXX_FLD (is_elf64, *phdr, p_memsz)) != 0)
 	    {
 	      xfree (pt_note);
-	      warning ("Could not read note at address 0x%s",
+	      warning (_("Could not read note at address 0x%s"),
 		       paddress (note_addr));
 	      break;
 	    }
 
 	  pt_end = pt_note + ELFXX_FLD (is_elf64, *phdr, p_memsz);
 	  nhdr = (void *) pt_note;
-	  while ((gdb_byte *) nhdr < pt_end)
+	  while ((const gdb_byte *) nhdr < pt_end)
 	    {
 	      const size_t namesz
 		= ELFXX_ROUNDUP_4 (is_elf64, ELFXX_FLD (is_elf64, *nhdr,
@@ -5862,14 +5861,14 @@ read_build_id (struct find_memory_region_callback_data *const p,
 	      const size_t descsz
 		= ELFXX_ROUNDUP_4 (is_elf64, ELFXX_FLD (is_elf64, *nhdr,
 							n_descsz));
-	      const size_t note_sz = ELFXX_SIZEOF (is_elf64, *nhdr) + namesz
-				     + descsz;
+	      const size_t note_sz = (ELFXX_SIZEOF (is_elf64, *nhdr) + namesz
+				      + descsz);
 
-	      if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0
+	      if (((const gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0
 		  || descsz == 0)
 		{
-		  warning ("Malformed PT_NOTE at address 0x%s\n",
-			   paddress ((CORE_ADDR) nhdr));
+		  warning (_("Malformed PT_NOTE at address 0x%s\n"),
+			   paddress (note_addr + (gdb_byte *) nhdr - pt_note));
 		  break;
 		}
 	      if (ELFXX_FLD (is_elf64, *nhdr, n_type) == NT_GNU_BUILD_ID
@@ -5981,9 +5980,10 @@ get_hex_build_id (const CORE_ADDR l_addr, const CORE_ADDR l_ld,
 	{
 	  /* Do not try to find hex_build_id again.  */
 	  bil->hex_build_id = xstrdup (BUILD_ID_INVALID);
-	  warning ("Could not determine load address; mapping entry with"
-		   " offset 0 corresponding to l_ld=0x%s could not be found; "
-		   "build-id can not be used.", paddress (l_ld));
+	  warning (_("Could not determine load address; mapping entry with "
+		     "offset 0 corresponding to l_ld = 0x%s could not be "
+		     "found; build-id can not be used."),
+		   paddress (l_ld));
 	}
     }
 
@@ -6050,7 +6050,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   VEC_reserve (mapping_entry_s, data.list, 16);
   if (linux_find_memory_regions_full (pid, find_memory_region_callback, &data,
       NULL) < 0)
-    warning ("Finding memory regions failed");
+    warning (_("Finding memory regions failed"));
 
   while (annex[0] != '\0')
     {
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 235a7c2..d0c8761 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -737,7 +737,7 @@ linux_find_memory_regions_gdb (struct gdbarch *gdbarch,
   /* We need to know the real target PID so
      linux_find_memory_regions_full can access /proc.  */
   if (current_inferior ()->fake_pid_p)
-    return 1;
+    return -1;
 
   cleanup = make_cleanup (free_current_contents, &memory_to_free);
   retval = linux_find_memory_regions_full (current_inferior ()->pid,
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 9de1ae7..7d4cce9 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -968,12 +968,14 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
 static int
 svr4_validate (const struct so_list *const so)
 {
-  gdb_byte *build_id;
-  size_t build_idsz;
-  size_t abfd_build_idsz;
-
   gdb_assert (so != NULL);
 
+  /* There is no way to safely fetch build-id from running inferior without OS
+     specific code.  The code from get_hex_build_id from gdbserver/linux-low.c
+     could be used for GNU/Linux NAT target.  */
+  if (so->build_id == NULL)
+    return 1;
+
   if (so->abfd == NULL)
     return 1;
 
@@ -982,96 +984,9 @@ svr4_validate (const struct so_list *const so)
       || elf_tdata (so->abfd)->build_id == NULL)
     return 1;
 
-  build_id = so->build_id;
-  build_idsz = so->build_idsz;
-  abfd_build_idsz = elf_tdata (so->abfd)->build_id->size;
-
-  if (build_id == NULL)
-    {
-      /* Get build_id from NOTE_GNU_BUILD_ID_NAME section.
-         This is a fallback mechanism for targets that do not
-	 implement TARGET_OBJECT_SOLIB_SVR4.  */
-
-      const asection *const asec
-	= bfd_get_section_by_name (so->abfd, NOTE_GNU_BUILD_ID_NAME);
-      ULONGEST bfd_sect_size;
-
-      if (asec == NULL)
-	return 1;
-
-      bfd_sect_size = bfd_get_section_size (asec);
-
-      if ((asec->flags & SEC_LOAD) == SEC_LOAD
-	  && bfd_sect_size != 0
-	  && strcmp (bfd_section_name (asec->bfd, asec),
-		     NOTE_GNU_BUILD_ID_NAME) == 0)
-	{
-	  const enum bfd_endian byte_order
-	    = gdbarch_byte_order (target_gdbarch ());
-	  Elf_External_Note *const note = xmalloc (bfd_sect_size);
-	  gdb_byte *const note_raw = (void *) note;
-	  struct cleanup *cleanups = make_cleanup (xfree, note);
-
-	  if (target_read_memory (bfd_get_section_vma (so->abfd, asec)
-				  + lm_addr_check (so, so->abfd),
-				  note_raw, bfd_sect_size) == 0)
-	    {
-	      build_idsz
-		= extract_unsigned_integer ((gdb_byte *) note->descsz,
-					    sizeof (note->descsz),
-					    byte_order);
-
-	      if (build_idsz == abfd_build_idsz)
-		{
-		  const char gnu[4] = "GNU\0";
-
-		  if (memcmp (note->name, gnu, 4) == 0)
-		    {
-		      ULONGEST namesz
-			= extract_unsigned_integer ((gdb_byte *) note->namesz,
-						    sizeof (note->namesz),
-						    byte_order);
-		      CORE_ADDR build_id_offs;
-
-		      /* Rounded to next sizeof (ElfXX_Word).  */
-		      namesz = ((namesz + (sizeof (note->namesz) - 1))
-				& ~((ULONGEST) (sizeof (note->namesz) - 1)));
-		      build_id_offs = (offsetof (Elf_External_Note, name)
-				       + namesz);
-		      build_id = xmalloc (build_idsz);
-		      memcpy (build_id, note_raw + build_id_offs, build_idsz);
-		    }
-		}
-
-	      if (build_id == NULL)
-		{
-		  /* If we are here, it means target memory read succeeded
-		     but note was not where it was expected according to the
-		     abfd.  Allow the logic below to perform the check
-		     with an impossible build-id and fail validation.  */
-		  build_idsz = 0;
-		  build_id = xmalloc (0);
-		}
-
-	    }
-	  do_cleanups (cleanups);
-	}
-    }
-
-  if (build_id != NULL)
-    {
-      const int match
-	= (abfd_build_idsz == build_idsz
-	   && memcmp (build_id, elf_tdata (so->abfd)->build_id->data,
-		      build_idsz) == 0);
-
-      if (build_id != so->build_id)
-	xfree (build_id);
-
-      return match;
-    }
-
-  return 1;
+  return (so->build_idsz == elf_tdata (so->abfd)->build_id->size
+	  && memcmp (so->build_id, elf_tdata (so->abfd)->build_id->data,
+		     so->build_idsz) == 0);
 }
 
 /* Implement the "open_symbol_file_object" target_so_ops method.
diff --git a/gdb/target.c b/gdb/target.c
index abf9058..d892e25 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3020,9 +3020,6 @@ target_fileio_close_cleanup (void *opaque)
   target_fileio_close (fd, &target_errno);
 }
 
-typedef int (read_alloc_pread_ftype) (int handle, gdb_byte *read_buf, int len,
-				      ULONGEST offset, int *target_errno);
-
 static read_alloc_pread_ftype target_fileio_read_alloc_1_pread;
 
 /* Helper for target_fileio_read_alloc_1 to make it interruptible.  */
@@ -3036,9 +3033,6 @@ target_fileio_read_alloc_1_pread (int handle, gdb_byte *read_buf, int len,
   return target_fileio_pread (handle, read_buf, len, offset, target_errno);
 }
 
-typedef LONGEST (read_stralloc_func_ftype) (const char *filename,
-					    gdb_byte **buf_p, int padding);
-
 static read_stralloc_func_ftype target_fileio_read_alloc_1;
 
 /* Read target file FILENAME.  Store the result in *BUF_P and
diff --git a/gdb/testsuite/gdb.base/solib-mismatch-lib.c b/gdb/testsuite/gdb.base/solib-mismatch-lib.c
index 19f1545..65b26af 100644
--- a/gdb/testsuite/gdb.base/solib-mismatch-lib.c
+++ b/gdb/testsuite/gdb.base/solib-mismatch-lib.c
@@ -1,6 +1,6 @@
 /* This testcase is part of GDB, the GNU debugger.
 
-   Copyright 2013 Free Software Foundation, Inc.
+   Copyright 2014 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
diff --git a/gdb/testsuite/gdb.base/solib-mismatch-libmod.c b/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
index 3b025a8..fc8827e 100644
--- a/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
+++ b/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
@@ -1,6 +1,6 @@
 /* This testcase is part of GDB, the GNU debugger.
 
-   Copyright 2013 Free Software Foundation, Inc.
+   Copyright 2014 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
diff --git a/gdb/testsuite/gdb.base/solib-mismatch.c b/gdb/testsuite/gdb.base/solib-mismatch.c
index 7a5d960..7bf425d 100644
--- a/gdb/testsuite/gdb.base/solib-mismatch.c
+++ b/gdb/testsuite/gdb.base/solib-mismatch.c
@@ -1,6 +1,6 @@
 /* This testcase is part of GDB, the GNU debugger.
 
-   Copyright 2013 Free Software Foundation, Inc.
+   Copyright 2014 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
diff --git a/gdb/testsuite/gdb.base/solib-mismatch.exp b/gdb/testsuite/gdb.base/solib-mismatch.exp
index 4504bc3..7a625ba 100644
--- a/gdb/testsuite/gdb.base/solib-mismatch.exp
+++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
@@ -1,4 +1,4 @@
-# Copyright 2013 Free Software Foundation, Inc.
+# Copyright 2014 Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -16,6 +16,11 @@
 standard_testfile
 set executable $testfile
 
+if ![is_remote target] {
+  untested "only gdbserver supports build-id reporting"
+  return -1
+}
+
 # Test overview:
 #  generate two shared objects. One that will be used by the process
 #  and another, modified, that will be found by gdb. Gdb should
@@ -69,7 +74,7 @@ if { [build_executable $testfile.exp $executable $srcfile $exec_opts] != 0 } {
 if { [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilerun}" "${binlibfilerun}" [list debug ldflags=-Wl,--build-id]] != ""
      || [gdb_gnu_strip_debug "${binlibfilerun}"]
      || [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilegdb}" "${binlibfilegdb}" [list debug ldflags=-Wl,--build-id]] != "" } {
-  untested "gdb_compile_shlib failed."
+  untested "compilation failed."
   return -1
 }
 

Re: [patchv3 0/8] Validate binary before use

[Pedro Alves]

On 02/27/2014 09:32 PM, Jan Kratochvil wrote:
> Hi,
> 
> git://sourceware.org/git/archer.git
> jankratochvil/gdbserverbuildid
>  - although it is not properly separated into 1..8 there as in this series

Thanks.  Personally for review, I prefer a git branch with the
exact same contents as posted.

(BTW, git am caught a couple whitespace issues:

$ git am /tmp/jan1
Applying: Move utility functions to common/
Applying: Merge multiple hex conversions
Applying: Create empty common/linux-maps.[ch] and common/common-target.[ch]
Applying: Prepare linux_find_memory_regions_full & co. for move
Applying: Move linux_find_memory_regions_full & co.
Applying: gdbserver build-id attribute generator
Applying: Validate symbol file using build-id
Applying: Tests for validate symbol file using build-id
/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:256: trailing whitespace.

/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:271: trailing whitespace.
# Keep previous so for debugging puroses
/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:274: trailing whitespace.
# Copy loaded so over the one gdb will find
warning: 3 lines add whitespace errors.)

I looked through the whole series, and sent out a few minor comments,
but it all looked good to me otherwise.

I think this is a quite nice feature.  Thanks for doing this (you both).

> 
> the is a follow-up to never checked in series:
> 	Message-ID: <1366127096-5744-1-git-send-email-ARistovski@qnx.com>
> 	https://sourceware.org/ml/gdb-patches/2013-04/msg00472.html

To save others the same, I followed a few links to find the description
of the series, here:

  https://sourceware.org/ml/gdb-patches/2013-04/msg00220.html

>    Implementing new feature into gdbserver and not linux-nat seems correct to
>    me according to:
>    https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity
>    I will list there this local/remote difference after check-in but that
>    should be OK after the final switch to gdbserver.

If only one side can be implemented, then yes, agreed, let's prefer the
GDBserver side.  However, if not hard, I'd rather we did both, as we
can't tell how long it will take until we finally share the
backends.  It's only the case of only implementing something _only_ on
the native side and not in gdbserver that puts us further away
from the end goal.  With that in mind, how hard would it be to support
this on current native as well?  Can we do it by sharing most of
the build id code, say by splitting out most of the Linux build
id stuff out a linux-buildid.c file that is built by both gdb
and gdbserver?

BTW, do you plan on contributing support for validation with
cores too?

I think this should be mentioned in NEWS.

The new attribute of the xml of qXfer:libraries-svr4 should
be mentioned in NEWS too.

Also, I think somewhere in the manual we should explain this:

+      warning (_("Shared object \"%s\" could not be validated "
+		 "and will be ignored."), so->so_name);

I wonder whether we'll need to let the user force-disable
this validation?  I think so.  Sometimes it might be useful to
force GDB into taking the symbols even if it's not an exact match
(say, if one rebuilds to get back symbols, or to get around
potential tooling problems).

> --- a/gdb/testsuite/gdb.base/solib-mismatch.exp> +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
> @@ -1,4 +1,4 @@
> -# Copyright 2013 Free Software Foundation, Inc.
> +# Copyright 2014 Free Software Foundation, Inc.

These need to be 2013-2014.

-- 
Pedro Alves

Re: [patchv3 0/8] Validate binary before use

[Aleksandar Ristovski]

On 14-02-28 08:46 AM, Pedro Alves wrote:
> I think this is a quite nice feature.  Thanks for doing this (you both).


Thanks Jan for the follow-up, and you for reviewing.

I would be unrealistic if I promised to get back to it any time soon.


---
Aleksandar Ristovski
QNX Software Systems

Re: [patchv3 0/8] Validate binary before use

[Jan Kratochvil]

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

On Fri, 28 Feb 2014 14:46:38 +0100, Pedro Alves wrote:
> On 02/27/2014 09:32 PM, Jan Kratochvil wrote:
> >    Implementing new feature into gdbserver and not linux-nat seems correct to
> >    me according to:
> >    https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity
> >    I will list there this local/remote difference after check-in but that
> >    should be OK after the final switch to gdbserver.
> 
> If only one side can be implemented, then yes, agreed, let's prefer the
> GDBserver side.  However, if not hard, I'd rather we did both, as we
> can't tell how long it will take until we finally share the
> backends.  It's only the case of only implementing something _only_ on
> the native side and not in gdbserver that puts us further away
> from the end goal.  With that in mind, how hard would it be to support
> this on current native as well?  Can we do it by sharing most of
> the build id code, say by splitting out most of the Linux build
> id stuff out a linux-buildid.c file that is built by both gdb
> and gdbserver?

Yes, it could be moved to gdb/common/ and called also from linux-nat.c.
I have not done so.

My goal of this patch series was not the validation as described below.
In fact I am not aware in which cases it is needed.  Or rather I am aware of
it for a different use case as it partially overlaps my non-upstreamed "locate
files by their build-id from a core file":
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-6.6-buildid-locate.patch


> BTW, do you plan on contributing support for validation with cores too?

No.  If related I find right to extend gdbserver to support cores, it would be
useful for ABRT to prevent needless uploads of many MBs of compressed core
files.  I have added "core file loading " to:
	https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity?action=diff&rev2=56&rev1=55

From this patch series I needed only the 'build-id' attribute generator in
gdbserver (Red Hat internal #1068777), this is why I took over this patchset.
I just found unfair to skip the validator part of it when I have taken the
other parts so I am pushing it all now.


> I think this should be mentioned in NEWS.
> 
> The new attribute of the xml of qXfer:libraries-svr4 should
> be mentioned in NEWS too.

OK, added to

* New features in the GDB remote stub, GDBserver

new text:

  ** qXfer:libraries-svr4 contains also optional attribute 'build-id'
     for each library.  GDB does not load library with build-id
     that does not match such attribute.


> Also, I think somewhere in the manual we should explain this:
> 
> +      warning (_("Shared object \"%s\" could not be validated "
> +		 "and will be ignored."), so->so_name);
> 
> I wonder whether we'll need to let the user force-disable
> this validation?  I think so.  Sometimes it might be useful to
> force GDB into taking the symbols even if it's not an exact match

Done.


> > --- a/gdb/testsuite/gdb.base/solib-mismatch.exp
> > +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
> > @@ -1,4 +1,4 @@
> > -# Copyright 2013 Free Software Foundation, Inc.
> > +# Copyright 2014 Free Software Foundation, Inc.
> 
> These need to be 2013-2014.

Why?  It counts since the first post to gdb-patches?


Thanks,
Jan

[-- Attachment #2: Type: message/rfc822, Size: 18157 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Validate symbol file using build-id
Date: Sun, 2 Mar 2014 10:15:42 +0100

gdb/
2014-02-26  Aleksandar Ristovski  <aristovski@qnx.com
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	Validate symbol file using build-id.
	* NEWS (Changes since GDB 7.7): Add 'set solib-build-id-force'
	and 'show solib-build-id-force'.  Add build-id attribute.
	* solib-darwin.c (_initialize_darwin_solib): Assign validate value.
	* solib-dsbt.c (_initialize_dsbt_solib): Ditto.
	* solib-frv.c (_initialize_frv_solib): Ditto.
	* solib-ia64-hpux.c (ia64_hpux_target_so_ops): Ditto.
	* solib-irix.c (_initialize_irix_solib): Ditto.
	* solib-osf.c (_initialize_osf_solib): Ditto.
	* solib-pa64.c (_initialize_pa64_solib): Ditto.
	* solib-som.c (_initialize_som_solib): Ditto.
	* solib-spu.c (set_spu_solib_ops): Ditto.
	* solib-svr4.c: Include rsp-low.h.
	(NOTE_GNU_BUILD_ID_NAME): New define.
	(svr4_validate): New function.
	(library_list_start_library): Parse 'build-id' attribute.
	(svr4_library_attributes): Add 'build-id' attribute.
	(_initialize_svr4_solib): Assign validate value.
	* solib-target.c (solib.h): Include.
	(_initialize_solib_target): Assign validate value.
	* solib.c (solib_build_id_force, show_solib_build_id_force): New.
	(solib_map_sections): Use ops->validate.
	(clear_so): Free build_id.
	(default_solib_validate): New function.
	(_initialize_solib): Add "solib-build-id-force".
	* solib.h (default_solib_validate): New declaration.
	* solist.h (struct so_list): New fields 'build_idsz' and 'build_id'.
	(target_so_ops): New field 'validate'.

gdb/doc/
2014-03-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo (Files): Add 'set solib-build-id-force'
	and 'show solib-build-id-force'.
---
 gdb/NEWS              | 10 +++++++++
 gdb/doc/gdb.texinfo   | 34 ++++++++++++++++++++++++++++++
 gdb/solib-darwin.c    |  1 +
 gdb/solib-dsbt.c      |  1 +
 gdb/solib-frv.c       |  1 +
 gdb/solib-ia64-hpux.c |  1 +
 gdb/solib-irix.c      |  1 +
 gdb/solib-osf.c       |  1 +
 gdb/solib-pa64.c      |  1 +
 gdb/solib-som.c       |  1 +
 gdb/solib-spu.c       |  1 +
 gdb/solib-svr4.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib-target.c    |  2 ++
 gdb/solib.c           | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib.h           |  4 ++++
 gdb/solist.h          | 14 +++++++++++++
 16 files changed, 182 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 2a384ba..629cc13 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -43,6 +43,12 @@ maint ada show ignore-descriptive-types
   the user manual for more details on descriptive types and the intended
   usage of this option.
 
+set solib-build-id-force (on|off)
+show solib-build-id-force
+  Inferior shared library and symbol file may contain unique build-id.
+  If both build-ids are present but they do not match then this setting
+  enables (on) or disables (off) loading of such symbol file.
+
 * New features in the GDB remote stub, GDBserver
 
   ** New option --debug-format=option1[,option2,...] allows one to add
@@ -51,6 +57,10 @@ maint ada show ignore-descriptive-types
      Timestamps can also be turned on with the
      "monitor set debug-format timestamps" command from GDB.
 
+  ** library-list-svr4 contains also optional attribute 'build-id' for
+     each library.  GDB does not load library with build-id that
+     does not match such attribute.
+
 * The 'record instruction-history' command now starts counting instructions
   at one.  This also affects the instruction ranges reported by the
   'record function-call-history' command when given the /i modifier.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b1b29bd..c683ede 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -17114,6 +17114,40 @@ libraries that were loaded by explicit user requests are not
 discarded.
 @end table
 
+@table @code
+@kindex set solib-build-id-force
+@item set solib-build-id-force @var{mode}
+Setting to override @value{GDBN} build-id check.
+
+Inferior shared library and symbol file may contain unique build-id.
+If both build-ids are present but they do not match then this setting
+enables (@var{mode} is @code{on}) or disables (@var{mode} is @code{off})
+loading of such symbol file.  On systems where build-id is not present
+in files this setting has no effect.  The default value is @code{off}.
+
+Loading non-matching symbol file may confuse debugging including breakage
+of backtrace output.
+
+By default @value{GDBN} will ignore symbol files with non-matching build-id
+while printing:
+
+@smallexample
+  Shared object "libfoo.so.1" could not be validated and will be ignored;
+  or use 'set solib-build-id-force'.
+@end smallexample
+
+Turning on this setting would load such symbol file while still printing:
+
+@smallexample
+  Shared object "libfoo.so.1" could not be validated but it is being loaded
+  due to 'set solib-build-id-force'.
+@end smallexample
+
+@kindex show solib-build-id-force
+@item show solib-build-id-force
+Display the current mode of build-id check override.
+@end table
+
 Sometimes you may wish that @value{GDBN} stops and gives you control
 when any of shared library events happen.  The best way to do this is
 to use @code{catch load} and @code{catch unload} (@pxref{Set
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index e8d4667..d65b2c8 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -650,4 +650,5 @@ _initialize_darwin_solib (void)
   darwin_so_ops.in_dynsym_resolve_code = darwin_in_dynsym_resolve_code;
   darwin_so_ops.lookup_lib_global_symbol = darwin_lookup_lib_symbol;
   darwin_so_ops.bfd_open = darwin_bfd_open;
+  darwin_so_ops.validate = default_solib_validate;
 }
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 0217a94..35223dd 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -1078,6 +1078,7 @@ _initialize_dsbt_solib (void)
   dsbt_so_ops.open_symbol_file_object = open_symbol_file_object;
   dsbt_so_ops.in_dynsym_resolve_code = dsbt_in_dynsym_resolve_code;
   dsbt_so_ops.bfd_open = solib_bfd_open;
+  dsbt_so_ops.validate = default_solib_validate;
 
   /* Debug this file's internals.  */
   add_setshow_zuinteger_cmd ("solib-dsbt", class_maintenance,
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index 9724a3c..18a4aba 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -1181,6 +1181,7 @@ _initialize_frv_solib (void)
   frv_so_ops.open_symbol_file_object = open_symbol_file_object;
   frv_so_ops.in_dynsym_resolve_code = frv_in_dynsym_resolve_code;
   frv_so_ops.bfd_open = solib_bfd_open;
+  frv_so_ops.validate = default_solib_validate;
 
   /* Debug this file's internals.  */
   add_setshow_zuinteger_cmd ("solib-frv", class_maintenance,
diff --git a/gdb/solib-ia64-hpux.c b/gdb/solib-ia64-hpux.c
index b53caa8..a39bb03 100644
--- a/gdb/solib-ia64-hpux.c
+++ b/gdb/solib-ia64-hpux.c
@@ -688,6 +688,7 @@ ia64_hpux_target_so_ops (void)
   ops->open_symbol_file_object = ia64_hpux_open_symbol_file_object;
   ops->in_dynsym_resolve_code = ia64_hpux_in_dynsym_resolve_code;
   ops->bfd_open = solib_bfd_open;
+  ops->validate = default_solib_validate;
 
   return ops;
 }
diff --git a/gdb/solib-irix.c b/gdb/solib-irix.c
index 6266ee0..1f126a3 100644
--- a/gdb/solib-irix.c
+++ b/gdb/solib-irix.c
@@ -652,4 +652,5 @@ _initialize_irix_solib (void)
   irix_so_ops.open_symbol_file_object = irix_open_symbol_file_object;
   irix_so_ops.in_dynsym_resolve_code = irix_in_dynsym_resolve_code;
   irix_so_ops.bfd_open = solib_bfd_open;
+  irix_so_ops.validate = default_solib_validate;
 }
diff --git a/gdb/solib-osf.c b/gdb/solib-osf.c
index 90a26e8..5490f83 100644
--- a/gdb/solib-osf.c
+++ b/gdb/solib-osf.c
@@ -633,6 +633,7 @@ _initialize_osf_solib (void)
   osf_so_ops.open_symbol_file_object = osf_open_symbol_file_object;
   osf_so_ops.in_dynsym_resolve_code = osf_in_dynsym_resolve_code;
   osf_so_ops.bfd_open = solib_bfd_open;
+  osf_so_ops.validate = default_solib_validate;
 
   /* FIXME: Don't do this here.  *_gdbarch_init() should set so_ops.  */
   current_target_so_ops = &osf_so_ops;
diff --git a/gdb/solib-pa64.c b/gdb/solib-pa64.c
index 099e1e7..da8a294 100644
--- a/gdb/solib-pa64.c
+++ b/gdb/solib-pa64.c
@@ -621,6 +621,7 @@ _initialize_pa64_solib (void)
   pa64_so_ops.open_symbol_file_object = pa64_open_symbol_file_object;
   pa64_so_ops.in_dynsym_resolve_code = pa64_in_dynsym_resolve_code;
   pa64_so_ops.bfd_open = solib_bfd_open;
+  pa64_so_ops.validate = default_solib_validate;
 
   memset (&dld_cache, 0, sizeof (dld_cache));
 }
diff --git a/gdb/solib-som.c b/gdb/solib-som.c
index cba50d1..ed2cbfe 100644
--- a/gdb/solib-som.c
+++ b/gdb/solib-som.c
@@ -816,6 +816,7 @@ _initialize_som_solib (void)
   som_so_ops.open_symbol_file_object = som_open_symbol_file_object;
   som_so_ops.in_dynsym_resolve_code = som_in_dynsym_resolve_code;
   som_so_ops.bfd_open = solib_bfd_open;
+  som_so_ops.validate = default_solib_validate;
 }
 
 void
diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
index 1c574b5..8385205 100644
--- a/gdb/solib-spu.c
+++ b/gdb/solib-spu.c
@@ -521,6 +521,7 @@ set_spu_solib_ops (struct gdbarch *gdbarch)
       spu_so_ops.current_sos = spu_current_sos;
       spu_so_ops.bfd_open = spu_bfd_open;
       spu_so_ops.lookup_lib_global_symbol = spu_lookup_lib_symbol;
+      spu_so_ops.validate = default_solib_validate;
     }
 
   set_solib_ops (gdbarch, &spu_so_ops);
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 4c94f9f..14c4c19 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -47,6 +47,9 @@
 #include "exceptions.h"
 #include "gdb_bfd.h"
 #include "probe.h"
+#include "rsp-low.h"
+
+#define NOTE_GNU_BUILD_ID_NAME  ".note.gnu.build-id"
 
 static struct link_map_offsets *svr4_fetch_link_map_offsets (void);
 static int svr4_have_link_map_offsets (void);
@@ -959,6 +962,31 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
   return (name_lm >= vaddr && name_lm < vaddr + size);
 }
 
+/* Validate SO by comparing build-id from the associated bfd and
+   corresponding build-id from target memory.  */
+
+static int
+svr4_validate (const struct so_list *const so)
+{
+  gdb_assert (so != NULL);
+
+  /* Target doesn't support reporting the build ID.  */
+  if (so->build_id == NULL)
+    return 1;
+
+  if (so->abfd == NULL)
+    return 1;
+
+  if (!bfd_check_format (so->abfd, bfd_object)
+      || bfd_get_flavour (so->abfd) != bfd_target_elf_flavour
+      || elf_tdata (so->abfd)->build_id == NULL)
+    return 1;
+
+  return (so->build_idsz == elf_tdata (so->abfd)->build_id->size
+	  && memcmp (so->build_id, elf_tdata (so->abfd)->build_id->data,
+		     so->build_idsz) == 0);
+}
+
 /* Implement the "open_symbol_file_object" target_so_ops method.
 
    If no open symbol file, attempt to locate and open the main symbol
@@ -1124,6 +1152,9 @@ library_list_start_library (struct gdb_xml_parser *parser,
   ULONGEST *lmp = xml_find_attribute (attributes, "lm")->value;
   ULONGEST *l_addrp = xml_find_attribute (attributes, "l_addr")->value;
   ULONGEST *l_ldp = xml_find_attribute (attributes, "l_ld")->value;
+  const struct gdb_xml_value *const att_build_id
+    = xml_find_attribute (attributes, "build-id");
+  const char *const hex_build_id = att_build_id ? att_build_id->value : NULL;
   struct so_list *new_elem;
 
   new_elem = XCNEW (struct so_list);
@@ -1135,6 +1166,25 @@ library_list_start_library (struct gdb_xml_parser *parser,
   strncpy (new_elem->so_name, name, sizeof (new_elem->so_name) - 1);
   new_elem->so_name[sizeof (new_elem->so_name) - 1] = 0;
   strcpy (new_elem->so_original_name, new_elem->so_name);
+  if (hex_build_id != NULL)
+    {
+      const size_t hex_build_id_len = strlen (hex_build_id);
+
+      if (hex_build_id_len > 0 && (hex_build_id_len & 1U) == 0)
+	{
+	  const size_t build_idsz = hex_build_id_len / 2;
+
+	  new_elem->build_id = xmalloc (build_idsz);
+	  new_elem->build_idsz = hex2bin (hex_build_id, new_elem->build_id,
+					  build_idsz);
+	  if (new_elem->build_idsz != build_idsz)
+	    {
+	      xfree (new_elem->build_id);
+	      new_elem->build_id = NULL;
+	      new_elem->build_idsz = 0;
+	    }
+	}
+    }
 
   *list->tailp = new_elem;
   list->tailp = &new_elem->next;
@@ -1169,6 +1219,7 @@ static const struct gdb_xml_attribute svr4_library_attributes[] =
   { "lm", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
   { "l_addr", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
   { "l_ld", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
+  { "build-id", GDB_XML_AF_OPTIONAL, NULL, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
@@ -3159,4 +3210,5 @@ _initialize_svr4_solib (void)
   svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
   svr4_so_ops.update_breakpoints = svr4_update_solib_event_breakpoints;
   svr4_so_ops.handle_event = svr4_handle_solib_event;
+  svr4_so_ops.validate = svr4_validate;
 }
diff --git a/gdb/solib-target.c b/gdb/solib-target.c
index bb34e4b..23b2d06 100644
--- a/gdb/solib-target.c
+++ b/gdb/solib-target.c
@@ -25,6 +25,7 @@
 #include "target.h"
 #include "vec.h"
 #include "solib-target.h"
+#include "solib.h"
 
 #include <string.h>
 
@@ -502,6 +503,7 @@ _initialize_solib_target (void)
   solib_target_so_ops.in_dynsym_resolve_code
     = solib_target_in_dynsym_resolve_code;
   solib_target_so_ops.bfd_open = solib_bfd_open;
+  solib_target_so_ops.validate = default_solib_validate;
 
   /* Set current_target_so_ops to solib_target_so_ops if not already
      set.  */
diff --git a/gdb/solib.c b/gdb/solib.c
index 3350bfd..5ce0d58 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -454,6 +454,20 @@ solib_bfd_open (char *pathname)
   return abfd;
 }
 
+/* Boolean for command 'set solib-build-id-force'.  */
+static int solib_build_id_force = 0;
+
+/* Implement 'show solib-build-id-force'.  */
+
+static void
+show_solib_build_id_force (struct ui_file *file, int from_tty,
+			   struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Loading of shared libraries "
+			    "with non-matching build-id is %s.\n"),
+		    value);
+}
+
 /* Given a pointer to one of the shared objects in our list of mapped
    objects, use the recorded name to open a bfd descriptor for the
    object, build a section table, relocate all the section addresses
@@ -486,6 +500,24 @@ solib_map_sections (struct so_list *so)
   /* Leave bfd open, core_xfer_memory and "info files" need it.  */
   so->abfd = abfd;
 
+  gdb_assert (ops->validate != NULL);
+
+  if (!ops->validate (so))
+    {
+      if (!solib_build_id_force)
+	{
+	  warning (_("Shared object \"%s\" could not be validated "
+		     "and will be ignored; or use 'set solib-build-id-force'."),
+		   so->so_name);
+	  gdb_bfd_unref (so->abfd);
+	  so->abfd = NULL;
+	  return 0;
+	}
+      warning (_("Shared object \"%s\" could not be validated "
+		 "but it is being loaded due to 'set solib-build-id-force'."),
+	       so->so_name);
+    }
+
   /* Copy the full path name into so_name, allowing symbol_file_add
      to find it later.  This also affects the =library-loaded GDB/MI
      event, and in particular the part of that notification providing
@@ -562,6 +594,9 @@ clear_so (struct so_list *so)
      of the symbol file.  */
   strcpy (so->so_name, so->so_original_name);
 
+  xfree (so->build_id);
+  so->build_id = NULL;
+
   /* Do the same for target-specific data.  */
   if (ops->clear_so != NULL)
     ops->clear_so (so);
@@ -1523,6 +1558,14 @@ remove_user_added_objfile (struct objfile *objfile)
     }
 }
 
+/* Default implementation does not perform any validation.  */
+
+int
+default_solib_validate (const struct so_list *const so)
+{
+  return 1; /* No validation.  */
+}
+
 extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
 
 void
@@ -1579,4 +1622,18 @@ PATH and LD_LIBRARY_PATH."),
 				     reload_shared_libraries,
 				     show_solib_search_path,
 				     &setlist, &showlist);
+
+  add_setshow_boolean_cmd ("solib-build-id-force", class_support,
+			   &solib_build_id_force, _("\
+Set loading of shared libraries with non-matching build-id."), _("\
+Show loading of shared libraries with non-matching build-id."), _("\
+Inferior shared library and symbol file may contain unique build-id.\n\
+If both build-ids are present but they do not match then this setting\n\
+enables (on) or disables (off) loading of such symbol file.\n\
+Loading non-matching symbol file may confuse debugging including breakage\n\
+of backtrace output."),
+			   NULL,
+			   show_solib_build_id_force,
+			   &setlist, &showlist);
+
 }
diff --git a/gdb/solib.h b/gdb/solib.h
index 29fe8f7..1f4d2b7 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -98,4 +98,8 @@ extern void update_solib_breakpoints (void);
 
 extern void handle_solib_event (void);
 
+/* Default validation always returns 1.  */
+
+extern int default_solib_validate (const struct so_list *so);
+
 #endif /* SOLIB_H */
diff --git a/gdb/solist.h b/gdb/solist.h
index ac1b1a7..68c88ee 100644
--- a/gdb/solist.h
+++ b/gdb/solist.h
@@ -75,6 +75,16 @@ struct so_list
        There may not be just one (e.g. if two segments are relocated
        differently); but this is only used for "info sharedlibrary".  */
     CORE_ADDR addr_low, addr_high;
+
+    /* Build id in raw format, contains verbatim contents of
+       .note.gnu.build-id including note header.  This is actual
+       BUILD_ID which comes either from the remote target via qXfer
+       packet or via reading target memory.  Therefore, it may differ
+       from the build-id of the associated bfd.  In a normal
+       scenario, this so would soon lose its abfd due to failed
+       validation.  */
+    size_t build_idsz;
+    gdb_byte *build_id;
   };
 
 struct target_so_ops
@@ -168,6 +178,10 @@ struct target_so_ops
        NULL, in which case no specific preprocessing is necessary
        for this target.  */
     void (*handle_event) (void);
+
+    /* Return 0 if SO does not match target SO it is supposed to
+       represent.  Return 1 otherwise.  */
+    int (*validate) (const struct so_list *so);
   };
 
 /* Free the memory associated with a (so_list *).  */
-- 
1.8.5.3

Re: [patchv3 0/8] Validate binary before use

[Pedro Alves]

On 03/02/2014 06:36 PM, Jan Kratochvil wrote:

as it partially overlaps my non-upstreamed "locate
> files by their build-id from a core file":
> 	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-6.6-buildid-locate.patch
> 
> 
>> BTW, do you plan on contributing support for validation with cores too?
> 

> No.  

OK.  I think it'd be useful, so that GDB can warn/reject in the
common scenario of the user not pointing at a sysroot with the
correct DSOs.

If related I find right to extend gdbserver to support cores, it would be
> useful for ABRT to prevent needless uploads of many MBs of compressed core
> files.  I have added "core file loading " to:
> 	https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity?action=diff&rev2=56&rev1=55

Thanks.  I think that feature may be a little beyond borderline
for that project page though, as core loading is not really part
of the native target.  IMO it'd good if this had its own project
page, where it'd give answer to questions like the below.

#0 - If implemented by gdbserver, it seems like it'd involve further user
action to trigger the feature?  How does one tell GDB/gdbserver to load the
ore on the remote side rather than locally?

#1 - couldn't this be addressed by just using fuse/ssh/somesuch to
access the core?  (that is, transparently from GDB).

#2 - if not, could we perhaps tweak the target stratum bits a little to
allow sitting core_ops on top of extended-remote, and have it read the
necessary bits off the remote core with target_fileio_xxx.  So we'd
still have host-side core_ops / bfd.  You'd load the core with e.g.,
"core remote:/.../core.pid").

>>> --- a/gdb/testsuite/gdb.base/solib-mismatch.exp
>>> +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
>>> @@ -1,4 +1,4 @@
>>> -# Copyright 2013 Free Software Foundation, Inc.
>>> +# Copyright 2014 Free Software Foundation, Inc.
>>
>> These need to be 2013-2014.
> 
> Why?  It counts since the first post to gdb-patches?

https://sourceware.org/ml/gdb-patches/2014-02/msg00859.html

-- 
Pedro Alves

Re: [patchv3 0/8] Validate binary before use

[Jan Kratochvil]

On Mon, 03 Mar 2014 14:27:14 +0100, Pedro Alves wrote:
> On 03/02/2014 06:36 PM, Jan Kratochvil wrote:
> > > BTW, do you plan on contributing support for validation with cores too?
> > 
> > No.  
> 
> OK.  I think it'd be useful, so that GDB can warn/reject in the
> common scenario of the user not pointing at a sysroot with the
> correct DSOs.

Yes, may extensions would be useful.  The question is if it should be
a blocker for this patch series.


> Thanks.  I think that feature may be a little beyond borderline
> for that project page though, as core loading is not really part
> of the native target.  IMO it'd good if this had its own project
> page, where it'd give answer to questions like the below.

Created:
	https://sourceware.org/gdb/wiki/GDBserverCoreFiles
And removed it from:
	https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity?action=diff&rev2=57&rev1=56


> #0 - If implemented by gdbserver, it seems like it'd involve further user
> action to trigger the feature?  How does one tell GDB/gdbserver to load the
> ore on the remote side rather than locally?

I did not think more about it as I have never started that project.

The basic functionality would be:
	$ gdbserver -c :1234 corefile
	$ gdb executable
	(gdb) target remote :1234
And if I did not forget anything it could work.

For more comfortable use it overlaps more with my non-upstreamed patches:
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-6.6-buildid-locate.patch
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-6.6-buildid-locate-core-as-arg.patch

So that one could possibly load executable+libraries by:
	$ gdbserver :1234 corefile
	$ gdb
	(gdb) target (extended-?)remote :1234
The same way one can load executable+libraries in linux-nat and the patches
above using:
	$ gdb corefile


> #1 - couldn't this be addressed by just using fuse/ssh/somesuch to
> access the core?  (that is, transparently from GDB).

It could but in many cases would be slower than uploading the whole corefile.

It all depends on upload bandwidth vs. RTT.  Depending on these two link
parameters currently for some people is faster to do core file upload, for
other people to do NFS-like corefile access.

The best choice would be the gdbserver-for-corefile as in such case the major
RTT hit is the link_map traversal while gdbserver can send
qXfer:libraries-svr4:read.

OTOH nowadays there is also NT_FILE but so far GDB does not use it for the
shared library list, it may also contain ELF files mmap()ed and not
dlopen()ed.


> #2 - if not, could we perhaps tweak the target stratum bits a little to
> allow sitting core_ops on top of extended-remote, and have it read the
> necessary bits off the remote core with target_fileio_xxx.  So we'd
> still have host-side core_ops / bfd.  You'd load the core with e.g.,
> "core remote:/.../core.pid").

The same performance problem as #1 above.


> >>> --- a/gdb/testsuite/gdb.base/solib-mismatch.exp
> >>> +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
> >>> @@ -1,4 +1,4 @@
> >>> -# Copyright 2013 Free Software Foundation, Inc.
> >>> +# Copyright 2014 Free Software Foundation, Inc.
> >>
> >> These need to be 2013-2014.
> > 
> > Why?  It counts since the first post to gdb-patches?
> 
> https://sourceware.org/ml/gdb-patches/2014-02/msg00859.html

Do you mean this part?
#  1. We start claiming copyright the year the file as committed
#     to a medium (hard drive), not the year it was published.

Does "medium" mean any disk even unrelated to the GDB project?
Or does "medium" mean "any medium used for GDB development"?

It is not discussed more above, I guess you mean the former but was it really
meant that way?  (Cced Joel)


Thanks,
Jan

Re: [patchv3 0/8] Validate binary before use

[Joel Brobecker]

> > >> These need to be 2013-2014.
> > > 
> > > Why?  It counts since the first post to gdb-patches?
> > 
> > https://sourceware.org/ml/gdb-patches/2014-02/msg00859.html
> 
> Do you mean this part?
> #  1. We start claiming copyright the year the file as committed
> #     to a medium (hard drive), not the year it was published.
> 
> Does "medium" mean any disk even unrelated to the GDB project?
> Or does "medium" mean "any medium used for GDB development"?

Sorry, Jan - I don't know the answer to Jan's question, whether
there is a distinction between the two situations. It doesn't seem
like we need the answer in the context of that patch, but perhaps
we could ask copyright-clerk if we want to satisfy our curiosity?

-- 
Joel

Re: [patchv3 0/8] Validate binary before use

[Jan Kratochvil]

On Mon, 10 Mar 2014 10:57:19 +0100, Joel Brobecker wrote:
> > > >> These need to be 2013-2014.
> > > > 
> > > > Why?  It counts since the first post to gdb-patches?
> > > 
> > > https://sourceware.org/ml/gdb-patches/2014-02/msg00859.html
> > 
> > Do you mean this part?
> > #  1. We start claiming copyright the year the file as committed
> > #     to a medium (hard drive), not the year it was published.
> > 
> > Does "medium" mean any disk even unrelated to the GDB project?
> > Or does "medium" mean "any medium used for GDB development"?
> 
> Sorry, Jan - I don't know the answer to Jan's question, whether
> there is a distinction between the two situations. It doesn't seem
> like we need the answer in the context of that patch, but perhaps
> we could ask copyright-clerk if we want to satisfy our curiosity?

OK, so we do not know whether copyright year is affected by "any disk even
unrelated to the GDB project".

But gdb-patches mailing list was considered as a "medium (hard drive)"
mentioned by the copyright-clerk, IIUC.


Thanks,
Jan

Re: [patchv3 0/8] Validate binary before use

[Pedro Alves]

On 03/08/2014 05:33 PM, Jan Kratochvil wrote:
> On Mon, 03 Mar 2014 14:27:14 +0100, Pedro Alves wrote:
>> On 03/02/2014 06:36 PM, Jan Kratochvil wrote:
>>>> BTW, do you plan on contributing support for validation with cores too?
>>>
>>> No.  
>>
>> OK.  I think it'd be useful, so that GDB can warn/reject in the
>> common scenario of the user not pointing at a sysroot with the
>> correct DSOs.
> 
> Yes, may extensions would be useful.  

> The question is if it should be a blocker for this patch series.

Not at all, IMO.  I was just really asking whether that was
already in the works.  If not, fine, it's still forward progress
without it.

>> Thanks.  I think that feature may be a little beyond borderline
>> for that project page though, as core loading is not really part
>> of the native target.  IMO it'd good if this had its own project
>> page, where it'd give answer to questions like the below.
> 
> Created:
> 	https://sourceware.org/gdb/wiki/GDBserverCoreFiles
> And removed it from:
> 	https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity?action=diff&rev2=57&rev1=56

Thanks.

> 
> 
>> #0 - If implemented by gdbserver, it seems like it'd involve further user
>> action to trigger the feature?  How does one tell GDB/gdbserver to load the
>> ore on the remote side rather than locally?
> 
> I did not think more about it as I have never started that project.
> 
> The basic functionality would be:
> 	$ gdbserver -c :1234 corefile
> 	$ gdb executable
> 	(gdb) target remote :1234
> And if I did not forget anything it could work.

Yeah.  I was more thinking of (persistent) extended-remote, like:

 	$ gdbserver --multi :1234
 	$ gdb
 	(gdb) target extended-remote :1234
        (gdb) ???

Just "core COREFILE" at that point currently makes GDB disconnect
from the remote, and load the core itself (on the host).  Ideally
we'd have some way to tell GDB we actually want to load the core on
the currently connected remote end.  I guess a new knob ('set core
host/target/auto'), plus a new option to the "core" command,
like "core -target" or some such.  Seems doable.

-- 
Pedro Alves