From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x842.google.com (mail-qt1-x842.google.com [IPv6:2607:f8b0:4864:20::842]) by sourceware.org (Postfix) with ESMTPS id 48D513857C66 for ; Sat, 8 Aug 2020 10:19:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 48D513857C66 Received: by mail-qt1-x842.google.com with SMTP id s23so3174132qtq.12 for ; Sat, 08 Aug 2020 03:19:27 -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=ac07oh6CyYluu/GL0m2t/cvow5MoRLxibtmAxzJslAs=; b=jzEsX073wl9ziKvYQ1+jcIy0/n73iOzGBOIXv1tAJqY1G9FJhca9gRoG4K6UvOKrim HbmFlHLXGzXQCzZJ4kLtadzuO/kSQnCRX7XIBGOHASVjjoXK2eqTUGI0KYJ0monQ5ojY 6HmkMBgzldud5GJyBS71UgZi42y5bpsJgGLBh25soKtNptvku/Il9YaEZMipZ0m4Vyxr fUo195d2MjYZOH06NdqB3zfpCtKzZCmBaVeHtIMJ/lssA2uNIltNXtrnunNY0jIoIy2x Vlw77wTSu+MByYnFgy67WcSfNv5PDNAj15u0SPMTzD2ZFo7HAFxpj0wmY5EIP/ytkeGE SMLg== X-Gm-Message-State: AOAM531v9DThtI8Nn+diC90vZY15LeGIUvHwSXlzaD7Zv8H+RyMsDu/i Gj+vDkwCW0kwiEoAlJ0zsVFdFE9M3tRPbQ== X-Google-Smtp-Source: ABdhPJxUTQRF+yVrHAfxpZFnstTlmoXmAcTtf4JtFLyhORHANIDGXzGLOoRpwSpIQHNFtPMlyKaHlA== X-Received: by 2002:ac8:1948:: with SMTP id g8mr18983285qtk.354.1596881966505; Sat, 08 Aug 2020 03:19:26 -0700 (PDT) Received: from ?IPv6:2804:7f0:8283:6f2c:b77d:47b2:730b:373? ([2804:7f0:8283:6f2c:b77d:47b2:730b:373]) by smtp.gmail.com with ESMTPSA id d20sm8621389qkl.36.2020.08.08.03.19.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 08 Aug 2020 03:19:25 -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: Luis Machado Message-ID: Date: Sat, 8 Aug 2020 07:19:23 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200807222044.2252664-1-kevinb@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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: Sat, 08 Aug 2020 10:19:29 -0000 Hi Kevin, Thanks for working on this. I gave this a try and the failures below are gone, but there is a new one that showed up for gdb.base/corefile.exp: FAIL: gdb.base/corefile.exp: core-file warning-free I'll take a look at it later. gdb.base/corefile2.exp came out clean though. On 8/7/20 7:20 PM, Kevin Buettner 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 > 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); > + > /* 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; > > /* 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; > 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 > @@ -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); > + > + 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 > + > + # 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 { > + -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 > + } > + } > + > + # 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 >