From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id D2C7F3858281 for ; Tue, 19 Jul 2022 15:10:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D2C7F3858281 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-212-j7efWOPNPwuIqHYYkK2A3w-1; Tue, 19 Jul 2022 11:10:36 -0400 X-MC-Unique: j7efWOPNPwuIqHYYkK2A3w-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 22CF03C1105D; Tue, 19 Jul 2022 15:10:36 +0000 (UTC) Received: from [10.97.116.21] (ovpn-116-21.gru2.redhat.com [10.97.116.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 60B0C1121314; Tue, 19 Jul 2022 15:10:35 +0000 (UTC) Message-ID: <45e2fd55-0ef7-5275-9b42-7b8480d84328@redhat.com> Date: Tue, 19 Jul 2022 12:10:34 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] gdbsupport: change path_join parameter to std::vector To: Simon Marchi , gdb-patches@sourceware.org Cc: Simon Marchi References: <20220719142741.3307396-1-simon.marchi@polymtl.ca> From: Bruno Larsen In-Reply-To: <20220719142741.3307396-1-simon.marchi@polymtl.ca> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Tue, 19 Jul 2022 15:10:42 -0000 On 7/19/22 11:27, Simon Marchi via Gdb-patches wrote: > From: Simon Marchi > > When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single > character name, we hit this assertion failure: > > $ ./gdb -q --data-directory=data-directory -nx ./x > /usr/include/c++/12.1.0/string_view:239: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed. > > The backtrace: > > #3 0x00007ffff6c0f002 in std::__glibcxx_assert_fail (file=, line=, function=, condition=) at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60 > #4 0x000055555da8a864 in std::basic_string_view >::operator[] (this=0x7fffffffcc30, __pos=1) at /usr/include/c++/12.1.0/string_view:239 > #5 0x00005555609dcb88 in path_join[abi:cxx11](gdb::array_view > const>) (paths=...) at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:203 > #6 0x000055555e0443f4 in path_join () at /home/simark/src/binutils-gdb/gdb/../gdbsupport/pathstuff.h:84 > #7 0x00005555609dc336 in gdb_realpath_keepfile[abi:cxx11](char const*) (filename=0x6060000a8d40 "/home/simark/build/binutils-gdb-one-target/gdb/./x") at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:122 > #8 0x000055555ebd2794 in exec_file_attach (filename=0x7fffffffe0f9 "./x", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:471 > #9 0x000055555f2b3fb0 in catch_command_errors (command=0x55555ebd1ab6 , arg=0x7fffffffe0f9 "./x", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:513 > #10 0x000055555f2b7e11 in captured_main_1 (context=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1209 > #11 0x000055555f2b9144 in captured_main (data=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1319 > #12 0x000055555f2b9226 in gdb_main (args=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1344 > #13 0x000055555d938c5e in main (argc=5, argv=0x7fffffffdcf8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32 > > The problem is this line in path_join: > > gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path)); > > ... where `path` is "x". IS_ABSOLUTE_PATH eventually calls > HAS_DRIVE_SPEC_1: > > #define HAS_DRIVE_SPEC_1(dos_based, f) \ > ((f)[0] && ((f)[1] == ':') && (dos_based)) > > This macro accesses indices 0 and 1 of the input string. However, `f` > is a string_view of length 1, so it's incorrect to try to access index > 1. We know that the string_view's underlying object is a null-terminated > string, so in practice there's no harm. But as far as the string_view > is concerned, index 1 is considered out of bounds. > > This patch makes the easy fix, that is to change the path_join parameter > from a vector of to a vector of `const char *`. Another solution would > be to introduce a non-standard gdb::cstring_view class, which would be a > view over a null-terminated string. With that class, it would be > correct to access index 1, it would yield the NUL character. If there > is interest in having this class (it has been mentioned a few times in > the past) I can do it and use it here. > > This was found by running tests such as gdb.ada/arrayidx.exp, which > produce 1-char long filenames, so adding a new test is not necessary. > > Change-Id: Ia41a16c7243614636b18754fd98a41860756f7af > --- > gdbsupport/pathstuff.cc | 8 ++++---- > gdbsupport/pathstuff.h | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc > index af10c6ebd2e8..390f10b1b5e4 100644 > --- a/gdbsupport/pathstuff.cc > +++ b/gdbsupport/pathstuff.cc > @@ -191,21 +191,21 @@ child_path (const char *parent, const char *child) > /* See gdbsupport/pathstuff.h. */ > > std::string > -path_join (gdb::array_view paths) > +path_join (gdb::array_view paths) > { > std::string ret; > > for (int i = 0; i < paths.size (); ++i) > { > - const gdb::string_view path = paths[i]; > + const char *path = paths[i]; > > if (i > 0) > - gdb_assert (path.empty () || !IS_ABSOLUTE_PATH (path)); > + gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path)); > > if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ())) > ret += '/'; > > - ret.append (path.begin (), path.end ()); > + ret.append (path); > } > > return ret; > diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h > index d01db89e085f..aa7b0ffa2fbd 100644 > --- a/gdbsupport/pathstuff.h > +++ b/gdbsupport/pathstuff.h > @@ -67,7 +67,7 @@ extern const char *child_path (const char *parent, const char *child); > The first element can be absolute or relative. All the others must be > relative. */ > > -extern std::string path_join (gdb::array_view paths); > +extern std::string path_join (gdb::array_view paths); > > /* Same as the above, but accept paths as distinct parameters. */ > > @@ -78,10 +78,10 @@ path_join (Args... paths) > /* It doesn't make sense to join less than two paths. */ > gdb_static_assert (sizeof... (Args) >= 2); > > - std::array views > - { gdb::string_view (paths)... }; > + std::array views > + { paths... }; Very minor nit, this array used to be called "views" because it held string_views, the name should probably be changed, maybe path_array or something? Cheers! Bruno Larsen > > - return path_join (gdb::array_view (views)); > + return path_join (gdb::array_view (views)); > } > > /* Return whether PATH contains a directory separator character. */ > > base-commit: 68cffbbd4406b4efe1aa6e18460b1d7ca02549f1