From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 6AD7A3858D1E for ; Mon, 6 May 2024 14:44:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6AD7A3858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linux.ibm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6AD7A3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.156.1 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715006677; cv=none; b=ps1cY+bfSoC8sq9W7dQ5NuGWjCnLlOpjDV/JzF6yp95W5sr2zz8kxhDG6ei6gpvUf35ItPQ4B5ipipfUzyw9djDdTTpoEqPFu9Nqfn9DDprarNbrgOeepGWQWGt8mxTG9+jiHyF2gnFeP1WGRmmUF9cF+XgFAamSWVM9i5mnW5k= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715006677; c=relaxed/simple; bh=v9dc9kDSniGDcdcs2WlRhhrHXosZPh+wC+dcT2QIcAg=; h=DKIM-Signature:Message-ID:Date:Subject:From:To:MIME-Version; b=Ymo3MDRW1ifCOrsIsHr0hO8P2+PWsBLJidCJKBZZZmRCjz/jZQjEMxuU8KV5PLO/O2ISbk+TSn1c3SiKaGP0aWr1qoNN3FYZIZtLs98AIdwZXjbxoe917iGzJ2wlEVSeABwQsybVnhz9kZO7R5gbGtILVJDRuam9BjrTVemmpKQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0353726.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 446EhjZ7010702; Mon, 6 May 2024 14:44:32 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : subject : from : to : cc : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pp1; bh=rO3gnFPOEw5fWdVg3C/OXWt3jgmxa1T1SnmxH9YOPSE=; b=dS0ososQQbgmg+fLxlSenEeM0gmdX2qhGmy9SjPakCEdAaoGu7oIQTJH2xWNLibdRnpB Kvd6Jzxrp2QjvLCoLo+z3CQmNnYUdHcYCcCNuY5U6zM3EVKkKkzOZw+W4Z1r7kvSDPP2 c3wC38vjkaYhgzlDjp9x31tgJsN36jCyO9RKQ06q93G2NBsvpded9VZTT36giRvNkelT XPA7C+6rPdWcldK202XFiiqGxLoUY4Uxoj/hqzNNVt8Hy52pAh4olmE9t5J8SQ7uaUQY QVVcezqzswnibW624a/RjJVRRFKSh+JM/Ymae3IXhpsaGIzUL9TqGKJbMP9OEJgl5KOJ qg== Received: from ppma12.dal12v.mail.ibm.com (dc.9e.1632.ip4.static.sl-reverse.com [50.22.158.220]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3xxyca0bt4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 06 May 2024 14:44:31 +0000 Received: from pps.filterd (ppma12.dal12v.mail.ibm.com [127.0.0.1]) by ppma12.dal12v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 446BfBkS030855; Mon, 6 May 2024 14:39:30 GMT Received: from smtprelay04.fra02v.mail.ibm.com ([9.218.2.228]) by ppma12.dal12v.mail.ibm.com (PPS) with ESMTPS id 3xwybtrnme-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 06 May 2024 14:39:30 +0000 Received: from smtpav05.fra02v.mail.ibm.com (smtpav05.fra02v.mail.ibm.com [10.20.54.104]) by smtprelay04.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 446EdPSB17236238 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 6 May 2024 14:39:27 GMT Received: from smtpav05.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9298320043; Mon, 6 May 2024 14:39:25 +0000 (GMT) Received: from smtpav05.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 1D08020040; Mon, 6 May 2024 14:39:25 +0000 (GMT) Received: from [9.171.78.172] (unknown [9.171.78.172]) by smtpav05.fra02v.mail.ibm.com (Postfix) with ESMTP; Mon, 6 May 2024 14:39:25 +0000 (GMT) Message-ID: <8881ba96-d1f4-41ca-a645-138cf6647ef1@linux.ibm.com> Date: Mon, 6 May 2024 16:39:24 +0200 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset From: Jens Remus To: Indu Bhagat , binutils@sourceware.org Cc: Andreas Krebbel References: <20240412144718.4191286-1-jremus@linux.ibm.com> <20240412144718.4191286-16-jremus@linux.ibm.com> <6445f04c-1cb8-4b44-988f-b57f30cb4f69@linux.ibm.com> Content-Language: en-US Organization: IBM Deutschland Research & Development GmbH In-Reply-To: <6445f04c-1cb8-4b44-988f-b57f30cb4f69@linux.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: nSordNiVCQrzRtsAdk0-SOXwws3dOTU6 X-Proofpoint-GUID: nSordNiVCQrzRtsAdk0-SOXwws3dOTU6 Content-Transfer-Encoding: 8bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26 definitions=2024-05-06_09,2024-05-06_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 bulkscore=0 adultscore=0 mlxlogscore=999 clxscore=1015 priorityscore=1501 impostorscore=0 lowpriorityscore=0 spamscore=0 phishscore=0 suspectscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000 definitions=main-2405060101 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP 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: Am 06.05.2024 um 13:41 schrieb Jens Remus: > Am 04.05.2024 um 02:22 schrieb Indu Bhagat: >> On 5/3/24 09:40, Jens Remus wrote: >>> Am 18.04.2024 um 22:38 schrieb Indu Bhagat: >>>> On 4/12/24 07:47, Jens Remus wrote: >>>>> If an architecture uses SFrame return-address (RA) tracking it must >>>>> specify the fixed RA offset as invalid. Otherwise, if an architecture >>>>> does not use RA tracking, it must specify a valid fixed RA offset. >>>>> >>>>> gas/ >>>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed >>>>>     RA offset. >>>>> >>>>> Signed-off-by: Jens Remus >>>>> --- >>>>> >>>>> Notes (jremus): >>>>>      Changes v2 -> v3: >>>>>      - New patch. >>>>>      This could be made dependent on ENABLE_CHECKING (configure option >>>>>      --enable-checking). >>>>> >>>>>   gas/gen-sframe.c | 12 ++++++++++++ >>>>>   1 file changed, 12 insertions(+) >>>>> >>>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c >>>>> index ca6565b0e45e..7e815f9603ef 100644 >>>>> --- a/gas/gen-sframe.c >>>>> +++ b/gas/gen-sframe.c >>>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg) >>>>>     /* Setup the version specific access functions.  */ >>>>>     sframe_set_version (SFRAME_VERSION_2); >>>>> +#ifdef SFRAME_FRE_RA_TRACKING >>>>> +  if (sframe_ra_tracking_p ()) >>>>> +    /* With RA tracking the fixed RA offset must be invalid.  */ >>>>> +    gas_assert (sframe_cfa_ra_offset () == >>>>> SFRAME_CFA_FIXED_RA_INVALID); >>>>> +  else >>>>> +    /* Without RA tracking the fixed RA offset may not be >>>>> invalid.  */ >>>>> +    gas_assert (sframe_cfa_ra_offset () != >>>>> SFRAME_CFA_FIXED_RA_INVALID); >>>>> +#else >>>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */ >>>>> +  gas_assert (sframe_cfa_ra_offset () != >>>>> SFRAME_CFA_FIXED_RA_INVALID); >>>>> +#endif >>>>> + >>>> >>>> I am not sure if the detailed checks are worth it here (simply >>>> because of code patterns that follow). >>> >>> I agree, provided the checks are performed elsewhere as you suggest. >>> >>> My intention was to have checks that assist with getting SFrame >>> support for another architecture implemented correctly, without >>> having to chase subtle issues. >>> >>>> >>>> We use the sframe_cfa_ra_offset () function later and only in >>>> output_sframe_internal () (shown below).  How about we simply put an >>>> assert there (and get rid of the proposed thunk above): >>>> >>>> #ifdef sframe_ra_tracking_p >>>>    if (!sframe_ra_tracking_p ()) >>> >>> See below. >>> >>>>      { >>>>        fixed_ra_offset = sframe_cfa_ra_offset (); >>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID); >>> >>> That is clever and accounts for one potential implementation issue! >>> >>>>      } >>>> #endif >>>>    out_one (fixed_ra_offset); >>>> >>>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in >>>> output_sframe_internal (). >>> >>> Above logic requires sframe_ra_tracking_p to be defined by an >>> architecture that is not using RA tracking. Not defining >>> sframe_ra_tracking_p would result in fixed_ra_offset being >>> unexpectedly initialized to SFRAME_CFA_FIXED_RA_INVALID instead of >>> being set to sframe_cfa_ra_offset(). >>> >>> All checks but this do test SFRAME_FRE_RA_TRACKING first, which >>> ensures both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, >>> and then the predicate sframe_ra_tracking_p to determine whether RA >>> tracking is used. >>> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns >>> true, then RA tracking is used. >>> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if >>> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is >>> not used. >>> >>> What about making the following change to make all RA tracking tests >>> consistent depend on SFRAME_FRE_RA_TRACKING? >>> >>> #ifdef SFRAME_FRE_RA_TRACKING >>>    if (!sframe_ra_tracking_p ()) >>> #endif >>>      { >>>        fixed_ra_offset = sframe_cfa_ra_offset (); >>>        /* Without RA tracking the fixed RA offset may not be >>> invalid.  */ >>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID); >>>      } >>>    out_one (fixed_ra_offset); >>> >> >> Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more >> appropriate. > > Included this in the previous patch in this series. This breaks x86-64, as gas/config/tc-i386.h does not define SFRAME_CFA_RA_REG. Either this specific check must stay, possibly with an additional comment: /* Offset for the return address from CFA is fixed for some ABIs (e.g., AMD64), output a SFRAME_CFA_FIXED_RA_INVALID otherwise. NOTE: sframe_ra_tracking_p may be defined without SFRAME_CFA_RA_REG (e.g., AMD64), so that SFRAME_FRE_RA_TRACKING won't be defined. */ #ifdef sframe_ra_tracking_p if (!sframe_ra_tracking_p ()) { fixed_ra_offset = sframe_cfa_ra_offset (); /* Without RA tracking the fixed RA offset may not be invalid. */ gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID); } #endif out_one (fixed_ra_offset); or gas/config/tc-i386.h needs to define SFRAME_CFA_RA_REG, for instance as follows: #define SFRAME_CFA_RA_REG DWARF2_DEFAULT_RETURN_COLUMN Since an architecture may not have a RA register I wonder whether keeping the existing logic as is would be better. What is your opinion? > >> >> But, I think calling the sframe_cfa_ra_offset () out of >> SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends >> which opt in for SFrame define these vars/functions. (The cross build >> will likely pass because of the way code is written, but I think you >> get the idea). >> >> I would do: >> >> #ifdef SFRAME_FRE_RA_TRACKING >>     if (!sframe_ra_tracking_p ()) >>       { >>         fixed_ra_offset = sframe_cfa_ra_offset (); >>         /* Without RA tracking the fixed RA offset may not be >> invalid.  */ >>         gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID); >>       } >> #endif >>    out_one (fixed_ra_offset); > > Good catch! I did not consider that SFrame may not be implemented by an > architecture at all. > >> >> >>> What would still not be checked is the implementation error to define >>> sframe_ra_tracking_p and have it return true without also defining >>> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used. >>> >>> Would it therefore make sense to add the following? >>> >>> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG) >>>    gas_assert (!sframe_ra_tracking_p ()) >>> #endif >>> >>> Also when using RA tracking an architecture should implement >>> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID. >>> >>> Would it therefore make sense to add the following? >>> >>> #ifdef SFRAME_FRE_RA_TRACKING >>>    if (sframe_ra_tracking_p ()) >>>      gas_assert (sframe_cfa_ra_offset () == >>> SFRAME_CFA_FIXED_RA_INVALID); >>> #endif >>> >> >> All these checks are around guarding against implementation errors, >> opinions may vary. If you feel these add value, then it makes sense to >> add them. >> >> (That said, I am thinking the name sframe_cfa_ra_offset is confusing; >> perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it >> and may be include this in my list of sframe-next patches.) > > I'll leave it up to you then. > >> >>>> >>>>>     /* Process all fdes and create SFrame stack trace information.  */ >>>>>     create_sframe_all (); >>>> >>> >>> Thanks and regards, >>> Jens >> > > Regards, > Jens Thanks and regards, Jens -- Jens Remus Linux on Z Development (D3303) and z/VSE Support +49-7031-16-1128 Office jremus@de.ibm.com IBM IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294 IBM Data Privacy Statement: https://www.ibm.com/privacy/