Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Sat, 7 Oct 2023 08:50:25 +0200
From:   Willy Tarreau <w@1wt.eu>
To:     Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>
Cc:     Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org
Subject: Re: [PATCH RFC] tools/nolibc: add support for constructors and
 destructors
Message-ID: <20231007065025.GZ20998@1wt.eu>
References: <20231005-nolibc-constructors-v1-1-776d56bbe917@weissschuh.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231005-nolibc-constructors-v1-1-776d56bbe917@weissschuh.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hi Thomas,

On Thu, Oct 05, 2023 at 06:45:07PM +0200, Thomas Wei?schuh wrote:
> With the startup code moved to C, implementing support for
> constructors and deconstructors is fairly easy to implement.
> 
> Examples for code size impact:
> 
>    text	   data	    bss	    dec	    hex	filename
>   21837	    104	     88	  22029	   560d	nolibc-test.before
>   22135	    120	     88	  22343	   5747	nolibc-test.after
>   21970	    104	     88	  22162	   5692 nolibc-test.after-only-crt.h-changes
> 
> The sections are defined by [0].
> 
> [0] https://refspecs.linuxfoundation.org/elf/gabi4+/ch5.dynamic.html
> 
> Signed-off-by: Thomas Wei?schuh <linux@weissschuh.net>
> ---
> Note:
> 
> This is only an RFC as I'm not 100% sure it belong into nolibc.
> But at least the code is visible as an example.

That's interesting, thanks for working on this! I thought about it in
the past but didn't see how to address it. I do think some users might
find it convenient with modular code that will require less ifdefs.
That may be particularly true with test programs that want to register
some test series for example. The code looks clean to me, and I suppose
you've tested it on multiple archs. However I'm having a comment below:
(...)

>  #endif /* _NOLIBC_CRT_H */
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index a3ee4496bf0a..f166b425613a 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -57,6 +57,9 @@ static int test_argc;
>  /* will be used by some test cases as readable file, please don't write it */
>  static const char *argv0;
>  
> +/* will be used by constructor tests */
> +static int constructor_test_value;
> +
>  /* definition of a series of tests */
>  struct test {
>  	const char *name;              /* test name */
> @@ -594,6 +597,18 @@ int expect_strne(const char *expr, int llen, const char *cmp)
>  #define CASE_TEST(name) \
>  	case __LINE__: llen += printf("%d %s", test, #name);
>  
> +__attribute__((constructor))
> +static void constructor1(void)
> +{
> +	constructor_test_value = 1;
> +}
> +
> +__attribute__((constructor))
> +static void constructor2(void)
> +{
> +	constructor_test_value *= 2;
> +}
> +

In the past I learned the hard way that you can never trust the execution
order of constructors, so if you're unlucky above you could very well end
up with 1 and that would be correct. I suggest that instead you do something
such as:

      constructor_test_value += 1;
...
      constructor_test_value += 2;

and check for value 3 in the test to make sure they were both executed
exactly once each.

Thanks!
Willy