[libcxx-commits] [PATCH] D96946: [libcxx][RFC] Unspecified behavior randomization in libcxx

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D96946#3124857 <https://reviews.llvm.org/D96946#3124857>, @danlark wrote:

> @ldionne call of last hope, if I should continue the effort :)

Sorry for dropping the ball here.

Yes, let's make this happen. I think this is useful. IMO, it would make most sense for it to be enabled unconditionally if the debug mode is enabled too instead of having a separate toggle just for it. I'm soon going to be cleaning up the debug mode (and in particular making sure it works at all), so this would become supported better. WDYT?



================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:30
+
+For example, as of LLVM version 12, libcxx sorting algorithm takes
+`O(n^2) worst case <https://bugs.llvm.org/show_bug.cgi?id=20837>`_ but according
----------------



================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:31
+For example, as of LLVM version 12, libcxx sorting algorithm takes
+`O(n^2) worst case <https://bugs.llvm.org/show_bug.cgi?id=20837>`_ but according
+to the standard its worst case should be `O(n log n)`. This effort helps users
----------------



================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:38
+
+* Introduce new macro `_LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY` which should
+  be a part of the libcxx config.
----------------
Can we rename this to `_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY` instead, since it is tied to the debug mode (if we apply my suggestions here)?


================
Comment at: libcxx/docs/UsingLibcxx.rst:244-250
+**_LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY**:
+  This macro enables the randomization of unspecified behavior in libcxx, for
+  example, for equal elements in sorting algorithm or randomizing both parts of
+  the partition after `std::nth_element` call. This effort helps you to migrate
+  to potential future faster versions of these algorithms and deflake your tests
+  which depend on such behavior. To fix the seed, use
+  `_LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY_SEED=seed` definition.
----------------
This should be moved to the documentation of the debug mode. In particular, I would like it if it were NOT advertised as something that can be tweaked by users. Users should only select the debug mode level.


================
Comment at: libcxx/include/__config:893
+    defined(_LIBCPP_CXX03_LANG)
+#error Supported unspecified stability is only for C++11 and higher
+#endif
----------------



================
Comment at: libcxx/include/__config:900
 #endif
 
 #ifdef _LIBCPP_DISABLE_EXTERN_TEMPLATE
----------------
What would you think of enabling this automatically when the debug mode is enabled?

```
#if _LIBCPP_DEBUG_LEVEL >= 2
# define _LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY
#endif
```


================
Comment at: libcxx/include/algorithm:3125
+
+  result_type operator()() {
+    std::uint_fast64_t __oldstate = __state;
----------------



================
Comment at: libcxx/include/algorithm:3131-3132
+
+  static _LIBCPP_CONSTEXPR result_type min() { return _Min; }
+  static _LIBCPP_CONSTEXPR result_type max() { return _Max; }
+
----------------



================
Comment at: libcxx/include/algorithm:3136-3137
+  static const void* const __seed_static;
+  std::uint_fast64_t __state;
+  std::uint_fast64_t __inc;
+  _LIBCPP_ALWAYS_INLINE result_type __seed() {
----------------
We don't generally qualify types when in namespace `std`. Applies elsewhere.


================
Comment at: libcxx/include/algorithm:3138
+  std::uint_fast64_t __inc;
+  _LIBCPP_ALWAYS_INLINE result_type __seed() {
+#ifdef _LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY_SEED
----------------



================
Comment at: libcxx/include/algorithm:3139
+  _LIBCPP_ALWAYS_INLINE result_type __seed() {
+#ifdef _LIBCPP_RANDOMIZE_UNSPECIFIED_STABILITY_SEED
+    return static_cast<std::uint_fast64_t>(
----------------



================
Comment at: libcxx/include/algorithm:3150-3151
+template <class _Void>
+const void* const __random_unspecified_behavior_gen_<_Void>::__seed_static =
+    &__seed_static;
+
----------------



================
Comment at: libcxx/include/algorithm:3153-3154
+
+using __random_unspecified_behavior_gen =
+    __random_unspecified_behavior_gen_<void>;
+
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96946/new/

https://reviews.llvm.org/D96946