[libc++] [ABI BREAK] Conform lognormal_distribution::param_type.

[• Quuxplusone]

Summary

Fixes https://github.com/llvm/llvm-project/issues/52906

Unless I'm mistaken, this

• preserves the size and layout of lognormal_distribution<T>; but
• shrinks the size of lognormal_distribution<T>::param_type (this is the ABI break).

[Mordante]

SGTM, but I'm not too familiar with this part of the library.

libcxx/include/__random/lognormal_distribution.h

39

If I'm correct we go from 3 * sizeof(result_type) + sizeof(bool) to 2 * sizeof(result_type). So it would be possible use an ABI macro. Right?

[• Quuxplusone]

libcxx/include/__random/lognormal_distribution.h

39

Yes, it is definitely possible to preserve the old code under a macro. The PR summary indicates my opinion on this.

[ldionne]

These changes are hard. We don't have any way to tell whether it's going to bite someone, and chances are that it won't hurt anyone. But if it does, it's exactly the kind of thing that's going to be a huge pain to debug, and it's also going to be unfixable from their end once they find out what the issue is.

For these reasons, and because the behavior we're fixing is bad but also not like fixing basic functionality, I think it would be better to hide this behind an ABI macro.

libcxx/test/std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/eval_param.PR52906.pass.cpp

15

Please link to the bug report (using llvm.org/PRXYZ)

22

[ldionne]

In D116344#3217496, @ldionne wrote:

For these reasons, and because the behavior we're fixing is bad but also not like fixing basic functionality

What I mean here is that their code probably worked previously, and they had never noticed this bug. It's not like if lognormal_distribution had never worked properly and we were suddenly making it work -- I think an ABI break would be legit, cause nobody could really have been using it in the first place. Now this is sneakier.

[• Quuxplusone]

Add a release note; add a macro to revert to the old behavior. The wording is copied from the release note above the new one, because I think "remove the bad UB code in LLVM 15" sounds like a great plan.

[• Quuxplusone]

libcxx/include/__random/lognormal_distribution.h

275–291

I was so tempted to make these hidden friends in the new code, but I didn't. ;)

The green code here is the same as what I was proposing in the previous diff. The changes between the #if and the #else blocks are minor, but many and scattered throughout, so I think this "one giant #if-#else" is the correct strategy here.

[ldionne]

Okay, so the strategy here is:

1. We don't think this is going to break anyone.
2. We make the change unconditionally, and *perhaps* break some people, who will revert the change with the macro.
3. These people will comment here and we can adjust what we do based on that feedback.

I'm fine with this approach since I don't think anybody's going to be impacted -- IOW this is a small risk for an interesting benefit. FWIW, "these people" are most likely vendors who will discover any potential breakage through their users complaining over the next year or so, if that happens at all. If it does, we will make this a proper ABI macro and only enable it in the ABI-unstable build.

Pinging libc++ vendors to see if they have an opinion. @Quuxplusone please feel free to ship this at the end of this week if nobody has raised any concerns.

[• Quuxplusone]

@ldionne wrote:

If it does, we will make this a proper ABI macro and only enable it in the ABI-unstable build.

I agree with everything you said except this sentence. I don't believe we know a realistic path to ever make "ABIv2" the default, which means that if we do this patch only in ABIv2, then we're not doing it by default for "new customers" (so to speak). And I think this UB is a big enough deal that it needs to be fixed-by-default for new customers. If anyone complains about the new lognormal_distribution (which I think we both believe is highly unlikely), then I think we would have to investigate some way to keep this patch enabled by default for ABIv1 users, but disabled for those specific vendors who complained — which I think means simply "do exactly this PR, but commit to supporting the _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION macro forever."
(TLDR: I do not think it should be acceptable to enable _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION by default in ABIv1, even if we get a few complaints. But again, we'll probably get zero complaints and so it'll be moot, which is great.)

[ldionne]

In D116344#3232200, @Quuxplusone wrote:

@ldionne wrote:

If it does, we will make this a proper ABI macro and only enable it in the ABI-unstable build.

I agree with everything you said except this sentence. I don't believe we know a realistic path to ever make "ABIv2" the default, which means that if we do this patch only in ABIv2, then we're not doing it by default for "new customers" (so to speak). And I think this UB is a big enough deal that it needs to be fixed-by-default for new customers. If anyone complains about the new lognormal_distribution (which I think we both believe is highly unlikely), then I think we would have to investigate some way to keep this patch enabled by default for ABIv1 users, but disabled for those specific vendors who complained — which I think means simply "do exactly this PR, but commit to supporting the _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION macro forever."
(TLDR: I do not think it should be acceptable to enable _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION by default in ABIv1, even if we get a few complaints. But again, we'll probably get zero complaints and so it'll be moot, which is great.)

