From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [96.47.72.81]) by sourceware.org (Postfix) with ESMTPS id D50CE385734C for ; Tue, 31 May 2022 16:04:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D50CE385734C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id A597D7BF08; Tue, 31 May 2022 16:04:36 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4LCHCm41Lvz4h7S; Tue, 31 May 2022 16:04:36 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1654013076; 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=NqtOgbRurNw6eDxFbp6DmMv/RwHcYXgHfgB6d5eFFUs=; b=yggRGWm1G96/3lMw0SkF/jD1HrmY6cSmwJWtrIFg4kkeNPCHaOmqfau/gj0iln2Wg250u2 rV4COaWgloH741ijJaf7ujGvVxCmT0msbrs4+Bd22ys1Pu5J/FpNmuvJTVo78Dh7yL14DV bfgWID8tBdAct9IkVHzkJiJfAdZDFzAUoiVBySIB+ygZ6ghJe6HQWLz35Ld8N/km89VV5V 0xwr4xeEIwWXGwG5NumcuDXn/2wgihGNhQEo1wvzZOQmLZvgGJjud+OwsZgZ5YZVBlpW8f HvecaE6VY0C2Y7h5CaOLcI/k49hafQ5+S/SIHoiGFV7XWRfv2e9fHzApUK6h5Q== Received: from [10.0.1.4] (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 09D572C15F; Tue, 31 May 2022 16:04:35 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <3ca3a276-fb2b-c000-ac0b-89bd8381b743@FreeBSD.org> Date: Tue, 31 May 2022 09:04:34 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <65f89b8748dfec7fd62681df2b1efcfa2c25026f.1654007211.git.aburgess@redhat.com> From: John Baldwin Subject: Re: [PATCH 4/5] gdb: ensure the cast in gdbarch_tdep is valid In-Reply-To: <65f89b8748dfec7fd62681df2b1efcfa2c25026f.1654007211.git.aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1654013076; 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=NqtOgbRurNw6eDxFbp6DmMv/RwHcYXgHfgB6d5eFFUs=; b=aJgEz5ILTreBV5P/z5deeFqv9V5Kq/HkMaCHkiVIOek2c3KXOwOh74CrEtjPQzdrE79qdS 7ZjHUv/PH6dXck7Aim4uVZtVNYkzf/7bc1wS/gZlXPzhGGJ6FS7+U8Ol7TjURyeBODp/56 N2p1l6ebu6yDxGGp82BOdEFiBGtkCmhn5xcyLhzraNHaiDrCMpZ3huYeBvcR8qnOps2k+W uwpQzApiXYrHLM+JYqIr6liWeWo03ZA2t0bTjzxgrNgHy0wzAoar6ihplkTshb22URN1Cl Hdww5ERTif/HWGGPy+tIIluyDoG6ISJ5FM/2y6rnDOGa5+BPUvwcY4h+l0b2Kg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1654013076; a=rsa-sha256; cv=none; b=Is030722UXltHDPYV3NDXnG1p9z4CSlAOviUBJN9F0cJvWFngmpCSz9v4T2z/XoItFtScj YiR/zjr28NjJvFETaNN4yMmCZH79q+7yRs8qhLRXNIf9l4UQSsihvo+6zRIXo0H489crHQ qW8giO8SddLvuqDyAUtf4SZyiASKhs93/vCW2K6PE+3JrVLDGQsPd3jVTkzYiFelsx1Fcy MN7VtEqEMOidPfsLEW7Bez/IX2DvHW6bwtSXZpPNlO/mwDLMyLCv9cRj/XqndHO6erq/1v dL6erDe1sTlS/Kjmw3ieQkk5ur5FpIXjxordSeUxHTFdD2EULyP42ZdiImNLCA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-Spam-Status: No, score=-12.6 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, SPF_HELO_NONE, SPF_PASS, 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 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, 31 May 2022 16:04:38 -0000 On 5/31/22 7:30 AM, Andrew Burgess via Gdb-patches wrote: > This commit builds on the previous commit and modifies the > gdbarch_tdep function to ensure that the cast being performed is > valid. > > To do this I make use of dynamic_cast to ensure that the generic > gdbarch_tdep pointer that we have is of the correct type. > > The only problem with this approach is that, in order to use > dynamic_cast, we need RTTI information, which requires the class to > have a vtable, which currently, is not something the various tdep > classes have. > > And so, in this commit, I add a virtual destructor to the gdbarch_tdep > class. > > With this change I can now add an assert in the gdbarch_tdep function. > > Obviously, this change comes at a cost, creation of the tdep classes > is now slightly more expensive (due to vtable initialisation), > however, this only happens when a new gdbarch is created, which is not > that frequent, so I don't see that as a huge concern. > > Then, there is an increased cost each time the tdep is accessed. This > is much more frequent, but I don't believe the cost is excessive (a > vtable pointer comparison), at least, no worse than many of our other > asserts. > > If we consider the motivating example that was discussed in the > previous commit; build GDB for all targets on an x86-64 GNU/Linux > system, and then attempt to "run" a RISC-V binary using the native > x86-64 Linux target. Previously this would trigger an assert while > accessing fields within a i386_gdbarch_tdep, like this: > > ../../src/gdb/i387-tdep.c:596: internal-error: i387_supply_fxsave: Assertion `tdep->st0_regnum >= I386_ST0_REGNUM' failed. > > But with the changes from this commit in place, we now see an > assertion failure like this: > > ../../src/gdb/gdbarch.h:166: internal-error: gdbarch_tdep: Assertion `dynamic_cast (tdep) != nullptr' failed. > > On the face of it, this might not seem like much of an improvement, > but I think it is. > > The previous assert was triggered by undefined behaviour. There's no > guarantee that we would see an assertion at all, a different > combination of native target and binary format might not trigger an > assert (and just do the wrong thing), or might crash GDB completely. > > In contrast, the new assert is based on defined behaviour, we'll > always assert if GDB goes wrong, and we assert early, at the point the > mistake is being made (casting the result of gdbarch_tdep to the wrong > type), rather than at some later point after the incorrect cast has > completed. > > Obviously, when we consider the original example, trying to run a > binary of the wrong architecture on a native target, having GDB fail > with an assertion is not a real solution. No user action should be > able to trigger an assertion failure. In a later commit I will offer > a real solution to this architecture mismatch problem. > --- > gdb/gdbarch.h | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h > index b2c91db0c4f..ea507d70ec9 100644 > --- a/gdb/gdbarch.h > +++ b/gdb/gdbarch.h > @@ -58,7 +58,13 @@ struct inferior; > > #include "regcache.h" > > -struct gdbarch_tdep {}; > +/* The base class for every architecture's tdep sub-class. We include a > + virtual destructor so that sub-classes will have RTTI information. */ > + > +struct gdbarch_tdep > +{ > + virtual ~gdbarch_tdep() = default; > +}; > > /* The architecture associated with the inferior through the > connection to the target. > @@ -157,6 +163,7 @@ static inline TDepType * > gdbarch_tdep (struct gdbarch *gdbarch) > { > struct gdbarch_tdep *tdep = gdbarch_tdep_1 (gdbarch); > + gdb_assert (dynamic_cast (tdep) != nullptr); > return static_cast (tdep); > } Maybe simplify to use dynamic_cast for the returned value, e.g. { TDepType *tdep = dynamic_cast (gdbarch_tdep_1 (gdbarch)); gdb_assert(tdep != nullptr); return (tdep); } -- John Baldwin