linear_congruential_engine: Fixes for __lce_alg_picker

[LRFLEW]

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

Fixes #33554

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: None (LRFLEW)

Changes

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 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 --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.");
 

[github-actions]

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

[LRFLEW]

I don't know why the formatter wants the comments to be unaligned (seems like a bizarre choice to me), but if that's what the project wants, I'll make the change.

[LRFLEW]

Looks like the failed tests are because the tests in rand.eng.lcong end up constructing engines that trigger the static assert. I'll look to update the tests specifically to avoid those cases for now.

[LRFLEW]

I've pushed some updated regression tests to account for the build failures. I reorganized some of the tests in rand.eng.lcong to include more test cases, while commenting out the test cases that are currently unsupported due to the static assert.

I also went ahead and added a specific regression test for the _OverflowOK fix, using the example I mentioned in #33554. I simply added it to rand.eng.lcong/alg.pass.cpp instead of making a new file because it seems to me like it's all testing the same idea (using the right algorithm to avoid integer overflow breaking the result), and figure it's better to keep like-tests together. If you want me to split this out to a separate file, let me know.

[ldionne]

Thanks for the fix, I think this LGTM with a green CI -- I don't have much to add.

Let's add a release note explaining that we changed the value returned by this utility to be Standards conforming, but that it's technically an ABI break to do that. I think this falls within the realm of what is reasonable to change, especially since the status quo is that we disagree with other implementations.

[LRFLEW]

I looked into the CI failures, and they all seem to say "The runner has received a shutdown signal." I'm guessing they timed out from the CI as a whole taking too long. I don't see anything I need to address so far, so those tasks may just need to be restarted. I don't think I have the permissions needed to restart them, so I don't think I can do anything about it myself right now.

[mordante]

Thanks for working on this! I'll restart the failed jobs.

[github-actions]

@LRFLEW Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may recieve a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!