Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+

From: David Laight <David.Laight@ACULAB.COM>
To: 'Arnout Vandecappelle' <arnout@mind.be>,
	"buildroot@buildroot.org" <buildroot@buildroot.org>
Subject: Re: [Buildroot] [git commit] package/ntp: Fix building with glibc 2.34+
Date: Thu, 23 Sep 2021 08:23:19 +0000	[thread overview]
Message-ID: <4348b4b5c0844714baffcfee40bfec1a@AcuMS.aculab.com> (raw)
In-Reply-To: <20210922210348.96B9A8CA2B@busybox.osuosl.org>

From: Arnout Vandecappelle
> Sent: 22 September 2021 21:13
...
> On attempt to build ntp with glibc 2.34 the following error happens:
> -------------------------------->8------------------------------
> In file included from .../output/host/lib/gcc/i586-buildroot-linux-gnu/10.3.0/include-
> fixed/pthread.h:42,
>                  from work_thread.c:13:
> work_thread.c:45:57: error: missing binary operator before token "("
>    45 | #if defined(PTHREAD_STACK_MIN) && THREAD_MINSTACKSIZE < PTHREAD_STACK_MIN
>       |                                                         ^~~~~~~~~~~~~~~~~
> -------------------------------->8------------------------------
> 
> That's because starting from glibc 2.34 PTHREAD_STACK_MIN gets determined
> dynamically in runtime via sysconf(), see [1].
...
> +In glibc 2.34+ PTHREAD_STACK_MIN is not a compile-time constant which
> +could mean different stack sizes at runtime on different architectures
> +and it also causes compile failure. Default glibc thread stack size
> +or 64Kb set by ntp should be good in glibc these days.
...
> + #ifndef THREAD_MINSTACKSIZE
> + # define THREAD_MINSTACKSIZE	(64U * 1024)
> + #endif
> +-#ifndef __sun
> ++#if !defined(__sun) && !defined(__GLIBC__)
> + #if defined(PTHREAD_STACK_MIN) && THREAD_MINSTACKSIZE < PTHREAD_STACK_MIN
> + # undef THREAD_MINSTACKSIZE
> + # define THREAD_MINSTACKSIZE PTHREAD_STACK_MIN

I'm not convinced that is the correct fix.
The same issues apply to the thread stack as they do to the signal stack.
Albeit the sizes are larger.

The very reason that PTHREAD_STACK_MIN isn't a compile-time constant
is that the value cannot be determined at compile time.
While 64k may seem ok for now, there is no reason for it to be enough.

Possibly something like:
#ifndef PTHREAD_STACK_MIN
#define THREAD_MINSTACKSIZE	(64U * 1024)
#else
#define THREAD_MINSTACKSIZE	(PTHREAD_STACK_MIN > 64U * 1024 ? PTHREAD_STACK_MIN : 64U * 1024)
#endif
would be better.
Although a form that only evaluates PTHREAD_STACK_MIN once may be better.

Quite why ntp is setting the thread stack size is any bodies guess though.
It can only get it wrong in a different way!
Although ISTR one version of ntp thinking itself so important
that it locked all its memory.
So maybe it is just trying to reduce that locked memory footprint.

Given that it's pretty much impossible for ntp to accurately set
the time to less than the RTT to the server (it can't tell whether
the RTT delays are on the tx or rx side) I can't see that it matters.

Anyone who needs sub-ms time sync has bigger problems.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
buildroot mailing list
buildroot@lists.buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot