[libc] Make use of 32-bit time_t a config option #102012

statham-arm · 2024-08-05T16:36:56Z

The 32-bit Arm builds of libc define time_t to be __INTPTR_TYPE__, i.e. a 32-bit integer. This is commented in the commit introducing it (75398f2) as being for compatibility with glibc. But in the near future not even every AArch32 build of glibc will have a 32-bit time_t: Debian is planning that their next release (trixie) will have switched to 64-bit. And non-Linux builds of this libc (e.g. baremetal) have no reason to need glibc compatibility in the first place – and every reason not to want to start using a 32-bit time_t in 2024 or later.

So I've replaced the #ifdef in llvm-libc-types/time_t.h with two versions of the header file, chosen in CMakeLists.txt via a new configuration option. This involved adding an extra parameter to the cmake add_header function to specify different names for the header file in the source and destination directories.

The 32-bit Arm builds of libc define time_t to be `__INTPTR_TYPE__`, i.e. a 32-bit integer. This is commented in the commit introducing it (75398f2) as being for compatibility with glibc. But in the near future not even every AArch32 build of glibc will have a 32-bit time_t: Debian is planning that their next release (trixie) will have switched to 64-bit. And non-Linux builds of this libc (e.g. baremetal) have no reason to need glibc compatibility in the first place – and every reason _not_ to want to start using a 32-bit time_t in 2024 or later. So I've replaced the `#ifdef` in `llvm-libc-types/time_t.h` with two versions of the header file, chosen in `CMakeLists.txt` via a new configuration option. This involved adding an extra parameter to the cmake `add_header` function to specify different names for the header file in the source and destination directories.

llvmbot · 2024-08-05T16:37:29Z

@llvm/pr-subscribers-libc

Author: Simon Tatham (statham-arm)

Changes

The 32-bit Arm builds of libc define time_t to be __INTPTR_TYPE__, i.e. a 32-bit integer. This is commented in the commit introducing it (75398f2) as being for compatibility with glibc. But in the near future not even every AArch32 build of glibc will have a 32-bit time_t: Debian is planning that their next release (trixie) will have switched to 64-bit. And non-Linux builds of this libc (e.g. baremetal) have no reason to need glibc compatibility in the first place – and every reason not to want to start using a 32-bit time_t in 2024 or later.

So I've replaced the #ifdef in llvm-libc-types/time_t.h with two versions of the header file, chosen in CMakeLists.txt via a new configuration option. This involved adding an extra parameter to the cmake add_header function to specify different names for the header file in the source and destination directories.

Full diff: https://github.com/llvm/llvm-project/pull/102012.diff

6 Files Affected:

(modified) libc/cmake/modules/LLVMLibCHeaderRules.cmake (+6-2)
(modified) libc/config/config.json (+6)
(modified) libc/docs/configure.rst (+2)
(modified) libc/include/llvm-libc-types/CMakeLists.txt (+10-1)
(added) libc/include/llvm-libc-types/time_t_32.h (+14)
(renamed) libc/include/llvm-libc-types/time_t_64.h (-4)

diff --git a/libc/cmake/modules/LLVMLibCHeaderRules.cmake b/libc/cmake/modules/LLVMLibCHeaderRules.cmake
index 3049f4db7301f..5b29fa7897e1c 100644
--- a/libc/cmake/modules/LLVMLibCHeaderRules.cmake
+++ b/libc/cmake/modules/LLVMLibCHeaderRules.cmake
@@ -10,7 +10,7 @@ function(add_header target_name)
   cmake_parse_arguments(
     "ADD_HEADER"
     ""    # No optional arguments
-    "HDR" # Single value arguments
+    "HDR;SRCHDR" # Single value arguments
     "DEPENDS"
     ${ARGN}
   )
@@ -21,7 +21,11 @@ function(add_header target_name)
   set(absolute_path ${CMAKE_CURRENT_SOURCE_DIR}/${ADD_HEADER_HDR})
   file(RELATIVE_PATH relative_path ${LIBC_INCLUDE_SOURCE_DIR} ${absolute_path})
   set(dest_file ${LIBC_INCLUDE_DIR}/${relative_path})
