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 9F03C3857709 for ; Fri, 29 Sep 2023 12:11:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9F03C3857709 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 38T8GGrD010392; Fri, 29 Sep 2023 14:11:52 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h= message-id:date:mime-version:subject:to:cc:references:from :in-reply-to:content-type:content-transfer-encoding; s= selector1; bh=Ka36/KBrcUYZnyZ4pW3fB/giXuY5LDsG22QJmGEk65o=; b=wa VQDbkHJNbHteyCOT9MZw/+JRIZ0Ex7xT/pFswo1yd53EaNXCuZapT9OZS2VZHXTO 7P9q6lta99+qFrNKmpL70x1XlClTrHFqiJrr+QXZ0PON4/LmFT1x6MWgvuQqslP0 34yl0KlCBofjcfQJ/7bKAfEzILrnU4yP1XQJJYRURgzNsU73TAXdgNlyONmmVWXR qElmGU+LrhsEsZFrLd5amZGaLeDL6q55yq0KrF7gWyoiw23rePabgkluu4oDHkXU tPTUT1N/XDL9lVCM2tqkTg2txwW0j/Vk7/cwi9c0de8zQySSELmNfLt+fo4bkgBx WCgNU//W0FHX3sAk6imA== 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 3tdtx1gyst-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 29 Sep 2023 14:11:52 +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 0705A100057; Fri, 29 Sep 2023 14:11:52 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id F24302721F2; Fri, 29 Sep 2023 14:11:51 +0200 (CEST) Received: from [10.252.0.246] (10.252.0.246) 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; Fri, 29 Sep 2023 14:11:51 +0200 Message-ID: Date: Fri, 29 Sep 2023 14:11:50 +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 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> <87wmwdt2bf.fsf@esperi.org.uk> <87r0mimero.fsf@esperi.org.uk> From: Torbjorn SVENSSON In-Reply-To: <87r0mimero.fsf@esperi.org.uk> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.252.0.246] X-ClientProxiedBy: EQNCAS1NODE4.st.com (10.75.129.82) 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.619,FMLib:17.11.176.26 definitions=2023-09-29_10,2023-09-28_03,2023-05-22_02 X-Spam-Status: No, score=-5.4 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-28 18:41, Nick Alcock wrote: > On 26 Sep 2023, Torbjorn SVENSSON said: > >> On 2023-09-26 16:51, Nick Alcock wrote: >>> On 13 Sep 2023, Torbjorn SVENSSON outgrape: >>>> On 2023-09-13 20:37, Nick Alcock wrote: >>>>> On 13 Sep 2023, Torbjörn SVENSSON verbalised: >>> Honestly I suspect all we need is a better name: >>> ctf_set_int_errno(...); >>> ctf_set_type_errno(...) >>> and then use one or the other, consistently. (Neither needs to call the >>> other: they're only two lines long!) >> >> Ok. I've updated the patch (V4) to be like you suggested above. > > Thanks! > >>>> I suppose the ctf_set_errno_unsigned could even be a macro in the ctf-impl.h header file. >>> I'd make both of them inline functions personally (I bet it would reduce >>> code size!) >> >> I do not see any major difference in code size for the ld.exe binary after the change. > > Oh well, it was just a pious hope really. > >>>>>> +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. >>> ... sizeof(signed int) > sizeof(unsigned long int)?! Is that even >>> possible? I would have assumed from the C type hierarchy and the integer >>> conversion rank rules would have required that unsigned long int was at >>> least as big as any non-long integral type, but I don't see anywhere >>> it's required in the standard, dammit... >> >> I don't know about the 'sizeof(signed int) > sizeof(unsigned long >> int)' part, but what I said was _integral type_, not _int_. In the > > Ah true. My apologies. > >> case where I saw the problem, it was ssize_t but I'm not sure what >> that maps to, but it's wider than unsigned long int apparently in this >> case. > > Aha! So this is *not* a problem with functions returning int -- it is > specifically a problem with functions returning *size_t types*. > So, I'm not sure if were are talking past each other, but I don't think it's a *size_t type* issue either. As I see it, there are 3 different scenarios. 1. The function returns a signed integral type, then -1 would be fine. Even a simple (int)-1 would work as it would be sign-extended. 2. The function returns an unsigned integral type that is as wide, or wider, than unsigned long. In this case, CTF_ERR will be zero extended and the value will be UINT_something_MAX (the number of binary ones depends on the sizeof(unsigned long)). 3. The function return an unsigned integral type that is narrower than unsigned long. In this case, the cast to unsigned long is wider than the return type and may overflow(?). Is it safe to return something bigger than can be represented in the return type? I don't know if the third option above is applicable as I have not checked the width of the types that is calling the ctf_set_errno() function before my patch, but the other 2 are there. In my mind, I see it as we need 2 different implementations. One that returns a simple -1 and another one that casts it for all unsigned calls, but maybe I'm oversimplifying this. To make things easier, I would actually consider just having the assign statement and the return statement inlined (without a macro or inline function) directly where they are supposed to be. Would you be open to removing the ctf_set_errno() function completely and just expand it to where it's called (almost like my v2 patch, but everywhere instead)? Alternatively, we could start the discussion on adopting the C11 standard instead, but I fear that this will be a much longer discussion than figuring out what the best approach would be for libctf. > My apologies, I misunderstood the entire problem. We probably *do* > still want ctf_set_errno_signed for functions returning int (for clarity > if nothing else), but for ssize_t in particular this won't do: we > probably want a ctf_set_errno_ssize_t or something. The name is awful > but I wasted a day failing to think of a better one :( > > There are very few functions returning (s)size_t in libctf: > > extern size_t ctf_archive_count (const ctf_archive_t *); > extern ssize_t ctf_type_lname (ctf_dict_t *, ctf_id_t, char *, size_t); > extern ssize_t ctf_type_size (ctf_dict_t *, ctf_id_t); > extern ssize_t ctf_type_align (ctf_dict_t *, ctf_id_t); > extern ssize_t ctf_member_next (ctf_dict_t *, ctf_id_t, ctf_next_t **, > const char **name, ctf_id_t *membtype, > int flags); > > Of these, ctf_archive_count () cannot fail, so the problem reduces to > ssize_t alone. These functions should probably > > return (ssize_t) ctf_set_errno_signed (...)) > > (it's rare enough that a utility functions to do this is probably > unnecessary). > > We also have (in ctf_type_lname): > > if (str == NULL) > return CTF_ERR; /* errno is set for us. */ > > This should probably become a straight -1 (no cast necessary). Yes, this is another place where the problem appears. Looks like the -Wsign-conversion does not warn when it's the return statement for the function. Doing a simple grep in the tree reveals 56 places that needs to be verified. > ctf_type_size () already gets this right (but needs _ssize_t adjustments > to its ctf_set_errno () calls, as does get_vbytes_common in ctf-open.c). > The same is true of ctf_type_align (), and, of course, ctf_member_next > (). > >>>>> 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. >>> OK, I'm back from various conferences so I can throw tests past this at >>> any time, it's largely automated. So once I stop faffing about and >>> changing my mind and we converge on something I'll throw it past every >>> test I've got. (It takes a day or so.) >> >> If you do not see any problem with the V4 patch, then please go ahead >> and run the tests that you have to get a verdict. > > ... sorry, I'm still flailing at it. Maybe the above is helpful? (It's > only a very small change atop what you've already done, I think.) > Let me know how you want to handle this. :) Kind regards, Torbjörn