Skip to content

[RFC][libc] Codify header inclusion policy #87017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 11, 2024
Merged

[RFC][libc] Codify header inclusion policy #87017

merged 3 commits into from
Apr 11, 2024

Conversation

nickdesaulniers
Copy link
Member

This RFC is meant to spur discussion; it's not yet clear if this policy is best (or even makes sense).

When supporting "overlay" vs "fullbuild" modes, "what ABI are you using?" becomes a fundamental question to have concrete answers for. Overlay mode MUST match the ABI of the system being overlayed onto; fullbuild more flexible (the only system ABI relevant is the OS kernel).

When implementing llvm-libc we generally prefer the include-what-you use style of avoiding transitive dependencies (since that makes refactoring headers more painful, and slows down build times). So what header do you include for any given type or function declaration? For any given userspace program, the answer is straightforward. But for llvm-libc which is trying to support multiple ABIs (at least one per configuration), the answer is perhaps less clear.

This proposal seeks to add one layer of indirection relative to what's being done today.

It then converts users of sigset_t and struct epoll_event and the epoll implemenations over to this convention as an example.

@llvmbot llvmbot added libc bazel "Peripheral" support tier build system: utils/bazel labels Mar 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 28, 2024

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

This RFC is meant to spur discussion; it's not yet clear if this policy is best (or even makes sense).

When supporting "overlay" vs "fullbuild" modes, "what ABI are you using?" becomes a fundamental question to have concrete answers for. Overlay mode MUST match the ABI of the system being overlayed onto; fullbuild more flexible (the only system ABI relevant is the OS kernel).

When implementing llvm-libc we generally prefer the include-what-you use style of avoiding transitive dependencies (since that makes refactoring headers more painful, and slows down build times). So what header do you include for any given type or function declaration? For any given userspace program, the answer is straightforward. But for llvm-libc which is trying to support multiple ABIs (at least one per configuration), the answer is perhaps less clear.

This proposal seeks to add one layer of indirection relative to what's being done today.

It then converts users of sigset_t and struct epoll_event and the epoll implemenations over to this convention as an example.

Patch is 23.10 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87017.diff

31 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCCompileOptionRules.cmake (+2)
  • (modified) libc/config/linux/x86_64/entrypoints.txt (+1-1)
  • (modified) libc/docs/dev/code_style.rst (+27)
  • (modified) libc/docs/usage_modes.rst (+5-1)
  • (modified) libc/src/signal/CMakeLists.txt (+9)
  • (modified) libc/src/signal/linux/CMakeLists.txt (+9-7)
  • (modified) libc/src/signal/linux/raise.cpp (+3-2)
  • (modified) libc/src/signal/linux/sigaction.cpp (+3-4)
  • (modified) libc/src/signal/linux/sigaddset.cpp (+2-2)
  • (modified) libc/src/signal/linux/sigdelset.cpp (+2-2)
  • (modified) libc/src/signal/linux/sigfillset.cpp (+2-2)
  • (modified) libc/src/signal/linux/signal_utils.h (+2-1)
  • (modified) libc/src/signal/linux/sigprocmask.cpp (+3-3)
  • (modified) libc/src/signal/sigaddset.h (+1-1)
  • (modified) libc/src/signal/sigdelset.h (+1-1)
  • (modified) libc/src/signal/sigemptyset.h (+1-1)
  • (modified) libc/src/signal/sigfillset.h (+1-1)
  • (modified) libc/src/signal/sigprocmask.h (+1-1)
  • (added) libc/src/signal/sigset_t.h (+15)
  • (modified) libc/src/sys/epoll/CMakeLists.txt (+8)
  • (modified) libc/src/sys/epoll/epoll_pwait.h (+2-5)
  • (modified) libc/src/sys/epoll/epoll_pwait2.h (+5-8)
  • (modified) libc/src/sys/epoll/epoll_wait.h (+3-5)
  • (modified) libc/src/sys/epoll/linux/CMakeLists.txt (+25-4)
  • (modified) libc/src/sys/epoll/linux/epoll_pwait.cpp (+3-7)
  • (modified) libc/src/sys/epoll/linux/epoll_pwait2.cpp (+4-8)
  • (modified) libc/src/sys/epoll/linux/epoll_wait.cpp (+3-6)
  • (added) libc/src/sys/epoll/struct_epoll_event.h (+15)
  • (modified) libc/src/sys/select/linux/select.cpp (+3-2)
  • (added) libc/src/time/struct_timespec.h (+15)
  • (modified) utils/bazel/llvm-project-overlay/libc/BUILD.bazel (+13)
diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 5bc0898298ce39..b9d2c8b5108fdc 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -43,6 +43,7 @@ function(_get_common_compile_options output_var flags)
     list(APPEND compile_options "-fpie")
 
     if(LLVM_LIBC_FULL_BUILD)
+      list(APPEND compile_options "-DLIBC_FULL_BUILD")
       # Only add -ffreestanding flag in full build mode.
       list(APPEND compile_options "-ffreestanding")
     endif()
@@ -117,6 +118,7 @@ function(_get_common_test_compile_options output_var c_test flags)
     list(APPEND compile_options "-fpie")
 
     if(LLVM_LIBC_FULL_BUILD)
+      list(APPEND compile_options "-DLIBC_FULL_BUILD")
       # Only add -ffreestanding flag in full build mode.
       list(APPEND compile_options "-ffreestanding")
       list(APPEND compile_options "-fno-exceptions")
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 5b428e51aee620..949317f5048d96 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -219,7 +219,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.sys.epoll.epoll_pwait
     # TODO: Need to check if pwait2 is available before providing.
     # https://github.com/llvm/llvm-project/issues/80060
-    # libc.src.sys.epoll.epoll_pwait2
+    libc.src.sys.epoll.epoll_pwait2
 
     # sys/mman.h entrypoints
     libc.src.sys.mman.madvise
diff --git a/libc/docs/dev/code_style.rst b/libc/docs/dev/code_style.rst
index 22a18b7a4cc1dd..22b7671775a1f5 100644
--- a/libc/docs/dev/code_style.rst
+++ b/libc/docs/dev/code_style.rst
@@ -186,3 +186,30 @@ We expect contributions to be free of warnings from the `minimum supported
 compiler versions`__ (and newer).
 
 .. __: https://libc.llvm.org/compiler_support.html#minimum-supported-versions
+
+Header Inclusion Policy
+=======================
+
+Because llvm-libc supports
+`Overlay Mode <https://libc.llvm.org/overlay_mode.html>`__ and
+`Fullbuild Mode <https://libc.llvm.org/fullbuild_mode.html>`__ care must be
+taken when ``#include``'ing certain headers.
+
+The ``include/`` directory contains public facing headers that users must
+consume for fullbuild mode. As such, types defined here will have ABI
+implications as these definitions may differ from the underlying system for
+overlay mode and are NEVER appropriate to include in ``libc/src/`` without
+preprocessor guards for ``LLVM_LIBC_FULL_BUILD``.
+
+Consider the case where an implementation in ``libc/src/`` may wish to refer to
+a ``sigset_t``, what header should be included? ``<signal.h>``, ``<spawn.h>``,
+``<sys/select.h>``?
+
+None of the above. Instead, code under ``src/`` should ``#include
+"src/signal/sigset_t.h"`` which contains preprocessor guards on
+``LLVM_LIBC_FULL_BUILD`` to either include the public type (fullbuild mode) or
+the underlying system header (overlay mode).
+
+Implementations in ``libc/src/`` should NOT be ``#include``'ing using ``<>`` or
+``"include/*``, except for these "proxy" headers that first check for
+``LLVM_LIBC_FULL_BUILD``.
diff --git a/libc/docs/usage_modes.rst b/libc/docs/usage_modes.rst
index 11c10623b61dba..8e5dcca6e0a75c 100644
--- a/libc/docs/usage_modes.rst
+++ b/libc/docs/usage_modes.rst
@@ -6,6 +6,10 @@ The libc can used in two different modes:
 
 #. The **overlay** mode: In this mode, the link order semantics are exploited
    to overlay implementations from LLVM's libc over the system libc. See
