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.129.124]) by sourceware.org (Postfix) with ESMTPS id ABA483858D20 for ; Mon, 11 Mar 2024 10:25:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ABA483858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org ABA483858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710152761; cv=none; b=NAXO7GWRfmbKkEgh2lPxI5UFtAOYE5hdxj/0LMoCGmqxsodiTxTR7kiAScW1zt/qkJVKa79Vjjrd9OCx+0ziK5lDeXlr6wqt3Goi5/9bVyXs5BXovat47v6Sxnh1VGbNRavvDDdb4W48SrGiiaCcENoHH+Wpxy7WO6NYuVD1cBc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710152761; c=relaxed/simple; bh=OlJi6ll1Kq9XDNZA0WTvDmWgYA+3IR1HkX+B3cv02BY=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=en7ZEx5LsmL2XdglraLPD4XFaqMJcFZolcwQhpul5cEQZW3afZqTF/01UtSmFUzBVfLVWa3N3NIkAczkYqH5kB3pFK0bblaNxHYuR+EU0qLxea7vwiRlVOKz0HT8rXkx+gexb5mwMwNrjMNt9qNa/AZRQ5pvV0VFPEUtHtwtAXg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1710152758; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ujCLvyWkFlzqcB3fR5nrv8SyKuXsZhUDPo3+Y4QorAw=; b=cJQOAI2jn7VVTbZYOLR6U3lEw6j2zW850Is/f5CEKxEqL4Ps54sOH4yX6Iy2H6u6H25b4d mJUg7ajEzGFH2e0MgXfiuShCZOprBmXA9J0kpcG1Myy7o2Y25fpq57Gmsnk/ViaDuud4M4 JkepCYnIl8a5MTNWX7uUYUdbFpVuJ+o= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-491-e-A3PwXgMQioqlDc-JgFAA-1; Mon, 11 Mar 2024 06:25:56 -0400 X-MC-Unique: e-A3PwXgMQioqlDc-JgFAA-1 Received: by mail-ej1-f70.google.com with SMTP id a640c23a62f3a-a462e4d8c44so38508666b.1 for ; Mon, 11 Mar 2024 03:25:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710152755; x=1710757555; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=vVxlLCOTZjIuZUMNwNmbqBX9zkGbcQth4k/nULxkJbM=; b=gNyip39d6L/QV6GHEMAXpTPTxvNJRRErto+6dvk7W4xqum5YdEH7An2vEyUpug094y AdIKVddW1w75TeLZWZtKjfV3XVvi6+SlFegjc2qqLtNCz/5E9ecyOzssJJwTorhyLHO7 LJm1viNYyhpzmXSpjomtzuc7VO6k3E4hYnQ66LPTlK4gr8/Juzi/typsDlkLTxObuIpj tITtfH+rvxfI5/WmPSPKAWEWcquQERFDBq3b3whY4IyfZJh+2ApKbTQ/6kiDhCpvCLOg P9wiHwAXL5jx6PP5W/KCltI0A56/6tKCbF1f9bBfttol5wHG46EgHVaCbzzBKZnhLPuV DOYA== X-Forwarded-Encrypted: i=1; AJvYcCVIUVkKZP+14pHZ2xwd0A8cE7bJyLDAYGqUvYt5PsJc9S1VMBpnZ2/0nyvDIAFddqNQ1mnaWi2tnNNAIIn78miqdBsx3eAPgD4j/g== X-Gm-Message-State: AOJu0YxwnzwrxrlNiaZx0HjYy+0Kd3LFLyh4GkGkaJh3qEkqHSc3cYJw IUn8BaHYevCzkUiS5qCBnw0ThgrbDevJpqhoDGCsUjoOdB+yhYLKW180JjEncrB1H/NLamiK3Sv w2apVH9j0fdgMxBaJXvtvA//7LnIYwHRB2IrtlXCaIqTavw3WUlwz3abG6MEgFHIhVy8= X-Received: by 2002:a17:906:1c99:b0:a45:da1:9375 with SMTP id g25-20020a1709061c9900b00a450da19375mr3713223ejh.30.1710152754912; Mon, 11 Mar 2024 03:25:54 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH7so0hKTRaJ3xjSeg4zzV+nqfAWZ0tyNhPcsz3AOXEdjJjfbZlNiDdpfU7fu7CXHeQNOM7jw== X-Received: by 2002:a17:906:1c99:b0:a45:da1:9375 with SMTP id g25-20020a1709061c9900b00a450da19375mr3713200ejh.30.1710152754375; Mon, 11 Mar 2024 03:25:54 -0700 (PDT) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id ku2-20020a170907788200b00a44936527b5sm2729880ejc.99.2024.03.11.03.25.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Mar 2024 03:25:54 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , Tom de Vries , gdb-patches@sourceware.org Subject: Re: [PATCH] [gdb/build] Fix static cast of virtual base In-Reply-To: <1e54180b-6665-43c7-9c3c-fdf72bd29a07@polymtl.ca> References: <20240222161804.1134-1-tdevries@suse.de> <87frx39s43.fsf@redhat.com> <1e54180b-6665-43c7-9c3c-fdf72bd29a07@polymtl.ca> Date: Mon, 11 Mar 2024 10:25:53 +0000 Message-ID: <8734sxqd3i.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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: Simon Marchi writes: > On 3/6/24 12:39, Andrew Burgess wrote: >> Tom de Vries writes: >>=20 >>> With this change in bfd/development.sh: >>> ... >>> -development=3Dtrue >>> +development=3Dfalse >>> ... >>> 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 \ >>> =E2=80=98T gdb::checked_static_cast(V*) [with T =3D tui_cmd_window*; = V =3D tui_win_info]=E2=80=99: >>> tui-command.c:65:15: required from here >>> gdb-checked-static-cast.h:63:14: error: cannot convert from pointer to = base \ >>> class =E2=80=98tui_win_info=E2=80=99 to pointer to derived class =E2= =80=98tui_cmd_window=E2=80=99 because \ >>> the base is virtual >>> T result =3D static_cast (v); >>> ^~~~~~~~~~~~~~~~~~ >>=20 >> 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. >>=20 >> I'd like to propose the patch below which would have prevented this >> issue sneaking in unseen. Thoughts? >>=20 >> Thanks, >> Andrew >>=20 >> --- >>=20 >> commit 9d7cb56af0361a2276621203e0e508b6fda780f9 >> Author: Andrew Burgess >> Date: Wed Mar 6 17:28:48 2024 +0000 >>=20 >> gdb: use static_cast in gdb::checked_static_cast >> =20 >> This commit: >> =20 >> commit 6fe4779ac4b1874c995345e3eabd89cb1a05fbdf >> Date: Sat Feb 24 11:00:20 2024 +0100 >> =20 >> [gdb/build] Fix static cast of virtual base >> =20 >> addressed an issue where GDB would not compile in production mode du= e >> 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. >> =20 >> The gdb::checked_static_cast actually uses dynamic_cast under the ho= od >> 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. >> =20 >> 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. >> =20 >> However, the gdb::checked_static_cast function already contains some >> static_assert calls that are intended to catch any issues with inval= id >> type casting, the goal of these asserts was to prevent issues like >> this: the build only failing in production mode. Clearly the curren= t >> asserts are not enough. >> =20 >> 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 u= se >> 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. >> =20 >> There should be no user visible changes after this commit. >> =20 >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D31399 >>=20 >> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-check= ed-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 =3D=3D nullptr) >> return nullptr; >> =20 >> - T result =3D dynamic_cast (v); >> - gdb_assert (result !=3D nullptr); >> + gdb_assert (dynamic_cast (v) !=3D 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 t= o >> + catch the virtual base case. This has the side effect of guarante= eing >> + that GDB will compile in production mode. */ >> + T result =3D static_cast (v); >> #else >> T result =3D static_cast (v); >> #endif >>=20 > > This is similar to what I proposed here: > > https://inbox.sourceware.org/gdb-patches/24af4ea8-5426-4ce4-b1c5-12858b38= a952@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. I agree yours is better. Do you have an official patch somewhere that I can add a +1 too? If not then below is an updated version inline with you proposal. Feel free to push your version if you have it somewhere. Thanks, Andrew -- commit 2a82aa7d140387fd728641846d9c4ec7d770b067 Author: Andrew Burgess Date: Wed Mar 6 17:28:48 2024 +0000 gdb: use static_cast in gdb::checked_static_cast =20 This commit: =20 commit 6fe4779ac4b1874c995345e3eabd89cb1a05fbdf Date: Sat Feb 24 11:00:20 2024 +0100 =20 [gdb/build] Fix static cast of virtual base =20 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. =20 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. =20 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. =20 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. =20 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. =20 There should be no user visible changes after this commit. =20 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D31399 =20 Co-Authored-By: Simon Marchi diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-= static-cast.h index 227010e46ea..24fa7a4ba04 100644 --- a/gdbsupport/gdb-checked-static-cast.h +++ b/gdbsupport/gdb-checked-static-cast.h @@ -54,16 +54,12 @@ checked_static_cast (V *v) =09=09 "types must be related"); =20 #ifdef DEVELOPMENT - if (v =3D=3D nullptr) - return nullptr; - - T result =3D dynamic_cast (v); - gdb_assert (result !=3D nullptr); -#else - T result =3D static_cast (v); + gdb_assert (v =3D=3D nullptr || dynamic_cast (v) !=3D nullptr); #endif =20 - return result; + /* If a base class of V is virtual then the dynamic_cast above will + succeed, but this static_cast will fail. */ + return static_cast (v); } =20 /* Same as the above, but to cast from a reference type to another. */