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.133.124]) by sourceware.org (Postfix) with ESMTPS id 298C13857BA8 for ; Tue, 31 May 2022 17:22:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 298C13857BA8 Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-312-9OXwrICQNcWqsb87-EurBw-1; Tue, 31 May 2022 13:22:48 -0400 X-MC-Unique: 9OXwrICQNcWqsb87-EurBw-1 Received: by mail-wm1-f70.google.com with SMTP id bg7-20020a05600c3c8700b0039468585269so6335346wmb.3 for ; Tue, 31 May 2022 10:22:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=4hGn3jTQNpUmJRQQo17dgvEez/44JaDJnjZ7ZnSAF9I=; b=Tj5BA6eN7NQ+5Tkvaui0KyiDsGiu/xlYM97kt7Bhp8wwWw7ex0azLItgxWogXDTQfk tsV2xSlEANwf1rpepoXcYTmMHHhWdyPOipcef3r8hgkXXgQ0dFzUD4/vQVcdH9wntI5y ok7HmGAgwsiFKRs8pT+Hqra2juk9uXub7lYGjSzcViuv8Lja/rB0LsNrbdfNTYVDcEKz cMzjzAJs5VDHv5RekUFz409T7ZttTKcYh0fnpFIv1bDzYC4VMvSvW7jfAvbvk6YE8SXg Ryha8LGt/m7seob8YfHS3l8UEMNfes/2Wu8nzB2bSSpgNZteDIOBgFvxOQQYMSfRIj1N AWGQ== X-Gm-Message-State: AOAM532hCzmdVJzs42NKvlKjeMhpUQlRuKqfsjpYr7t4aON4uMSkno+t ww/ieRk5br+Ec7SmHGOiOKZrUDt4+IRWNf2EYTQFFfUsuN9qIznuyM4HvJzr9hoyHLxswKH+65r dkUdzGLWP9UFmUFRp/A+IZA== X-Received: by 2002:a1c:f710:0:b0:394:1960:e8a1 with SMTP id v16-20020a1cf710000000b003941960e8a1mr24767673wmh.154.1654017767429; Tue, 31 May 2022 10:22:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzppqR6ksa1DhUhZeKbSzK9mxASPLVubwUd+1CySeqP9KRZSzPSTjjtLwG7mMQ+LrE7kXduhw== X-Received: by 2002:a1c:f710:0:b0:394:1960:e8a1 with SMTP id v16-20020a1cf710000000b003941960e8a1mr24767656wmh.154.1654017767178; Tue, 31 May 2022 10:22:47 -0700 (PDT) Received: from localhost (host109-152-215-36.range109-152.btcentralplus.com. [109.152.215.36]) by smtp.gmail.com with ESMTPSA id b6-20020adfd1c6000000b0020d0f111241sm14177610wrd.24.2022.05.31.10.22.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 May 2022 10:22:46 -0700 (PDT) From: Andrew Burgess To: John Baldwin , gdb-patches@sourceware.org Subject: Re: [PATCH 4/5] gdb: ensure the cast in gdbarch_tdep is valid In-Reply-To: <3ca3a276-fb2b-c000-ac0b-89bd8381b743@FreeBSD.org> References: <65f89b8748dfec7fd62681df2b1efcfa2c25026f.1654007211.git.aburgess@redhat.com> <3ca3a276-fb2b-c000-ac0b-89bd8381b743@FreeBSD.org> Date: Tue, 31 May 2022 18:22:46 +0100 Message-ID: <878rqhd3bt.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, 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 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 17:22:51 -0000 John Baldwin writes: > 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); I didn't do that originally so as to keep the "slower" code enclosed within the assert. If in some future world we added a mode in which we could compile out the assert, then the slower dynamic cast would be skipped. But we don't currently support compiling out asserts, so this was probably premature optimisation. I'll update the code as you suggest. Thanks, Andrew > } > > -- > John Baldwin