[libc++] Conditionally declare lgamma_r as noexcept

[yuxuanchen1997]

An older PR #102036 suggested that LLVM libc declares lgamma_r as noexcept and is incompatible with this redeclaration. However, I recently discovered that glibc also declares the math functions to be noexcept under C++ mode.

This line usually don't cause issues because both the glibc and this file are included as "system headers". According to this godbolt, both GCC and clang ignore the different exception specification between multiple declarations if they are in system headers.

However, this seems not the case for NVCC/EDG, so a fix for this redeclaration is still desirable. This patch proposes that we should declare the function as noexcept under known libc integrations to keep the declared function consistent.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[philnik777]

I think #153631 (comment) is rather what we should go for.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Yuxuan Chen (yuxuanchen1997)

Changes

An older PR #102036 suggested that LLVM libc declares lgamma_r as noexcept and is incompatible with this redeclaration. However, I recently discovered that glibc also declares the math functions to be noexcept under C++ mode.

This line usually don't cause issues because both the glibc and this file are included as "system headers". According to this godbolt, both GCC and clang ignore the different exception specification between multiple declarations if they are in system headers.

However, this seems not the case for NVCC/EDG, so a fix for this redeclaration is still desirable. This patch proposes that we should declare the function as noexcept under known libc integrations to keep the declared function consistent.

---

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

1 Files Affected:

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

diff --git a/libcxx/include/__random/binomial_distribution.h b/libcxx/include/__random/binomial_distribution.h
index b4b4340827761..80a910b611adf 100644
--- a/libcxx/include/__random/binomial_distribution.h
+++ b/libcxx/include/__random/binomial_distribution.h
@@ -97,13 +97,19 @@ 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*);
+// Some libc declares the math functions to be `noexcept`.
+#if _LIBCPP_GLIBC_PREREQ(2, 8) || defined(__LLVM_LIBC__)
+#  define _LIBCPP_LGAMMA_R_NOEXCEPT _NOEXCEPT
+#else
+#  define _LIBCPP_LGAMMA_R_NOEXCEPT
+#endif
+
+#if !defined(_LIBCPP_MSVCRT_LIKE)
+    extern "C" double lgamma_r(double, int*) _LIBCPP_LGAMMA_R_NOEXCEPT;
 #endif
 
 inline _LIBCPP_HIDE_FROM_ABI double __libcpp_lgamma(double __d) {
-#if defined(_LIBCPP_MSVCRT_LIKE) || defined(__LLVM_LIBC__)
+#if defined(_LIBCPP_MSVCRT_LIKE)
   return lgamma(__d);
 #else
   int __sign;

[yuxuanchen1997]

I think #153631 (comment) is rather what we should go for.

Hi. Would appreciate progress on that work. If no one is working on it, I can also contribute some cycles. In the meantime, this patch can unblock some libc++ CUDA usages.

[ldionne]

I'm fine with this as a workaround until we settle on the right approach in #153631. I think we agree that the status quo doesn't work, so let's land this and then clean it up when we agree on the long term path forward.

[jhuber6]

This broke the libc version of the build, works if I delete that new macro that was added. I'm confused how this works, since AFAICT a macro won't expand another macro when used in and _NOEXCEPT isn't a standard term as far as I'm aware, so doesn't this just expand to text? Am I missing something?

[philnik777]

This broke the libc version of the build, works if I delete that new macro that was added. I'm confused how this works, since AFAICT a macro won't expand another macro when used in and _NOEXCEPT isn't a standard term as far as I'm aware, so doesn't this just expand to text? Am I missing something?

This should expand normally. Could you provide the diagnostic?

FWIW IMO this has been landed way too eagerly. There hasn't been any more discussion on the other thread so far and this patch has been landed with me opposing it. There was literally no time for me to respond. I'm not a huge fan of changing things without any sort of CI testing for the configuration that is "fixed". OTOH this would have been caught if we had LLVM libc CI coverage, so please make sure to provide that ASAP.

[jhuber6]

This broke the libc version of the build, works if I delete that new macro that was added. I'm confused how this works, since AFAICT a macro won't expand another macro when used in and _NOEXCEPT isn't a standard term as far as I'm aware, so doesn't this just expand to text? Am I missing something?

This should expand normally. Could you provide the diagnostic?

FWIW IMO this has been landed way too eagerly. There hasn't been any more discussion on the other thread so far and this patch has been landed with me opposing it. There was literally no time for me to respond. I'm not a huge fan of changing things without any sort of CI testing for the configuration that is "fixed". OTOH this would have been caught if we had LLVM libc CI coverage, so please make sure to provide that ASAP.

Yeah, I need to get one of the builders updated so it actually hits this path when building at least.

[nico]

Does this need an #else? Else the macro isn't defined if the inner #if here isn't true, right?

We're getting this on our bots, I'm guessing due to this:

[187/130551] CXX android_clang_arm/obj/third_party/abseil-cpp/absl/strings/cordz_functions/cordz_functions.o
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF android_clang_arm/obj/third_party/abseil-cpp/absl/strings/cordz_functions/co...(too long)
In file included from ../../third_party/abseil-cpp/absl/strings/internal/cordz_functions.cc:20:
In file included from ../../third_party/libc++/src/include/random:1685:
../../third_party/libc++/src/include/__random/binomial_distribution.h:112:42: error: expected function body after function declarator
  112 | extern "C" double lgamma_r(double, int*) _LIBCPP_LGAMMA_R_NOEXCEPT;
      |                                          ^
1 error generated.

[jhuber6]

Same thing I'm seeing

[nico]

#157610 added it. Thanks!

[nico]

We're also getting build errors on fuchsia with this:

[282/101447] CXX obj/third_party/abseil-cpp/absl/strings/cordz_functions/cordz_functions.o
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/third_party/abseil-cpp/absl/strings/cordz_functions/cordz_functions.o.d ...(too long)
In file included from ../../third_party/abseil-cpp/absl/strings/internal/cordz_functions.cc:20:
In file included from ../../third_party/libc++/src/include/random:1685:
../../third_party/libc++/src/include/__random/binomial_distribution.h:114:19: error: exception specification in declaration does not match previous declaration
  114 | extern "C" double lgamma_r(double, int*) _LIBCPP_LGAMMA_R_NOEXCEPT;
      |                   ^
../../third_party/fuchsia-sdk/sdk/obj/x64-api-28/sysroot/include/math.h:398:8: note: previous declaration is here
  398 | double lgamma_r(double, int*);
      |        ^
1 error generated.

[nico]

@petrhosek Do you know what this should check for on Fuchsia?

[nico]

https://llvm.org/devmtg/2021-11/slides/2021-BuildinganOperatingSystemfromScratchwithLLVM.pdf says "We are evolving our C library and incrementally replacing existing parts with LLVM libc counterparts". I suppose that means we're going down the __LLVM_LIBC__ branch.

libc headers originally didn't have noexcept in math.h, see LHS of https://reviews.llvm.org/D76723 which moved to a first header generation script. That script learned to write noexcept in https://reviews.llvm.org/D141095 in early 2023. The Fuchsia SDK seems to ship headers that don't have that yet. (Or it doesn't use LLVM libc's math.h header at all, but uses enough of it that LLVM_LIBC ends up being set.)

(That header generation script was then later replaced by a different header generation script, which seems to have printed NOEXCEPT from the start, and which is the mechanism that's currently in use.)

[nico]

Made #157710 for now.