-   :ref:`overlay_mode` for more information about this mode.
+   :ref:`overlay_mode` for more information about this mode. In this mode, libc
+   uses the ABI of the system it's being overlayed onto. Headers are NOT
+   generated. libllvmlibc.a is the only build artifact.
 #. The **fullbuild** mode: In this mode, LLVM's libc is used as the only libc
    for the binary. See :ref:`fullbuild_mode` for information about this mode.
+   In this mode, libc uses its own ABI. Headers are generated along with a
+   libc.a.
diff --git a/libc/src/signal/CMakeLists.txt b/libc/src/signal/CMakeLists.txt
index c70ab952b9950f..e1dc32f1fff2ae 100644
--- a/libc/src/signal/CMakeLists.txt
+++ b/libc/src/signal/CMakeLists.txt
@@ -71,3 +71,12 @@ add_entrypoint_object(
   DEPENDS
     .${LIBC_TARGET_OS}.sigdelset
 )
+
+add_header_library(
+  sigset_t
+  HDRS
+    sigset_t.h
+  DEPENDS
+    # TODO: this dependency is only for fullbuild mode...
+    libc.include.llvm-libc-types.sigset_t
+  )
diff --git a/libc/src/signal/linux/CMakeLists.txt b/libc/src/signal/linux/CMakeLists.txt
index 77a2453b25a0ac..a5a8de30e325b9 100644
--- a/libc/src/signal/linux/CMakeLists.txt
+++ b/libc/src/signal/linux/CMakeLists.txt
@@ -3,8 +3,10 @@ add_header_library(
   HDRS
     signal_utils.h
   DEPENDS
+    libc.include.signal
     libc.include.sys_syscall
     libc.src.__support.OSUtil.osutil
+    libc.src.signal.sigset_t
 )
 
 add_entrypoint_object(
@@ -28,9 +30,9 @@ add_entrypoint_object(
     ../raise.h
   DEPENDS
     .signal_utils
-    libc.include.signal
     libc.include.sys_syscall
     libc.src.__support.OSUtil.osutil
+    libc.src.signal.sigset_t
 )
 
 add_object_library(
@@ -57,10 +59,10 @@ add_entrypoint_object(
     ../sigaction.h
   DEPENDS
     .__restore
-    libc.include.signal
     libc.include.sys_syscall
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
+    libc.src.signal.sigset_t
 )
 
 add_entrypoint_object(
@@ -84,10 +86,10 @@ add_entrypoint_object(
     ../sigprocmask.h
   DEPENDS
     .signal_utils
-    libc.include.signal
     libc.include.sys_syscall
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
+    libc.src.signal.sigset_t
 )
 
 add_entrypoint_object(
@@ -98,8 +100,8 @@ add_entrypoint_object(
     ../sigemptyset.h
   DEPENDS
     .signal_utils
-    libc.include.signal
     libc.src.errno.errno
+    libc.src.signal.sigset_t
 )
 
 add_entrypoint_object(
@@ -110,8 +112,8 @@ add_entrypoint_object(
     ../sigaddset.h
   DEPENDS
     .signal_utils
-    libc.include.signal
     libc.src.errno.errno
+    libc.src.signal.sigset_t
 )
 
 add_entrypoint_object(
@@ -133,8 +135,8 @@ add_entrypoint_object(
     ../sigfillset.h
   DEPENDS
     .signal_utils
-    libc.include.signal
     libc.src.errno.errno
+    libc.src.signal.sigset_t
 )
 
 add_entrypoint_object(
@@ -145,6 +147,6 @@ add_entrypoint_object(
     ../sigdelset.h
   DEPENDS
     .signal_utils
-    libc.include.signal
     libc.src.errno.errno
+    libc.src.signal.sigset_t
 )
diff --git a/libc/src/signal/linux/raise.cpp b/libc/src/signal/linux/raise.cpp
index dd6f5eb4b35754..e533a2d212c909 100644
--- a/libc/src/signal/linux/raise.cpp
+++ b/libc/src/signal/linux/raise.cpp
@@ -7,14 +7,15 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/signal/raise.h"
-#include "src/signal/linux/signal_utils.h"
 
 #include "src/__support/common.h"
+#include "src/signal/linux/signal_utils.h"
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
 LLVM_LIBC_FUNCTION(int, raise, (int sig)) {
-  ::sigset_t sigset;
+  sigset_t sigset;
   block_all_signals(sigset);
   long pid = LIBC_NAMESPACE::syscall_impl<long>(SYS_getpid);
   long tid = LIBC_NAMESPACE::syscall_impl<long>(SYS_gettid);
diff --git a/libc/src/signal/linux/sigaction.cpp b/libc/src/signal/linux/sigaction.cpp
index 7ddc2dc5cbcc70..7daf88a4f322e3 100644
--- a/libc/src/signal/linux/sigaction.cpp
+++ b/libc/src/signal/linux/sigaction.cpp
@@ -7,12 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/signal/sigaction.h"
-#include "src/errno/libc_errno.h"
-#include "src/signal/linux/signal_utils.h"
 
 #include "src/__support/common.h"
-
-#include <signal.h>
+#include "src/errno/libc_errno.h"
+#include "src/signal/linux/signal_utils.h"
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/linux/sigaddset.cpp b/libc/src/signal/linux/sigaddset.cpp
index 536391734e0587..f1264d10971d4e 100644
--- a/libc/src/signal/linux/sigaddset.cpp
+++ b/libc/src/signal/linux/sigaddset.cpp
@@ -7,11 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/signal/sigaddset.h"
+
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 #include "src/signal/linux/signal_utils.h"
-
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/linux/sigdelset.cpp b/libc/src/signal/linux/sigdelset.cpp
index 5cb645e461cf8e..1eed6d1501b58b 100644
--- a/libc/src/signal/linux/sigdelset.cpp
+++ b/libc/src/signal/linux/sigdelset.cpp
@@ -7,11 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/signal/sigdelset.h"
+
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 #include "src/signal/linux/signal_utils.h"
-
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/linux/sigfillset.cpp b/libc/src/signal/linux/sigfillset.cpp
index e17c85a897ce74..ccf9ced10024c8 100644
--- a/libc/src/signal/linux/sigfillset.cpp
+++ b/libc/src/signal/linux/sigfillset.cpp
@@ -7,11 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/signal/sigfillset.h"
+
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 #include "src/signal/linux/signal_utils.h"
-
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/linux/signal_utils.h b/libc/src/signal/linux/signal_utils.h
index 5e9dd9a5c53ab0..30ae0b95f678f8 100644
--- a/libc/src/signal/linux/signal_utils.h
+++ b/libc/src/signal/linux/signal_utils.h
@@ -11,8 +11,9 @@
 
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
+#include "src/signal/sigset_t.h"
 
-#include <signal.h>
+#include <signal.h> // sigaction
 #include <stddef.h>
 #include <sys/syscall.h>          // For syscall numbers.
 
diff --git a/libc/src/signal/linux/sigprocmask.cpp b/libc/src/signal/linux/sigprocmask.cpp
index 79a35dd59d75c8..f347f28a8f5d06 100644
--- a/libc/src/signal/linux/sigprocmask.cpp
+++ b/libc/src/signal/linux/sigprocmask.cpp
@@ -7,13 +7,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/signal/sigprocmask.h"
+
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 #include "src/signal/linux/signal_utils.h"
+#include "src/signal/sigset_t.h"
 
-#include "src/__support/common.h"
-
-#include <signal.h>
 #include <sys/syscall.h> // For syscall numbers.
 
 namespace LIBC_NAMESPACE {
diff --git a/libc/src/signal/sigaddset.h b/libc/src/signal/sigaddset.h
index 626eb20a295c83..1e57609b501d5f 100644
--- a/libc/src/signal/sigaddset.h
+++ b/libc/src/signal/sigaddset.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC_SIGNAL_SIGADDSET_H
 #define LLVM_LIBC_SRC_SIGNAL_SIGADDSET_H
 
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/sigdelset.h b/libc/src/signal/sigdelset.h
index c4fdb9975fa3d0..649546baa98845 100644
--- a/libc/src/signal/sigdelset.h
+++ b/libc/src/signal/sigdelset.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC_SIGNAL_SIGDELSET_H
 #define LLVM_LIBC_SRC_SIGNAL_SIGDELSET_H
 
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/sigemptyset.h b/libc/src/signal/sigemptyset.h
index f3763d1f4f3d44..0f7f87cef28cd4 100644
--- a/libc/src/signal/sigemptyset.h
+++ b/libc/src/signal/sigemptyset.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC_SIGNAL_SIGEMPTYSET_H
 #define LLVM_LIBC_SRC_SIGNAL_SIGEMPTYSET_H
 
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/sigfillset.h b/libc/src/signal/sigfillset.h
index d8e3168871ea8a..35500f1f792bde 100644
--- a/libc/src/signal/sigfillset.h
+++ b/libc/src/signal/sigfillset.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC_SIGNAL_SIGFILLSET_H
 #define LLVM_LIBC_SRC_SIGNAL_SIGFILLSET_H
 
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/sigprocmask.h b/libc/src/signal/sigprocmask.h
index e0658860579e4e..fea8e8ca82a6b1 100644
--- a/libc/src/signal/sigprocmask.h
+++ b/libc/src/signal/sigprocmask.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC_SIGNAL_SIGPROCMASK_H
 #define LLVM_LIBC_SRC_SIGNAL_SIGPROCMASK_H
 
-#include <signal.h>
+#include "src/signal/sigset_t.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/signal/sigset_t.h b/libc/src/signal/sigset_t.h
new file mode 100644
index 00000000000000..60f9eeeb8aeb8a
--- /dev/null
+++ b/libc/src/signal/sigset_t.h
@@ -0,0 +1,15 @@
+// TODO: license block
+#ifndef LLVM_LIBC_SRC_SIGNAL_SIGSET_T_H
+#define LLVM_LIBC_SRC_SIGNAL_SIGSET_T_H
+
+#ifdef LIBC_FULL_BUILD
+
+#include "include/llvm-libc-types/sigset_t.h"
+
+#else
+
+#include <signal.h>
+
+#endif // LIBC_FULL_BUILD
+
+#endif // LLVM_LIBC_SRC_SIGNAL_SIGSET_T_H
diff --git a/libc/src/sys/epoll/CMakeLists.txt b/libc/src/sys/epoll/CMakeLists.txt
index d4991a238e2a77..d0efeb9a588c73 100644
--- a/libc/src/sys/epoll/CMakeLists.txt
+++ b/libc/src/sys/epoll/CMakeLists.txt
@@ -2,6 +2,14 @@ if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
   add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
 endif()
 
+add_header_library(
+  struct_epoll_event
+  HDRS
+    struct_epoll_event.h
+  DEPENDS
+    libc.include.llvm-libc-types.struct_epoll_event
+)
+
 add_entrypoint_object(
   epoll_wait
   ALIAS
diff --git a/libc/src/sys/epoll/epoll_pwait.h b/libc/src/sys/epoll/epoll_pwait.h
index 9dcb55533009f9..1b832718de8a28 100644
--- a/libc/src/sys/epoll/epoll_pwait.h
+++ b/libc/src/sys/epoll/epoll_pwait.h
@@ -9,11 +9,8 @@
 #ifndef LLVM_LIBC_SRC_SYS_EPOLL_EPOLL_PWAIT_H
 #define LLVM_LIBC_SRC_SYS_EPOLL_EPOLL_PWAIT_H
 
-// TODO: Use this include once the include headers are also using quotes.
-// #include "include/llvm-libc-types/sigset_t.h"
-// #include "include/llvm-libc-types/struct_epoll_event.h"
-
-#include <sys/epoll.h>
+#include "src/signal/sigset_t.h"
+#include "src/sys/epoll/struct_epoll_event.h"
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/sys/epoll/epoll_pwait2.h b/libc/src/sys/epoll/epoll_pwait2.h
index 622ede6a0f9f9a..3fd598f73f993f 100644
--- a/libc/src/sys/epoll/epoll_pwait2.h
+++ b/libc/src/sys/epoll/epoll_pwait2.h
@@ -9,18 +9,15 @@
 #ifndef LLVM_LIBC_SRC_SYS_EPOLL_EPOLL_PWAIT2_H
 #define LLVM_LIBC_SRC_SYS_EPOLL_EPOLL_PWAIT2_H
 
-// TODO: Use this include once the include headers are also using quotes.
-// #include "include/llvm-libc-types/sigset_t.h"
-// #include "include/llvm-libc-types/struct_epoll_event.h"
-// #include "include/llvm-libc-types/struct_timespec.h"
-
-#include <sys/epoll.h>
+#include "src/signal/sigset_t.h"
+#include "src/sys/epoll/struct_epoll_event.h"
+#include "src/time/struct_timespec.h"
 
 namespace LIBC_NAMESPACE {
 
 // TODO: sigmask and timeout should be nullable
-int epoll_pwait2(int epfd, epoll_event *events, int maxevents,
-                 const timespec *timeout, const sigset_t *sigmask);
+int epoll_pwait2(int epfd, struct epoll_event *events, int maxevents,
+                 const struct timespec *timeout, const sigset_t *sigmask);
 
 } // namespace LIBC_NAMESPACE
 
diff --git a/libc/src/sys/epoll/epoll_wait.h b/libc/src/sys/epoll/epoll_wait.h
index d51c9100846ce0..0cd2f7e9b57ad0 100644
--- a/libc/src/sys/epoll/epoll_wait.h
+++ b/libc/src/sys/epoll/epoll_wait.h
@@ -9,14 +9,12 @@
 #ifndef LLVM_LIBC_SRC_SYS_EPOLL_EPOLL_WAIT_H
 #define LLVM_LIBC_SRC_SYS_EPOLL_EPOLL_WAIT_H
 
-// TODO: Use this include once the include headers are also using quotes.
-// #include "include/llvm-libc-types/struct_epoll_event.h"
-
-#include <sys/epoll.h>
+#include "src/sys/epoll/struct_epoll_event.h"
 
 namespace LIBC_NAMESPACE {
 
-int epoll_wait(int epfd, epoll_event *events, int maxevents, int timeout);
+int epoll_wait(int epfd, struct epoll_event *events, int maxevents,
+               int timeout);
 
 } // namespace LIBC_NAMESPACE
 
diff --git a/libc/src/sys/epoll/linux/CMakeLists.txt b/libc/src/sys/epoll/linux/CMakeLists.txt
index a27905d962dc57..108c6b9bb05301 100644
--- a/libc/src/sys/epoll/linux/CMakeLists.txt
+++ b/libc/src/sys/epoll/linux/CMakeLists.txt
@@ -3,12 +3,19 @@ add_entrypoint_object(
   SRCS
     epoll_wait.cpp
   HDRS
+    # TODO: these 2 should not be necessary
+    ../../../signal/sigset_t.h
+    ../../../time/struct_timespec.h
+
     ../epoll_wait.h
   DEPENDS
-    libc.include.sys_epoll
     libc.include.sys_syscall
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
+    libc.src.sys.epoll.struct_epoll_event
+    # TODO: why don't these work for overlay mode?
+    # libc.src.signal.sigset_t
+    # libc.src.time.struct_timespec
 )
 
 add_entrypoint_object(
@@ -16,13 +23,20 @@ add_entrypoint_object(
   SRCS
     epoll_pwait.cpp
   HDRS
+    # TODO: these 2 should not be necessary
+    ../../../signal/sigset_t.h
+    ../../../time/struct_timespec.h
+
     ../epoll_pwait.h
   DEPENDS
-    libc.include.sys_epoll
     libc.include.signal
     libc.include.sys_syscall
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
+    libc.src.sys.epoll.struct_epoll_event
+    # TODO: why don't these work for overlay mode?
+    # libc.src.signal.sigset_t
+    # libc.src.time.struct_timespec
 )
 
 add_entrypoint_object(
@@ -30,12 +44,19 @@ add_entrypoint_object(
   SRCS
     epoll_pwait2.cpp
   HDRS
+    # TODO: these 2 should not be necessary
+    ../../../signal/sigset_t.h
+    ../../../time/struct_timespec.h
+
     ../epoll_pwait2.h
   DEPENDS
-    libc.include.sys_epoll
     libc.include.signal
-    libc.include.time
     libc.include.sys_syscall
+    libc.include.time
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
+    libc.src.sys.epoll.struct_epoll_event
+    # TODO: why don't these work for overlay mode?
+    # libc.src.signal.sigset_t
+    # libc.src.time.struct_timespec
 )
diff --git a/libc/src/sys/epoll/linux/epoll_pwait.cpp b/libc/src/sys/epoll/linux/epoll_pwait.cpp
index ee1b4e66e98444..7e0d6988b02ff3 100644
--- a/libc/src/sys/epoll/linux/epoll_pwait.cpp
+++ b/libc/src/sys/epoll/linux/epoll_pwait.cpp
@@ -10,15 +10,11 @@
 
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
-
 #include "src/errno/libc_errno.h"
-#include <sys/syscall.h> // For syscall numbers.
+#include "src/signal/sigset_t.h"
+#include "src/sys/epoll/struct_epoll_event.h"
 
-// TODO: Use this include once the include headers are also using quotes.
-// #include "include/llvm-libc-types/sigset_t.h"
-// #include "include/llvm-libc-types/struct_epoll_event.h"
-
-#include <sys/epoll.h>
+#include <sys/syscall.h> // For syscall numbers.
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/sys/epoll/linux/epoll_pwait2.cpp b/libc/src/sys/epoll/linux/epoll_pwait2.cpp
index 671dede2a1058d..f97804cfe48b4e 100644
--- a/libc/src/sys/epoll/linux/epoll_pwait2.cpp
+++ b/libc/src/sys/epoll/linux/epoll_pwait2.cpp
@@ -10,16 +10,12 @@
 
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
-
 #include "src/errno/libc_errno.h"
-#include <sys/syscall.h> // For syscall numbers.
+#include "src/signal/sigset_t.h"
+#include "src/sys/epoll/struct_epoll_event.h"
+#include "src/time/struct_timespec.h"
 
-// TODO: Use this include once the include headers are also using quotes.
-// #include "include/llvm-libc-types/sigset_t.h"
-// #include "include/llvm-libc-types/struct_epoll_event.h"
-// #include "include/llvm-libc-types/struct_timespec.h"
-
-#include <sys/epoll.h>
+#include <sys/syscall.h> // For syscall numbers.
 
 namespace LIBC_NAMESPACE {
 
diff --git a/libc/src/sys/epoll/linux/epoll_wait.cpp b/libc/src/sys/epoll/linux/epoll_wait.cpp
index 0c43edf7645454..a025bfb2e8d783 100644
--- a/libc/src/sys/epoll/linux/epoll_wait.cpp
+++ b/libc/src/sys/epoll/linux/epoll_wait.cpp
@@ -11,13 +11,10 @@
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
-#include <sys/syscall.h> // Fo...
[truncated]

@nickdesaulniers
Copy link
Member Author

I'm not sure I'm happy with the result, but I think it's as close as possible to what was discussed yesterday.

michaelrj-google
michaelrj-google reviewed Mar 28, 2024
View reviewed changes
Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the design overall, but I have a few comments

libc/cmake/modules/LLVMLibCCompileOptionRules.cmake Outdated
@@ -43,6 +43,7 @@ function(_get_common_compile_options output_var flags)
list(APPEND compile_options "-fpie")

if(LLVM_LIBC_FULL_BUILD)
list(APPEND compile_options "-DLIBC_FULL_BUILD")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want the default state if the user doesn't set any build flags to be fullbuild or overlay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the default state is overlay mode, and probably we won't be able to change the default until our implementation is complete enough, and most of our downstream users switch to full build mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cmake I agree that leaving the default as overlay mode for now makes sense, but in the code I think if no compile flags are set it should default to fullbuild. My guess is that once we make this choice we won't want to switch later, and I think people creating their own build systems will want fullbuild eventually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possibly still worth having this conversation, but the diff this thread is off of was made obsolete by #87598. File a new bug if you'd like to discuss this more?

@SchrodingerZhu
Copy link
Contributor

Just noticed that having intermediate layer also address the ABI problem: the implementation source knows what struct are provided.

This was referenced Apr 3, 2024
Merged
Merged
@nickdesaulniers
Copy link
Member Author

See also #87598.

This was referenced Apr 4, 2024
Merged
Merged
lntue added a commit that referenced this pull request Apr 5, 2024
@lntue
[libc] Add proxy header math_macros.h. (#87598)
5748ad8 
Context: #87017

- Add proxy header `libc/hdr/math_macros.h` that will:
  - include `<math.h>` in overlay mode,
- include `"include/llvm-libc-macros/math-macros.h"` in full build mode.
- Its corresponding CMake target `libc.hdr.math_macros` will only depend
on `libc.include.math` and `libc.include.llvm-libc-macros.math_macros`
in full build mode.
- Replace all `#include "include/llvm-libc-macros/math-macros.h"` with
`#include "hdr/math_macros.h"`.
- Add dependency to `libc.hdr.math_macros` CMake target when using
`add_fp_unittest`.
- Update the remaining dependency.
- Update bazel overlay: add `libc:hdr_math_macros` target, and replacing
all dependency on `libc:llvm_libc_macros_math_macros` with
`libc:hdr_math_macros`.
This was referenced Apr 6, 2024
Closed
Closed
@nickdesaulniers
[RFC][libc] Codify header inclusion policy
4e2833a 
This RFC is meant to spur discussion; it's not yet clear if this policy is best
(or even makes sense).

When supporting "overlay" vs "fullbuild" modes, "what ABI are you using?"
becomes a fundamental question to have concrete answers for. Overlay mode MUST
match the ABI of the system being overlayed onto; fullbuild more flexible (the
only system ABI relevant is the OS kernel).

When implementing llvm-libc we generally prefer the include-what-you use style
of avoiding transitive dependencies (since that makes refactoring headers more
painful, and slows down build times). So what header do you include for any
given type or function declaration? For any given userspace program, the answer
is straightforward. But for llvm-libc which is trying to support multiple ABIs
(at least one per configuration), the answer is perhaps less clear.

This proposal seeks to add one layer of indirection relative to what's being
done today.

It then converts users of sigset_t, struct epoll_event, and struct timespec
over to this convention as an example.
lntue
lntue reviewed Apr 10, 2024
View reviewed changes
lntue
lntue reviewed Apr 10, 2024
View reviewed changes
lntue
lntue approved these changes Apr 11, 2024
View reviewed changes
Copy link
Contributor

@lntue lntue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with just one nit(?) about extra struct's.

@nickdesaulniers nickdesaulniers merged commit f626a35 into llvm:main Apr 11, 2024
@nickdesaulniers nickdesaulniers deleted the brave_new_world2 branch April 11, 2024 17:12
@lntue lntue mentioned this pull request Apr 11, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelrj-google michaelrj-google michaelrj-google left review comments

@lntue lntue lntue approved these changes

@gchatelet gchatelet Awaiting requested review from gchatelet

@petrhosek petrhosek Awaiting requested review from petrhosek

@frobtech frobtech Awaiting requested review from frobtech

@SchrodingerZhu SchrodingerZhu Awaiting requested review from SchrodingerZhu

@jhuber6 jhuber6 Awaiting requested review from jhuber6

@rupprecht rupprecht Awaiting requested review from rupprecht rupprecht is a code owner

Assignees
No one assigned
Labels
bazel "Peripheral" support tier build system: utils/bazel libc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nickdesaulniers @llvmbot @SchrodingerZhu @lntue @michaelrj-google