-  set(src_file ${CMAKE_CURRENT_SOURCE_DIR}/${ADD_HEADER_HDR})
+  if(ADD_HEADER_SRCHDR)
+    set(src_file ${CMAKE_CURRENT_SOURCE_DIR}/${ADD_HEADER_SRCHDR})
+  else()
+    set(src_file ${CMAKE_CURRENT_SOURCE_DIR}/${ADD_HEADER_HDR})
+  endif()
 
   add_custom_command(
     OUTPUT ${dest_file}
diff --git a/libc/config/config.json b/libc/config/config.json
index 538fea53cc704..2e72c0a3fd1d6 100644
--- a/libc/config/config.json
+++ b/libc/config/config.json
@@ -88,5 +88,11 @@
       "value": true,
       "doc": "Make setjmp save the value of x18, and longjmp restore it. The AArch64 ABI delegates this register to platform ABIs, which can choose whether to make it caller-saved."
     }
+  },
+  "time": {
+    "LIBC_CONF_TIME_64BIT": {
+      "value": false,
+      "doc": "Force the size of time_t to 64 bits, even on platforms where compatibility considerations would otherwise make it 32-bit."
+    }
   }
 }
diff --git a/libc/docs/configure.rst b/libc/docs/configure.rst
index 950de0eee4c05..54ca5d55d7b24 100644
--- a/libc/docs/configure.rst
+++ b/libc/docs/configure.rst
@@ -52,3 +52,5 @@ to learn about the defaults for your platform and target.
 * **"string" options**
     - ``LIBC_CONF_MEMSET_X86_USE_SOFTWARE_PREFETCHING``: Inserts prefetch for write instructions (PREFETCHW) for memset on x86 to recover performance when hardware prefetcher is disabled.
     - ``LIBC_CONF_STRING_UNSAFE_WIDE_READ``: Read more than a byte at a time to perform byte-string operations like strlen.
+* **"time" options**
+    - ``LIBC_CONF_TIME_64BIT``: Force the size of time_t to 64 bits, even on platforms where compatibility considerations would otherwise make it 32-bit.
diff --git a/libc/include/llvm-libc-types/CMakeLists.txt b/libc/include/llvm-libc-types/CMakeLists.txt
index d8b975572e0dd..3dfa39b9b132b 100644
--- a/libc/include/llvm-libc-types/CMakeLists.txt
+++ b/libc/include/llvm-libc-types/CMakeLists.txt
@@ -58,7 +58,16 @@ add_header(pthread_rwlock_t HDR pthread_rwlock_t.h DEPENDS .__futex_word .pid_t)
 add_header(pthread_rwlockattr_t HDR pthread_rwlockattr_t.h)
 add_header(pthread_t HDR pthread_t.h DEPENDS .__thread_type)
 add_header(rlim_t HDR rlim_t.h)
-add_header(time_t HDR time_t.h)
+if(LIBC_CONF_TIME_64BIT)
+  # Set time_t to 64 bit as requested in configuration
+  add_header(time_t HDR time_t.h SRCHDR time_t_64.h)
+elseif(LIBC_TARGET_ARCHITECTURE_IS_ARM)
+  # Set time_t to 32 bit for compatibility with glibc
+  add_header(time_t HDR time_t.h SRCHDR time_t_32.h)
+else()
+  # Other platforms default to 64-bit time_t
+  add_header(time_t HDR time_t.h SRCHDR time_t_64.h)
+endif()
 add_header(stack_t HDR stack_t.h DEPENDS .size_t)
 add_header(suseconds_t HDR suseconds_t.h)
 add_header(struct_flock HDR struct_flock.h DEPENDS .off_t .pid_t)
diff --git a/libc/include/llvm-libc-types/time_t_32.h b/libc/include/llvm-libc-types/time_t_32.h
new file mode 100644
index 0000000000000..ce5a3e8b73463
--- /dev/null
+++ b/libc/include/llvm-libc-types/time_t_32.h
@@ -0,0 +1,14 @@
+//===-- Definition of the type time_t -------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TYPES_TIME_T_H
+#define LLVM_LIBC_TYPES_TIME_T_H
+
+typedef __INT32_TYPE__ time_t;
+
+#endif // LLVM_LIBC_TYPES_TIME_T_H
diff --git a/libc/include/llvm-libc-types/time_t.h b/libc/include/llvm-libc-types/time_t_64.h
similarity index 85%
rename from libc/include/llvm-libc-types/time_t.h
rename to libc/include/llvm-libc-types/time_t_64.h
index 59953b343ba96..c2c909e226144 100644
--- a/libc/include/llvm-libc-types/time_t.h
+++ b/libc/include/llvm-libc-types/time_t_64.h
@@ -9,10 +9,6 @@
 #ifndef LLVM_LIBC_TYPES_TIME_T_H
 #define LLVM_LIBC_TYPES_TIME_T_H
 
-#if (defined(__arm__) || defined(_M_ARM))
-typedef __INTPTR_TYPE__ time_t;
-#else
 typedef __INT64_TYPE__ time_t;
