[libc] move fork into threads folder

[michaelrj]

Summary

Fork, as a thread function, should go in the threads folder.
Additionally, it depends on the thread mutex, and it was causing build
issues for targets where we don't support threads.

[sivachandra]

I did not go into the specifics of this patch (I am on vacation) but I have concern about it because of the subject:

Fork, as a thread function, should go in the threads folder.
Additionally, it depends on the thread mutex, and it was causing build
issues for targets where we don't support threads.

Two things:

1. Why is fork a thread function?
2. If the logic of "it depends on the thread mutex" is applied to other parts of the libc (for example File), they all should be moved into the threads directory.

[michaelrj]

I meant that the fork_callbacks utility should be moved into the threads folder since it depends on the other utilities in the threads folder and is only used by the pthreads utilities. I think having file be separate makes sense because that's a category that is more specific than threads. The goal is to group similar things so that they can be found, and while I would expect fork related utilities to be in a threads folder, I wouldn't expect file related utilities to be in a threads folder.

[sivachandra]

In D137867#3926026, @michaelrj wrote:

I meant that the fork_callbacks utility should be moved into the threads folder since it depends on the other utilities in the threads folder and is only used by the pthreads utilities. I think having file be separate makes sense because that's a category that is more specific than threads. The goal is to group similar things so that they can be found, and while I would expect fork related utilities to be in a threads folder, I wouldn't expect file related utilities to be in a threads folder.

I still don't understand - perhaps you are thinking fork is related to threads? Also, can you explain what prompted this change? Was something failing on arm32?

[lntue]

In D137867#3926145, @sivachandra wrote:

In D137867#3926026, @michaelrj wrote:

I meant that the fork_callbacks utility should be moved into the threads folder since it depends on the other utilities in the threads folder and is only used by the pthreads utilities. I think having file be separate makes sense because that's a category that is more specific than threads. The goal is to group similar things so that they can be found, and while I would expect fork related utilities to be in a threads folder, I wouldn't expect file related utilities to be in a threads folder.

I still don't understand - perhaps you are thinking fork is related to threads? Also, can you explain what prompted this change? Was something failing on arm32?

It was actually blocking macos builds.

[michaelrj]

yes, the windows build was failing (and anything else that doesn't have a threads implementation) because the fork_callbacks cmake target was including mutex unconditionally.

[sivachandra]

In D137867#3926258, @michaelrj wrote:

yes, the windows build was failing (and anything else that doesn't have a threads implementation) because the fork_callbacks cmake target was including mutex unconditionally.

The fork callback machinery uses a mutex and is not related to threads. So, the pattern to follow would be this: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/File/CMakeLists.txt#L1. Which is to exclude listing a target if mutex is not available. This patch is essentially doing that, but clubbing fork callbacks machinery with thread machinery is incorrect.

[lntue]

In D137867#3926310, @sivachandra wrote:

In D137867#3926258, @michaelrj wrote:

yes, the windows build was failing (and anything else that doesn't have a threads implementation) because the fork_callbacks cmake target was including mutex unconditionally.

The fork callback machinery uses a mutex and is not related to threads. So, the pattern to follow would be this: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/File/CMakeLists.txt#L1. Which is to exclude listing a target if mutex is not available. This patch is essentially doing that, but clubbing fork callbacks machinery with thread machinery is incorrect.

Putting fork_callback inside the target check (without the return part) and adding message Skipping ... with reason similar to unit tests sounds good to me.

libc/src/__support/CMakeLists.txt

DEPENDS
     .uint128
     libc.include.errno
     libc.src.__support.CPP.limits
     libc.src.__support.FPUtil.fp_bits
     libc.src.__support.builtin_wrappers
     libc.src.errno.errno
 )
 
-add_object_library(
-  fork_callbacks
-  SRCS
-    fork_callbacks.cpp
-  HDRS
-    fork_callbacks.h
-  DEPENDS
-    .threads.mutex
-)
 
 add_header_library(
   integer_operations
   HDRS
     integer_operations.h
 )
 
 add_header_library(

libc/src/__support/threads/CMakeLists.txt

 if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.mutex)
   add_header_library(
     mutex
     HDRS
       mutex.h
     DEPENDS
       .${LIBC_TARGET_OS}.mutex
   )
+
+  add_object_library(
+    fork_callbacks
+    SRCS
+      fork_callbacks.cpp
+    HDRS
+      fork_callbacks.h
+    DEPENDS
+      .mutex
+  )
 endif()
 
 add_header_library(
   thread_common
   HDRS
     thread.h
   DEPENDS
     libc.src.__support.common

libc/src/__support/threads/fork_callbacks.h

 //===-- At-fork callback helpers  -------------------------------*- C++ -*-===//
 //
 // 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_SRC_SUPPORT_THREAD_FORK_CALLBACKS_H
+#define LLVM_LIBC_SRC_SUPPORT_THREAD_FORK_CALLBACKS_H
+
 namespace __llvm_libc {
 
 using ForkCallback = void(void);
 
 bool register_atfork_callbacks(ForkCallback *prepare_cd,
                                ForkCallback *parent_cb, ForkCallback *child_cb);
 void invoke_prepare_callbacks();
 void invoke_parent_callbacks();
 void invoke_child_callbacks();
 
 } // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_SUPPORT_THREAD_FORK_CALLBACKS_H

libc/src/pthread/CMakeLists.txt

add_entrypoint_object(
   pthread_atfork
   SRCS
     pthread_atfork.cpp
   HDRS
     pthread_atfork.h
   DEPENDS
     libc.include.errno
     libc.include.pthread
-    libc.src.__support.fork_callbacks
+    libc.src.__support.threads.fork_callbacks
 )

libc/src/pthread/pthread_atfork.cpp

 //===-- Linux implementation of the pthread_atfork function ---------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "pthread_atfork.h"
 
 #include "src/__support/common.h"
-#include "src/__support/fork_callbacks.h"
+#include "src/__support/threads/fork_callbacks.h"
 
 #include <errno.h>
 #include <pthread.h> // For pthread_* type definitions.
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(int, pthread_atfork,
                    (__atfork_callback_t prepare, __atfork_callback_t parent,
                     __atfork_callback_t child)) {
   return register_atfork_callbacks(prepare, parent, child) ? 0 : ENOMEM;
 }
 
 } // namespace __llvm_libc

libc/src/unistd/linux/CMakeLists.txt

add_entrypoint_object(
   SRCS
     fork.cpp
   HDRS
     ../fork.h
   DEPENDS
     libc.include.errno
     libc.include.unistd
     libc.include.sys_syscall
-    libc.src.__support.fork_callbacks
+    libc.src.__support.threads.fork_callbacks
     libc.src.__support.OSUtil.osutil
     libc.src.__support.threads.thread
     libc.src.errno.errno
 )
 
 add_entrypoint_object(
   execv
   SRCS

libc/src/unistd/linux/fork.cpp

 //===-- Linux implementation of fork --------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/unistd/fork.h"
 
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
-#include "src/__support/fork_callbacks.h"
+#include "src/__support/threads/fork_callbacks.h"
 #include "src/__support/threads/thread.h" // For thread self object
 
 #include <errno.h>
 #include <sys/syscall.h> // For syscall numbers.
 
 namespace __llvm_libc {
 
 // The implementation of fork here is very minimal. We will add more