From dcdc059ae7d5a357cfc4f26d679d85bc2710b0d8 Mon Sep 17 00:00:00 2001 From: Khalil Estell Date: Sat, 24 Oct 2026 08:02:50 -0700 Subject: [PATCH] :bug: Avoid lambdas in comparison and scaling for LLVM 23 LTO With LLVM 23 (clang 23.1.2), full LTO with -g produces a module that fails verification ("invalid retained nodes, retained node does not belong to subprogram") when a function template holding a lambda is instantiated both in a module unit and in a TU that imports it. ld.lld aborts the link; Apple's ld64 warns and strips the debug info. LLVM 22.1.8 is not affected. The lambdas in the framework's runtime paths become named function templates, so there is no local type for the linker to merge: - compare_in_common_type(): the fallback comparison is now compare_in_common_type_fallback(). The comment above it records the details for upstream. - div_round(): the remainder is now div_remainder(). - scale_impl(): the floating-point scaling is now scale_fp_for(). libhal hit the first one on Linux x86_64 and arm64 (-std=c++23 -stdlib=libc++ -flto -g -Os -fsanitize=address, ld.lld): its spi module and that module's unit test both compared quantity values. Verified by building libhal against this change with the hertz comparison put back: the verifier error is gone and its tests pass. The mp-units tests pass in header mode with LLVM 23 (build:all, 64/64). --- .../include/mp-units/framework/quantity.h | 37 ++++++++++++------- src/core/include/mp-units/framework/scaling.h | 35 ++++++++++++------ 2 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/core/include/mp-units/framework/quantity.h b/src/core/include/mp-units/framework/quantity.h index e910d20cd97..46649c50f71 100644 --- a/src/core/include/mp-units/framework/quantity.h +++ b/src/core/include/mp-units/framework/quantity.h @@ -299,22 +299,33 @@ template } } +// Note from libhal: this used to be a lambda inside `compare_in_common_type()`. LLVM 23 (seen +// with clang 23.1.2, 85ac560262434c9ccfc0c183ec22d4138ed647fb) emits a broken module under full +// LTO with `-g` when a function template holding a lambda is instantiated both in a module unit +// and in a TU that imports that module unit: "invalid retained nodes, retained node does not +// belong to subprogram". `ld.lld` aborts the link ("Broken module found"); Apple's ld64 only warns +// and strips the debug info. libhal hit it with `-std=c++23 -stdlib=libc++ -flto -g -Os +// -fsanitize=address` on Linux x86_64 and arm64, where a module unit and its unit test both +// compared `quantity` values. LLVM 22.1.8 is not affected. A named function +// has no local type for the linker to merge, so it avoids the bug. Like the lambda, it is called +// from either branch below, so there is still no fallthrough `return` for MSVC Debug C4702. +template +[[nodiscard]] constexpr auto compare_in_common_type_fallback(const Q1& lhs, const Q2& rhs, Cmp cmp) +{ + using ct = std::common_type_t; + const ct ct_lhs(lhs); + const ct ct_rhs(rhs); + MP_UNITS_DIAGNOSTIC_PUSH + MP_UNITS_DIAGNOSTIC_IGNORE_FLOAT_EQUAL + return cmp(ct_lhs.numerical_value_ref_in(ct::unit), ct_rhs.numerical_value_ref_in(ct::unit)); + MP_UNITS_DIAGNOSTIC_POP +} + template [[nodiscard]] constexpr auto compare_in_common_type(const Q1& lhs, const Q2& rhs, Cmp cmp) { using ct = std::common_type_t; using ct_rep = value_type_t; - // `fallback_cmp` is a lambda so it can be invoked from either branch below — a bare fallthrough - // `return` after the `if constexpr` chain would trip MSVC Debug C4702 ("unreachable code") on - // instantiations where the optimization branch returns. - auto fallback_cmp = [&] { - const ct ct_lhs(lhs); - const ct ct_rhs(rhs); - MP_UNITS_DIAGNOSTIC_PUSH - MP_UNITS_DIAGNOSTIC_IGNORE_FLOAT_EQUAL - return cmp(ct_lhs.numerical_value_ref_in(ct::unit), ct_rhs.numerical_value_ref_in(ct::unit)); - MP_UNITS_DIAGNOSTIC_POP - }; if constexpr (!treat_as_floating_point && sizeof(ct_rep) < sizeof(int128_t)) { // widen to a double-width integer so the unit-ratio scaling can't overflow using wide_t = double_width_int_for_t; @@ -324,10 +335,10 @@ template return cmp(scale(lhs_m, lhs.numerical_value_is_an_implementation_detail_), scale(rhs_m, rhs.numerical_value_is_an_implementation_detail_)); } else { - return fallback_cmp(); + return compare_in_common_type_fallback(lhs, rhs, cmp); } } else { - return fallback_cmp(); + return compare_in_common_type_fallback(lhs, rhs, cmp); } } diff --git a/src/core/include/mp-units/framework/scaling.h b/src/core/include/mp-units/framework/scaling.h index fc458d397db..0deae9440bd 100644 --- a/src/core/include/mp-units/framework/scaling.h +++ b/src/core/include/mp-units/framework/scaling.h @@ -86,6 +86,18 @@ template return value / value; } +// Note from libhal: this and `scale_fp_for()` below were immediately-invoked lambdas. They are +// named functions to avoid the LLVM 23 LTO bug described above `compare_in_common_type_fallback()` +// in `quantity.h`. +template +[[nodiscard]] constexpr auto div_remainder(const T& dividend, const D& divisor, const Q& quot) +{ + if constexpr (requires { dividend % divisor; }) + return dividend % divisor; + else + return dividend - quot * divisor; +} + /** * @brief Integer division with the quotient rounded according to @c Mode */ @@ -106,12 +118,7 @@ template // `%` is optional for a `RealScalar` though, so the subtraction stays as the general form: it // needs only the arithmetic the concept mandates, which is what the emulated 128-bit integer // and any other representation type without `%` rely on. - const auto rem = [&] { - if constexpr (requires { dividend % divisor; }) - return dividend % divisor; - else - return dividend - quot * divisor; - }(); + const auto rem = div_remainder(dividend, divisor, quot); using rem_type = std::remove_const_t; const rem_type no_rem = get_zero(rem); if constexpr (Mode == rounding_mode::rounded_down) @@ -232,6 +239,15 @@ template } } +template +[[nodiscard]] constexpr auto scale_fp_for(const From& value) +{ + if constexpr (treat_as_integral> || !std::floating_point>) + return scale_fp(static_cast(value)); + else + return scale_fp(value); +} + template requires UnitMagnitudeScalable [[nodiscard]] constexpr auto scale_impl(M m, const From& value) @@ -255,12 +271,7 @@ template // To could overflow the wrapper's narrow rep (e.g. scaled_integer> // times itself yields P<-50>, which cannot be downshifted to P<-15> within // a 32-bit int). Converting to To first keeps all arithmetic in To's type. - const auto scaled = [&] { - if constexpr (treat_as_integral> || !std::floating_point>) - return scale_fp(static_cast(value)); - else - return scale_fp(value); - }(); + const auto scaled = scale_fp_for(value); // A floating-point destination represents the scaled value with at most half ULP error // (`rounded` semantics, the only policy valid for it), so rounding applies only when the // destination is integral and a mode other than the `silent_cast` truncation is requested.