From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by sourceware.org (Postfix) with ESMTPS id 2CAD23865493 for ; Thu, 20 Aug 2020 14:57:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2CAD23865493 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f66.google.com with SMTP id 88so2336909wrh.3 for ; Thu, 20 Aug 2020 07:57:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=BS/wcG/n37eZkuV5NAbTqDuBEZez4Lj24WA26Jzn040=; b=haWBUQKuTcXWKAgcHKwAQX8WG6LMZfbz2Bhl7h61R/odl5zP0HFkXpTIKl/B4yD29s N0mHL9cnV5XFv2X06yIcIkjXwM/o1TX6djB9VM2cVPwVGCZHv30TeMiINmOouFXwa8rh sTMkkwDbNkDmh+EAKZarQ3QJyJBNHOlHiyzOv28pFe0y7SNRV5xJNC7HiYGzc/RdF2UD +LD7Akhd9cQQljQYbOA4c4IZlLLBQa2/+Z+a6VRIJSwJyaM9cBwweh8H4dmlPNuMARrN yBkPVNcA8TAC/isgIDKp3wCh5ejdpASRjMigPRP0kzs5TYlYLqurmVnDTNdxqOjym/oj Tygw== X-Gm-Message-State: AOAM5328H1rETamlc3NvPdztnS98KxTz0/pv2ypcUDuCuJlmUJ+3XBoT PSIVnB8IhZ8dOZzBHZDkyWa/nR9pam1bHg== X-Google-Smtp-Source: ABdhPJwwJitBJj9H3swA4spNqSvC20yendzOAcDI8JIXus5osvRwglMU505y4dqv5W/r2D3iUKc0/A== X-Received: by 2002:adf:b30c:: with SMTP id j12mr3575919wrd.420.1597935468878; Thu, 20 Aug 2020 07:57:48 -0700 (PDT) Received: from ?IPv6:2001:8a0:f905:5600:56ee:75ff:fe8d:232b? ([2001:8a0:f905:5600:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id x2sm4877779wrg.73.2020.08.20.07.57.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 20 Aug 2020 07:57:47 -0700 (PDT) Subject: Re: [PATCH] Work around incorrect/broken pathnames in NT_FILE note To: Kevin Buettner , gdb-patches@sourceware.org References: <20200807222044.2252664-1-kevinb@redhat.com> From: Pedro Alves Message-ID: <0430a5a8-e5b5-d63e-047c-0d25fb5767ac@palves.net> Date: Thu, 20 Aug 2020 15:57:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200807222044.2252664-1-kevinb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Aug 2020 14:57:53 -0000 Hi Kevin, This LGTM, with a few minor issues pointed out below addressed. On 8/7/20 11:20 PM, Kevin Buettner via Gdb-patches wrote: > Luis Machado reported some regressions after I pushed recent core file > related patches fixing BZ 25631: > > FAIL: gdb.base/corefile.exp: backtrace in corefile.exp > FAIL: gdb.base/corefile.exp: core-file warning-free > FAIL: gdb.base/corefile.exp: print func2::coremaker_local > FAIL: gdb.base/corefile.exp: up in corefile.exp > FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit) > > While I haven't been able to reproduce these failures, I think I > understand why they're happening. It is my hope that this commit will > fix those regressions. > > Luis is testing in a docker container which is using the AUFS storage > driver. It turns out that the kernel is placing docker host paths in > the NT_FILE note instead of paths within the container. > > I've made a similar docker environment (though apparently not similar > enough to reproduce the regressions). This is one of the paths that > I see mentioned in the warning messages printed while loading the > core file during NT_FILE note processing - note that I've shortened > the path component starting with "d07c4": > > /var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so > > This is a path on the docker host; it does not exist in the > container. In the docker container, this is the path: > > /lib/x86_64-linux-gnu/ld-2.27.so > > My first thought was to disable all NT_FILE mappings when any path was > found to be bad. This would have caused GDB to fall back to accessing > memory using the file stratum as it did before I added the NT_FILE > note loading code. After further consideration, I realized that we > could do better than this. For file-backed memory access, we can > still use the NT_FILE mappings when available, and then attempt to > access memory using the file stratum constrainted to those address > ranges corresponding to the "broken" mappings. > > In order to test it, I made some additions to corefile2.exp in which > the test case's executable is renamed. The core file is then loaded; > due to the fact that the executable has been renamed, those mappings > will be unavailable. After loading the core file, the executable is > renamed back to its original name at which point it is loaded using > GDB's "file" command. The "interesting" tests are then run. These > tests will print out values in file-backed memory regions along with > mmap'd regions placed within/over the file-backed regions. Despite > the fact that the executable could not be found during the NT_FILE > note processing, these tests still work correctly due to the fact that > memory is availble from the file stratum combined with the fact that "availAble" > the broken NT_FILE mappings are used to prevent file-backed access > outside of the "broken" mappings. > > gdb/ChangeLog: > > * corelow.c (class core_target): Add field > 'm_core_unavailable_mappings'. > (core_target::build_file_mappings): Print only one warning > per inaccessible file. Add unavailable/broken mappings > to m_core_unavailable_mappings. > (core_target::xfer_partial): Call... > (core_target::xfer_memory_via_mappings): New method. > > gdb/testsuite/ChangeLog: > > * gdb.base/corefile2.exp (renamed binfile): New tests. > --- > gdb/corelow.c | 84 +++++++++++++++++++++++++--- > gdb/testsuite/gdb.base/corefile2.exp | 35 ++++++++++++ > 2 files changed, 111 insertions(+), 8 deletions(-) > > diff --git a/gdb/corelow.c b/gdb/corelow.c > index b6ee219f57..2e6be74a24 100644 > --- a/gdb/corelow.c > +++ b/gdb/corelow.c > @@ -131,9 +131,21 @@ class core_target final : public process_stratum_target > information about memory mapped files. */ > target_section_table m_core_file_mappings {}; > > + /* Unavailable mappings. These correspond to pathnames which either > + weren't found or could not be opened. Knowing these addresses can > + still be useful. */ > + std::vector m_core_unavailable_mappings; > + > /* Build m_core_file_mappings. Called from the constructor. */ > void build_file_mappings (); > > + /* Helper method for xfer_partial. */ > + enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf, > + const gdb_byte *writebuf, > + ULONGEST offset, > + ULONGEST len, > + ULONGEST *xfered_len); Seems like a tabs vs spaces mixup here. > + > /* FIXME: kettenis/20031023: Eventually this field should > disappear. */ > struct gdbarch *m_core_gdbarch = NULL; > @@ -182,6 +194,7 @@ void > core_target::build_file_mappings () > { > std::unordered_map bfd_map; > + std::unordered_map unavailable_paths; > And unordered_map to store a boolean that means "I've seen it" sounds like the "wrong" data structure. A more natural fit would be: std::unordered_set unavailable_paths; Then either the string is in the set or is isn't. > /* See linux_read_core_file_mappings() in linux-tdep.c for an example > read_core_file_mappings method. */ > @@ -216,9 +229,13 @@ core_target::build_file_mappings () > = exec_file_find (filename, NULL); > if (expanded_fname == nullptr) > { > - warning (_("Can't open file %s during file-backed mapping " > - "note processing"), > - filename); > + m_core_unavailable_mappings.emplace_back (start, end - start); > + if (!unavailable_paths[filename]) > + warning (_("Can't open file %s during file-backed mapping " > + "note processing"), > + filename); > + /* Print just one warning per path. */ > + unavailable_paths[filename] = true; and then: /* Print just one warning per path. */ if (unavailable_paths.insert (filename).second) warning (_("Can't open file %s during file-backed mapping " "note processing"), filename); unordered_set::insert does not insert if the element already exists, and it returns a pair where you can check on the second field whether something was inserted or not. > return; > } > > @@ -227,6 +244,7 @@ core_target::build_file_mappings () > > if (bfd == nullptr || !bfd_check_format (bfd, bfd_object)) > { > + m_core_unavailable_mappings.emplace_back (start, end - start); > /* If we get here, there's a good chance that it's due to > an internal error. We issue a warning instead of an > internal error because of the possibility that the Now that we're expecting to reach here, the comment about an internal error seems stale. > @@ -268,6 +286,8 @@ core_target::build_file_mappings () > ts->owner = nullptr; > ts->the_bfd_section = sec; > }); > + > + normalize_mem_ranges (&m_core_unavailable_mappings); > } > > static void add_to_thread_list (bfd *, asection *, void *); > @@ -728,6 +748,57 @@ core_target::files_info () > print_section_info (&m_core_section_table, core_bfd); > } > > +/* Helper method for core_target::xfer_partial. */ > + > +enum target_xfer_status > +core_target::xfer_memory_via_mappings (gdb_byte *readbuf, > + const gdb_byte *writebuf, > + ULONGEST offset, ULONGEST len, > + ULONGEST *xfered_len) > +{ > + enum target_xfer_status xfer_status; > + > + xfer_status = section_table_xfer_memory_partial > + (readbuf, writebuf, > + offset, len, xfered_len, > + m_core_file_mappings.sections, > + m_core_file_mappings.sections_end); Wrap in parens so that emacs will indent this correctly: xfer_status = (section_table_xfer_memory_partial (readbuf, writebuf, offset, len, xfered_len, m_core_file_mappings.sections, m_core_file_mappings.sections_end)); That's what the GNU standards say to do. > + > + if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ()) > + return xfer_status; > + > + /* There are instances - e.g. when debugging within a docker > + container using the AUFS storage driver - where the pathnames > + obtained from the note section are incorrect. Despite the path > + being wrong, just knowing the start and end addresses of the > + mappings is still useful; we can attempt an access of the file > + stratum constrained to the address ranges corresponding to the > + unavailable mappings. */ > + > + ULONGEST memaddr = offset; > + ULONGEST memend = offset + len; > + > + for (const auto &mr : m_core_unavailable_mappings) > + { > + if (address_in_mem_range (memaddr, &mr)) > + { > + if (!address_in_mem_range (memend, &mr)) > + len = mr.start + mr.length - memaddr; > + > + xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY, > + NULL, > + readbuf, > + writebuf, > + offset, > + len, > + xfered_len); > + break; > + } > + } > + > + return xfer_status; > +} > + > enum target_xfer_status > core_target::xfer_partial (enum target_object object, const char *annex, > gdb_byte *readbuf, const gdb_byte *writebuf, > @@ -761,11 +832,8 @@ core_target::xfer_partial (enum target_object object, const char *annex, > result. If not, check the stratum beneath us, which should > be the file stratum. */ > if (m_core_file_mappings.sections != nullptr) > - xfer_status = section_table_xfer_memory_partial > - (readbuf, writebuf, > - offset, len, xfered_len, > - m_core_file_mappings.sections, > - m_core_file_mappings.sections_end); > + xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset, > + len, xfered_len); > else > xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf, > writebuf, offset, len, > diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp > index 5de7ead4d4..11915a6185 100644 > --- a/gdb/testsuite/gdb.base/corefile2.exp > +++ b/gdb/testsuite/gdb.base/corefile2.exp > @@ -143,6 +143,41 @@ gdb_test_multiple $test "" { > } > } > > +# Test again with executable renamed during loading of core file. > + > +with_test_prefix "renamed binfile" { > + # We don't use clean_restart here since we want to defer loading > + # of $binfile until after the core file has been loaded. (BFD > + # will complain that $binfile has disappeared after the rename > + # if it's loaded first.) > + gdb_exit > + gdb_start > + gdb_reinitialize_dir $srcdir/$subdir Please still call clean_restart, but just don't pass it any argument. In that case, it skips loading any file. > + > + # Rename $binfile so that it won't be found during loading of > + # the core file. > + set hide_binfile [standard_output_file "${testfile}.hide"] > + remote_exec host "mv -f $binfile $hide_binfile" > + > + # Load core file - check that a warning is printed. > + global xfail > + if { $xfail } { setup_xfail "*-*-*" } > + gdb_test_multiple "core-file $corefile" $test { $test here is still "maint print core-file-backed-mappings". > + -re "warning: Can't open file.*during.* note processing.*Core was generated by .*\#0 .*\(\).*\r\n$gdb_prompt $" { > + pass $test > + } > + -re "\r\n$gdb_prompt $" { > + fail $test > + } This match is already done internally by gdb_test_multiple. That suggests that this gdb_test_multiple call could be a gdb_test call instead. Thanks, Pedro Alves > + } > + > + # Restore $binfile and then load it. > + remote_exec host "mv -f $hide_binfile $binfile" > + gdb_load ${binfile} > + > + do_tests > +} > + > # Restart and run to the abort call. > > clean_restart $binfile >