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 9A68638582A3 for ; Mon, 16 Oct 2023 13:02:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9A68638582A3 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 ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9A68638582A3 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=91.207.212.93 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697461343; cv=none; b=l+o5j2+kObRfAaEfq6JsGuqJvqi/s0uLe037hD54volJABODooBqAiJnaf6kGV9vFBsGSQLw/Z7177uteF+Frl3drjM4639IhInSfDdVb8bMKr2h+hpj8OjXRPeBpAZEErFaPGvw5S+bnKXvo4CbmsR429ugGezoI++j9BrhQ/U= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697461343; c=relaxed/simple; bh=X+JtVaMCDkZRSCTcwywC68gy1z4rvwHNNjKby0A4ZbY=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=rKIGbI25m8pal+GqW4Rg1aGIRLHxTDJ3A3a/wbK8lVNEb42GXyMsTAX/3h3FqpdtxRs1SlQIyb2aMnnv0N+p3fnnRfqrLakFYzjUrQPKPUWqAnwl0QWrIwDSFbNmvKFBCenEi2Aj5LoW82mMotm3QYA+YN87RDtTCx4GotOA0DE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0046660.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 39GCVAeO006055; Mon, 16 Oct 2023 15:02:17 +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=kPa0RhZrP7+yzPsIaSw4uR7RW0ZMeQoPBUHmtABtdKg=; b=CN RLMPEmtKvUllj0qcMPNgAQEOIz0qpfIRMSboxh4XaWHttRmf+4lM/A3cw94A4sJb auYcHJxMkZY8gtHIKGbkL9OVpKCwHGVa9XSxKyMAEbHDsXQcss1fQPjdiiaAYCNR rrqR86YvXEnj1RV2iLqYzus946bEQj65VBHTKjL0PL/EXEAp7s//Kqb4FttP9uEQ a67j3hB0BrmK5rX9+VqovpeeKhWMIsM7lyd4vQVuQBD+VyY7vQFkUlDLRccL2sPE mhba+HsSIrrwt0t+ifuLNFf/pmKxYTjDdwt7saBLAg7F57u8K1q4YUObydksysna AivUS7KjmeoWrWGwgLsg== 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 3tqkbe7q45-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 16 Oct 2023 15:02:17 +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 D567D10005B; Mon, 16 Oct 2023 15:02:16 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id C7E9E236943; Mon, 16 Oct 2023 15:02:16 +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; Mon, 16 Oct 2023 15:02:16 +0200 Message-ID: <8c54b317-2ec4-49f2-813e-2e6b05f36824@foss.st.com> Date: Mon, 16 Oct 2023 15:02:15 +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> <482ae0a8-85c7-3a46-df1e-2d5850b7824b@foss.st.com> <87ttqrae3o.fsf@esperi.org.uk> Content-Language: en-US From: Torbjorn SVENSSON In-Reply-To: <87ttqrae3o.fsf@esperi.org.uk> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit 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.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-16_06,2023-10-12_01,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-10-15 21:18, Nick Alcock wrote: > On 13 Oct 2023, Torbjorn SVENSSON told this: > >> 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. > > The underlying fault is client-side and is visible in anything that does > a ctf_member_next on a platform with sizeof(ssize_t) > sizeof(unsigned > long). The specific instance you saw (a hanging linker) requires CTF in > the input and a linker that deduplicates it, and right now that is ELF > only (because it was painful enough to add that I didn't have the > stamina to add anything else -- if anything else *is* added, it will > probably be PE next, but right now linking PE just blindly concatenates > CTF sections, which isn't very useful). Right. > >>> 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. > > The only one of those i can see is ctf_lookup_symbol_idx. That's > returning a symbol index, not a type ID, so should definitely not be > returning a ctf_id_t. Perhaps it should be an int, but I'm not sure how > big symbol tables in the set of all executable formats can actually > be... > > Hm, actually, there is one bit you might want to adjust in ctf-lookup.c. > You fixed one call to ctf_lookup_symbol_idx () in > ctf_lookup_by_sym_or_name (): > > if ((symidx = ctf_lookup_symbol_idx (fp, symname)) == (unsigned long) -1) > goto try_parent; > > but the other one, in ctf_lookup_symbol_idx () itself (doing a recursive > call to check parent dicts), still says > > if ((psym = ctf_lookup_symbol_idx (fp->ctf_parent, symname)) > != CTF_ERR) > > Both symidx and psym are unsigned long variables, so probably both > should be checking against (unsigned long) -1. Thanks for the pointer. I found that I had replaced some (unsigned long)-1 with CTF_ERR that should be exactly that. V8 is reverting those chunks to be like they were before. >>> (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. > > Aha right. > >> 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? > > You can just say > > Affected functions adjusted. > > or something. A giant list adds no value, I think. I tried to compile an exhaustive list, but it was too big for my taste, so I went with your proposal (part of V8). > >>> 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 :) > > ... I think there's one more tiny change :/ > >>> + /* 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? > > I was wondering if this was too clever (or unclear!) but the intent is > to only do the later things if the earlier ones didn't fail, i.e. what > would be foo || bar || baz in the shell. (Later operations will also > fail if any of them fail, but the fprintf's from the first failure will > suffice to make the whole test fail -- so maybe it's safe to just do > them in sequence even if the earlier ones failed. Excessive paranoia on > my part perhaps, given that we don't *expect* any of these to fail: it's > just setting things up for the *actual* tests, of ctf_member_info and > ctf_member_next.) > >>> + 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); > > (here.) Ah, okay. In any case, I think it would be clearer if you get all the lines in one go that fails than just the first one (in case of multiple failures...). - But, that's only my 2 cents.