New patch attached. Details inlined below.


On 13-03-28 02:37 PM, Jan Kratochvil wrote:
> On Fri, 22 Mar 2013 14:12:18 +0100, Aleksandar Ristovski wrote:
>> (depends on the gdbserver changes:
>> http://sourceware.org/ml/gdb-patches/2013-03/msg00838.html)
>>
>> ChangeLog:
>>
>>
>> 	* mips-linux-tdep.c (mips_linux_init_abi): Assign validate value.
>> 	* ppc-linux-tdep.c (ppc_linux_init_abi): Ditto.
>> 	* solib-darwin.c (_initialize_darwin_solib): Ditto.
>> 	* 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-sunos.c (_initialize_sunos_solib): Ditto.
>> 	* solib-svr4.c (lm_addr_check): Add const for 'so' type.
>> 	(svr4_validate_build_id): New function.
>> 	(svr4_validate): New function.
>> 	(library_list_start_library): Parse 'build-id' attribute.
>> 	(svr4_library_pattributes): Add 'build-id' attribute.
>> 	(_initialize_svr4_solib): Assign validate value.
>> 	* solib-svr4.h (DYNAMIC_NAME): New define.
>> 	(NOTE_GNU_BUILD_ID_NAME): New define.
>> 	* solib-target.c (solib.h): Include.
>> 	(_initialize_solib_target): Assign validate value.
>> 	* solib.c (solib_map_sections): Use ops->validate.
>> 	(free_so): Free build_id.
>> 	(solib_validate): New function.
>> 	* solib.h (solib_validate): New declaration.
>> 	* solist.h (so_list): New fields 'build_idsz' and 'build_id'.
>> 	(target_so_ops): New field 'validate'.
>>
>>
>>
>> Test ChangeLog:
>> 	* gdb.base/solib-mismatch-lib.c: New file.
>> 	* gdb.base/solib-mismatch-libmod.c: New file.
>> 	* gdb.base/solib-mismatch.c: New file.
>> 	* gdb.base/solib-mismatch.exp: New file.
>>
>>
>> Thanks,
>>
>> Aleksandar
>>
>
>> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
>> index 2ebe2fe..2e950b6 100644
>> --- a/gdb/mips-linux-tdep.c
>> +++ b/gdb/mips-linux-tdep.c
>> @@ -1495,6 +1495,8 @@ mips_linux_init_abi (struct gdbarch_info info,
>>         mips_svr4_so_ops.in_dynsym_resolve_code
>>   	= mips_linux_in_dynsym_resolve_code;
>>       }
>> +  if (mips_svr4_so_ops.validate == NULL)
>> +    mips_svr4_so_ops.validate = solib_validate;
>
> I do not see why this override should be done.  There is above:
>        mips_svr4_so_ops = svr4_so_ops;
>
> so unless you have a specific reason why the verification cannot work for MIPS
> - and such reason should be put in a comment there - just remove these two
> '+' linse.

[AR] Ok. Removed.


>
>
>>     set_solib_ops (gdbarch, &mips_svr4_so_ops);
>>
>>     set_gdbarch_write_pc (gdbarch, mips_linux_write_pc);
>> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
>> index 0fc6fe0..92dc41a 100644
>> --- a/gdb/ppc-linux-tdep.c
>> +++ b/gdb/ppc-linux-tdep.c
>> @@ -1324,6 +1324,8 @@ ppc_linux_init_abi (struct gdbarch_info info,
>>   	  powerpc_so_ops.in_dynsym_resolve_code =
>>   	    powerpc_linux_in_dynsym_resolve_code;
>>   	}
>> +      if (powerpc_so_ops.validate == NULL)
>> +	powerpc_so_ops.validate = solib_validate;
>
> Likewise.

[AR] Ok.

>
>
>>         set_solib_ops (gdbarch, &powerpc_so_ops);
>>
>>         set_gdbarch_skip_solib_resolver (gdbarch, glibc_skip_solib_resolver);
>> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
>> index b9a4be1..b588f86 100644
>> --- a/gdb/solib-darwin.c
>> +++ b/gdb/solib-darwin.c
>> @@ -647,4 +647,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 = solib_validate;
>>   }
>> diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
>> index ea2acd1..c5f84f3 100644
>> --- a/gdb/solib-dsbt.c
>> +++ b/gdb/solib-dsbt.c
>> @@ -1182,6 +1182,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 = 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 57e418f..e8de6ed 100644
>> --- a/gdb/solib-frv.c
>> +++ b/gdb/solib-frv.c
>> @@ -1182,6 +1182,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 = 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 67085d7..b5d3b6b 100644
>> --- a/gdb/solib-ia64-hpux.c
>> +++ b/gdb/solib-ia64-hpux.c
>> @@ -686,6 +686,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 = solib_validate;
>>
>>     return ops;
>>   }
>> diff --git a/gdb/solib-irix.c b/gdb/solib-irix.c
>> index af3e7d6..642e973 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 = solib_validate;
>>   }
>> diff --git a/gdb/solib-osf.c b/gdb/solib-osf.c
>> index d05c5c1..f7298e3 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 = 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 f646cfb..06749d0 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 = solib_validate;
>>
>>     memset (&dld_cache, 0, sizeof (dld_cache));
>>   }
>> diff --git a/gdb/solib-som.c b/gdb/solib-som.c
>> index ff7fbaa..0d68aac 100644
>> --- a/gdb/solib-som.c
>> +++ b/gdb/solib-som.c
>> @@ -811,6 +811,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 = solib_validate;
>>   }
>>
>>   void
>> diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
>> index 7be5232..94b9a36 100644
>> --- a/gdb/solib-spu.c
>> +++ b/gdb/solib-spu.c
>> @@ -519,6 +519,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 = solib_validate;
>
> In whis case the override of SVR4_SO_OPS.VALIDATE seems as a safe bet, Ulrich
> Weigand could test if it works with SPU but for the initial check-in I find it
> OK as you wrote it.

