Re: [PATCH] refreshed patch for PR 11804 and PR 9904

[Jan Kratochvil <jan.kratochvil@redhat.com>]

Hello,

> which I've refreshed for gdb 7.4.1. So here it is,

this is unfortunately not sufficient, 7.4.1 is too old.  The patch is not
applicable to FSF GDB HEAD - to be accepted.  See how to download HEAD:
	http://sources.redhat.com/gdb/current/

Primarily linux_nat_find_memory_regions is now reworked and renamed as
linux_find_memory_regions as GDB can now gcore also from remotely run process
via gdbserver.  The testcase should be verified it also works with gdbserver
afterwards, see gdb/testsuite/boards/native-gdbserver.exp.

Also there should be descriptive comments before each modified function.
I can write those otherwise.


On Wed, 18 Jul 2012 02:50:08 +0200, Jean-Marc Saffroy wrote:

static int
objfile_find_memory_regions (find_memory_region_ftype func, void *obfd)
  /* Call callback function for each objfile section.  */
  ALL_OBJSECTIONS (objfile, objsec)

Here is missing a check:
    if (separate_debug_objfile_backlink != NULL)
      continue;
as separate debug info files (in /usr/lib/debug, sometimes with .debug
extension) are irrelevant for gcore.


> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
[...]
> @@ -4370,6 +4370,40 @@ read_mapping (FILE *mapfile,
[...]
> +	  if (fscanf (mapfile, "%64s%lu kB\n", keyword, &number) != 2)
> +	    {
> +	      warning (_("Error parsing /proc/PID/{s,}maps file"));
> +	      do
> +		ch = fgetc (mapfile);
> +	      while (ch != EOF && ch != '\n');
> +	      break;
> +	    }
> +	  if (number != 0 && (strcmp (keyword, "Shared_Dirty:") == 0
> +			      || strcmp (keyword, "Private_Dirty:") == 0
> +			      || strcmp (keyword, "Swap:") == 0))

The patch is primarily missing the check for "Anonymous:" entry in recent
Linux kernels, described by Ulrich Weigand in:
	http://sourceware.org/ml/gdb-patches/2010-09/msg00398.html

Therefore if "Anonymous:" entry is not present at all (=not supported by Linux
kernel on that host) we must write out very every area as we have no way to
verify for swap-cached pages (having Anonymous > 0).

Sure if "Anonymous:" is present and it is > 0 it also must be written out.

You can reproduce the "Anonymous:" problem with
	http://people.redhat.com/jkratoch/relroswap.c
although it is not suitable for GDB testsuite as it needs to do huge memory
allocations to swap out (+swap in) the testcase pages.
(If you have a better idea how to make "Anonymous:" entry non-zero it is sure
welcome.  I have tried madvise (MADV_DONTNEED) but it does not work.)


> +	    *modified = 1;
> +	  else if (*modified == -1)
> +	    {
> +	      /* Valid line proves an smaps file is being read in.  */
> +	      *modified = 0;
> +	    }
> +	}
> +    }
> +
>    return (ret != 0 && ret != EOF);
>  }
>  
[...]
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-relro.exp
> @@ -0,0 +1,80 @@
> +# Copyright 2010 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/>.
> +
> +if {[skip_shlib_tests]} {
> +    return 0
> +}
> +
> +set testfile "gcore-relro"

Is hould use 'standard_testfile' like other testcases recently modified by Tom
Tromey.


> +set srcmainfile ${testfile}-main.c
> +set srclibfile ${testfile}-lib.c
> +set libfile ${objdir}/${subdir}/${testfile}-lib.so
> +set objfile ${objdir}/${subdir}/${testfile}-main.o
> +set executable ${testfile}-main
> +set binfile ${objdir}/${subdir}/${executable}
> +set gcorefile ${objdir}/${subdir}/${executable}.gcore
> +
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${srclibfile} ${libfile} {debug}] != ""
> +     || [gdb_compile ${srcdir}/${subdir}/${srcmainfile} ${objfile} object {debug}] != "" } {
> +     untested ${testfile}.exp
> +     return -1
> +}
> +set opts [list debug shlib=${libfile} additional_flags=-Wl,-z,relro]
> +if { [gdb_compile ${objfile} ${binfile} executable $opts] != "" } {
> +     unsupported "-Wl,-z,relro compilation failed"
> +     return -1
> +}
> +
> +clean_restart $executable
> +gdb_load_shlibs $libfile
[...]


Thanks,
Jan