-#endif
 
 #endif // LLVM_LIBC_TYPES_TIME_T_H

lntue · 2024-08-06T04:36:33Z

libc/cmake/modules/LLVMLibCHeaderRules.cmake

@@ -21,7 +21,11 @@ function(add_header target_name)
  set(absolute_path ${CMAKE_CURRENT_SOURCE_DIR}/${ADD_HEADER_HDR})
  file(RELATIVE_PATH relative_path ${LIBC_INCLUDE_SOURCE_DIR} ${absolute_path})
  set(dest_file ${LIBC_INCLUDE_DIR}/${relative_path})
-  set(src_file ${CMAKE_CURRENT_SOURCE_DIR}/${ADD_HEADER_HDR})
+  if(ADD_HEADER_SRCHDR)


I think the whole project is kind of used to HDR/HDRS/SRCS are the source files. Can you change the argument to something like DEST_HDR, and update the dest_file/absolute_path instead?

lntue · 2024-08-06T04:41:53Z

libc/include/llvm-libc-types/CMakeLists.txt

@@ -58,7 +58,16 @@ add_header(pthread_rwlock_t HDR pthread_rwlock_t.h DEPENDS .__futex_word .pid_t)
 add_header(pthread_rwlockattr_t HDR pthread_rwlockattr_t.h)
 add_header(pthread_t HDR pthread_t.h DEPENDS .__thread_type)
 add_header(rlim_t HDR rlim_t.h)
-add_header(time_t HDR time_t.h)
+if(LIBC_CONF_TIME_64BIT)


nit:

if(LIBC_TARGET_ARCHITECTURE_IS_ARM AND NOT LIBC_CONF_TIME_64BIT) # Set time_t to 32-bit for compatibility with glibc add_header(time_t HDR time_t_32.h DEST_HDR time_t.h) else # Use 64-bit time_t add_header(time_t HDR time_t_32.h DEST_HDR time_t.h) endif()

lntue

This way of selecting headers to be copying over for public includes is probably cleanest for full build, but it will be a bit troublesome for proxy-headers and other headers inside libc/include that directly include llvm-libc-types/time_t.h.

For the proxy-header libc/src/types/time_t.h, you can use some flag like LIBC_TYPES_TIME_T_IS_64_BIT and LIBC_TYPES_TIME_T_IS_32_BIT to include the correct ones, and define it in common flags for objects: inside the if at https://github.com/llvm/llvm-project/blob/main/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake#L86 and similar one in the same file for tests.

For the other problem that other headers inside libc/include/ also directly include time_t, I can think of a solution, that is directly copy either time_t_64.h/time_t_32.h to libc/include/llvm-libc-types/time_t.h instead.

And I think this might be a simpler solution: is you do the selection and copy to time_t.h in the same folder, before add_header(time_t HDR time_t.h DEPENDS <copy_command>). And in that case, you will not need to update proxy headers or add extra compile options. WDYT?

lntue · 2024-08-06T05:06:14Z

Also, you might one to just leave a copy of libc/include/llvm-libc-types/time_t.h (same content as time_t_64.h) so that it won't break downstream projects with different build systems.

statham-arm · 2024-08-06T11:14:12Z

For the other problem that other headers inside libc/include/ also directly include time_t, I can think of a solution, that is directly copy either time_t_64.h/time_t_32.h to libc/include/llvm-libc-types/time_t.h instead.

And I think this might be a simpler solution: is you do the selection and copy to time_t.h in the same folder, before add_header(time_t HDR time_t.h DEPENDS <copy_command>). And in that case, you will not need to update proxy headers or add extra compile options. WDYT?

To check I've understood correctly, do you mean copying one of my two time_t header files to the path libc/include/llvm-libc-types/time_t.h in the source tree, rather than the build directory?

I'd rather avoid that if possible, because it's often useful to be able to do multiple out-of-tree builds from the same source directory with different configurations. So involving the source tree in your build in any way that is not read-only is dangerous.

