From patchwork Thu Apr 25 18:24:39 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 32418 Received: (qmail 64793 invoked by alias); 25 Apr 2019 18:24:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 64784 invoked by uid 89); 25 Apr 2019 18:24:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS, TRACKER_ID autolearn=ham version=3.3.1 spammy=pss, decides 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 18:24:43 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4D27F307D85A for ; Thu, 25 Apr 2019 18:24:40 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id A99721001E6C; Thu, 25 Apr 2019 18:24:39 +0000 (UTC) From: Sergio Durigan Junior To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [PATCH+8.3?] Implement dump of mappings with ELF headers by gcore References: <20190423234635.26916-1-sergiodj@redhat.com> <20190425102100.08da12eb@f29-4.lan> Date: Thu, 25 Apr 2019 14:24:39 -0400 In-Reply-To: <20190425102100.08da12eb@f29-4.lan> (Kevin Buettner's message of "Thu, 25 Apr 2019 10:21:00 -0700") Message-ID: <87mukec594.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes On Thursday, April 25 2019, Kevin Buettner wrote: > 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! Thanks for the review, Kevin. > [...] >> 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. Ah, OK. I tend to use them when I'm writing ChangeLog entries, but that's a matter of style. I'll remove them. >> 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. Yeah. I had the same thought, and reached the same conclusion :-). >> { >> /* 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. Alright. > (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.) I have a patch here to add SELFMAG and ELFMAG to elf/common.h. I'm not entirely sure where I should send it (binutils? gcc?), but I plan to propose it once I'm finished with this build-id patch. >> + { >> + /* 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. Thanks, I pushed the patch below to master: 57e5e645010430b3d73f8c6a757d09f48dc8f8d5 I'll wait for another GM to give an opinion about including this into 8.3 Thanks, diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index 5de985def3..c1666d189a 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) { /* 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,55 @@ 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 (); + + /* The EI_MAG* and ELFMAG* constants come from + . */ + if (h[EI_MAG0] == ELFMAG0 && h[EI_MAG1] == ELFMAG1 + && h[EI_MAG2] == ELFMAG2 && h[EI_MAG3] == ELFMAG3) + { + /* This mapping contains an ELF header, so we + should dump it. */ + dump_p = 1; + } + } + } + + return dump_p; } /* Implement the "info proc" command. */ @@ -1306,7 +1353,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