I knew you (or someone else) was going to suggest that, which is why I worded my comment above to unambiguously suggest that the default was going to be "no ABI break". I think there's definitely logic in what you suggest, however the default so far has always be that non-benign [1] ABI breaking changes were opted-into, not opted-out. We could discuss changing that policy, but it's a much much larger discussion. It would indeed make some sense that the library be "unstable" by default, but that we provide mechanisms for vendors to keep their flavour of the library ABI stable. But that's an entirely new dynamic for the project and we would need vendors to be organized and to pay attention to what's shipped on their systems a lot more than they need to be today.

[1]: By a non-benign change, I mean a change that affects some people. For example, if we break the ABI in such a tiny way that nobody ever notices cause nobody was ever depending on that ABI, it's a benign change and I'm supportive of not even having a macro for it. That would just make our lives harder without benefit.

[• Quuxplusone]

Pinging libc++ vendors to see if they have an opinion. @Quuxplusone please feel free to ship this at the end of this week if nobody has raised any concerns.

I'll try to remember to land this on Monday.

libcxx/docs/ReleaseNotes.rst

- The C++03 emulation type for ``std::nullptr_t`` has been removed in favor of
   comment `here <https://reviews.llvm.org/D109459>`_ if you are broken by this change
   and need to define the macro.
 
 - On Apple platforms, ``std::random_device`` is now implemented on top of ``arc4random()``
   instead of reading from ``/dev/urandom``. Any implementation-defined token used when
   constructing a ``std::random_device`` will now be ignored instead of interpreted as a
   file to read entropy from.
 
+- ``std::lognormal_distribution::param_type`` used to store a data member of type
+  ``std::normal_distribution``; now this member is stored in the ``lognormal_distribution``
+  class itself, and the ``param_type`` stores only the mean and standard deviation,
+  as required by the Standard. This changes ``sizeof(std::lognormal_distribution::param_type)``.
+  You can define the ``_LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION`` macro to return to the
+  previous behavior. That macro will be removed in LLVM 15. Please comment
+  `here <https://llvm.org/PR52906>`_ if you are broken by this change and need to
+  define the macro.
+
 Build System Changes
 --------------------
 
 - Building the libc++ shared or static library requires a C++ 20 capable compiler.
   Consider using a Bootstrapping build to build libc++ with a fresh Clang if you
   can't use the system compiler to build libc++ anymore.
 
 - Historically, there has been numerous ways of building libc++ and libc++abi. This has

libcxx/include/__random/lognormal_distribution.h

 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#ifdef _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION
