From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37397 invoked by alias); 19 Mar 2015 23:08:48 -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 37384 invoked by uid 89); 19 Mar 2015 23:08:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 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 23:08:46 +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 t2JN8jcd013585 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 19 Mar 2015 19:08:45 -0400 Received: from localhost (unused-10-15-17-126.yyz.redhat.com [10.15.17.126]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2JN8i5W021357 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Thu, 19 Mar 2015 19:08:44 -0400 From: Sergio Durigan Junior To: Pedro Alves Cc: 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> <550AA75E.8050602@redhat.com> X-URL: http://blog.sergiodj.net Date: Thu, 19 Mar 2015 23:08:00 -0000 In-Reply-To: <550AA75E.8050602@redhat.com> (Pedro Alves's message of "Thu, 19 Mar 2015 10:39:26 +0000") Message-ID: <87h9tgtwjn.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00608.txt.bz2 On Thursday, March 19 2015, Pedro Alves wrote: > 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 Indeed, this is not true. I apologize for making this confusion. >> 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. I totally agree. As I did with the patch to add the new enum memory_mapping_state, I am also withdrawing this patch. I will send a new series soon. Thanks, -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/