Re: [PATCH v2] Y2038: provide kernel support indication

[Joseph Myers <joseph at codesourcery dot com>]

On Wed, 17 Oct 2018, Albert ARIBAUD (3ADEV) wrote:

> diff --git a/misc/Versions b/misc/Versions
> index 900e4ffb79..48285d8c42 100644
> --- a/misc/Versions
> +++ b/misc/Versions
> @@ -158,6 +158,8 @@ libc {
>    GLIBC_2.26 {
>      preadv2; preadv64v2; pwritev2; pwritev64v2;
>    }
> +  GLIBC_2.29 {
> +  }

Why?  I don't see any SHLIB_COMPAT or similar conditionals that might 
require such an empty symbol version.

> +/* Support for Y2038-proof times.
> +
> +   If defined, the underlying kernel always provides a set of syscalls
> +   (__NR_* names and numbers) which can handle times beyond Y2038.
> +
> +   If undefined, whether the underlying kernel provides this set or not
> +   must be checked dynamically.
> +
> +   Definition is commented out until Y2038 syscalls are merged in the
> +   kernel.

I am unable to tell from this comment what the correct definition is for 
64-bit systems where time_t in the default syscalls has always been 
64-bit.  That information needs to be completely obvious from the comment; 
it needs to be completely explicit whether the macro relates to new 
syscalls whose names explicitly refer to 64-bit times, or whether it 
relates to syscalls supporting 64-bit times whose names might not mention 
64-bit when on 64-bit platforms.

If the latter, the macro should of course be defined on 64-bit systems.  
If the former, however, while the macro should then not be defined on 
64-bit systems, the contents of y2038-support.c (both versions) must also 
be conditioned out for __TIMESIZE == 64; there should be no additional 
code added to libc binaries whatever in support of this feature on systems 
where time_t is already 64-bit.  (The declarations in y2038-support.h 
should probably also be disabled in that case, to result in compile 
failures if anything tries to call those functions with __TIMESIZE == 64.)

> +/* By default the underlying Linux kernel is assumed not to support Y2038.
> + * Any Linux architecture may (test and) claim Y2038 kernel support by
> + * setting __y2038_linux_support.

We don't use leading '*' on each comment line.

> +bool
> +__linux_y2038_get_kernel_support (void)
> +{
> +  return __y2038_linux_support;
> +}
> +strong_alias (__linux_y2038_get_kernel_support, __y2038_get_kernel_support)
> +
> +void
> +__linux_y2038_set_kernel_support (bool support)
> +{
> +  __y2038_linux_support = support;

Are you sure about using direct read / assignment like this rather than 
relaxed atomics, which seem more logically correct for such cases (and 
hopefully will generate the same code) where multiple threads might be 
using the variable at once?

> +strong_alias (__linux_y2038_set_kernel_support, __y2038_set_kernel_support)

Why?  The functions should never be accessed under the __linux_* names; 
I'd expect them to be defined directly with the __y2038_* names, in both 
files, rather than complicating things with __default_* / __linux_* names 
and aliases.  (You need to use libc_hidden_proto / libc_hidden_def, 
however, since you have GLIBC_PRIVATE exports, so that accesses from 
within libc.so use a hidden symbol name rather than going via the PLT.)

> +/* Indicates Y2038 support.
> + * Can be read directly from within libc linux-related files.
> + * Can be written non-zero to indicate support or lack thereof.
> + * Other libraries (e.g. librt) cannot access this variable and must
> + * call __y2038_get_kernel_support() and __y2038_set_kernel_support().  */
> +extern bool __y2038_linux_support;

Same point about comment formatting.  If you have accessor functions, I 
don't think you should be accessing this variable directly other than 
through inline versions of those accessor functions.  So either make this 
variable static and remove that declaration, or add "#if IS_IN (libc)" 
inline definitions of the accessors (and make that declaration, which 
should also use attribute_hidden, conditional on IS_IN (libc) as well).

-- 
Joseph S. Myers
joseph@codesourcery.com