+
 template<class _RealType = double>
 class _LIBCPP_TEMPLATE_VIS lognormal_distribution
 {
 public:
     // types
     typedef _RealType result_type;
 
     class _LIBCPP_TEMPLATE_VIS param_type
     {
         normal_distribution<result_type> __nd_;
     public:
         typedef lognormal_distribution distribution_type;
 
         _LIBCPP_INLINE_VISIBILITY
         explicit param_type(result_type __m = 0, result_type __s = 1)
             : __nd_(__m, __s) {}
 
         _LIBCPP_INLINE_VISIBILITY
         result_type m() const {return __nd_.mean();}

 inline _LIBCPP_INLINE_VISIBILITY
 basic_istream<_CharT, _Traits>&
 operator>>(basic_istream<_CharT, _Traits>& __is,
            lognormal_distribution<_RT>& __x)
 {
     return __is >> __x.__p_.__nd_;
 }
 
+#else // _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION
+
+template<class _RealType = double>
+class _LIBCPP_TEMPLATE_VIS lognormal_distribution
+{
+public:
+    // types
+    typedef _RealType result_type;
+
+    class _LIBCPP_TEMPLATE_VIS param_type
+    {
+        result_type __m_;
+        result_type __s_;
+    public:
+        typedef lognormal_distribution distribution_type;
+
+        _LIBCPP_INLINE_VISIBILITY
+        explicit param_type(result_type __m = 0, result_type __s = 1)
+            : __m_(__m), __s_(__s) {}
+
+        _LIBCPP_INLINE_VISIBILITY
+        result_type m() const {return __m_;}
+        _LIBCPP_INLINE_VISIBILITY
+        result_type s() const {return __s_;}
+
+        friend _LIBCPP_INLINE_VISIBILITY
+        bool operator==(const param_type& __x, const param_type& __y)
+            {return __x.__m_ == __y.__m_ && __x.__s_ == __y.__s_;}
+        friend _LIBCPP_INLINE_VISIBILITY
+        bool operator!=(const param_type& __x, const param_type& __y)
+            {return !(__x == __y);}
+    };
+
+private:
+    normal_distribution<result_type> __nd_;
+
+public:
+    // constructor and reset functions
+#ifndef _LIBCPP_CXX03_LANG
+    _LIBCPP_INLINE_VISIBILITY
+    lognormal_distribution() : lognormal_distribution(0) {}
+    _LIBCPP_INLINE_VISIBILITY
+    explicit lognormal_distribution(result_type __m, result_type __s = 1)
+        : __nd_(__m, __s) {}
+#else
+    _LIBCPP_INLINE_VISIBILITY
+    explicit lognormal_distribution(result_type __m = 0,
+                                    result_type __s = 1)
+        : __nd_(__m, __s) {}
+#endif
+    _LIBCPP_INLINE_VISIBILITY
+    explicit lognormal_distribution(const param_type& __p)
+        : __nd_(__p.m(), __p.s()) {}
+    _LIBCPP_INLINE_VISIBILITY
+    void reset() {__nd_.reset();}
+
+    // generating functions
+    template<class _URNG>
+    _LIBCPP_INLINE_VISIBILITY
+    result_type operator()(_URNG& __g)
+    {
+        return _VSTD::exp(__nd_(__g));
+    }
+
+    template<class _URNG>
+    _LIBCPP_INLINE_VISIBILITY
+    result_type operator()(_URNG& __g, const param_type& __p)
+    {
+        typename normal_distribution<result_type>::param_type __pn(__p.m(), __p.s());
+        return _VSTD::exp(__nd_(__g, __pn));
+    }
+
+    // property functions
+    _LIBCPP_INLINE_VISIBILITY
+    result_type m() const {return __nd_.mean();}
+    _LIBCPP_INLINE_VISIBILITY
+    result_type s() const {return __nd_.stddev();}
+
+    _LIBCPP_INLINE_VISIBILITY
+    param_type param() const {return param_type(__nd_.mean(), __nd_.stddev());}
+    _LIBCPP_INLINE_VISIBILITY
+    void param(const param_type& __p)
+    {
+        typename normal_distribution<result_type>::param_type __pn(__p.m(), __p.s());
+        __nd_.param(__pn);
+    }
+
+    _LIBCPP_INLINE_VISIBILITY
+    result_type min() const {return 0;}
+    _LIBCPP_INLINE_VISIBILITY
+    result_type max() const {return numeric_limits<result_type>::infinity();}
+
+    friend _LIBCPP_INLINE_VISIBILITY
+        bool operator==(const lognormal_distribution& __x,
+                        const lognormal_distribution& __y)
+        {return __x.__nd_ == __y.__nd_;}
+    friend _LIBCPP_INLINE_VISIBILITY
+        bool operator!=(const lognormal_distribution& __x,
+                        const lognormal_distribution& __y)
+        {return !(__x == __y);}
+
+    template <class _CharT, class _Traits, class _RT>
+    friend
+    basic_ostream<_CharT, _Traits>&
+    operator<<(basic_ostream<_CharT, _Traits>& __os,
+               const lognormal_distribution<_RT>& __x);
+
+    template <class _CharT, class _Traits, class _RT>
+    friend
+    basic_istream<_CharT, _Traits>&
+    operator>>(basic_istream<_CharT, _Traits>& __is,
+               lognormal_distribution<_RT>& __x);
+};
+
+template <class _CharT, class _Traits, class _RT>
+inline _LIBCPP_INLINE_VISIBILITY
+basic_ostream<_CharT, _Traits>&
+operator<<(basic_ostream<_CharT, _Traits>& __os,
+           const lognormal_distribution<_RT>& __x)
+{
+    return __os << __x.__nd_;
+}
+
+template <class _CharT, class _Traits, class _RT>
+inline _LIBCPP_INLINE_VISIBILITY
+basic_istream<_CharT, _Traits>&
+operator>>(basic_istream<_CharT, _Traits>& __is,
+           lognormal_distribution<_RT>& __x)
+{
+    return __is >> __x.__nd_;
+}
+
+#endif // _LIBCPP_ABI_OLD_LOGNORMAL_DISTRIBUTION
+
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___RANDOM_LOGNORMAL_DISTRIBUTION_H

libcxx/test/std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/eval_param.PR52906.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <random>
+
+// template<class RealType = double>
+// class lognormal_distribution
+
+// template<class _URNG> result_type operator()(_URNG& g, const param_type& parm);
+//   https://llvm.org/PR52906
+
+#include <random>
+#include <cassert>
+
+#include "test_macros.h"
+
+int main(int, char**)
+{
+    typedef std::lognormal_distribution<> D;
+    typedef D::param_type P;
+    typedef std::mt19937 G;
+    G g;
+    D d;
+
+    const P p1 = d.param();
+    const P p2 = d.param();
+    assert(p1 == p2);
+    (void) d(g, p1); // This line must not modify p1.
+    assert(p1 == p2);
+    LIBCPP_ASSERT(p1 == d.param());
+
+    return 0;
+}