Re: [PATCH] Skip VDSO when reading SO list

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

Thanks for the comments!

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> On Mon, 19 Aug 2013 16:44:13 +0200, Andreas Arnez wrote:
>> 2013-08-19  Andreas Arnez  <arnez@linux.vnet.ibm.com>
>> 
>> 	* solib-svr4.c (svr4_read_so_list): Skip the VDSO when reading
>                                                      vDSO

OK, will replace all occurrences of 'VDSO' by 'vDSO'.

>> 	link map entries.
>
> Could you include here the testcase from:
> 	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-core-open-vdso-warning.patch

Sure.

>> Index: gdb/gdb/solib-svr4.c
>> ===================================================================
>> --- gdb.orig/gdb/solib-svr4.c
>> +++ gdb/gdb/solib-svr4.c
>> @@ -1310,6 +1310,7 @@ static int
>>  svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
>>  		   struct so_list ***link_ptr_ptr, int ignore_first)
>>  {
>> +  struct so_list *first = NULL;
>>    CORE_ADDR next_lm;
>>  
>>    for (; lm != 0; prev_lm = lm, lm = next_lm)
>> @@ -1349,10 +1350,22 @@ svr4_read_so_list (CORE_ADDR lm, CORE_AD
>>  	{
>>  	  struct svr4_info *info = get_svr4_info ();
>>  
>> +	  first = new;
>>  	  info->main_lm_addr = new->lm_info->lm_addr;
>>  	  do_cleanups (old_chain);
>>  	  continue;
>>  	}
>> +
>> +      /* The l_name of a VDSO sometimes lies in read-only memory that
>                             vDSO
>> +	 is excluded from a core dump.  In order to avoid the "can't
>> +	 read pathname" warning, we try to identify the VDSO.  One
>                                                         vDSO
>> +	 criteria is that the l_name address matches that of the main
>> +	 executable.  */
>> +      if (first && new->lm_info->l_name == first->lm_info->l_name)
>
> Here should be also '&& ignore_first'.

Hm, this shouldn't be necessary, because 'first' is only set when
'ignore_first' is set.  Or did I miss something?

>> +	{
>> +	  do_cleanups (old_chain);
>> +	  continue;
>> +	}
>
> And move this block below so that the condition is evaluated only if
> target_read_string has really failed.
>
> The purpose is that no workarounds should complicate the code in the case the
> system components are already bug-free (after glibc gets fixed).

That's a good point.  Still, after thinking about this some more, I
prefer the order in the original patch, because it prevents a bogus
l_name from being detected in a second scan when the core dump is
debugged on a system with a different glibc version.  Users may also
experience this after a glibc update.  Thoughts?