From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99860 invoked by alias); 25 Apr 2019 17:21:07 -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 99849 invoked by uid 89); 25 Apr 2019 17:21:06 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=H*M:lan 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 ESMTP; Thu, 25 Apr 2019 17:21:04 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 85BF6C7E71 for ; Thu, 25 Apr 2019 17:21:03 +0000 (UTC) Received: from f29-4.lan (ovpn-116-111.phx2.redhat.com [10.3.116.111]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5BF1C5D9D1; Thu, 25 Apr 2019 17:21:00 +0000 (UTC) Date: Thu, 25 Apr 2019 17:21:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Sergio Durigan Junior Subject: Re: [PATCH+8.3?] Implement dump of mappings with ELF headers by gcore Message-ID: <20190425102100.08da12eb@f29-4.lan> In-Reply-To: <20190423234635.26916-1-sergiodj@redhat.com> References: <20190423234635.26916-1-sergiodj@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-04/txt/msg00540.txt.bz2 On Tue, 23 Apr 2019 19:46:35 -0400 Sergio Durigan Junior wrote: > This patch has a long story, but it all started back in 2015, with [...] Thanks for the detailed commit comment! [...] > I think it makes sense to include this patch into 8.3, since it's > pretty "self-contained", but I will leave that decision to the GMs. > > gdb/ChangeLog: > 2019-04-24 Sergio Durigan Junior > > PR corefiles/11608 > PR corefiles/18187 > * linux-tdep.c (dump_mapping_p): Add new parameters "ADDR" and > "OFFSET". Verify if current mapping contains an ELF header. > (linux_find_memory_regions_full): Adjust call to > "dump_mapping_p". I don't think that the double quotes enclosing ADDR and OFFSET are needed here. > gdb/testsuite/ChangeLog: > 2019-04-24 Sergio Durigan Junior > > PR corefiles/11608 > PR corefiles/18187 > * gdb.base/coredump-filter-build-id.exp: New file. > * lib/future.exp (gdb_find_eu-unstrip): New procedure. > --- > gdb/linux-tdep.c | 71 +++++++++++++++---- > .../gdb.base/coredump-filter-build-id.exp | 69 ++++++++++++++++++ > gdb/testsuite/lib/future.exp | 10 +++ > 3 files changed, 137 insertions(+), 13 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/coredump-filter-build-id.exp > > diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c > index 5de985def3..d71a00666f 100644 > --- a/gdb/linux-tdep.c > +++ b/gdb/linux-tdep.c > @@ -586,8 +586,8 @@ mapping_is_anonymous_p (const char *filename) > } > > /* Return 0 if the memory mapping (which is related to FILTERFLAGS, V, > - MAYBE_PRIVATE_P, and MAPPING_ANONYMOUS_P) should not be dumped, or > - greater than 0 if it should. > + MAYBE_PRIVATE_P, MAPPING_ANONYMOUS_P, ADDR and OFFSET) should not > + be dumped, or greater than 0 if it should. > > In a nutshell, this is the logic that we follow in order to decide > if a mapping should be dumped or not. > @@ -625,12 +625,17 @@ mapping_is_anonymous_p (const char *filename) > see 'p' in the permission flags, then we assume that the mapping > is private, even though the presence of the 's' flag there would > mean VM_MAYSHARE, which means the mapping could still be private. > - This should work OK enough, however. */ > + This should work OK enough, however. > + > + - Even if, at the end, we decided that we should not dump the > + mapping, we still have to check if it is something like an ELF > + header (of a DSO or an executable, for example). If it is, and > + if the user is interested in dump it, then we should dump it. */ > > static int > dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v, > int maybe_private_p, int mapping_anon_p, int mapping_file_p, > - const char *filename) > + const char *filename, ULONGEST addr, ULONGEST offset) I was surprised to see that addr is ULONGEST instead of CORE_ADDR, but I see that this matches the type of of the variables passed by the caller. Both are typedef'd to unsigned long long, so it probably doesn't matter. > { > /* Initially, we trust in what we received from our caller. This > value may not be very precise (i.e., it was probably gathered > @@ -640,6 +645,7 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v, > (assuming that the version of the Linux kernel being used > supports it, of course). */ > int private_p = maybe_private_p; > + int dump_p; > > /* We always dump vDSO and vsyscall mappings, because it's likely that > there'll be no file to read the contents from at core load time. > @@ -680,13 +686,13 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v, > /* This is a special situation. It can happen when we see a > mapping that is file-backed, but that contains anonymous > pages. */ > - return ((filterflags & COREFILTER_ANON_PRIVATE) != 0 > - || (filterflags & COREFILTER_MAPPED_PRIVATE) != 0); > + dump_p = ((filterflags & COREFILTER_ANON_PRIVATE) != 0 > + || (filterflags & COREFILTER_MAPPED_PRIVATE) != 0); > } > else if (mapping_anon_p) > - return (filterflags & COREFILTER_ANON_PRIVATE) != 0; > + dump_p = (filterflags & COREFILTER_ANON_PRIVATE) != 0; > else > - return (filterflags & COREFILTER_MAPPED_PRIVATE) != 0; > + dump_p = (filterflags & COREFILTER_MAPPED_PRIVATE) != 0; > } > else > { > @@ -695,14 +701,53 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v, > /* This is a special situation. It can happen when we see a > mapping that is file-backed, but that contains anonymous > pages. */ > - return ((filterflags & COREFILTER_ANON_SHARED) != 0 > - || (filterflags & COREFILTER_MAPPED_SHARED) != 0); > + dump_p = ((filterflags & COREFILTER_ANON_SHARED) != 0 > + || (filterflags & COREFILTER_MAPPED_SHARED) != 0); > } > else if (mapping_anon_p) > - return (filterflags & COREFILTER_ANON_SHARED) != 0; > + dump_p = (filterflags & COREFILTER_ANON_SHARED) != 0; > else > - return (filterflags & COREFILTER_MAPPED_SHARED) != 0; > + dump_p = (filterflags & COREFILTER_MAPPED_SHARED) != 0; > } > + > + /* Even if we decided that we shouldn't dump this mapping, we still > + have to check whether (a) the user wants us to dump mappings > + containing an ELF header, and (b) the mapping in question > + contains an ELF header. If (a) and (b) are true, then we should > + dump this mapping. > + > + A mapping contains an ELF header if it is a private mapping, its > + offset is zero, and its first word is ELFMAG. */ > + if (!dump_p && private_p && offset == 0 > + && (filterflags & COREFILTER_ELF_HEADERS) != 0) > + { > + /* Let's check if we have an ELF header. */ > + gdb::unique_xmalloc_ptr header; > + int errcode; > + > + /* Useful define specifying the size of the ELF magical > + header. */ > +#ifndef SELFMAG > +#define SELFMAG 4 > +#endif > + > + /* Read the first SELFMAG bytes and check if it is ELFMAG. */ > + if (target_read_string (addr, &header, SELFMAG, &errcode) == SELFMAG > + && errcode == 0) > + { > + const char *h = header.get (); > + > + if (h[EI_MAG0] == ELFMAG0 && h[EI_MAG1] == ELFMAG1 > + && h[EI_MAG2] == ELFMAG2 && h[EI_MAG3] == ELFMAG3) Somewhere in here, either above the "if" or in the comment for SELFMAG, I think it'd be useful to note that the ELFMAG0..ELFMAG3 and EI_MAG0..EI_MAG3 come from elf/common.h. (My concern here was whether ELFMAG0, etc. were defined by a system header. If so, we can't use those defines here. But since they're defined in elf/common.h, they're perfectly fine. This is why I think it'd be useful to note where we expect them to come from. Otherwise, such a comment would just be clutter.) Regarding SELFMAG, it's a shame that it's not it's not also defined in elf/common.h. I see that SELFMAG is also used in gdbserver/linux-ppc-low.c. I'm guessing that it obtains this value from either /usr/include/elf.h or /usr/include/linux/elf.h. (I don't want to hold up your patch to make that header file change though; there might be a good reason to not touch elf/common.h.) > + { > + /* This mapping contains an ELF header, so we > + should dump it. */ > + dump_p = 1; > + } > + } > + } > + > + return dump_p; > } > > /* Implement the "info proc" command. */ > @@ -1306,7 +1351,7 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch, > if (has_anonymous) > should_dump_p = dump_mapping_p (filterflags, &v, priv, > mapping_anon_p, mapping_file_p, > - filename); > + filename, addr, offset); > else > { > /* Older Linux kernels did not support the "Anonymous:" counter. > diff --git a/gdb/testsuite/gdb.base/coredump-filter-build-id.exp b/gdb/testsuite/gdb.base/coredump-filter-build-id.exp > new file mode 100644 > index 0000000000..fc2b039406 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/coredump-filter-build-id.exp > @@ -0,0 +1,69 @@ > +# Copyright 2019 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 . > + > +# Test whether GDB's gcore/generate-core-file command can dump memory > +# mappings with ELF headers, containing a build-id note. > +# > +# Due to the fact that we don't have an easy way to process a corefile > +# and look for specific notes using GDB/dejagnu, we rely on an > +# external tool, eu-unstrip, to verify if the corefile contains > +# build-ids. > + > +standard_testfile "normal.c" > + > +# This test is Linux x86_64 only. > +if { ![istarget *-*-linux*] } { > + untested "$testfile.exp" > + return -1 > +} > +if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } { > + untested "$testfile.exp" > + return -1 > +} > + > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } { > + return -1 > +} > + > +if { ![runto_main] } { > + untested "could not run to main" > + return -1 > +} > + > +# First we need to generate a corefile. > +set corefilename "[standard_output_file gcore.test]" > +if { ![gdb_gcore_cmd "$corefilename" "save corefile"] } { > + verbose -log "Could not save corefile" > + untested "$testfile.exp" > + return -1 > +} > + > +# Determine if GDB dumped the mapping containing the build-id. This > +# is done by invoking an external program (eu-unstrip). > +if { [catch "exec [gdb_find_eu-unstrip] -n --core $corefilename" output] == 0 } { > + set line [lindex [split $output "\n"] 0] > + set test "gcore dumped mapping with build-id" > + > + verbose -log "First line of eu-unstrip: $line" > + > + if { [regexp "^${hex}\\+${hex} \[a-f0-9\]+@${hex}.*[string_to_regexp $binfile]$" $line] } { > + pass "$test" > + } else { > + fail "$test" > + } > +} else { > + verbose -log "Could not execute eu-unstrip program" > + untested "$testfile.exp" > +} > diff --git a/gdb/testsuite/lib/future.exp b/gdb/testsuite/lib/future.exp > index a56cd019b4..122e652858 100644 > --- a/gdb/testsuite/lib/future.exp > +++ b/gdb/testsuite/lib/future.exp > @@ -162,6 +162,16 @@ proc gdb_find_readelf {} { > return $readelf > } > > +proc gdb_find_eu-unstrip {} { > + global EU_UNSTRIP_FOR_TARGET > + if [info exists EU_UNSTRIP_FOR_TARGET] { > + set eu_unstrip $EU_UNSTRIP_FOR_TARGET > + } else { > + set eu_unstrip [transform eu-unstrip] > + } > + return $eu_unstrip > +} > + > proc gdb_default_target_compile {source destfile type options} { > global target_triplet > global tool_root_dir The test case LGTM though I'm not familiar with eu-unstrip. I think it's okay for master with the nits that I found fixed. I don't have an opinion about including it in 8.3. Kevin