From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id DPtZIgBdgWefngkAWB0awg (envelope-from ) for ; Fri, 10 Jan 2025 12:46:40 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1736531200; bh=gxiu/LFbNfNeyFIYTIA8apipQval+ITmSW7KYeAYOl0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=xQQCuSfmSq8JLnpg1fnyVsAwq2WBKIbwVFbxtBj7FAQg+01+R3iwvShCKAFL/yfXN GMgTdZ4To6SCb+qghxdCKU0+54J0V1mYmU/FB2D5OOHv+hw4CMF6bvoKcr979uHDZR EP/+NTQV+eSG4VRfDvoXbM3xGPRKFT9omPYKFG0g= Received: by simark.ca (Postfix, from userid 112) id 7F0BB1E0C0; Fri, 10 Jan 2025 12:46:40 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable autolearn_force=no version=4.0.0 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=vr3HIkhj; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=Tw/flueD; dkim-atps=neutral Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id EBD841E05C for ; Fri, 10 Jan 2025 12:46:39 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8BFC23858432 for ; Fri, 10 Jan 2025 17:46:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8BFC23858432 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=vr3HIkhj; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=Tw/flueD Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 083613858CD9 for ; Fri, 10 Jan 2025 17:45:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 083613858CD9 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 083613858CD9 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736531152; cv=none; b=Us4MaPi/D3qZURw8yXDwSjzXfsD5VJ818mn5P1R5GzA4auInh4aJZ3TjLvP46UNJw6ThOClG+XIB270k1YUpiAJM9hOdlusokVX5youQrzI2NQ86Zh78YHf/uSwGV2e7AppuNmqPh2F7oe/Hwt+/s1X1bKIQMwjpJ6xyQD82oFU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736531152; c=relaxed/simple; bh=gxiu/LFbNfNeyFIYTIA8apipQval+ITmSW7KYeAYOl0=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=MFiMtK+enuRox4dhecIvv6Bo7ic2eVErE6JuRAGO0Zsz1Wqb8IZS1Tl5QiTcnAkEmfCCs+W1p00AgYanaDloSx7I23DWvSnelqkVzkF903uPc6mZxWVwkWG4gDXky1ZH44Obc60XpKLO3k7Q1a30WfIa5StevxJ162N4FctUXL4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 083613858CD9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1736531151; bh=gxiu/LFbNfNeyFIYTIA8apipQval+ITmSW7KYeAYOl0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=vr3HIkhjhVl1EKSG+h8P/rhrQROM4jD6VZgSA3ipwtPNVWX1zO+6ggr+1zCDRKsDT QL3AIVcA8pKDSZpy0WGanXKKCVBjGOtm338AVf30xTEjzsqeWYB70xr61hfydSaqj8 e3dG5wd/TFpAt0GmXClZDTwAqZoLbJ3ZhymMByrM= Received: by simark.ca (Postfix, from userid 112) id 936011E0F7; Fri, 10 Jan 2025 12:45:51 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1736531149; bh=gxiu/LFbNfNeyFIYTIA8apipQval+ITmSW7KYeAYOl0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Tw/flueDlNwE1PosQcWRkn5E54O+T2dyZmhojnhcr+XiMwLojFUQkFZeS/OUwiQW8 lgZhJEE2z9OK73mrj9WgAjyme/2uruIOq9kTtI83DTrqJseeD88yZDtkSPVJzA0Pv7 n0uKeYOh+2uWIJXts7+j1ZWOxKMd3Scns1jC4jSI= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (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 84C851E05C; Fri, 10 Jan 2025 12:45:49 -0500 (EST) Message-ID: <321e71e0-43de-4604-bb7e-34f6f64b83bf@simark.ca> Date: Fri, 10 Jan 2025 12:45:49 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding To: Thiago Jung Bauermann , gdb-patches@sourceware.org Cc: "Aktemur, Tankut Baris" , "Maciej W. Rozycki" References: <20250110164430.3376697-1-thiago.bauermann@linaro.org> <20250110164430.3376697-3-thiago.bauermann@linaro.org> Content-Language: en-US From: Simon Marchi In-Reply-To: <20250110164430.3376697-3-thiago.bauermann@linaro.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On 2025-01-10 11:44, Thiago Jung Bauermann wrote: > This allows checking the size of the given buffer. Changes > frame_register_unwind (), frame_unwind_register (), get_frame_register () > and deprecated_frame_register_read (). > > As pointed out by Baris, in the case of MIPS target code this is best > done by changing a couple of alloca-based buffers in > mips_read_fp_register_single and mips_print_fp_register to > gdb::byte_vector instances. > --- > gdb/amd64-windows-tdep.c | 4 ++-- > gdb/frame.c | 22 ++++++++++-------- > gdb/frame.h | 10 ++++----- > gdb/mips-tdep.c | 48 ++++++++++++++++++++++------------------ > 4 files changed, 47 insertions(+), 37 deletions(-) > > Changes from RFCv4 patch 2: > > - Allocated gdb::byte_vectors instead of making array_views in mips-tdep.c, > as suggested by Baris. This required changing raw_buffer in > mips_print_fp_register (the caller of mips_read_fp_register_single and > mips_read_fp_register_double) to a gdb::byte_vector as well, and also > changing the rare_buffer arguments to array views. > > - Don't use std::optional for the buffer argument in frame_register_unwind, > as suggested by Simon. > > - Changed the gdb_assert in frame_register_unwind to require the buffer > argument to have the same size as the register. > > diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c > index 9e255bb2d433..ca7b7d96bc22 100644 > --- a/gdb/amd64-windows-tdep.c > +++ b/gdb/amd64-windows-tdep.c > @@ -801,7 +801,7 @@ amd64_windows_frame_decode_insns (const frame_info_ptr &this_frame, > std::array buf; > int frreg = amd64_windows_w2gdb_regnum[frame_reg]; > > - get_frame_register (this_frame, frreg, buf.data ()); > + get_frame_register (this_frame, frreg, buf); > save_addr = extract_unsigned_integer (buf, byte_order); > > frame_debug_printf (" frame_reg=%s, val=%s", > @@ -1097,7 +1097,7 @@ amd64_windows_frame_cache (const frame_info_ptr &this_frame, void **this_cache) > > /* Get current PC and SP. */ > pc = get_frame_pc (this_frame); > - get_frame_register (this_frame, AMD64_RSP_REGNUM, buf.data ()); > + get_frame_register (this_frame, AMD64_RSP_REGNUM, buf); > cache->sp = extract_unsigned_integer (buf, byte_order); > cache->pc = pc; > > diff --git a/gdb/frame.c b/gdb/frame.c > index ba4a07179f64..b64f9067ab9e 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -1112,7 +1112,7 @@ frame_save_as_regcache (const frame_info_ptr &this_frame) > { > auto cooked_read = [this_frame] (int regnum, gdb::array_view buf) > { > - if (!deprecated_frame_register_read (this_frame, regnum, buf.data ())) > + if (!deprecated_frame_register_read (this_frame, regnum, buf)) > return REG_UNAVAILABLE; > else > return REG_VALID; > @@ -1177,7 +1177,8 @@ void > frame_register_unwind (const frame_info_ptr &next_frame, int regnum, > int *optimizedp, int *unavailablep, > enum lval_type *lvalp, CORE_ADDR *addrp, > - int *realnump, gdb_byte *bufferp) > + int *realnump, > + gdb::array_view buffer) > { > struct value *value; > There are some comments referring to `bufferp` that would need to be updated. IMO the line: /* gdb_assert (bufferp != NULL); */ ... can just be removed. > @@ -1202,13 +1203,15 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum, > else > *realnump = -1; > > - if (bufferp) > + if (!buffer.empty ()) > { > + gdb_assert (buffer.size () == value->type ()->length ()); Regarding this, I think I'm going to backtrack. I started to survey the callers (and the callers of callers) of this function, and found one case where this doesn't hold, that looks legitimate: - frame_register_unwind, called by - frame_unwind_register, called by - i386_unwind_pc i386_unwind_pc has an 8 bytes static array and asks to unwind the PC register. This function is used for both amd64 (8 bytes PC) and i386 (4 bytes PC). This function would now have to create a view of the right size, something like: static CORE_ADDR i386_unwind_pc (struct gdbarch *gdbarch, const frame_info_ptr &next_frame) { gdb_byte buf[8]; auto view = gdb::make_array_view (buf, builtin_type (gdbarch)->builtin_func_ptr->length ()); frame_unwind_register (next_frame, gdbarch_pc_regnum (gdbarch), view); return extract_typed_address (view.data (), builtin_type (gdbarch)->builtin_func_ptr); } Without doing changes like this all over the place, things will break. I'm not saying it can't be done, but it would require a lot of effort and careful review (and testing when possible). So, I think it would be safer for now to revert to what you had before, assert that the buffer is large enough for the value. If you agree, then this LGTM with those fixed. Approved-By: Simon Marchi Simon