Received: by 2002:a05:6358:c692:b0:131:369:b2a3 with SMTP id fe18csp4818892rwb; Mon, 31 Jul 2023 12:46:02 -0700 (PDT) X-Google-Smtp-Source: APBJJlHNSPFExmPyl6x++nXVV1JlGS33rOwAPyVEDDgeIhwGWj/CAsYzbe0NAndDZpD5GvPM+Kwc X-Received: by 2002:a17:906:6487:b0:994:5350:5a2b with SMTP id e7-20020a170906648700b0099453505a2bmr441897ejm.62.1690832761897; Mon, 31 Jul 2023 12:46:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690832761; cv=none; d=google.com; s=arc-20160816; b=OAoLom+Ww82gF6pOKEcwHVoGhRUgsv4tZWSZYvzSzhWVabmRY0K+6L3Si1i1lJWRVT 7xU5BV+bwblM96lRsy5YwpMXBAIGHZyjZnERTqlWHu93Etz+PHSM5TmQzvwpqY148arj wo+2+2vjDihEyKAIuBMUO1yFwYfwlrS6xna/izbGu9hQi1ZM4TExicUYHaFn4RlWiI2K xAKyjUCVd0U8/0yRoc9KhErVHYnnKOuC1Y7cnVB7ooStJbo/YWTPJXLfb7sSn5jfbQeO G2oU+H4jeYQuO/wkjk8vfML9AaVW8R+MqsicI5TlGrYrFOUdciqh6v66G01pzfWJByO4 Knjg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=rE3oFKzfZ7gCQW4p8/jA0ok6pbBm2Bid0nQKfa91FPg=; fh=QkGc1uX6bcnNKaaZ9jpCjDoUtotU31mipXxrnawiQXU=; b=CiMwDnHQgX/BVoUfcmsg00NSMZ5woTqAGi28wy9qxUnTJ1BE5KkkonM8JQ0JPKd1qM NyZgJnAv24qsNIoKMX63lwoMLQqTThDR0MblljrAc/gvwd/V4MZvndC65EtR5XR3ksAH Yr/n5TMYIYiQSmDCF8NXafRfdK/q5QfGlnKAowvrP5aHPY/veA3NeBPzyCcWhbcrAsfC ce4B6QXImhI1GdtL7J8oDZTx7NzBuqpEmxk2y+ZaEjKjtgtKNBInaEss1Uh6OThIwSdF yZ2wc+mwynAUG3R/Ezq1DEuu/pIBE0GWnj0iUBFmfSLnRvT+Z2uP4hhmyTwHaScJYvkY 5KyA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@weissschuh.net header.s=mail header.b="DGJR/lKa"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y17-20020a170906915100b009924806cd0asi2624251ejw.295.2023.07.31.12.45.37; Mon, 31 Jul 2023 12:46:01 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@weissschuh.net header.s=mail header.b="DGJR/lKa"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231862AbjGaSgN (ORCPT + 99 others); Mon, 31 Jul 2023 14:36:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58830 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229935AbjGaSgM (ORCPT ); Mon, 31 Jul 2023 14:36:12 -0400 Received: from todd.t-8ch.de (todd.t-8ch.de [IPv6:2a01:4f8:c010:41de::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 023C419AE; Mon, 31 Jul 2023 11:36:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=weissschuh.net; s=mail; t=1690828566; bh=+ecIanusPEM9grLteoH4m5s9pVLXerZghs/LdT94HCs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DGJR/lKaDiHzpLrZgoy+WsW3XvLafzayhfbo83CH3WCJFtFKsjPFVYbgNwfyYpbzw 4aeAhuVb7m5wnQGpjFu+/uWlKNenVayrCw+HG6GE0Pwzu5y0nQ0X02jj+cIGGhsN76 tk5VVOXfVRQTNhF3g2O9Mrck4vOxQYjLsOw1n+sg= Date: Mon, 31 Jul 2023 20:36:05 +0200 From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= To: Willy Tarreau Cc: Zhangjin Wu , linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, shuah@kernel.org, tanyuan@tinylab.org Subject: Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers Message-ID: <26fd12c7-3c9c-4e1e-a8bf-9529cd624e81@t-8ch.de> References: <20230731073243.21265-1-falcon@tinylab.org> <20230731110226.115351-1-falcon@tinylab.org> <20230731165334.GA17823@1wt.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230731165334.GA17823@1wt.eu> X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,SPF_PASS, T_SCC_BODY_TEXT_LINE,T_SPF_HELO_TEMPERROR autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2023-07-31 18:53:34+0200, Willy Tarreau wrote: > Hi guys, > > On Mon, Jul 31, 2023 at 05:30:23PM +0200, Thomas Weißschuh wrote: > > > > > > > > Why not a simple 'static __attribute__((unused))' line, then, no need to > > > > > > > add them again next time. > > > > > > > > > > > > > > -static int expect_zr(int expr, int llen) > > > > > > > +static __attribute__((unused)) > > > > > > > +int expect_zr(int expr, int llen) > > > > > > > { > > > > > > > > > > > > Personally I don't like having dead code lying around that needs to be > > > > > > maintained and skipped over while reading. > > > > > > It's not a given that we will need those helpers in the future at all. > > > > > > > > > > > > > > > > It is reasonable in some degree from current status, especially for > > > > > these ones are newly added, but let us think about these scenes: when we > > > > > would drop or change some test cases in the future and the helpers may > > > > > would be not referenced by any test cases in a short time, and warnings > > > > > there, but some other cases may be added later to use them again ... > > > > > > > > That doesn't seem very likely. > > > > Did it happen recently? > > > > > > > > > > Yeah, it did happen, but I can not remember which one, a simple statistic > > > does show it may be likely: > > > > I can't find it. > > > > > $ grep EXPECT_ -ur tools/testing/selftests/nolibc/nolibc-test.c | grep -v define | sed -e 's/.*\(EXPECT_[A-Z0-9]*\).*/\1/g' | sort | uniq -c | sort -k 1 -g -r > > > 55 EXPECT_EQ > > > 37 EXPECT_SYSER > > > 21 EXPECT_SYSZR > > > 11 EXPECT_SYSNE > > > 9 EXPECT_VFPRINTF > > > 4 EXPECT_PTRGT > > > 4 EXPECT_GE > > > 3 EXPECT_STRZR > > > 3 EXPECT_NE > > > 3 EXPECT_LT > > > 3 EXPECT_GT > > > 2 EXPECT_STRNZ > > > 2 EXPECT_STREQ > > > 2 EXPECT_PTRLT > > > 1 EXPECT_SYSER2 > > > 1 EXPECT_SYSEQ > > > 1 EXPECT_PTRNZ > > > 1 EXPECT_PTRNE > > > 1 EXPECT_PTRER2 > > > 1 EXPECT_PTRER > > > 1 EXPECT_PTREQ > > > > > > 7 helpers are only used by once, another 3 helpers are used twice, and > > > another 4 are only used by three times. > > > > Why can't we just drop them when they are not used anymore? > > Actually we don't know if they're used or not given that the purpose of > the nolibc-test.c file is for it to be easy to add new tests, and the > collection of macros above serves this purpose. It's not just a series > of test but rather a small test framework. So the fact that right now > no single test uses some of them doesn't mean that someone else will > not have to reimplement them in two months. Reimplementing them would mean to copy one of the sibling test macros and changing the name and the condition operator in one place. I regarded that as an acceptable effort instead of having to work around the warnings. The warnings themselves I see as useful as they can give developers early feedback on their code. They would have avoided some of the issues with the recent pipe() series. Do you have a preferred solution for the overall situation? > However I share your concern that the file has become ugly over time. > I've recently been wondering why we wouldn't move all that to an external > include file. It could also encourage us to differentiate between the > macros used to only evaluate a result, and the tests themselves, as > we'd be certain that none of them could call a test function directly. > > > > Btw, just thought about gc-section, do we need to further remove dead code/data > > > in the binary? I don't think it is necessary for nolibc-test itself, but with > > > '-Wl,--gc-sections -Wl,--print-gc-sections' may be a good helper to show us > > > which ones should be dropped or which ones are wrongly declared as public? > > > > > > Just found '-O3 + -Wl,--gc-section + -Wl,--print-gc-sections' did tell > > > us something as below: > > > > > > removing unused section '.text.nolibc_raise' > > > removing unused section '.text.nolibc_memmove' > > > removing unused section '.text.nolibc_abort' > > > removing unused section '.text.nolibc_memcpy' > > > removing unused section '.text.__stack_chk_init' > > > removing unused section '.text.is_setting_valid' > > Just a note Zhangjin, it would really help if you wouldn't mix different > topics in mails. It's easy enough to start a separate thread since it's > a completely separate one here. > > > > These info may help us further add missing 'static' keyword or find > > > another method to to drop the wrongly used status of some functions from > > > the code side. > > > > > > It is very easy to add the missing 'static' keyword for is_setting_valid(), but > > > for __stack_chk_init(), is it ok for us to convert it to 'static' and remove > > > the 'weak' attrbute and even the 'section' attribute? seems it is only used by > > > our _start_c() currently. > > > > Making is_setting_valid(), __stack_chk_init() seems indeed useful. > > Also all the run_foo() test functions. > > Most of them could theoretically be turned to static. *But* it causes a > problem which is that it will multiply their occurrences in multi-unit > programs, and that's in part why we've started to use weak instead. Also > if you run through gdb and want to mark a break point, you won't have the > symbol when it's static, and the code will appear at multiple locations, > which is really painful. I'd instead really prefer to avoid static when > we don't strictly want to inline the code, and prefer weak when possible > because we know many of them will be dropped at link time (and that's > the exact purpose). Thanks for the clarification. I forgot about that completely! The stuff from nolibc-test.c itself (run_foo() and is_settings_valid()) should still be done. Thomas