[libc++] Add a thread-safe version of std::lgamma in the dylib #153631

ldionne · 2025-08-14T17:37:25Z

Libc++ currently redeclares ::lgamma_r on platforms that provide it. This causes issues when building with modules, and redeclaring functions provided by another library (here the C library) is bad hygiene.

Instead, introduce our own version of a thread-safe gamma function inside the dylib and fall back on ::lgamma_r without redeclaring it when we have a back-deployment constraint.

llvmbot · 2025-08-14T17:38:00Z

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This causes issues when building with modules, and redeclaring functions provided by another library (here the C library) is bad hygiene. Instead, use <math.h> to access the declaration provided by the system.

As a drive-by, this also starts using ::lgamma_r instead of ::lgamma when building on top of LLVM-libc. We didn't do that previously due to a declration conflict, but using the _r version when we can should only make things thread safe.

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

1 Files Affected:

(modified) libcxx/include/__random/binomial_distribution.h (+4-8)

diff --git a/libcxx/include/__random/binomial_distribution.h b/libcxx/include/__random/binomial_distribution.h
index b4b4340827761..ab0da2de38b89 100644
--- a/libcxx/include/__random/binomial_distribution.h
+++ b/libcxx/include/__random/binomial_distribution.h
@@ -14,6 +14,7 @@
 #include <__random/uniform_real_distribution.h>
 #include <cmath>
 #include <iosfwd>
+#include <math.h> // for ::lgamma_r
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -97,17 +98,12 @@ class binomial_distribution {
   }
 };
 
-// The LLVM C library provides this with conflicting `noexcept` attributes.
-#if !defined(_LIBCPP_MSVCRT_LIKE) && !defined(__LLVM_LIBC__)
-extern "C" double lgamma_r(double, int*);
-#endif
-
 inline _LIBCPP_HIDE_FROM_ABI double __libcpp_lgamma(double __d) {
-#if defined(_LIBCPP_MSVCRT_LIKE) || defined(__LLVM_LIBC__)
-  return lgamma(__d);
+#if defined(_LIBCPP_MSVCRT_LIKE)
+  return ::lgamma(__d);
 #else
   int __sign;
-  return lgamma_r(__d, &__sign);
+  return ::lgamma_r(__d, &__sign);
 #endif
 }

philnik777

Is lgamma_r unconditionally provided? I think it would be really nice if we could get a private implementation from LLVM libc so that we can be thread-safe on every platform.

ldionne · 2025-08-14T18:37:18Z

Is lgamma_r unconditionally provided? I think it would be really nice if we could get a private implementation from LLVM libc so that we can be thread-safe on every platform.

I do think it is unconditionally provided by llvm-libc. What you're thinking about is some kind of hand-in-hand like thing but just for that function?

philnik777 · 2025-08-14T18:42:03Z

Is lgamma_r unconditionally provided? I think it would be really nice if we could get a private implementation from LLVM libc so that we can be thread-safe on every platform.

I do think it is unconditionally provided by llvm-libc. What you're thinking about is some kind of hand-in-hand like thing but just for that function?

Yeah. We could just define our own __lgamma_r or something so we can have it on all platforms. That would also remove the need to expose extensions through _REENTRANT or anything like that. Actually, we could just always define our own __lgamma_r and for back-deployment we can add an __asm__("lgamma_r"). Apple platforms are the only ones where we provide any back-deployment guarantees, so that should be maintainable, and eventually we can remove that workaround as well.

yuxuanchen1997 · 2025-09-03T23:02:55Z

Is lgamma_r unconditionally provided? I think it would be really nice if we could get a private implementation from LLVM libc so that we can be thread-safe on every platform.

I do think it is unconditionally provided by llvm-libc. What you're thinking about is some kind of hand-in-hand like thing but just for that function?

From the #libcxx channel, someone mentioned that LLVM libc doesn't implement lgamma_r function. Maybe this suggestion is a longer shot than expected?

I would prefer some solution to be landed. This redecl is problematic because it may conflict with the exception spec of the decl in math.h. GCC and Clang ignore this discrepancy in system headers but not all compilers do so.

yuxuanchen1997 · 2025-09-03T23:14:38Z

libcxx/include/__random/binomial_distribution.h

undef _LIBCPP_MUST_UNDEFINE_REENTRANT as well?

yuxuanchen1997 · 2025-09-03T23:25:17Z

libcxx/include/__random/binomial_distribution.h

This should probably be a feature test macro for lgamma_r? You have CI failures here on apple platforms.

ldionne · 2025-09-04T15:23:06Z

@philnik777 I was going to go down the route of adding __lgamma_r to libc++.dylib and dancing around back-deployment, but then I found out that GCC provides __builtin_lgamma_r while Clang doesn't: https://godbolt.org/z/4jWrWzf6n

We already provide definitions for std::lgamma in our headers and those forward to the compiler builtins. It would feel a bit silly to add an indirection through the dylib to implement __lgamma_r when Clang just seems to be missing a builtin (which would increase GCC compatibility).

Do you agree that requesting a Clang builtin seems like the better approach? Then we would simply use the builtin from here unconditionally.

ldionne · 2025-09-04T15:24:09Z

In the meantime, I'd be OK with taking a temporary patch such as #156547.

philnik777 · 2025-09-04T17:11:15Z

@philnik777 I was going to go down the route of adding __lgamma_r to libc++.dylib and dancing around back-deployment, but then I found out that GCC provides __builtin_lgamma_r while Clang doesn't: https://godbolt.org/z/4jWrWzf6n

We already provide definitions for std::lgamma in our headers and those forward to the compiler builtins. It would feel a bit silly to add an indirection through the dylib to implement __lgamma_r when Clang just seems to be missing a builtin (which would increase GCC compatibility).

