Some tests might spin up several threads and FastRandomContext is not thread safe.
Fix that by giving each thread their own randomness context (as opposed to e.g. making FastRandomContext thread safe or add locks elsewhere).
Also, add the g_ prefix to it (according to developer notes), since I am touching it anyway.
This seems overkill, as most tests are single-threaded. The multi-threaded test could just have one RNG per thread?
By that I mean having an explicit “FastRandomContext rng(true);” in each of the threads.
InsecureRandRange helpers. I’d prefer to just add the thread_local keyword unless there are observable performance regressions.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
No idea if this is significant, but mentioning just in case:
Thread-local support used to be optional before this (was only used in sync.h in debug mode optionally). This change ignores HAVE_THREAD_LOCAL and makes it mandatory.
Should those tests perhaps fail explicitly, like in sync.cpp?
0#if !defined(HAVE_THREAD_LOCAL)
1static_assert(false, "thread_local is not supported");
2#endif
Or should we make it mandatory in ./configure?
Some earlier related discussion:
We bumped the requirement from g++ 4.7 to g++ 4.8 in #11755
@theuni wrote there:
thread_local was also off-limits due to missing support in osx’s clang. It’s been added as of XCode 8 on September 13, 2016. I’m uneasy with that as a minimum requirement.
From Apple docs: “Xcode 8.1 requires a Mac running macOS 10.11.5 or later”. We still support macOS 10.10. We could consider bumping that to 10.11.5, or accept that some tests won’t run.
Additionally, last time I checked this, the use of thread_local causes new symbols to be pulled in from glibc on Linux, meaning that we would lose back-compat with older versions. I’d have to re-test to see where the break lies.
So perhaps this depends on if we drop e.g. Ubuntu Trusty 14.04 in the next release.
thread_local was added in C++11, which we require right now and we are about to switch to C++14 soon, so a compiler not supporting it shouldn’t be a problem.
@MarcoFalke That reasoning seems backwards. We identify what C++ version (and features) we can use based on the platforms we want to support. I don’t mean to say that this particular change is a problem, but just because we’re considering C++14 does not mean we should drop support for every platform that doesn’t support every feature from C++11 or C++14.
Does this mean we have now removed support for OSX 10.10 for the unit tests with this change?
We identify what C++ version (and features) we can use based on the platforms we want to support.
It’s probably a bit of both. It’s easier to support older platforms if it only causes minor inconveniences.
This change indeed breaks macOS 10.10 support for the unit tests. Afaik it takes a patch with an ifdef to restore support for the majority of tests. @jonasschnelli might be able to confirm since he has VMs with older macOS versions.