[AR]. Ok. Change stays, but solib_validate is renamed to 
default_solib_validate.

>
>
>>       }
>>
>>     set_solib_ops (gdbarch, &spu_so_ops);
>> diff --git a/gdb/solib-sunos.c b/gdb/solib-sunos.c
>> index 5863fc2..fec2e9a 100644
>> --- a/gdb/solib-sunos.c
>> +++ b/gdb/solib-sunos.c
>> @@ -738,6 +738,7 @@ _initialize_sunos_solib (void)
>>     sunos_so_ops.open_symbol_file_object = open_symbol_file_object;
>>     sunos_so_ops.in_dynsym_resolve_code = sunos_in_dynsym_resolve_code;
>>     sunos_so_ops.bfd_open = solib_bfd_open;
>> +  sunos_so_ops.validate = solib_validate;
>>
>>     /* FIXME: Don't do this here.  *_gdbarch_init() should set so_ops.  */
>>     current_target_so_ops = &sunos_so_ops;
>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
>> index 0f70097..9dd9cab 100644
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>> @@ -189,7 +189,7 @@ has_lm_dynamic_from_link_map (void)
>>   }
>>
>>   static CORE_ADDR
>> -lm_addr_check (struct so_list *so, bfd *abfd)
>> +lm_addr_check (const struct so_list *so, bfd *abfd)
>
> Such change is OK although it needs to be checked in separately.

[AR] Ok, separated.

>
>
>>   {
>>     if (!so->lm_info->l_addr_p)
>>       {
>> @@ -871,6 +871,87 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>>     return (name_lm >= vaddr && name_lm < vaddr + size);
>>   }
>>
>> +
>> +/* If build-id exists, compare it with target in-memory contents.
>> +   Return 1 if they match, 0 if they don't.
>> +   If there was no build-id, return 1 (could not be verified).  */
>> +
>> +static int
>> +svr4_validate_build_id (const struct so_list *const so)
>
> If you are used to it then OK but for GDB one does not have to make 'const'
> even the variables themselves ('so'), that does not bring much benefits.
> This is everywhere in the patch.

[AR] I consider it good practice much the same as const-qualifying local 
vars.

>
>
>> +{
>> +  asection *asect;
>> +  int res = 1;
>> +  size_t size;
>
> There are multiple sizes around, please call it "asect_size" or somehow
> similar to see this "size" is for the section.

[AR] Ok.

>
>
>> +  const CORE_ADDR l_addr = lm_addr_check (so, so->abfd);
>
> Move this variable to the inner block where it is used, its computation
> possibly would not be used.

[AR] ok.

>
>
>> +
>> +  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;
>> +
>> +  asect = bfd_get_section_by_name (so->abfd, NOTE_GNU_BUILD_ID_NAME);
>> +
>> +  if (!asect || !so->lm_info->l_addr_p)
>
> Coding style for pointer comparison according to doc/gdbinte.texinfo:
> 	if (asect == NULL || !so->lm_info->l_addr_p)
>
> But that l_addr_p check is not needed, drop it, lm_addr_check cannot fail,
> l_addr_p is only internal field for lm_addr_check.

[AR] Ok.

>
>
>> +    {
>> +      if (info_verbose)
>> +	  warning (_("Could not verify '%s' section.\n"),
>> +		   NOTE_GNU_BUILD_ID_NAME);
>> +      return 1;
>> +    }
>> +
>> +  size = bfd_get_section_size (asect);
>> +
>> +  if ((asect->flags & SEC_LOAD) == SEC_LOAD && size != 0)
>> +    {
>> +      gdb_byte *build_id = so->build_id;
>> +      size_t build_idsz = so->build_idsz;
>
> You never set so->build_id anywhere so it will be always NULL.
>
> But in fact I do not see why there exists so->build_id at all.  It is only
> verified once when loading the library and then if it remains loaded one can
> pick the build_id from elf_tdata (so->abfd)->build_id which has been already
> verified as matching.
>
> So just drop so->build_id, it is a duplcate to elf_tdata (so->abfd)->build_id.


[AR] I believe it is clearer by now, we need so->build_id as so->abfd 
may be closed due to mismatch.


>
>
>
>> +      gdb_byte *const data = xmalloc (size);
>> +      struct cleanup *const cleanups = make_cleanup (xfree, data);
>> +      /* Zero based vma, after undoing link script non zero base
>> +	 and/or prelinked rebasing.  */
>
> I do not understand this comment but as the code will change I prefer
> a removal of the comment or a new/different comment.

[AR] ok. Comment removed. It wanted to say that this is load address, as 
if the shared object was link-edited with base 0 (i.e. this is after 
applying displacement).

>
>
>> +      const CORE_ADDR sect_lma = l_addr + bfd_section_vma (so->abfd, asect);
>
> "lma" is irrelevant here, it needs to be VMA as you do runtime
> target_read_memory for that address.

[AR] I read "lma" as load memory address, i.e. address where it is 
actually loaded. This is to differentiate load address from virtual 
address as set by the link-editor.

>
> I won't argue if the expression is right or wrong but it at least looks as
> a duplication of computation done for target_section->addr:
>
>   * Move the 'ops->validate (se)' call in solib_map_sections after the loop
>     for (p = so->sections; p < so->sections_end; p++)
>
>   * Then just use here the VMS address from so->sections but one needs to find
>     it first:
>       struct target_section *p;
>       for (p = solib->sections; p < solib->sections_end; p++)
>         if (p->the_bfd_section == asect)
>           break;
>       use p->addr
>
>
>> +
>> +      /* Relocated or not, contents will be corectly loaded from
>> +	 the file by bfd library.  */
>> +      bfd_get_section_contents ((bfd *) so->abfd, asect, data, 0, size);
>
> Remove the bfd_get_section_contents call completely,
> use elf_tdata (so->abfd)->build_id , there it is already read in and parsed.
>
>
>> +
>> +      if (build_id == NULL)
>
> Here could be a comment - if it was intended so:
>
> /* Even if gdbserver failed to find the build-id read it by
>     target_read_memory.  */
>
>
>> +	{
>> +	  build_idsz = size;
>> +	  build_id = xmalloc (size);
>> +	  make_cleanup (xfree, build_id);
>
> If you start to store the build-id to so->build_id then the xfree is no longer
> appropriate here.

[AR]


>
>
>> +	  if (target_read_memory (sect_lma, build_id, size) != 0)
>> +	    {
>> +	      build_id = NULL;
>> +	      res = -1;
>
> Return value -1 is not documented for this function; but see below.
>
>
>> +	    }
>> +	}
>> +
>> +      if (build_id != NULL)
>> +	res = size == build_idsz
>> +	      && memcmp (build_id, data, size) == 0;
>
> The line does not need wrapping, it is under 80 columns:
> 	res = size == build_idsz && memcmp (build_id, data, size) == 0;
>
>
>> +
>> +      if (res == -1 && info_verbose)
>> +	warning (_("Could not verify section %s\n"), NOTE_GNU_BUILD_ID_NAME);
>> +
>> +      do_cleanups (cleanups);
>> +    }
>> +
>> +  return res != 0;
>
> When the variable is called 'res' then one expects it will be returned as is,
> therefore 'return res;'.
>
>
>> +}
>> +
>> +/* Validate SO by checking whether opened file matches
>> +   in-memory object.  */
>> +
>> +static int
>> +svr4_validate (const struct so_list *const so)
>> +{
>> +  gdb_assert (so != NULL);
>> +  gdb_assert (so->abfd != NULL);
>> +
>> +  return svr4_validate_build_id (so);
>> +}
>
> I do not see why there is this wrapper svr4_validate, you could just move
> those two gdb_assert to svr4_validate_build_id and use directly
> svr4_validate_build_id, it has no other caller anyway.
>
>
>> +
>>   /* Implement the "open_symbol_file_object" target_so_ops method.
>>
>>      If no open symbol file, attempt to locate and open the main symbol
>> @@ -999,7 +1080,11 @@ 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;
>> +  size_t hex_build_id_len;
>
> Sometimes you call it "build_idsz" and sometimes "build_id_len", unify it.
> (I prefer the latter.)
>
>
>>
>>     new_elem = XZALLOC (struct so_list);
>>     new_elem->lm_info = XZALLOC (struct lm_info);
>> @@ -1010,6 +1095,21 @@ 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 && (hex_build_id_len = strlen (hex_build_id)) > 0)
>
> Do not use assignment this way, make it a separate line, see GNU Coding
> Standards:
> 	Try to avoid assignments inside if-conditions
>
>
>> +    {
>> +      new_elem->build_id = xmalloc (hex_build_id_len / 2 + 1);
>
> Why the '+ 1' here?  NEW_ELEM->BUILD_ID is in binary form so there is no '\0'
> terminator.
>
>
>> +      new_elem->build_idsz = hex2bin (hex_build_id, new_elem->build_id,
>> +				      hex_build_id_len);
>> +      if (new_elem->build_idsz != (hex_build_id_len / 2))
>
> Check it the opposite way:
>           if (2 * new_elem->build_idsz != hex_build_id_len)
>
> as otherwise odd length of the "build-id" attribute string will not be caught
> as invalid.
>
>
>> +	{
>> +	  warning (_("Gdbserver returned invalid hex encoded build_id '%s'"
>                        gdbserver                              build-id
>
>> +		  "(%zu/%zu)\n"),
>
> I would omit this (%zu/%zu) part.  Primarily currently %z is not allowed as it
> is not compatible with some OSes, there is a plan to import %z printf support
> in gdb/gnulib/ but so far there wasn't a need for it.
>
> Besides that you should describe what the two numbers mean otherwise they are
> mostly useless.  And after all when you already print the XML string the
> numbers do not add any more info to it.
>
>
>> +		   hex_build_id, hex_build_id_len, new_elem->build_idsz);
>> +	  xfree (new_elem->build_id);
>> +	  new_elem->build_id = NULL;
>> +	  new_elem->build_idsz = 0;
>
> I am not completely sure warning cannot throw, better to do these cleanups
> before the warning call (moreover when %zu/%zu will no longer be printed).
>
>
>> +	}
>> +    }
>>
>>     *list->tailp = new_elem;
>>     list->tailp = &new_elem->next;
>> @@ -1044,6 +1144,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_NONE, NULL, NULL },
>
> There has to be be GDB_XML_EF_OPTIONAL.
>
>
>>     { NULL, GDB_XML_AF_NONE, NULL, NULL }
>>   };
>>
>> @@ -2458,4 +2559,5 @@ _initialize_svr4_solib (void)
>>     svr4_so_ops.lookup_lib_global_symbol = elf_lookup_lib_symbol;
>>     svr4_so_ops.same = svr4_same;
>>     svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
>> +  svr4_so_ops.validate = svr4_validate;
>>   }
>> diff --git a/gdb/solib-svr4.h b/gdb/solib-svr4.h
>> index 66a06e1..ba17dbe 100644
>> --- a/gdb/solib-svr4.h
>> +++ b/gdb/solib-svr4.h
>> @@ -20,6 +20,9 @@
>>   #ifndef SOLIB_SVR4_H
>>   #define SOLIB_SVR4_H
>>
>> +#define DYNAMIC_NAME ".dynamic"
>
> I do not see used it anywhere.
>
>
>> +#define NOTE_GNU_BUILD_ID_NAME  ".note.gnu.build-id"
>
> It is used only in solib-svr4.c so it should be in that file.
>
>
>> +
>>   struct objfile;
>>   struct target_so_ops;
>>
>> diff --git a/gdb/solib-target.c b/gdb/solib-target.c
>> index d897bc0..f43037a 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 "gdb_string.h"
>>
>> @@ -500,6 +501,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 = 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 8129c0f..0d68373 100644
>> --- a/gdb/solib.c
>> +++ b/gdb/solib.c
>> @@ -477,6 +477,17 @@ solib_map_sections (struct so_list *so)
>>   	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
>>       }
>>
>> +  gdb_assert (ops->validate != NULL);
>> +
>> +  if (!ops->validate (so))
>> +    {
>> +      warning (_("Shared object \"%s\" could not be validated and will be ignored."),
>
> Number of columns exceeds 80 characters.
>
>
>
> And move it under 'for (p = so->sections; p < so->sections_end; p++)' with the
> reasons described above.
>
>
>> +	       so->so_name);
>> +      gdb_bfd_unref (so->abfd);
>> +      so->abfd = NULL;
>> +      return 0;
>> +    }
>> +
>>     for (p = so->sections; p < so->sections_end; p++)
>>       {
>>         /* Relocate the section binding addresses as recorded in the shared
>> @@ -551,6 +562,7 @@ free_so (struct so_list *so)
>>   {
>>     struct target_so_ops *ops = solib_ops (target_gdbarch ());
>>
>> +  xfree (so->build_id);
>>     free_so_symbols (so);
>>     ops->free_so (so);
>>
>> @@ -1448,6 +1460,14 @@ gdb_bfd_lookup_symbol (bfd *abfd,
>>     return symaddr;
>>   }
>>
>> +/* Default implementation does not perform any validation.  */
>> +
>> +int
>> +solib_validate (const struct so_list *const so)
>
> Up to you but I would prefer "default_solib_validate", "solib_validate" name
> seems as if it does some validation.
>
>
>> +{
>> +  return 1; /* No validation.  */
>
> Formatting:
>    /* No validation.  */
>    return 1;
>
>
>> +}
>> +
>>   extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
>>
>>   void
>> diff --git a/gdb/solib.h b/gdb/solib.h
>> index b811866..ae42e9d 100644
>> --- a/gdb/solib.h
>> +++ b/gdb/solib.h
>> @@ -90,4 +90,8 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
>>   								      void *),
>>   						    void *data);
>>
>> +/* Default validation always returns 1.  */
>> +
>> +extern int solib_validate (const struct so_list *so);
>> +
>>   #endif /* SOLIB_H */
>> diff --git a/gdb/solist.h b/gdb/solist.h
>> index f784fc3..72e003d 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
>            build-id
>
>> +       .note.gnu.build-id including note header.
>
> It really should not contain the note header, it should be exactly 20 bytes
> with the standard build-id in place.
>
> I wanted to check it with gdbserver but gdbserver does not work for me due to
> those errors like:
>
> gdbserver: Error parsing {s,}maps file '/usr/lib64/libdl-2.17.so'^M
>
>
>> 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
>> @@ -148,6 +158,10 @@ struct target_so_ops
>>          core file (in particular, for readonly sections).  */
>>       int (*keep_data_in_core) (CORE_ADDR vaddr,
>>   			      unsigned long size);
>> +
>> +    /* 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.7.10.4
>>
>
>> diff --git a/gdb/testsuite/gdb.base/solib-mismatch-lib.c b/gdb/testsuite/gdb.base/solib-mismatch-lib.c
>> new file mode 100644
>> index 0000000..19f1545
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/solib-mismatch-lib.c
>> @@ -0,0 +1,29 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2013 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
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +
>> +int _bar = 42;
>> +
>> +int bar(void)
>> +{
>> +  return _bar + 21;
>> +}
>> +
>> +int foo(void)
>> +{
>> +  return _bar;
>> +}
>> diff --git a/gdb/testsuite/gdb.base/solib-mismatch-libmod.c b/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
>> new file mode 100644
>> index 0000000..3b025a8
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/solib-mismatch-libmod.c
>> @@ -0,0 +1,29 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2013 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
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +
>> +int _bar = 21;
>> +
>> +int bar(void)
>> +{
>> +  return 42 - _bar;
>> +}
>> +
>> +int foo(void)
>> +{
>> +  return 24 + bar();
>> +}
>> diff --git a/gdb/testsuite/gdb.base/solib-mismatch.c b/gdb/testsuite/gdb.base/solib-mismatch.c
>> new file mode 100644
>> index 0000000..85a2784
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/solib-mismatch.c
>> @@ -0,0 +1,59 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2013 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
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +
>> +#include <dlfcn.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <signal.h>
>> +
>> +#define lib "./solib-mismatch.so"
>
> Macros should use uppercased names.

[AR] ok.

>
>
>> +
>> +
>> +/* First argument is working directory. */
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +  void *h;
>> +  int (*foo)(void);
>> +  char buff[1024];
>> +
>> +  if (argc < 2)
>> +    {
>> +      printf ("ERROR - CWD not provided\n");
>> +      return 1;
>> +    }
>> +
>> +  if (chdir (argv[1]) != 0)
>> +    {
>> +      printf ("ERROR - Could not cd to '%s'\n", argv[1]);
>> +      return 1;
>> +    }
>> +
>> +  h = dlopen(lib, RTLD_NOW);
>> +
>> +  if (h == NULL)
>> +    {
>> +      printf ("ERROR - could not open lib %s\n", lib);
>> +      return 1;
>> +    }
>> +  foo = dlsym(h, "foo");
>> +  printf ("foo: %p\n", foo); /* set breakpoint 1 here */
>> +  dlclose(h);
>> +  return 0;
>> +}
>> +
>> diff --git a/gdb/testsuite/gdb.base/solib-mismatch.exp b/gdb/testsuite/gdb.base/solib-mismatch.exp
>> new file mode 100644
>> index 0000000..c2192a5
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp
>> @@ -0,0 +1,186 @@
>> +# Copyright 2013 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
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +# are we on a target board
>
> I do not see why this comment is here.

