From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) by sourceware.org (Postfix) with ESMTPS id 153EF3858D1E for ; Wed, 20 Sep 2023 17:44:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 153EF3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=foss.st.com Received: from pps.filterd (m0369458.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 38KBEiRG004702; Wed, 20 Sep 2023 19:44:14 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h= message-id:date:mime-version:subject:from:to:cc:references :in-reply-to:content-type:content-transfer-encoding; s= selector1; bh=xZtYyckBuJwvKPzvIigAXGkKccbHgxL403Thd091yCc=; b=Hw 0bkaqa/gdTNPTtQOkaejVfDEqnOAqQU/ovWcKO3Te1mY4yiDrV9QgLcSnHYbi70q smSBoi1dLBxe4Tc/uAintMFfP5dgN+gKFND78/+C2SNSQ3iMEGKMHMHY53Pnbkmr NB2O/y4f16bUe+r8LC6EpAziQjUgyncy7KwjxZiKo5lmXM7lWX2Ha6bWXnQpA3Ql 2eSA2GWgB6UnUDIPr5inaxyNNQr+grlv0gDpo7CvzQtNnZTpfJUEw/00vTvK5Kmd EYtnm73xJMdQZmkk+AlxxADJY1Tfsdy0JS7U6a4UwoNSDia4fPHT5G2cMf7WEA75 VY4d1iOfnj5qt1cY5KMA== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3t5nx0pwnm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 20 Sep 2023 19:44:14 +0200 (MEST) Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 4D13C100058; Wed, 20 Sep 2023 19:44:13 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 4079A2781CE; Wed, 20 Sep 2023 19:44:13 +0200 (CEST) Received: from [10.74.18.1] (10.74.18.1) by SHFDAG1NODE3.st.com (10.75.129.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Wed, 20 Sep 2023 19:44:12 +0200 Message-ID: <3866cb95-4dec-a2d8-caf2-c70b89d7b27d@foss.st.com> Date: Wed, 20 Sep 2023 19:44:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] libctf: ctf_member_next needs to return (ssize_t)-1 on error Content-Language: en-US From: Torbjorn SVENSSON To: Nick Alcock CC: , , Yvan ROUX References: <878r9ba2sf.fsf@esperi.org.uk> <20230913095727.1420654-1-torbjorn.svensson@foss.st.com> <87a5tp2a3n.fsf@esperi.org.uk> <657dadf8-4b7b-67e6-9de2-9a1cdb79f081@foss.st.com> In-Reply-To: <657dadf8-4b7b-67e6-9de2-9a1cdb79f081@foss.st.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.74.18.1] X-ClientProxiedBy: EQNCAS1NODE3.st.com (10.75.129.80) To SHFDAG1NODE3.st.com (10.75.129.71) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.601,FMLib:17.11.176.26 definitions=2023-09-20_08,2023-09-20_01,2023-05-22_02 X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE,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: Ping? On 2023-09-13 22:20, Torbjorn SVENSSON wrote: > > > On 2023-09-13 20:37, Nick Alcock wrote: >> On 13 Sep 2023, Torbjörn SVENSSON verbalised: >> >>> v1 -> v2: >>> Changed all functions with signed interger return type to return -1 >>> based on >>> comment from Alan. >>> >>> v2 -> v3: >>> Added ctf_set_errno_signed function to return a signed -1 value based on >>> comment from Nick. >>> >>> Ok for trunk? >> >> If this touches exactly those functions that return int, and fixes the >> reported bug, it's good as far as I'm concerned, except for a couple of >> possible comment improvements: > > I've verified the calls by building binutils (with the configure flags > mentioned in my last mail) with CFLAGS="-Wsign-conversion -Wconversion" > and looking for any warnings related to ctf_set_errno. After applying > this patch, there were no warnings left. > >>> +/* Store the specified error code into the CTF dict, and then return -1 >>> +   (CTF_ERR) for the benefit of the caller. */ >> >> It's not CTF_ERR in this case, it's just -1. Perhaps: > > True, but why is then ctf_set_errno returning CTF_ERR? > I somehow want to make it obvious that it's not wrong and that it should > *never* be CTF_ERR in the signed function or the problem would reappear. > > The other possibility is to do the inverse, meaning that the > ctf_set_errno function is returning an integer (-1) and that there is a > function ctf_set_errno_unsigned that is calling the ctf_set_errno > function but casting the returned value to unsigned long (or ctf_id_t). > I personally think this solution is a bit more clean as -1 is the error > value from all functions, just a matter if it's signed or unsigned. > > I.e: > > int > ctf_set_errno (ctf_dict_t *fp, int err) > { >   fp->ctf_errno = err; >   return -1; > } > > unsigned long > ctf_set_errno_unsigned (ctf_dict_t *fp, int err) > { >   return (unsigned long)ctf_set_errno (fp, err); > } > > I suppose the ctf_set_errno_unsigned could even be a macro in the > ctf-impl.h header file. > > > >> /* Store the specified error code into the CTF dict, and then return -1 >>     for the benefit of the caller, which is expected to return int, >>     as opposed to ctf_id_t. */ >> > > Ok! > >>> +int >>> +ctf_set_errno_signed (ctf_dict_t *fp, int err) >>> +{ >>> +  fp->ctf_errno = err; >>> +  /* Don't rely on CTF_ERR here as it will not properly sign extend >>> on 64-bit >>> +     Windows ABI.  */ >>> +  return -1; >>> +} >> >> ... that Windows is not really the problem here. It's more >> >> /* Don't rely on CTF_ERR here; it is a ctf_id_t (unsigned long), and >>     it will be truncated to a non--1 value on platforms on which int >>     and unsigned long are different sizes.  */ >> >> perhaps? (At least, I think that's what's going on.) > > The problem happens when the signed integral type is wider than unsigned > long. > >  /* Don't rely on CTF_ERR here; it is a ctf_id_t (unsigned long), and >      it will be extended to a non--1 value on platforms on which int >      is larger than unsigned long are different sizes.  */ > >> >> This probably needs testing on a wide variety of platforms with >> different type sizes. I'll add throwing this through my entire test >> matrix to my todo list, and fix any bugs observed: but the basic idea >> looks sound to me. > > Do you want to run this full matrix before or after submitting the patch? > If it's before; when do you think you will have time to do that? > > > Let me know how you want to proceed.