(I was already a bit culture-shocked by the part of the libc build that auto-updated libc/docs/configure.rst when I edited config.json! But that only violates the letter of this principle, not the spirit: the change it made in the source directory isn't specific to any build configuration.)

But I take your point about headers in the source libc/include subdir transitively including each other in a way that doesn't pay attention to my selection operation in the build dir. Certainly there needs to be some answer to that.

When I first started looking at this problem, my first guess was that there would be an autogenerated header file called something like llvm-libc-config.h, similar to libcxx/include/__config_site, where you could write macro definitions of things discovered at cmake configure time. Then I could have written something into that file like #define __LIBC_COPT_TIME_T_IS_64_BIT, and time_t.h could have checked that ifdef instead of __arm__ (or perhaps as well) to decide on its time_t definition, and everyone would be happy -- that macro definition would be available to the libc build and to user applications including the headers. But as far as I can see there is no such config header in libc.

That said: I don't think I personally have any use case for wanting to do multiple out-of-tree builds from the same source directory with different sizes of time_t. I do want to do lots of builds from the same source dir, but they're all embedded, and I'll want all of them to have 64-bit time_t. So if you really think writing to the source dir is the best thing in this case, I don't think it'll inconvenience me too much.

lntue · 2024-08-06T11:47:46Z

I agree about the danger of modifying files in the source tree with build actions. In that case, I think the following would solve our problems:

Copy either time_t_32.h or time_t_64.h to public include time_t.h as in your current change.
Keep the current libc/include/llvm-libc-types/time_t.h, adding the condition && !defined(LIBC_TYPES_TIME_T_64_BIT) in addition to current arm target detection.
Add -DLIBC_TYPES_TIME_T_64_BIT to common object and test options in full build mode if LIBC_CONF_TIME_64BIT is on.
WDYT?

Now the decision results in LIBC_TYPES_TIME_T_IS_32_BIT being set in the cmake scripts. That controls which of time_t_{32,64}.h is copied to the output header time_t.h. But it also sets a flag on the compile command lines, which cause hdr/llvm-libc-types/time_t.h to #include the same one of those headers at library build time.

statham-arm · 2024-08-07T16:05:42Z

That sounds reasonable to me. I've pushed a patch that implements this, I think.

I've flipped it round so that the #define is used for a 32-bit time_t. That way the common case (and hopefully more common as we approach Y2038) is the default one, without needing an extra command-line option.

And I've centralised the cmake code that decides on the size of time_t, so that the same variable is used to control both the command-line #define and the choice of output time_t.h.

lntue · 2024-08-08T14:13:57Z

libc/cmake/modules/LLVMLibCCompileOptionRules.cmake

@@ -71,6 +71,10 @@ function(_get_compile_options_from_config output_var)
    list(APPEND config_options "-DLIBC_QSORT_IMPL=${LIBC_CONF_QSORT_IMPL}")
  endif()

+  if(LIBC_TYPES_TIME_T_IS_32_BIT)


We should only pass the compile flag in full build mode:

if(LIBC_TYPES_TIME_T_IS_32_BIT AND LLVM_LIBC_FULL_BUILD)

lntue · 2024-08-08T14:15:05Z

libc/include/llvm-libc-types/time_t_32.h

+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TYPES_TIME_T_H


This file and time_t_64.h are both using the same header guard as time_t.h, time_t will not be defined when included from time_t.h.

Good point – which must mean that nothing at all in the build is currently using this time_t.h to redirect to the right sub-header, or else my own test builds would have failed!

Did you try with full build mode? I found out the header guard issue from failed tests when trying your PR in full build mode.

Ah, it was in the tests? That explains it.

I'm mostly using full build mode, because I'm testing this libc on bare metal. But I haven't figured out how to run the tests in that setup yet, so that's why I didn't spot this. Good thing you did.

Yes, it's good that we can exercise the same code paths on multiple targets. It does help to find hidden issues.

nickdesaulniers · 2024-10-01T17:07:06Z

I suspect that we could just force time_t to be 64b always on 32b ARM as part of our own ABI.

statham-arm requested review from sivachandra and mikhailramalho August 5, 2024 16:36

llvmbot added the libc label Aug 5, 2024

mikhailramalho requested review from lntue and michaelrj-google August 5, 2024 16:44

lntue reviewed Aug 6, 2024

View reviewed changes

lntue reviewed Aug 8, 2024

View reviewed changes

Review nits: disambiguate guard macros, conditionalise -D

5131158

lntue approved these changes Aug 8, 2024

View reviewed changes

statham-arm merged commit 722c066 into llvm:main Aug 8, 2024
6 of 7 checks passed

statham-arm deleted the time_t-32bit-optional branch August 8, 2024 15:36

[libc] Make use of 32-bit time_t a config option #102012

[libc] Make use of 32-bit time_t a config option #102012

Uh oh!

Conversation

statham-arm commented Aug 5, 2024

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lntue left a comment

Choose a reason for hiding this comment

Uh oh!

lntue commented Aug 6, 2024

Uh oh!

statham-arm commented Aug 6, 2024

Uh oh!

lntue commented Aug 6, 2024

Uh oh!

statham-arm commented Aug 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickdesaulniers commented Oct 1, 2024

Uh oh!

Uh oh!