[AR] copy-paste; removed

>
>
>> +standard_testfile
>> +set executable $testfile
>> +
>> +# 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
>> +#  detect the mismatch and refuse to use mismatched shared object.
>> +
>> +if { [get_compiler_info] } {
>> +  untested ${testfile}.exp
>
> Use specific text, like:
>       untested "get_compiler_info failed."

[AR] ok.

>
>> +}
>> +
>> +# First version of the object, to be loaded by ld
>> +set srclibfilerun ${testfile}-lib.c
>
> Please make empty lines before comments.
>

[AR] ok.

>
>> +# Modified version of the object to be loaded by gdb
>> +# Code in -libmod.c is tuned so it gives a mismatch but
>> +# leaves .dynamic at the same point.
>> +set srclibfilegdb ${testfile}-libmod.c
>> +
>> +# So file name:
>> +set binlibfilebase ${testfile}.so
>> +
>> +# Setup run directory (where program is run from)
>> +#   It contains executable and '-lib' version of the library.
>> +set binlibfiledirrun ${objdir}/${subdir}/${testfile}_wd
>> +set binlibfilerun ${binlibfiledirrun}/${binlibfilebase}
>
> Use [standard_output_file ...], not ${objdir}.
>

[AR] ok.

>
>> +
>> +# Second solib version is in current directory, '-libmod' version.
>> +set binlibfiledirgdb ${objdir}/${subdir}
>
> Likewise.
>
>
>> +set binlibfilegdb ${binlibfiledirgdb}/${binlibfilebase}
>> +
>> +# Executeable
>> +set srcfile ${testfile}.c
>> +set executable ${testfile}
>> +set objfile ${objdir}/${subdir}/${executable}.o
> Likewise.
>> +set binfile ${objdir}/${subdir}/${executable}
> Likewise.
>> +
>> +send_user "Current WD: [eval pwd]\r\n"
>
> Testfiles must not print messages during normal run.  Use "verbose -log" if needed.
>
> 3 times more below.

