[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.

libcxx/src/math.cpp

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?

I wanted to avoid having a function that is sometimes defined inline and sometimes defined in the dylib, since I think it's more difficult to reason about ODR and ABI-wise.

Is that worth carrying around the (IMO) somewhat weird naming forever? I personally don't find it particularly confusing, since _LIBCPP_HIDE_FROM_ABI causes different mangling and thus makes it very clearly distinct in terms of ABI.

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?

It would be great if we could get an answer here @lntue (I don't know is fine as well)

libcxx/src/math.cpp

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-11-18T13:14:48Z

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);
+}


Is that worth carrying around the (IMO) somewhat weird naming forever? I personally don't find it particularly confusing, since _LIBCPP_HIDE_FROM_ABI causes different mangling and thus makes it very clearly distinct in terms of ABI.

philnik777 · 2025-11-18T13:17:39Z

libcxx/include/__math/gamma.h

+#else
+// When deploying to older targets, call `lgamma_r` directly but avoid declaring the actual
+// function since different platforms declare the function slightly differently.
+double __lgamma_r_shim(double, int*) _NOEXCEPT __asm__("lgamma_r");


Suggested change

double __lgamma_r_shim(double, int*) _NOEXCEPT __asm__("lgamma_r");

double __lgamma_r_shim(double, int*) _NOEXCEPT __asm__("_lgamma_r");

I think __asm__ ignores that on Apple platforms an underscore is prepended to all functions, so this has to be added manually.

philnik777 · 2025-11-18T13:19:05Z

libcxx/include/__math/gamma.h

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


It would be great if we could get an answer here @lntue (I don't know is fine as well)

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

libcxx/src/math.cpp Outdated Show resolved Hide resolved

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

ldionne added 4 commits November 17, 2025 16:18

Handle ABIlist

f580f12

Link against -lm

58a05fd

Workaround broken flag detection on GCC

fac6c31

ldionne force-pushed the review/dont-redeclare-lgamma_r branch from e13ba84 to fac6c31 Compare November 17, 2025 21:59

ldionne added 2 commits November 17, 2025 17:03

Remove EXPORTED_FROM_ABI

5f5e543

Move out of __math namespace

7228037

philnik777 reviewed Nov 18, 2025

View reviewed changes

	double __lgamma_r_shim(double, int*) _NOEXCEPT __asm__("lgamma_r");
	double __lgamma_r_shim(double, int*) _NOEXCEPT __asm__("_lgamma_r");

[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!

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!

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