From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 169CB3858D35 for ; Mon, 6 Jul 2020 09:19:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 169CB3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 73D6BAE0E; Mon, 6 Jul 2020 09:19:23 +0000 (UTC) Subject: Re: [PATCH][gdb/tui,c++17] Fix NULL string_view in tui_partial_win_by_name To: Simon Marchi , gdb-patches@sourceware.org Cc: Tom Tromey References: <20200705214048.GA21240@delia> <5561a801-080e-72cb-8350-36d3e0aadf7f@simark.ca> From: Tom de Vries Autocrypt: addr=tdevries@suse.de; keydata= xsBNBF0ltCcBCADDhsUnMMdEXiHFfqJdXeRvgqSEUxLCy/pHek88ALuFnPTICTwkf4g7uSR7 HvOFUoUyu8oP5mNb4VZHy3Xy8KRZGaQuaOHNhZAT1xaVo6kxjswUi3vYgGJhFMiLuIHdApoc u5f7UbV+egYVxmkvVLSqsVD4pUgHeSoAcIlm3blZ1sDKviJCwaHxDQkVmSsGXImaAU+ViJ5l CwkvyiiIifWD2SoOuFexZyZ7RUddLosgsO0npVUYbl6dEMq2a5ijGF6/rBs1m3nAoIgpXk6P TCKlSWVW6OCneTaKM5C387972qREtiArTakRQIpvDJuiR2soGfdeJ6igGA1FZjU+IsM5ABEB AAHNH1RvbSBkZSBWcmllcyA8dGRldnJpZXNAc3VzZS5kZT7CwKsEEwEIAD4WIQSsnSe5hKbL MK1mGmjuhV2rbOJEoAUCXSW0JwIbAwUJA8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAh CRDuhV2rbOJEoBYhBKydJ7mEpsswrWYaaO6FXats4kSgc48H/Ra2lq5p3dHsrlQLqM7N68Fo eRDf3PMevXyMlrCYDGLVncQwMw3O/AkousktXKQ42DPJh65zoXB22yUt8m0g12xkLax98KFJ 5NyUloa6HflLl+wQL/uZjIdNUQaHQLw3HKwRMVi4l0/Jh/TygYG1Dtm8I4o708JS4y8GQxoQ UL0z1OM9hyM3gI2WVTTyprsBHy2EjMOu/2Xpod95pF8f90zBLajy6qXEnxlcsqreMaqmkzKn 3KTZpWRxNAS/IH3FbGQ+3RpWkNGSJpwfEMVCeyK5a1n7yt1podd1ajY5mA1jcaUmGppqx827 8TqyteNe1B/pbiUt2L/WhnTgW1NC1QDOwE0EXSW0JwEIAM99H34Bu4MKM7HDJVt864MXbx7B 1M93wVlpJ7Uq+XDFD0A0hIal028j+h6jA6bhzWto4RUfDl/9mn1StngNVFovvwtfzbamp6+W pKHZm9X5YvlIwCx131kTxCNDcF+/adRW4n8CU3pZWYmNVqhMUiPLxElA6QhXTtVBh1RkjCZQ Kmbd1szvcOfaD8s+tJABJzNZsmO2hVuFwkDrRN8Jgrh92a+yHQPd9+RybW2l7sJv26nkUH5Z 5s84P6894ebgimcprJdAkjJTgprl1nhgvptU5M9Uv85Pferoh2groQEAtRPlCGrZ2/2qVNe9 XJfSYbiyedvApWcJs5DOByTaKkcAEQEAAcLAkwQYAQgAJhYhBKydJ7mEpsswrWYaaO6FXats 4kSgBQJdJbQnAhsMBQkDwmcAACEJEO6FXats4kSgFiEErJ0nuYSmyzCtZhpo7oVdq2ziRKD3 twf7BAQBZ8TqR812zKAD7biOnWIJ0McV72PFBxmLIHp24UVe0ZogtYMxSWKLg3csh0yLVwc7 H3vldzJ9AoK3Qxp0Q6K/rDOeUy3HMqewQGcqrsRRh0NXDIQk5CgSrZslPe47qIbe3O7ik/MC q31FNIAQJPmKXX25B115MMzkSKlv4udfx7KdyxHrTSkwWZArLQiEZj5KG4cCKhIoMygPTA3U yGaIvI/BGOtHZ7bEBVUCFDFfOWJ26IOCoPnSVUvKPEOH9dv+sNy7jyBsP5QxeTqwxC/1ZtNS DUCSFQjqA6bEGwM22dP8OUY6SC94x1G81A9/xbtm9LQxKm0EiDH8KBMLfQ== Message-ID: Date: Mon, 6 Jul 2020 11:19:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <5561a801-080e-72cb-8350-36d3e0aadf7f@simark.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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: Mon, 06 Jul 2020 09:19:25 -0000 On 7/6/20 4:47 AM, Simon Marchi wrote: > 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. Sounds like a good rationale to me. > 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 suppose it's a trade-off, but I agree ideally we use std/string_view. > I still think your patch is good (it would be necessary in any case). Ack, committed, and thanks for the explanation. - Tom