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 6D9DE3858D1E for ; Fri, 13 Oct 2023 18:31:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6D9DE3858D1E 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 (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 39DGG5SK010985; Fri, 13 Oct 2023 20:31:11 +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=Uv4zlN3zMo0q3BOgSmzpvNq02XSRdnBEQ+3O3mZkJwo=; b=Be BW1LrZ3QiJBSAStrce2AilbGwl7LfuZo15B478Z/EwrlA6nC2ipcfPM346QVRqYA lboGFNXJzNkJLPXD9PGSswZNtoQuEaPcOK7oqvgDb+WmzdtmduJkMBYC1/Bszbds uMS0sSKFbCbJHbqSOd/mx5JHJyN2Z6rZeBC2G6IEov//ZvWUT5QCDTyCr286ES5r weLFxOiaC40RvNfjSGCulniwuJu+QwLvHoeBogdvWKv15Y5XTqTa1Q3EwBDBLXoL 0670B93OJuBLbBoYaBpIhcb8MpfEtdx+oj7UbTgqGHxI6u1cjk31ycJC0PTXismP 514tWz7bb+epNdMaOjvg== 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 3tkhjgst2s-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 13 Oct 2023 20:31:11 +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 9BE4E10004F; Fri, 13 Oct 2023 20:31:10 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 8BBBD2C41FE; Fri, 13 Oct 2023 20:31:10 +0200 (CEST) Received: from [10.252.28.160] (10.252.28.160) 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, 13 Oct 2023 20:31:10 +0200 Message-ID: <482ae0a8-85c7-3a46-df1e-2d5850b7824b@foss.st.com> Date: Fri, 13 Oct 2023 20:31:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] libctf: check for problems with error returns To: Nick Alcock , CC: References: <20231009151146.3818141-1-torbjorn.svensson@foss.st.com> <20231013140152.427376-1-nick.alcock@oracle.com> Content-Language: en-US From: Torbjorn SVENSSON In-Reply-To: <20231013140152.427376-1-nick.alcock@oracle.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.252.28.160] X-ClientProxiedBy: SHFCAS1NODE1.st.com (10.75.129.72) To SHFDAG1NODE3.st.com (10.75.129.71) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-13_09,2023-10-12_01,2023-05-22_02 X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,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: Hi Nick, Thanks for validating this patch! On 2023-10-13 16:01, Nick Alcock wrote: > We do this as a writable test because the only known-affected platforms > (with ssize_t longer than unsigned long) use PE, and we do not have support > for CTF linkage in the PE linker yet. Is it visible in PE too or only PE32+? Maybe not important, but the Arm built variant does not trigger the fault (same source tree as I found the issue in, but they build 32-bit and I build 64-bit) when I've executed the GCC testsuite. > PR libctf/30836 > * libctf/testsuite/libctf-writable/libctf-errors.*: New test. > --- > .../testsuite/libctf-writable/libctf-errors.c | 74 +++++++++++++++++++ > .../libctf-writable/libctf-errors.lk | 1 + > 2 files changed, 75 insertions(+) > create mode 100644 libctf/testsuite/libctf-writable/libctf-errors.c > create mode 100644 libctf/testsuite/libctf-writable/libctf-errors.lk > > Your patch looks good, and passes every test I can throw at it. I think > it can go in. You cleaned up a bunch of outright errors in this area, > too, especially in ctf-dedup: thanks! There is still some potential for cleanup as some functions are returning "unsigned long", but think it should perhaps be ctf_id_t instead. > (You probably want to adjust the commit log so that the version history > is at the bottom rather than the top, or drop it entirely.) My plan was to drop that part. Do you think I should have a changelog entry for the commit and if so, what should I write in it? Should I list every function that is touched (more or less half of the ctf_* functions defined...) or is there some better way to document this change? > Here's a testcase that fails on mingw64 in the absence of your patch, > without requiring a cross-build to an ELF arch. (It also checks at > least one instance of the other classes of error return in libctf.) > > I'll push this after your commit goes in. (I can push it, with an > adjusted commit log, if you want. Fine either way. Either reply to my question about changelog or just merge it with the correct answer :) > > diff --git a/libctf/testsuite/libctf-writable/libctf-errors.c b/libctf/testsuite/libctf-writable/libctf-errors.c > new file mode 100644 > index 00000000000..71f8268cfad > --- /dev/null > +++ b/libctf/testsuite/libctf-writable/libctf-errors.c > @@ -0,0 +1,74 @@ > +/* Make sure that error returns are correct. Usually this is trivially > + true, but on platforms with unusual type sizes all the casting might > + cause problems with unexpected sign-extension and truncation. */ > + > +#include > +#include > +#include > + > +int > +main (int argc, char *argv[]) > +{ > + ctf_dict_t *fp; > + ctf_next_t *i = NULL; > + size_t boom = 0; > + ctf_id_t itype, stype; > + ctf_encoding_t encoding = {0}; > + ctf_membinfo_t mi; > + ssize_t ret; > + int err; > + > + if ((fp = ctf_create (&err)) == NULL) > + { > + fprintf (stderr, "%s: cannot create: %s\n", argv[0], ctf_errmsg (err)); > + return 1; > + } > + > + /* First error class: int return. */ > + > + if (ctf_member_count (fp, 1024) >= 0) > + fprintf (stderr, "int return: non-error return: %i\n", > + ctf_member_count(fp, 1024)); > + > + /* Second error class: type ID return. */ > + > + if (ctf_type_reference (fp, 1024) != CTF_ERR) > + fprintf (stderr, "ctf_id_t return: non-error return: %li\n", > + ctf_type_reference (fp, 1024)); > + > + /* Third error class: ssize_t return. Create a type to iterate over first. */ > + > + if ((itype = ctf_add_integer (fp, CTF_ADD_ROOT, "int", &encoding)) == CTF_ERR) > + fprintf (stderr, "cannot add int: %s\n", ctf_errmsg (ctf_errno (fp))); > + else if ((stype = ctf_add_struct (fp, CTF_ADD_ROOT, "foo")) == CTF_ERR) > + fprintf (stderr, "cannot add struct: %s\n", ctf_errmsg (ctf_errno (fp))); > + else if (ctf_add_member (fp, stype, "bar", itype) < 0) > + fprintf (stderr, "cannot add member: %s\n", ctf_errmsg (ctf_errno (fp))); Should these be if-else-if-else-if-statments like above or just 3 "free-standing" if-statements? I.e. should the 3 potential issue be visible in the same run or should they require 3 consecutive runs if all of them are failing? > + > + if (ctf_member_info (fp, stype, "bar", &mi) < 0) > + fprintf (stderr, "cannot get member info: %s\n", ctf_errmsg (ctf_errno (fp))); > + > + /* Iteration should never produce an offset bigger than the offset just returned, > + and should quickly terminate. */ > + > + while ((ret = ctf_member_next (fp, stype, &i, NULL, NULL, 0)) >= 0) { > + if (ret > mi.ctm_offset) > + fprintf (stderr, "ssize_t return: unexpected offset: %zi\n", ret); > + if (boom++ > 1000) > + { > + fprintf (stderr, "member iteration went on way too long\n"); > + exit (1); > + } > + } > + > + /* Fourth error class (trivial): pointer return. */ > + if (ctf_type_aname (fp, 1024) != NULL) > + fprintf (stderr, "pointer return: non-error return: %p\n", > + ctf_type_aname (fp, 1024)); > + > + ctf_file_close (fp); > + > + printf("All done.\n"); > + > + return 0; > +} > diff --git a/libctf/testsuite/libctf-writable/libctf-errors.lk b/libctf/testsuite/libctf-writable/libctf-errors.lk > new file mode 100644 > index 00000000000..b944f73d013 > --- /dev/null > +++ b/libctf/testsuite/libctf-writable/libctf-errors.lk > @@ -0,0 +1 @@ > +All done. >