From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18483 invoked by alias); 19 Mar 2015 10:39:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18434 invoked by uid 89); 19 Mar 2015 10:39:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 19 Mar 2015 10:39:29 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2JAdRwE005254 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 19 Mar 2015 06:39:28 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2JAdQl9023844; Thu, 19 Mar 2015 06:39:27 -0400 Message-ID: <550AA75E.8050602@redhat.com> Date: Thu, 19 Mar 2015 10:39:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Sergio Durigan Junior , GDB Patches Subject: Re: [PATCH 2/4] Update gcore_create_callback to better handle different states of mappings References: <1426707523-6499-1-git-send-email-sergiodj@redhat.com> <1426707523-6499-3-git-send-email-sergiodj@redhat.com> In-Reply-To: <1426707523-6499-3-git-send-email-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00578.txt.bz2 On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote: > This patch updates gdb/gcore.c's gcore_create_callback function and > changes its logic to improve the handling of different states of > memory mappings. > > One of the major modifications is that mappings marked as UNMODIFIED > are now completely ignored by the function (i.e., not even the segment > headers are created anymore). This is now possible because of the > introduction of the new UNKNOWN state (see below). We now know that > when the mapping is UNMODIFIED, it means that the user has either > chose to ignore it via /proc/PID/coredump_filter, or that it is marked > as VM_DONTDUMP (and therefore should not be dumped anyway). This is > what the Linux kernel does, too. > > The new UNKNOWN state is being used to identify situations when we > don't really know whether the mapping should be dumped or not. Based > on that, we run an existing heuristic responsible for deciding if we > should include the mapping's contents or not. > > One last thing worth mentioning is that this check: > > if (read == 0 && write == 0 && exec == 0 > && modified_state == MEMORY_MAPPING_UNMODIFIED) > > has been simplified to: > > if (read == 0) > > This is because if the mapping has not 'read' permission set, it does > not make sense to include its contents in the corefile (GDB would > actually not be allowed to do that). I don't think this is true. GDB can certainly read memory of mappings that don't have the read permission bit set: ptrace bypasses the memory protections. Otherwise, how could gdb poke breakpoints instructions? AFAICS, it can even read memory mapped with no permissions at all: (top-gdb) info inferiors Num Description Executable * 1 process 24337 /home/pedro/gdb/mygit/build/gdb/gdb (top-gdb) shell cat /proc/24337/maps ... 3615fb4000-36161b3000 ---p 001b4000 fd:00 263789 /usr/lib64/libc-2.18.so ... (top-gdb) x/4b 0x3615fb4000 0x3615fb4000: 0xf6 0x9a 0xf7 0x15 > Therefore, we just create a > segment header for it. The Linux kernel differs from GDB here because > it actually include the contents of this mapping in the corefile, but > this is because it can read its contents. > > Changes from v2: > > - Return immediately if modified_state == MEMORY_MAPPING_UNMODIFIED > > - Improve comments explaining the case above, and also the case when > "read == 0". > > gdb/ChangeLog: > 2015-03-18 Sergio Durigan Junior > Jan Kratochvil > Oleg Nesterov > > PR corefiles/16092 > * gcore.c (gcore_create_callback): Update code to handle the case > when 'modified_state == MEMORY_MAPPING_UNMODIFIED'. Simplify > condition used to decide when to create only a segment header for > the mapping. Improve check to decide when to run a heuristic to > decide whether to dump the mapping's contents. > --- > gdb/gcore.c | 39 +++++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 14 deletions(-) > > diff --git a/gdb/gcore.c b/gdb/gcore.c > index 751ddac..8dfcc02 100644 > --- a/gdb/gcore.c > +++ b/gdb/gcore.c > @@ -422,23 +422,34 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read, > asection *osec; > flagword flags = SEC_ALLOC | SEC_HAS_CONTENTS | SEC_LOAD; > > - /* If the memory segment has no permissions set, ignore it, otherwise > - when we later try to access it for read/write, we'll get an error > - or jam the kernel. */ > - if (read == 0 && write == 0 && exec == 0 > - && modified_state == MEMORY_MAPPING_UNMODIFIED) > + if (modified_state == MEMORY_MAPPING_UNMODIFIED) > { > - if (info_verbose) > - { > - fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n", > - plongest (size), paddress (target_gdbarch (), vaddr)); > - } > - > + /* When the memory mapping is marked as unmodified, this means > + that it should not be included in the coredump file (either > + because it was marked as VM_DONTDUMP, or because the user > + explicitly chose to ignore it using the > + /proc/PID/coredump_filter mechanism). > + > + We could in theory create a section header for it (i.e., mark > + it as '~(SEC_LOAD | SEC_HAS_CONTENTS)', just like we do when > + the mapping does not have the 'read' permission set), but the > + Linux kernel itself ignores these mappings, and so do we. */ > return 0; It really seems wrong to me to bake in Linuxisms into this generic function, shared with all supported configurations. As mentioned elsewhere, I think we should instead leave it up to the (Linux-specific) caller to simply not call this function if the mapping shouldn't be dumped at all. > } > - > - if (write == 0 && modified_state == MEMORY_MAPPING_UNMODIFIED > - && !solib_keep_data_in_core (vaddr, size)) > + else if (read == 0) > + { > + /* If the memory segment has no read permission set, then we have to > + generate a segment header for it, but without contents (i.e., > + FileSiz = 0), otherwise when we later try to access it for > + read/write, we'll get an error or jam the kernel. > + > + The Linux kernel differs from GDB in this point because it > + can actually read this mapping, so it dumps this mapping's > + contents in the corefile. */ > + flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS); > + } > + else if (write == 0 && modified_state == MEMORY_MAPPING_UNKNOWN_STATE > + && !solib_keep_data_in_core (vaddr, size)) > { > /* See if this region of memory lies inside a known file on disk. > If so, we can avoid copying its contents by clearing SEC_LOAD. */ > Thanks, Pedro Alves