From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 489113858D34 for ; Wed, 6 Mar 2024 18:27:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 489113858D34 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 489113858D34 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=132.207.4.11 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709749680; cv=none; b=sZMCGy4Gr94PgondHBgF4QPuT+REgejpYtDJQu92RytoSsbRNHqa8dOOhj5jHi3/uxz5izw8luTIpvpF42rEURAt4IhImreap0yWsQ/jOG7e0p9waOblQE4g0/VYbTHnIt3ybXGSrtVhkgIrhrbL9wCwXu1K6KMl37M6Vd0eYx0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709749680; c=relaxed/simple; bh=TijoIk+vvHYt7LmvGlKC+n1oLx+S/6eo9SqUWX84iJM=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=HNY1Bc2mTk+/y57upHAv2sDa+XXGk04aNd2TcYvcOARaQ2mqAyDLzzrAEYNcQ+srfHwQNkvFM47SQtRggOHa5LFA9UidJeeZzhoelwN/VxnbCGDc2DNhfmv5r8JV1UT0DBmc1IV155YaoJjJmehs0aoKF+E8YOF0G0Gb4gkddbc= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 426IRpB8006579 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 6 Mar 2024 13:27:56 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 426IRpB8006579 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1709749676; bh=ihlnz5eoOpL9JGh672sCyxReTflAKffuDrnTwdbnQYo=; h=Date:Subject:To:From:In-Reply-To:From; b=DI/AT76YA+oNeYZTRQfxfKolxcqnx+lPOU1ER6PviqlhIKfjsiQzJlyFvwKKjejlg B0IRowFzQ7cHeMvyqSFExVcDxbNanzISo6Nf1WXCmqNgUjcESGYW6g5JAgcpB32fmf OKnDEjUvBFEj7kIfF3/1WtjhcO2oTAtLx/8h6Pl4= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id F11B21E092; Wed, 6 Mar 2024 13:27:50 -0500 (EST) Message-ID: <1e54180b-6665-43c7-9c3c-fdf72bd29a07@polymtl.ca> Date: Wed, 6 Mar 2024 13:27:50 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] [gdb/build] Fix static cast of virtual base Content-Language: fr To: Andrew Burgess , Tom de Vries , gdb-patches@sourceware.org References: <20240222161804.1134-1-tdevries@suse.de> <87frx39s43.fsf@redhat.com> From: Simon Marchi In-Reply-To: <87frx39s43.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 6 Mar 2024 18:27:51 +0000 X-Spam-Status: No, score=-3037.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/6/24 12:39, Andrew Burgess wrote: > Tom de Vries writes: > >> With this change in bfd/development.sh: >> ... >> -development=true >> +development=false >> ... >> we run into: >> ... >> In file included from tui-data.h:28:0, >> from tui-command.c:24: >> gdb-checked-static-cast.h: In instantiation of \ >> ‘T gdb::checked_static_cast(V*) [with T = tui_cmd_window*; V = tui_win_info]’: >> tui-command.c:65:15: required from here >> gdb-checked-static-cast.h:63:14: error: cannot convert from pointer to base \ >> class ‘tui_win_info’ to pointer to derived class ‘tui_cmd_window’ because \ >> the base is virtual >> T result = static_cast (v); >> ^~~~~~~~~~~~~~~~~~ > > Issues like there were supposed to be spotted by the static_asserts in > gdb::checked_static_cast. Clearly the existing asserts are not good > enough. > > I'd like to propose the patch below which would have prevented this > issue sneaking in unseen. Thoughts? > > Thanks, > Andrew > > --- > > commit 9d7cb56af0361a2276621203e0e508b6fda780f9 > Author: Andrew Burgess > Date: Wed Mar 6 17:28:48 2024 +0000 > > gdb: use static_cast in gdb::checked_static_cast > > This commit: > > commit 6fe4779ac4b1874c995345e3eabd89cb1a05fbdf > Date: Sat Feb 24 11:00:20 2024 +0100 > > [gdb/build] Fix static cast of virtual base > > addressed an issue where GDB would not compile in production mode due > to a use of gdb::checked_static_cast. The problem was that we were > asking GDB to cast from a virtual base class to a sub-class, this > works fine when using dynamic_cast, but does not work with > static_cast. > > The gdb::checked_static_cast actually uses dynamic_cast under the hood > in development mode in order to ensure that the cast is valid, while > in a production build we use static_cast as this is more efficient. > > What this meant however, was that when gdb::checked_static_cast was > used to cast from a virtual base class, the dynamic_cast of a > non-production build worked fine, while the production build's > static_cast caused a build failure. > > However, the gdb::checked_static_cast function already contains some > static_assert calls that are intended to catch any issues with invalid > type casting, the goal of these asserts was to prevent issues like > this: the build only failing in production mode. Clearly the current > asserts are not enough. > > I don't think there is a std::is_virtual_base type trait check, so > what I propose instead is that in non-production mode we also make use > of static_cast. This will ensure that any errors that crop up in > production mode should also be revealed in non-production mode, and > should catch issues like this in the future. > > There should be no user visible changes after this commit. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31399 > > diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h > index 227010e46ea..33cab48cd9b 100644 > --- a/gdbsupport/gdb-checked-static-cast.h > +++ b/gdbsupport/gdb-checked-static-cast.h > @@ -57,8 +57,14 @@ checked_static_cast (V *v) > if (v == nullptr) > return nullptr; > > - T result = dynamic_cast (v); > - gdb_assert (result != nullptr); > + gdb_assert (dynamic_cast (v) != nullptr); > + > + /* If a base class of V is virtual then the dynamic_cast will succeed, > + but the production mode static_cast will fail. So having checked with > + the dynamic_cast that we didn't get nullptr, now use static_cast to > + catch the virtual base case. This has the side effect of guaranteeing > + that GDB will compile in production mode. */ > + T result = static_cast (v); > #else > T result = static_cast (v); > #endif > This is similar to what I proposed here: https://inbox.sourceware.org/gdb-patches/24af4ea8-5426-4ce4-b1c5-12858b38a952@simark.ca/ The idea is the same, to have a static_cast in the DEVELOPMENT branch. I kinda like my version better, as it factors out the static cast (notice that both branches have identical static_cast lines after your patch) and the ifdef is just around a single assert. Also, I'm pretty sure the nullptr check is not necessary, as both dynamic_cast and static_cast can handle it. Simon