From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id fhWdKAiAJml6kjAAWB0awg (envelope-from ) for ; Tue, 25 Nov 2025 23:20:24 -0500 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=CIkmkDBj; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 8E9691E048; Tue, 25 Nov 2025 23:20:24 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.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,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 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 11D561E048 for ; Tue, 25 Nov 2025 23:20:23 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5CE443858D35 for ; Wed, 26 Nov 2025 04:20:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5CE443858D35 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=CIkmkDBj Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id BEF673858C83 for ; Wed, 26 Nov 2025 04:19:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BEF673858C83 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org BEF673858C83 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::52f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1764130748; cv=none; b=geSfTCtybyZaiwSO2vtzRqQ8FPw90oiBb6Q1G0FDANjOXM8vFCx+SZhsiicPmmZkZ+2yeuEe6qk1POM90frJpNU368G8yQbt33TALi6uynIjMdbvqYPXdOxxUdYXP2L/cWvirNc8n2aL0aATPRQdjQW7l19hnWJAsbEmtGKZCUs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1764130748; c=relaxed/simple; bh=WP/WVpbYATd+tvllnFSdaoxWfqP2RxSsuT3c2wUrJvI=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=wZsee4BBCt+Cs1yt8pLZ0PfyjFYDRKhAz0Fhs4atRmYZ8rcE2+QBrz5uXQazrFKj7hAKI/UfU9QvSOxj5BziKOgOXwLTPGa6B0DOlCGwYtAfUsUHGoZD92b3yCriJjIrWdtFFgVHnxV3Gcb5n5i9EVGkjiGm/dhyM49mjjZPzdE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BEF673858C83 Received: by mail-pg1-x52f.google.com with SMTP id 41be03b00d2f7-b553412a19bso5548460a12.1 for ; Tue, 25 Nov 2025 20:19:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1764130746; x=1764735546; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=1/F2b4pbZ/UN4+pjUIpSsA6/SUAhlSjV7LTMdGTYsw8=; b=CIkmkDBjrqzKWqSa5lRNRsRpJxbsBeHQdbyf0qkC4+fZoFhxVY2dvP/4bV9s88A6Pq pgoPXRPExBvA9XNmjm+9jMyVVR3fpHlHQWe0Bs8KorDUM0+YJKPqRBrJIuDHAHgn3uWY M9DKI/2UJfqdycahhKuKYYARqx10kS2F96uVXLcI6ByKQMHL4YriCrIbVOcl9Jin8zay ecbQd4+KzC/zjnpYWxbCKK4L9qsV6j0+lc516eFjmQkdlbK5RqdnFCmkj3b8PI/9sIaZ PVI48v2iJsfhQwm+xC4rFmVqbRzUU8i+xN6+wd2mgFaA1cCPhKIpotNsi9FeUUN44tvW plFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764130746; x=1764735546; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=1/F2b4pbZ/UN4+pjUIpSsA6/SUAhlSjV7LTMdGTYsw8=; b=tE/oPxogEkJ3AF9TeS4hx4DAy+COfk+oSdRXdZYp+hoqSZYRyGq8gtpGEqRwJW8NTb s7lIMj8N1KqBCGdcEG9mY4m9cVEAWaJH1ePaWFbp74xV+L55z1ZgSMMCuf2xQHs1mLeh 18swI13KR1yEBgHWoOIEvAqI5LJBTEFoFR/KUkzhldCkhPqQilw1OtE6Uy9BnQAJFS9f 3Lf+M68crUmzRHCOdG3MHwnfSHw1TXHXs3Kjjy/RzQWZgn0huqaP4fH4L/zOu/mqu7ST UpPCEUttiqJ9aw4pX3PjE1NkfNgqArebKA/RwIq2xZSxn/ny/V0ALNsNFGHNUJui8IEQ BDEw== X-Gm-Message-State: AOJu0YxXCY23BuzjnxvCwX973XEbqtfGIjq7gLGCA9hMyAKrs06IbEEd vFTXMn3gNasUoWuLERZmlv9FQXlq1yf2glhRZZO1AzO2rN4gn2DsOj4sr/Z8SCycZMs= X-Gm-Gg: ASbGncu+O3DqADZzSJtWC968Iu/UZUSYlHy9BTyuwKNuHyGT/mt/H+2IiD4NuqV71z4 xN4A44Yu12c2omzek8ZVEXqqkmnov6E72Ek3ZR8b6sr5rZgbKUVpStTzelnTMzXZ8MXeyamcaKr 39oN72tf6Mhx5qNTJiTCoHUdU0B4bAM1xEb+21USxFAs51QGP75R/M2+6ewzSUNzHzQ793bXms/ q+TCZCLL973sCuKjwp60R3Xkv1lphrLylwGXct0f6viicS303Bg1YORTDsHIb29GeIL7E98+jsP rjqO5UBFk7uNc+sSx1m+mKL+wfJlCbFPl3Snp6TH97G/OXk8xYj9WU2e2oBhBDhPh8EKbazC97c Vw89CMMzo+spTW9ioBNS0HvZ1KPzhIFOxi+C5/1TCjaPbr6QdV7Wo9tYn39eNAPWcP8AU8ZGuFr SQvjrKcLVxP/gulk0BdXWZ X-Google-Smtp-Source: AGHT+IGVC4oF+jC0RVuMhUSG1N0Nqoh/2YvUv2XbCPqPxL5VDuKjVN6E3XwkmT5yX21xrs+cctETkg== X-Received: by 2002:a05:7300:fb8a:b0:2a4:3593:6466 with SMTP id 5a478bee46e88-2a719297727mr13918677eec.22.1764130745516; Tue, 25 Nov 2025 20:19:05 -0800 (PST) Received: from localhost ([2804:14d:7e39:8083:2b62:e85c:5936:d298]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-2a93c5562b2sm18439993eec.3.2025.11.25.20.19.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Nov 2025 20:19:05 -0800 (PST) From: Thiago Jung Bauermann To: "Schimpe, Christina" Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH 1/9] gdb: Generalize handling of the shadow stack pointer. In-Reply-To: (Christina Schimpe's message of "Mon, 17 Nov 2025 11:18:33 +0000") References: <20250923111842.4091694-1-christina.schimpe@intel.com> <20250923111842.4091694-2-christina.schimpe@intel.com> <87bjlnvouh.fsf@linaro.org> User-Agent: mu4e 1.12.13; emacs 30.2 Date: Wed, 26 Nov 2025 01:19:02 -0300 Message-ID: <87a509qvd5.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 Hello Christina, "Schimpe, Christina" writes: > Thanks a lot for this detailed review! > I applied most of your comments, please find my feedback to your review b= elow. You're welcome! >> -----Original Message----- >> From: Thiago Jung Bauermann >> Sent: Friday, 31 October 2025 02:32 >> To: Schimpe, Christina >> Cc: gdb-patches@sourceware.org >> Subject: Re: [PATCH 1/9] gdb: Generalize handling of the shadow stack >> pointer. >>=20 >> Christina Schimpe writes: >>=20 >> > -static value * >> > -amd64_linux_dwarf2_prev_ssp (const frame_info_ptr &this_frame, >> > - void **this_cache, int regnum) >> > -{ >> > - value *v =3D frame_unwind_got_register (this_frame, regnum, regnum); >> > - gdb_assert (v !=3D nullptr); >> > - >> > - gdbarch *gdbarch =3D get_frame_arch (this_frame); >> > - >> > - if (v->entirely_available () && !v->optimized_out ()) >> > - { >> > - int size =3D register_size (gdbarch, regnum); >> > - bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); >> > - CORE_ADDR ssp =3D extract_unsigned_integer (v->contents_all ().= data (), >> > - size, byte_order); >> > - >> > - /* Using /proc/PID/smaps we can only check if the current shadow >> > - stack pointer SSP points to shadow stack memory. Only if this is >> > - the case a valid previous shadow stack pointer can be >> > - calculated. */ >> > - std::pair range; >> > - if (linux_address_in_shadow_stack_mem_range (ssp, &range)) >> > - { >> > - /* The shadow stack grows downwards. To compute the previous >> > - shadow stack pointer, we need to increment SSP. */ >> > - CORE_ADDR new_ssp >> > - =3D ssp + amd64_linux_shadow_stack_element_size_aligned (gdbarch= ); >> > - >> > - /* There can be scenarios where we have a shadow stack pointer >> > - but the shadow stack is empty, as no call instruction has >> > - been executed yet. If NEW_SSP points to the end of or before >> > - (<=3D) the current shadow stack memory range we consider >> > - NEW_SSP as valid (but empty). */ >> > - if (new_ssp <=3D range.second) >>=20 >> IIUC, the '<=3D' comparison above isn't preserved by this patch. This fu= nction is >> replaced by dwarf2_prev_ssp, which uses >> gdbarch_address_in_shadow_stack_memory_range for this if condition, >> whose comparison in find_addr_mem_range is: >>=20 >> bool addr_in_mem_range >> =3D (addr >=3D map.start_address && addr < map.end_address); >>=20 >> Is this intended? > > Arg, thanks for catching that! > > I think I missed that because I introduced a typo/bug in the call > > || gdbarch_address_in_shadow_stack_memory_range (gdbarch, > ssp, > &range)) > > which made the unwinding work properly in case of amd64. > However, the proper fix should be to pass new_ssp to gdbarch_address_in_s= hadow_stack_memory_range > instead, and to implement gdbarch_top_addr_empty_shadow_stack also for am= d64. > > Does that make sense? Yes, I agree. >> > - return frame_unwind_got_address (this_frame, regnum, new_ssp); >> > - } >> > - } >> > - >> > - /* Return a value which is marked as unavailable in case we could n= ot >> > - calculate a valid previous shadow stack pointer. */ >> > - value *retval >> > - =3D value::allocate_register (get_next_frame_sentinel_okay (this_= frame), >> > - regnum, register_type (gdbarch, regnum)); >> > - retval->mark_bytes_unavailable (0, retval->type ()->length ()); >> > - return retval; >> > -} >>=20 >> >>=20 >> > diff --git a/gdb/shadow-stack.c b/gdb/shadow-stack.c new file mode >> > 100644 index 00000000000..d153d5fc846 >> > --- /dev/null >> > +++ b/gdb/shadow-stack.c >> > @@ -0,0 +1,167 @@ >> > +/* Manage a shadow stack pointer for GDB, the GNU debugger. >> > + >> > + Copyright (C) 2024-2025 Free Software Foundation, Inc. >>=20 >> Should this really start at 2024? According to Andrew Burgess=C2=B9: > > Yes, 2024 is correct in this case since our gdb-oneapi supported bt shado= w since 2024.=20 Ah, right. Thanks for clarifying. >> > +enum class ssp_update_direction >> > +{ >> > + /* Update ssp towards the bottom of the shadow stack. */ >> > + bottom =3D 0, >> > + >> > + /* Update ssp towards the top of the shadow stack. */ >> > + top >> > +}; >>=20 >> I find the bottom/top nomenclature confusing, especially because it's >> supposed to mean the same thing whether the stack grows up or down. In >> my mind, if the stack grow down then top means "oldest element", but if = the >> stack grows up, then top means "newest element". >> But in this patch it seems that top means "newest element" regardless of= the >> direction of stack growth. > > Yes, that was my understanding. So independent in which direction a shado= w stack > grows based on the architecture/OS, top always means newest element. But= I think > it is not a problem to take one of your suggestions. Thank you. In my view it also matches the nomenclature in frame.h, which also doesn't use vertical concepts. E.g., /* Given a FRAME, return the next (more inner, younger) or previous (more outer, older) frame. */ extern frame_info_ptr get_prev_frame (const frame_info_ptr &); extern frame_info_ptr get_next_frame (const frame_info_ptr &); >> I would suggest changing the enum names above to something that's not >> related to the vertical axis, so that their meaning will be clear regard= less of >> which direction the stack grows. A few suggestions: >> shrink/grow, older/younger, outer/inner. > > I'd take outer/inner and describe it as follows: > > enum class ssp_update_direction > { > /* Update ssp towards the oldest (outermost) element of the shadow > stack. */ > outer =3D 0, > > /* Update ssp towards the most recent (innermost) element of the > shadow stack. */ > inner > }; > > Is that understandable ? Yes, thanks for making the change. >> > +/* See shadow-stack.h. */ >> > + >> > +void shadow_stack_push (gdbarch *gdbarch, regcache *regcache, >>=20 >> There's no need for a gdbarch argument. You can get it from the regcache. > > Fixed. > >> > + const CORE_ADDR new_addr) >> > +{ >> > + if (!gdbarch_address_in_shadow_stack_memory_range_p (gdbarch) >> > + || gdbarch_ssp_regnum (gdbarch) =3D=3D -1) >> > + return; >> > + >> > + bool shadow_stack_enabled; >> > + std::optional ssp >> > + =3D gdbarch_get_shadow_stack_pointer (gdbarch, regcache, >> > + shadow_stack_enabled); >> > + if (!ssp.has_value () || !shadow_stack_enabled) >> > + return; >> > + >> > + const CORE_ADDR new_ssp >> > + =3D update_shadow_stack_pointer (gdbarch, *ssp, >> > + ssp_update_direction::top); >> > + >> > + /* If NEW_SSP does not point to shadow stack memory, we assume the = stack >> > + is full. */ >> > + std::pair range; >> > + if (!gdbarch_address_in_shadow_stack_memory_range (gdbarch, >> > + new_ssp, >> > + &range)) >>=20 >> Range isn't really needed by this function. I suggest changing >> gdbarch_address_in_shadow_stack_memory_range to allow for it to be >> nullptr and then pass nullptr here. > > I agree, fixed. > >> Also, the line above fits in 80 columns and doesn't need to be broken, e= ven if >> "&range" is changed to "nullptr". > > It is more than 80 columns for me. Hm. When I edited it here and changed "&range" to "nullptr" the line ended exactly at column 80. Which is arguably not ideal, so I don't mind either way. >> > + error (_("No space left on the shadow stack.")); >> > + >> > + /* On x86 there can be a shadow stack token at bit 63. For x32, t= he >> > + address size is only 32 bit. Always write back the full 8 bytes= to >> > + include the shadow stack token. */ >>=20 >> s/8 bytes/element size/ > > Fixed. > >>=20 >> > + const int element_size >> > + =3D gdbarch_shadow_stack_element_size_aligned (gdbarch); >> > + >> > + const bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); >> > + >> > + write_memory_unsigned_integer (new_ssp, element_size, byte_order, >> > + (ULONGEST) new_addr); >> > + >> > + regcache_raw_write_unsigned (regcache, >> > + gdbarch_ssp_regnum (gdbarch), >> > + new_ssp); >>=20 >> The line above fits in 80 columns and doesn't need to be broken. > > I count 81 columns and there is also a soft limit of 74 characters: > > https://sourceware.org/legacy-ml/gdb-patches/2014-01/msg00216.html Ah, I wasn't aware of the soft limit. Thanks for pointing it out. > So I'll keep it as is, if that's fine for you. Yes, of course. >> > diff --git a/gdb/shadow-stack.h b/gdb/shadow-stack.h new file mode >> > 100644 index 00000000000..5c3ba80974e >> > --- /dev/null >> > +++ b/gdb/shadow-stack.h >> > @@ -0,0 +1,39 @@ >> > +/* Definitions to manage a shadow stack pointer for GDB, the GNU debu= gger. >> > + >> > + Copyright (C) 2024-2025 Free Software Foundation, Inc. >> > + >> > + This file is part of GDB. >> > + >> > + This program is free software; you can redistribute it and/or >> > + modify >> > + >> > + it under the terms of the GNU General Public License as published = by >> > + the Free Software Foundation; either version 3 of the License, or >> > + (at your option) any later version. >> > + >> > + This program is distributed in the hope that it will be useful, >> > + but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > + GNU General Public License for more details. >> > + >> > + You should have received a copy of the GNU General Public License >> > + along with this program. If not, see >> > + . */ >> > + >> > +#ifndef GDB_SHADOW_STACK_H >> > +#define GDB_SHADOW_STACK_H >> > + >> > +/* If shadow stack is enabled, push the address NEW_ADDR on the shadow >> > + stack and update the shadow stack pointer accordingly. */ >> > + >> > +void shadow_stack_push (gdbarch *gdbarch, regcache *regcache, >>=20 >> Recently, the project has been trying to make the header files contain a= ll the >> headers and definitions that they need, for the benefit of IDE and langu= age >> server users, so that these tools don't emit spurious errors when showin= g a >> header file. > > Ah, ok I wasn't aware. Do you have a link for that ? I think I cannot fol= low 100 %. There was a discussion about it in this thread: https://sourceware.org/pipermail/gdb-patches/2024-February/206632.html It resulted in this patch: https://inbox.sourceware.org/gdb-patches/20240326190806.89541-4-simon.march= i@efficios.com/ And it's also in the wiki=C2=B2: A .c, .cc or .h file should directly include the .h file of every declaration and/or definition it directly refers to. Exception: Do not include defs.h, server.h, common-defs.h directly. --=20 Thiago =C2=B2 https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#I= nclude_Files