From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx07-00178001.pphosted.com (mx08-00178001.pphosted.com [91.207.212.93]) by sourceware.org (Postfix) with ESMTPS id B1EF63858D20 for ; Wed, 13 Sep 2023 21:23:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B1EF63858D20 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 (m0046661.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 38DIjNB5010120; Wed, 13 Sep 2023 22:20:46 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h= message-id:date:mime-version:subject:references:from:to:cc :in-reply-to:content-type:content-transfer-encoding; s= selector1; bh=p3zGE4lJNpcFmAvTN8KujTcs5wGrnxeXDJSqwZB9N1U=; b=Si n5EvEHPHN/9xlsbRxnrGYe1cvXt91yOeaVG5GIX6/m8cpHkEFFqHdXRoTPSo2H/O tOfhbH5SUQY2W4OvVvZ99RAkU5UqCneZvYOWibdiGCVuR+TgzAXCMm24OU56qcEG XnNk7bLh8qo2Ys5DC0YYoJxb+WROdxUCvGz3TSxQ/X881EFeowZ8bDHrJxrU/R3O y4uS09fIVOnIe6xev5uVonCH/gW+jnYxbJxesH+ktzZ6+XDTtRT213sAs1y099zd IBX3Ujpn5l/v0JaB8MyGMW4of9NXNECe9ZmoTXPyJe3mzpY8bWCK8c2mjfIjTl7u Jv31/KX0P8SI5bLVBtYg== 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 3t2y7nd01m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 13 Sep 2023 22:20:46 +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 DF6EB100057; Wed, 13 Sep 2023 22:20:45 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id D72D92085A4; Wed, 13 Sep 2023 22:20:45 +0200 (CEST) Received: from [10.252.1.152] (10.252.1.152) 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, 13 Sep 2023 22:20:45 +0200 Message-ID: <657dadf8-4b7b-67e6-9de2-9a1cdb79f081@foss.st.com> Date: Wed, 13 Sep 2023 22:20:44 +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 References: <878r9ba2sf.fsf@esperi.org.uk> <20230913095727.1420654-1-torbjorn.svensson@foss.st.com> <87a5tp2a3n.fsf@esperi.org.uk> From: Torbjorn SVENSSON To: Nick Alcock CC: , , Yvan ROUX In-Reply-To: <87a5tp2a3n.fsf@esperi.org.uk> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.252.1.152] X-ClientProxiedBy: SHFCAS1NODE2.st.com (10.75.129.73) 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-13_15,2023-09-13_01,2023-05-22_02 X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,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: 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.