From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id E2BC63858D35 for ; Mon, 6 Jul 2020 02:47:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E2BC63858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3FB501E794; Sun, 5 Jul 2020 22:47:36 -0400 (EDT) Subject: Re: [PATCH][gdb/tui,c++17] Fix NULL string_view in tui_partial_win_by_name To: Tom de Vries , gdb-patches@sourceware.org Cc: Tom Tromey References: <20200705214048.GA21240@delia> From: Simon Marchi Message-ID: <5561a801-080e-72cb-8350-36d3e0aadf7f@simark.ca> Date: Sun, 5 Jul 2020 22:47:36 -0400 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: <20200705214048.GA21240@delia> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_PASS, 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: Mon, 06 Jul 2020 02:47:38 -0000 On 2020-07-05 5:40 p.m., Tom de Vries wrote: > Hi, > > When building gdb with CFLAGS=-std=gnu17 and CXXFLAGS=-std=gnu++17 and running > test-case gdb.tui/new-layout.exp, we run into: > ... > UNRESOLVED: gdb.tui/new-layout.exp: left window box after shrink (ll corner) > FAIL: gdb.tui/new-layout.exp: right window box after shrink (ll corner) > ... > > In a minimal form, we run into an abort when issuing a winheight command: > ... > $ gdb -tui -ex "winheight src - 5" > > Aborted (core dumped) > $ > ... > with this backtrace at the abort: > ... > \#0 0x0000000000438db0 in std::char_traits::length (__s=0x0) > at /usr/include/c++/9/bits/char_traits.h:335 > \#1 0x000000000043b72e in std::basic_string_view std::char_traits >::basic_string_view (this=0x7fffffffd4f0, \ > __str=0x0) at /usr/include/c++/9/string_view:124 > \#2 0x000000000094971b in tui_partial_win_by_name (name="src") > at src/gdb/tui/tui-win.c:663 > ... > due to a NULL comparison which constructs a string_view object from NULL: > ... > 657 /* Answer the window represented by name. */ > 658 static struct tui_win_info * > 659 tui_partial_win_by_name (gdb::string_view name) > 660 { > 661 struct tui_win_info *best = nullptr; > 662 > 663 if (name != NULL) > ... > > In gdbsupport/gdb_string_view.h, we either use: > - gdb's copy of libstdc++-v3/include/experimental/string_view, or > - the standard implementation of string_view, when built with C++17 or later > (which in gcc's case comes from libstdc++-v3/include/std/string_view) > > In the first case, there's support for constructing a string_view from a NULL > pointer: > ... > /*constexpr*/ basic_string_view(const _CharT* __str) > : _M_len{__str == nullptr ? 0 : traits_type::length(__str)}, > _M_str{__str} > { } > ... > but in the second case, there's not: > ... > __attribute__((__nonnull__)) constexpr > basic_string_view(const _CharT* __str) noexcept > : _M_len{traits_type::length(__str)}, > _M_str{__str} > { } > ... > > Fix this by removing the NULL comparison altogether. > > Build on x86_64-linux with CFLAGS=-std=gnu17 and CXXFLAGS=-std=gnu++17, and > tested. > > Any comments? This is the second string_view problem in a short amount of time, the other being: https://sourceware.org/bugzilla/show_bug.cgi?id=26183 When importing the string_view support, I ended up copying experimental/string_view (which is a version from before it was a standard) from libstdc++, rather than std/string_view. In this message: https://sourceware.org/legacy-ml/gdb-patches/2018-04/msg00111.html I said it was because "it is easier to adapt to the c++ standard we follow", but I don't remember the specific reason. I think we should change our string_view to be an adapted copy of std/string_view instead, to minimize the chances of differences with the standard version. I started to do it the other day (but did not finish adapting the tests), and didn't see any blocker. I still think your patch is good (it would be necessary in any case). Simon