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

[Arnout Vandecappelle <arnout@mind.be>]

On 23/09/2021 10:23, David Laight wrote:
> 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.

  It really doesn't matter that much. The only place where THREAD_MINSTACKSIZE 
is used is in the code leading up to this:

                 if (nstacksize != ostacksize)
                         rc = pthread_attr_setstacksize(&thr_attr, nstacksize);
                 if (0 != rc)
                         msyslog(LOG_ERR,
                                 "start_blocking_thread: 
pthread_attr_setstacksize(0x%lx -> 0x%lx) -> %s",
                                 (u_long)ostacksize, (u_long)nstacksize,
                                 strerror(rc));

  Note that there's no error handling other than printing an error message. So, 
if PTHREAD_STACK_MIN turns out to be less than 64K (which is very, very unlikely 
to be the case), then the only thing that will happen is that 
pthread_attr_setstacksize will fail to set the stack size to a lower value and 
we keep the default stack size (and print an error message).

  I do agree however that your alternative fix is better (though Id keep the 
existing __sun ifdeffery, just to make the change minimal). So feel free to send 
a patch that changes the existing patch.


> Quite why ntp is setting the thread stack size is any bodies guess though.

  I guess because there are some threads that need more than 4K stack (which is 
minstacksize on some platforms), and on the other hand they don't want to waste 
stack space on threads that really don't need that much.

> It can only get it wrong in a different way!

  For relatively small code, it's not that hard to measure or calculate the 
amount of stack you need.

> 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.

  I do indeed see an mlockall() in ntpd.c, but I don't see any code doing any 
prefaulting of the stack, so it doesn't seem to be very effective :-)


> 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.

  Yeah, it looks like ntp is a pretty dead-end project...

  Regards,
  Arnout

> 
> 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