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

Thu Feb 18 07:19:50 PST 2021

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

First of all, thanks for writing a design document and explaining the purpose of your patch. Let me try to understand the use case a bit better.

So basically, let's say you're trying to adopt a new implementation of `std::sort` internally. You change that, but then you realize that a bunch of tests break because they were relying on the exact order of equal elements after `std::sort`, which is really unspecified. But since those tests are failing, you can't use the new `std::sort` implementation until all the tests have been fixed. So, in order to make that transition easier and allow fixing tests one by one, you add this feature. You then turn it on locally for some projects/tests inside your organization, and you fix them one by one. Once you're done, you can switch the algorithm implementation, and turn off the randomization. Is that the idea?

The main problem I see with this reasoning is that it doesn't really help libc++ as a project to migrate to a new algorithm. Libc++ is used by plenty of people who might be relying on the order of elements after calling `std::sort`, and they'll never know about this setting before we break their code. Are we comfortable with breaking such users? I mean, we're technically allowed to by the Standard, but still. I *think* I'd be fine with that, but this needs a bit of discussion.

Technically speaking, there's a few things on my mind while reviewing this:

1. Is there a bad interaction because we instantiate some algorithms in the `dylib`. I checked and I don't think that's the case.
2. Is there a way we can achieve the same with some sort of sanitizer or tool external to libc++ instead? If we could do that, I think it would certainly deliver more value in the long term, and it would certainly be more powerful than manually-added checks in libc++.
3. How can we minimize the complexity we're adding to libc++? This is the Standard Library, and literally everybody wants to add their "very small tweak" to it. If we're not very aggressive about reducing complexity, it becomes a mess. For example, do you really need to be able to specify the seed? Can you explain why? And in case it almost always makes sense to specify a seed, can we make it mandatory instead? I'm trying to reduce the number of configurations we have to maintain.
4. If we do this, then does it make sense to group it with other features such as the Debug mode? It seems to me that this effort, iterator invalidation checking and other similar checks kind of fall into the same bucket of "things that turn silent misuses into loud failures so you diagnose them before they hit prod". If we decide to go ahead with your patch, I think it would be useful to have a wider vision for how we handle these sorts of additions in the future.

Requesting changes so it shows up properly in the queue. Let's have a discussion.

Thanks!

================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:18
+
+Under this assumption all elements in vector whose first elements are equal do
+not guarantee any order. Unfortunately, this prevents libcxx introducing other
----------------
`all elements in the vector`

================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:19-21
+not guarantee any order. Unfortunately, this prevents libcxx introducing other
+algorithms because tests might silently fail and the users might heavily depend
+on the stability of algorithms and containers.
----------------
Do you mean that it prevents libc++ from changing the implementation of algorithms like `std::sort`? I think it's useful to try and use the term "implementation" here, because just saying "algorithm" isn't clear (`std::sort` is commonly referred to as an "algorithm", but you really mean the underlying implementation of it).

================
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
----------------
`LLVM`

================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:43
+  sorted.
+* This macro is off by default because users should turn it only for testing
+  purposes and/or migrations if they happen to libcxx.
----------------
`turn it on`, or alternatively `enable it`

================
Comment at: libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst:49
+  static `std::random_device` for seeding the random number generator. This
+  guarantees the same stability guarantee within run but not through different
+  runs, for example, for tests become flaky and eventually be seen as broken.
----------------
`within a run`

================
Comment at: libcxx/include/algorithm:3113
+
+template <class _Dummy>
+class _LIBCPP_TYPE_VIS __random_unspecified_behavior_gen_
----------------
Why are we making this a template?

================
Comment at: libcxx/include/algorithm:3127-3132
+    result_type operator()() {
+      std::uint_fast64_t __oldstate = __state;
+      __state = __oldstate * 6364136223846793005ULL + __inc;
+      std::uint_fast32_t __xorshifted = ((__oldstate >> 18u) ^ __oldstate) >> 27u;
+      std::uint_fast32_t __rot = __oldstate >> 59u;
+      return (__xorshifted >> __rot) | (__xorshifted << ((-__rot) & 31));
----------------
I'm far from being a PRNG expert - can you explain what you're doing here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96946