[AR] Ok, informative message incorporated in the PASS/FAIL/UNTESTED entry.

>
>
>> +
>> +file mkdir "${binlibfiledirrun}"
>> +
> Initialize it by:
> 	set exec_opts {}
> as IIRC regex testcases have persistent variables during one runtest run.
>
>> +if { ![istarget "*-*-nto-*"] } {
>> +  set exec_opts [list debug shlib_load]
>> +}
>> +
>> +if { [prepare_for_testing $testfile.exp $executable $srcfile $exec_opts] != 0 } {
>> +  untested ${testfile}.exp
>
> untested is not needed, prepare_for_testing complains on its own.

[AR] ok.

>
>
>> +  return -1
>> +}
>> +
>> +if { [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilerun}" "${binlibfilerun}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != ""
>> +     || [gdb_gnu_strip_debug "${binlibfilerun}"]
>> +     || [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilegdb}" "${binlibfilegdb}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != "" } {
>> +  untested ${testfile}.exp
>
> Use specific message.
>

[AR] ok.

>
>> +  return -1
>> +}
>> +
>> +proc solib_matching_test { solibfile symsloaded } {
>> +  global gdb_prompt
>> +  global testfile
>> +  global executable
>> +  global srcdir
>> +  global subdir
>> +  global binlibfiledirrun
>> +  global binlibfiledirgdb
>> +  global srcfile
>> +
>> +  gdb_exit
>> +  gdb_start
>> +  gdb_reinitialize_dir $srcdir/$subdir
>
> use clean_restart
>
>
>> +
>> +  send_gdb "set verbose 1\n"
>
> Never (only in some exceptional cases) use send_gdb, it creates races wrt
> syncing on end of the commands.  Use gdb_test or gdb_test_no_output.

[AR] I very much dislike using gdb_test unless I actually am doing a 
test. Otherwise, we end up with testcases that tend to have 30-40 passes 
but only 2-3 relevant. Thus, when these 2-3 relevant ones start to FAIL 
it is easy to neglect that due to false cozy feeling that, well, *most* 
are still passing.

>
>
>> +  send_gdb "file \"${binlibfiledirrun}/${executable}\"\n"
>
> Do not use "file" by hand, clean_restart does it.

[AR] ok.

>
>
>> +  send_gdb "set solib-search-path \"${binlibfiledirgdb}\"\n"
>> +  send_gdb "cd ${binlibfiledirgdb}\n"
>> +  send_gdb "show solib-search-path\n"
>
>> +  send_gdb "set auto-solib-add off\n"
>
> Why is this command here?  Could you put a comment there?

[AR] so that we can have control over when is the output printed and 
match it correctly. I added the comment.

>
>
>> +  send_gdb "set args \"${binlibfiledirrun}\"\n"
>> +  send_gdb "set verbose 1\n"
>> +
>> +  set bp_location [gdb_get_line_number "set breakpoint 1 here"]
>> +
>> +  send_gdb "tbreak ${srcfile}:${bp_location}\n"
>
> Do not use send_gdb and there is gdb_breakpoint function.

[AR] I am not testing setting  breakpoints. I do not want these to show 
up as PASS-es. These passes are irrelevant. The assumption is that 
breakpoints do work; there are other tests for breakponts.

>
>
>> +  gdb_expect {
>> +    -re ".*Temporary breakpoint.*${gdb_prompt} $" {
>> +    }
>> +    default {
>> +      untested "${testfile}: Failed to set temp. breakpoint at ${bp_location}"
>> +      return -1
>> +    }
>> +  }
>> +
>> +  send_gdb "run\r\n"
>
> Use runto_main.  And check its result code.

[AR] The same. I am not testing run to main. I am testing this 
particular feature. There are other tests that test runto_main.

>
> Currently gdbserver (in non-extended mode) is never tested as "run" will never
> start gdbserver.
>
> After I fixed it I get from gdbserver messages like:
>
> gdbserver: Error parsing {s,}maps file '/home/jkratoch/redhat/gdb-qnx/gdb/testsuite/gdb.base/solib-mismatch'^M
>
>
>
>> +  gdb_expect {
>> +    -re "Starting program.*${gdb_prompt} $" {
>> +    }
>> +    default {
>> +      untested "${testfile}: Failed to hit breakpoint at ${bp_location}"
>> +      return -1
>> +    }
>> +  }
>> +
>> +  send_gdb "sharedlibrary\n"
>
> Use gdb_test.

[AR] the same. Irrelevant PASS-es. I do not want them.

>
>> +  gdb_expect {
>> +    -re ".*${gdb_prompt} $" {
>> +    }
>> +    default {
>> +      untested "${testfile}: sharedlibrary failure"
>> +      return -1
>> +    }
>> +  }
>> +
>> +  gdb_test "info sharedlibrary ${solibfile}" \
>> +    ".*From.*To.*Syms.*Read.*Shared.*\r\n.*${symsloaded}.*" \
>          ^^
> BTW leading .* is excessive, gdb_test regex does not have anchored its start.

[AR] ok.

>
>
>> +    "Symbols for ${solibfile} loaded: expected '${symsloaded}'"
>
> Protect ${symsloaded} by [string_to_regexp $string] as user
> may have regex-unsafe characters there.

[AR] symsloaded is argument passed to solib_matching_test, and the test 
is the only user. Ther eare no other users, and the string may contain 
only 'Yes' or 'No'.

>
>
>> +
>> +  send_gdb "p/x foo\n"
>
> Use gdb_test.
>
>
>> +  gdb_expect {
>> +    -re ".*${gdb_prompt} $" {
>> +#Nothing, just drain the buffer
>> +    }
>> +    default {
>> +      untested "${testfile}: Failed 'info symbol foo'"
>> +      return -1
>> +    }
>> +  }
>> +
>> +  send_gdb "detach\n"
>
> I do not see a need for this command, remove it.
>
>
>> +  gdb_expect {
>> +    -re ".*Detaching from program.*${executable}.*${gdb_prompt} $" {
>> +#Nothing, just drain the buffer
>> +    }
>> +    default {
>> +      untested "${testfile}: Could not detach from ${testpid}"
>> +      return -1
>> +    }
>> +  }
>> +  return 0
>> +}
>> +
>> +# Copy binary to working dir so it pulls in the library from that dir
>> +# (by the virtue of $ORIGIN).
>> +file copy -force "${binlibfiledirgdb}/${executable}" \
>> +		 "${binlibfiledirrun}/${executable}"
>> +
>> +# Test unstripped, .dynamic matching
>> +send_user "test unstripped, .dynamic matching\r\n"
>> +solib_matching_test "${binlibfilebase}" "No"
>> +
>> +# Test --only-keep-debug, .dynamic matching so
>> +send_user "test --only-keep-debug\r\n"
>> +# Keep original so for debugging purposes
>> +file copy -force "${binlibfilegdb}" "${binlibfilegdb}-orig"
>> +set objcopy_program [transform objcopy]
>> +set result [catch "exec $objcopy_program --only-keep-debug ${binlibfilegdb}"]
>> +if {$result != 0} {
>> +  untested "${testfile} test --only-keep-debug"
>> +  return -1
>> +}
>> +solib_matching_test "${binlibfilebase}" "No"
>> +
>> +# Now test it does not mis-invalidate matching libraries
>> +send_user "test matching libraries\r\n"
>> +# Keep previous so for debugging puroses
>> +file copy -force "${binlibfilegdb}" "${binlibfilegdb}-orig1"
>> +# Copy loaded so over the one gdb will find
>> +file copy -force "${binlibfilerun}" "${binlibfilegdb}"
>> +solib_matching_test "${binlibfilebase}" "Yes"
>
>
> The testcase is not safe for gdbserver targets not sharing the filesystem with
> the runtest machine.  There are functions like gdb_download for it although
> AFAIK many GDB testcases do not work well with such targets.  I do not have
> experience with it and/or such a gdbserver setup.
>
>
> Thanks,
> Jan
>


Thanks,


Aleksandar