Skip to content

Conversation

mikhailramalho
Copy link
Member

@mikhailramalho mikhailramalho commented Aug 15, 2025

This PR modifies the static_asserts checking the expected sizes in __barrier_type.h, so that we can guarantee that our internal implementation fits the public header.

Signed-off-by: Mikhail R. Gadelha <[email protected]>
@llvmbot
Copy link
Member

llvmbot commented Aug 15, 2025

@llvm/pr-subscribers-libc

Author: Mikhail R. Gadelha (mikhailramalho)

Changes

This PR removes the hardcoded values for the sizeof(LIBC_NAMESPACE::CndVar) and sizeof(LIBC_NAMESPACE::Mutex) and replaces them with the actual calls to sizeof. Fixes compilation error in 32-bit systems.


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

1 Files Affected:

  • (modified) libc/include/llvm-libc-types/__barrier_type.h (+6-3)
diff --git a/libc/include/llvm-libc-types/__barrier_type.h b/libc/include/llvm-libc-types/__barrier_type.h
index 59712619e917d..23593372af56e 100644
--- a/libc/include/llvm-libc-types/__barrier_type.h
+++ b/libc/include/llvm-libc-types/__barrier_type.h
@@ -9,13 +9,16 @@
 #ifndef LLVM_LIBC_TYPES__BARRIER_TYPE_H
 #define LLVM_LIBC_TYPES__BARRIER_TYPE_H
 
+#include "src/__support/threads/CndVar.h"
+#include "src/__support/threads/mutex.h"
+
 typedef struct __attribute__((aligned(8 /* alignof (Barrier) */))) {
   unsigned expected;
   unsigned waiting;
   bool blocking;
-  char entering[24 /* sizeof (CndVar) */];
-  char exiting[24 /* sizeof (CndVar) */];
-  char mutex[24 /* sizeof (Mutex) */];
+  char entering[sizeof(LIBC_NAMESPACE::CndVar)];
+  char exiting[sizeof(LIBC_NAMESPACE::CndVar)];
+  char mutex[sizeof(LIBC_NAMESPACE::Mutex)];
 } __barrier_type;
 
 #endif // LLVM_LIBC_TYPES__BARRIER_TYPE_H

@frobtech
Copy link
Contributor

Sorry, this is a non-starter. The public header files in include/ cannot refer to anything outside include/. The whole point of these is to define the public ABI. The approach you'll need to take instead is to have the implementation code static_assert that the sizes and alignments maintained in the public header are always sufficient. If those assertions ever fail, it will have to be an explicit ABI-breaking manual change to update the public ABI.

Signed-off-by: Mikhail R. Gadelha <[email protected]>
@SchrodingerZhu
Copy link
Contributor

Yeah, more importantly, the headers are intended for C language. C++ namespace cannot work here.

@SchrodingerZhu
Copy link
Contributor

I am leaning to use __mutex_type

@mikhailramalho
Copy link
Member Author

Got it, I'll try to find another solution.

Signed-off-by: Mikhail R. Gadelha <[email protected]>
@mikhailramalho
Copy link
Member Author

Folks, I've changed the static_assert to check if the sizes are smaller than the ones defined in the public headers. WDYT?

I'll update the PR title before we land it, if this is accepted. The PR summary was updated.

@mikhailramalho
Copy link
Member Author

ping

@mikhailramalho mikhailramalho merged commit 2b1dcf5 into llvm:main Aug 21, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants