[libcxx-commits] [libcxx] linear_congruential_engine: Fixes for __lce_alg_picker (PR #81080)

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: None (LRFLEW)

<details>
<summary>Changes</summary>

This fixes two major mistakes in the implementation of `linear_congruential_engine` that allowed it to produce incorrect output. Specifically, these mistakes are in `__lce_alg_picker`, which is used to determine whether Schrage's algorithm is valid and needed.

The first mistake is in the definition of `_OverflowOK`. The code comment and the description of [D65041](https://reviews.llvm.org/D65041) both indicate that it's supposed to be true iff `m` is a power of two. However, the definition used does not work out to that, and instead is true whenever `m` is even. This could result in `linear_congruential_engine` using an invalid implementation, as it would incorrectly assume that any integer overflow can't change the result. I changed the implementation to one that accurately checks if `m` is a power of two. Technically, this implementation has an edge case where it considers `0` to be a power of two, but in this case this is actually accurate behavior, as `m = 0` indicates a modulus of 2^w where w is the size of `result_type` in bits, which *is* a power of two.

The second mistake is in the static assert. The original static assert erroneously included an unnecessary `a != 0 || m != 0`. Combined with the `|| !_MightOverflow`, this actually resulted in the static assert being impossible to fail. Applying De Morgan's law and expanding `_MightOverflow` gives that the only way this static assert can be triggered is if `a == 0 && m == 0 && a != 0 && m != 0 && ...`, which clearly cannot be true. I simply removed the explicit checks against `a` and `m`, as the intended checks are already included in `_MightOverflow` and `_SchrageOK`, and their inclusion doesn't provide any obvious semantic benefit.

This should fix all the current instances where `linear_congruential_engine` uses an invalid implementation. This technically isn't a complete implementation, though, since the static assert will cause some instantiations of `linear_congruential_engine` not disallowed by the standard from compiling. However, this should still be an improvement, as all compiling instantiations of `linear_congruential_engine` should use a valid implementation. Fixing the cases where the static assert triggers will require adding additional implementations, some of which will be fairly non-trivial, so I'd rather leave those for another PR so they don't hold up these more important fixes.

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


1 Files Affected:

- (modified) libcxx/include/__random/linear_congruential_engine.h (+3-3) 


``````````diff
diff --git a/libcxx/include/__random/linear_congruential_engine.h b/libcxx/include/__random/linear_congruential_engine.h
index 51f6b248d8f974..5f2f845c98b141 100644
--- a/libcxx/include/__random/linear_congruential_engine.h
+++ b/libcxx/include/__random/linear_congruential_engine.h
@@ -31,10 +31,10 @@ template <unsigned long long __a,
           unsigned long long __m,
           unsigned long long _Mp,
           bool _MightOverflow = (__a != 0 && __m != 0 && __m - 1 > (_Mp - __c) / __a),
-          bool _OverflowOK    = ((__m | (__m - 1)) > __m),                    // m = 2^n
-          bool _SchrageOK     = (__a != 0 && __m != 0 && __m % __a <= __m / __a)> // r <= q
+          bool _OverflowOK    = ((__m & (__m - 1)) == 0ull),                           // m = 2^n
+          bool _SchrageOK     = (__a != 0 && __m != 0 && __m % __a <= __m / __a)>      // r <= q
 struct __lce_alg_picker {
-  static_assert(__a != 0 || __m != 0 || !_MightOverflow || _OverflowOK || _SchrageOK,
+  static_assert(!_MightOverflow || _OverflowOK || _SchrageOK,
                 "The current values of a, c, and m cannot generate a number "
                 "within bounds of linear_congruential_engine.");
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/81080