From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 7EBB23858D1E for ; Mon, 6 May 2024 11:42:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7EBB23858D1E 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 7EBB23858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.158.5 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714995729; cv=none; b=n5f+0ZJFF1ym8aE1Uir51hgDSCsHKHWH0vfjkaojdI/IkEL7H60OJKGvw3WWzgcpS47N4m06CFBMLdF0c5xYl0M7RUmpkupjpOBpHe4rBRzwN3c033bj4vW+xtrXUqcdIR+uHa2Utc81FH7e1IQqZBKJlomyMmxWNExr+yeDJlk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714995729; c=relaxed/simple; bh=smHYFrwUoo6j+6UaPCFr4/Wq84JrLXVUZWl5JqX9sQ4=; h=DKIM-Signature:Message-ID:Date:Subject:To:From:MIME-Version; b=fIiWmPzQF7HSCWHfzoFseV1QTvfXZJGl3TrzJO+zSROkhDfUHIKf9cDYH7WnFITPeryKgsU0FR396m7yQGt8cQx3DCwUKuCQtFFZDiJaklZGTtpxKA+uKj/qdocv+RhtPQnVJwycszQHnqCGodqfxzY/tVf3m0AS5pYR3Hq9v5U= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0353723.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 446BXg7Y017685; Mon, 6 May 2024 11:42:04 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pp1; bh=ALxP0niV+CuOx4dt2G9PUg1tBICF9PL2vTAJ+9LuYkA=; b=lrShrK36HIRHLJoq8fJ0+7NHxeCvBP8DCXDEKEEFwBk2wNt2GqP8uNqKlLxhE0gxKr5h kAxG7bgM18o7i1wS724fzJ3+6CFPzrIAF1nPoTKoe2HAuWK3OsOy24/y4fPq1/g+HESl tXPFjyTnyy0TMtbt9dJJwPHKJvIPCieae+bj6JMayK8Qj94097aUwnE55j7MXNYnCgXq 9BDuxlSq2YY9jDETFzcy0WesTq3WIzzyY4mngHVKjX1wY7RXb2F1Y0xm7jzXtpasggWF t+JMYIWxhHlV4H7SoaQhldSVO0pMEBWJ9UbedcKaOqCv4COIq6imjzHO2Pyrei3bFtfp VQ== Received: from ppma13.dal12v.mail.ibm.com (dd.9e.1632.ip4.static.sl-reverse.com [50.22.158.221]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3xxxebg0rn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 06 May 2024 11:42:04 +0000 Received: from pps.filterd (ppma13.dal12v.mail.ibm.com [127.0.0.1]) by ppma13.dal12v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 446A4oit022515; Mon, 6 May 2024 11:42:03 GMT Received: from smtprelay07.fra02v.mail.ibm.com ([9.218.2.229]) by ppma13.dal12v.mail.ibm.com (PPS) with ESMTPS id 3xx1jkqavj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 06 May 2024 11:42:03 +0000 Received: from smtpav05.fra02v.mail.ibm.com (smtpav05.fra02v.mail.ibm.com [10.20.54.104]) by smtprelay07.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 446BfvGF53019064 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 6 May 2024 11:41:59 GMT Received: from smtpav05.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B262E20040; Mon, 6 May 2024 11:41:57 +0000 (GMT) Received: from smtpav05.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 607032004E; Mon, 6 May 2024 11:41:57 +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 11:41:57 +0000 (GMT) Message-ID: <6445f04c-1cb8-4b44-988f-b57f30cb4f69@linux.ibm.com> Date: Mon, 6 May 2024 13:41:56 +0200 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset To: Indu Bhagat , binutils@sourceware.org Cc: Andreas Krebbel References: <20240412144718.4191286-1-jremus@linux.ibm.com> <20240412144718.4191286-16-jremus@linux.ibm.com> From: Jens Remus Content-Language: en-US Organization: IBM Deutschland Research & Development GmbH In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: inM1hdDAjzoxz8LuLO4BWxWgYTAUkrXq X-Proofpoint-GUID: inM1hdDAjzoxz8LuLO4BWxWgYTAUkrXq 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.1011,Hydra:6.0.650,FMLib:17.11.176.26 definitions=2024-05-06_07,2024-05-06_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 priorityscore=1501 adultscore=0 spamscore=0 lowpriorityscore=0 phishscore=0 impostorscore=0 malwarescore=0 bulkscore=0 clxscore=1015 mlxlogscore=999 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000 definitions=main-2405060080 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 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. > > 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 -- 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/