Do you agree that requesting a Clang builtin seems like the better approach? Then we would simply use the builtin from here unconditionally.

I don't think that's the right approach. Adding a builtin is basically just having double __builtin_lgamma_r(double, int*) __asm__("lgamma_r") in the compiler. At that point we might as well just do that ourselves instead of asking for a builtin they have to maintain forever across all platforms. I'm pretty much only comfortable with using the other math builtins because they already exist. I wouldn't ever want to ask for another one of them.

jhuber6 · 2025-09-05T19:29:03Z

Is lgamma_r unconditionally provided? I think it would be really nice if we could get a private implementation from LLVM libc so that we can be thread-safe on every platform.

I do think it is unconditionally provided by llvm-libc. What you're thinking about is some kind of hand-in-hand like thing but just for that function?

I don't think we have a single generic implementation in llvm-libc yet, maybe @lntue has something.

philnik777 · 2025-09-24T08:20:21Z

libcxx/src/math.cpp

+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace __math {
+
+_LIBCPP_EXPORTED_FROM_ABI __lgamma_result __lgamma_thread_safe_impl(double __d) noexcept {


The _LIBCPP_EXPORTED_FROM_ABI shouldn't be required here.

It doesn't hurt to have it though and we do have a bunch of functions in .cpp files that specify _LIBCPP_EXPORTED_FROM_ABI too, which IMO helps spot which functions are exported and which ones are not. In this case it's not super helpful since there's currently only one function in that .cpp file, but still.

I have to say I completely disagree. IMO this significantly hurts readability. We should instead just have non-_Uglified names for library-internal functions (which itself helps with readability in general). Spotting exported ones is trivial in that case. Removing the attribute here also ensures that the correct declarations are visible at the definition, which helps avoid declaration mismatches.

This causes issues when building with modules, and redeclaring functions provided by another library (here the C library) is bad hygiene. Instead, use <math.h> to access the declaration provided by the system. As a drive-by, this also starts using ::lgamma_r instead of ::lgamma when building on top of LLVM-libc. We didn't do that previously due to a declration conflict, but using the _r version when we can should only make things thread safe.

philnik777 · 2025-10-08T07:40:53Z

libcxx/include/__math/gamma.h

+inline _LIBCPP_HIDE_FROM_ABI __lgamma_result __lgamma_thread_safe(double __d) _NOEXCEPT {
+  return __math::__lgamma_thread_safe_impl(__d);
+}


What's the reason for having another wrapper here?

philnik777 · 2025-10-08T07:43:31Z

libcxx/include/__math/gamma.h

+struct __lgamma_result {
+  double __result;
+  int __sign;
+};


We're never using the __sign. Should we remove that part? @lntue do you think this makes any difference in the implementation?

philnik777 · 2025-10-08T07:48:18Z

libcxx/src/math.cpp

+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace __math {
+
+_LIBCPP_EXPORTED_FROM_ABI __lgamma_result __lgamma_thread_safe_impl(double __d) noexcept {


I have to say I completely disagree. IMO this significantly hurts readability. We should instead just have non-_Uglified names for library-internal functions (which itself helps with readability in general). Spotting exported ones is trivial in that case. Removing the attribute here also ensures that the correct declarations are visible at the definition, which helps avoid declaration mismatches.

philnik777 · 2025-10-08T07:48:37Z

libcxx/src/math.cpp

+#include <math.h> // for lgamma_r
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace __math {


Do we really want to enshrine the __math namespace forever in our ABI?

ldionne requested a review from a team as a code owner August 14, 2025 17:37

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 14, 2025

philnik777 reviewed Aug 14, 2025

View reviewed changes

philnik777 mentioned this pull request Sep 3, 2025

[libc++] Conditionally declare lgamma_r as noexcept #156547

Merged

yuxuanchen1997 reviewed Sep 3, 2025

View reviewed changes

libcxx/include/__random/binomial_distribution.h Outdated

Copy link

Member

yuxuanchen1997 Sep 3, 2025

Choose a reason for hiding this comment

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

undef _LIBCPP_MUST_UNDEFINE_REENTRANT as well?

yuxuanchen1997 reviewed Sep 3, 2025

View reviewed changes

ldionne force-pushed the review/dont-redeclare-lgamma_r branch from 0a9e0e0 to ddd93ec Compare September 16, 2025 18:00

ldionne changed the title ~~[libc++] Avoid redeclaring lgamma_r~~ [libc++] Add a thread-safe version of std::lgamma in the dylib Sep 16, 2025

philnik777 approved these changes Sep 24, 2025

View reviewed changes

ldionne added 3 commits October 7, 2025 21:06

Handle ABIlist

6ebae60

Link against -lm

e13ba84

ldionne force-pushed the review/dont-redeclare-lgamma_r branch from a1aa577 to e13ba84 Compare October 8, 2025 01:09

philnik777 reviewed Oct 8, 2025

View reviewed changes

[libc++] Add a thread-safe version of std::lgamma in the dylib #153631

Are you sure you want to change the base?

[libc++] Add a thread-safe version of std::lgamma in the dylib #153631

Uh oh!

Conversation

ldionne commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 14, 2025

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

ldionne commented Aug 14, 2025

Uh oh!

philnik777 commented Aug 14, 2025

Uh oh!

yuxuanchen1997 commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldionne commented Sep 4, 2025

Uh oh!

ldionne commented Sep 4, 2025

Uh oh!

philnik777 commented Sep 4, 2025

Uh oh!

jhuber6 commented Sep 5, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldionne commented Aug 14, 2025 •

edited

Loading

yuxuanchen1997 commented Sep 3, 2025 •

edited

Loading