| Message ID | oro8f9avq4.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | require et random_device for cons token test | expand |
On 24/03/21 03:53 -0300, Alexandre Oliva wrote: > >On target systems that don't support any random_device, not even the >default one, It should be impossible to have no random_device. As a fallback a pseudo random number generator should be used. >other random_device constructor tests are disabled by >dg-require-effective-target random_device. The token.cc also >exercises the default constructor, in a way that doesn't expect an >exception to be raised, but it's not guarded by the same requirement. > >Other potentially-raising ctors in token.cc expect exceptions and >handle them, but the ("default")-constructed one does not, so the >program terminates and the test fails without exercising the other >constructor variants. Why does the "default" token cause an exception? >This patch arranges to disable the test altogether when the >random_device feature is not available. A reasonable alternative >would be to install a std::runtime_error handler around the test01 >body, so that we exercise at least the exception raising, but then >test03 would have to be relaxed, since without even "default", it >likely wouldn't meet the tested requirement there. > >Regstrapped on x86_64-linux-gnu and cross-tested for x86_64-vx7r2 along >with other patches, mostly for the testsuite. Ok to install? No, that would disable the test for Windows, where it works OK. The 'random_device' effective-target is poorly named, it checks whether the _GLIBCXX_USE_RANDOM_TR1 macro is defined, which is even more poorly named. That macro is defined when /dev/random and /dev/urandom are available. But we support std::random_device without those files (e.g. on Windows). If the default constructor throws then that suggests your target is misconfigured. Why isn't the mt19937 PRNG being used? >for libstdc++-v3/ChangeLog > > * testsuite/26_numerics/random/random_device/cons/token.cc: > Require effective target feature random_device. >--- > .../26_numerics/random/random_device/cons/token.cc | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc b/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc >index defb8d58c586a..105ae0ba87743 100644 >--- a/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc >+++ b/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc >@@ -1,4 +1,5 @@ > // { dg-do run { target c++11 } } >+// { dg-require-effective-target random_device } > // { dg-require-cstdint "" } > // > // 2008-11-24 Edward M. Smith-Rowland <3dw4rd@verizon.net> > > >-- >Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Vim, Vi, Voltei pro Emacs -- GNUlius Caesar >
On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > It should be impossible to have no random_device. As a fallback a > pseudo random number generator should be used. > If the default constructor throws then that suggests your target is > misconfigured. Why isn't the mt19937 PRNG being used? This is an x86_64-vx7r2 target, it has USE_RDRAND and USE_RDSEED both enabled as the only RAND backends. AFAICT both of them fail their cpuid tests, so _M_init falls through to the default, and throws. I suppose we need to cover the case in which all of the compile-time presumed-available random backends turn out to not be available at run-time, and introduce an MT19937 dynamic fallback in the following default: block, no? default: { } } std::__throw_runtime_error( __N("random_device::random_device(const std::string&):" " device not available")); > The 'random_device' effective-target is poorly named, I see, thanks, I didn't think of checking its definition.
On 24/03/21 08:59 +0000, Jonathan Wakely via Libstdc++ wrote: >On 24/03/21 03:53 -0300, Alexandre Oliva wrote: >> >>On target systems that don't support any random_device, not even the >>default one, > >It should be impossible to have no random_device. As a fallback a >pseudo random number generator should be used. > >>other random_device constructor tests are disabled by >>dg-require-effective-target random_device. The token.cc also >>exercises the default constructor, in a way that doesn't expect an >>exception to be raised, but it's not guarded by the same requirement. >> >>Other potentially-raising ctors in token.cc expect exceptions and >>handle them, but the ("default")-constructed one does not, so the >>program terminates and the test fails without exercising the other >>constructor variants. > >Why does the "default" token cause an exception? > >>This patch arranges to disable the test altogether when the >>random_device feature is not available. A reasonable alternative >>would be to install a std::runtime_error handler around the test01 >>body, so that we exercise at least the exception raising, but then >>test03 would have to be relaxed, since without even "default", it >>likely wouldn't meet the tested requirement there. >> >>Regstrapped on x86_64-linux-gnu and cross-tested for x86_64-vx7r2 along >>with other patches, mostly for the testsuite. Ok to install? > >No, that would disable the test for Windows, where it works OK. > >The 'random_device' effective-target is poorly named, it checks >whether the _GLIBCXX_USE_RANDOM_TR1 macro is defined, which is even >more poorly named. That macro is defined when /dev/random and >/dev/urandom are available. But we support std::random_device without >those files (e.g. on Windows). And that macro is defined according to the same conditions as _GLIBCXX_USE_DEV_RANDOM which is tested via #ifdef in cons/token.cc. So the test already guards the parts that should depend on the _GLIBCXX_USE_RANDOM_TR1 macro. Everything else in the test *should* work unconditionally. In theory. We should find out why they don't (and probably fix the library code, not disable the test). Several of the other tests in that directory should *not* require that effective-target. They should now work unconditionally too. Tangentially, it would also be good to improve the definition of the effective-target so it is true for Windows and other targets that are guaranteed to have a useful (non-pseudo RNG) std::random_device.
On 24/03/21 07:33 -0300, Alexandre Oliva wrote: >On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > >> It should be impossible to have no random_device. As a fallback a >> pseudo random number generator should be used. > >> If the default constructor throws then that suggests your target is >> misconfigured. Why isn't the mt19937 PRNG being used? > >This is an x86_64-vx7r2 target, it has USE_RDRAND and USE_RDSEED both >enabled as the only RAND backends. AFAICT both of them fail their cpuid >tests, so _M_init falls through to the default, and throws. > >I suppose we need to cover the case in which all of the compile-time >presumed-available random backends turn out to not be available at >run-time, and introduce an MT19937 dynamic fallback in the following >default: block, no? Hmm, that would be tricky because it's currently a static decision made during preprocessing. We have no disciminator to tell us at runtime that the _M_mt member of the union is active: union { struct { void* _M_file; result_type (*_M_func)(void*); int _M_fd; }; mt19937 _M_mt; }; Does vxworks provide any platform-specific source of randomness, like Linux getrandom(2) or BSD arc4random(3) or Windows rand_s? If yes, we should add support for that (in the next stage 1). But even if it does, we should still have a dynamic fallback at runtime. I have a patch coming to do that ...
> On Mar 24, 2021, at 4:59 AM, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On 24/03/21 03:53 -0300, Alexandre Oliva wrote: >> >> On target systems that don't support any random_device, not even the >> default one, > > It should be impossible to have no random_device. Not true; deeply embedded systems might not have one. Among GCC platforms, pdp11 doesn't have one, and at least some vax platforms probably don't either. > As a fallback a > pseudo random number generator should be used. Presumably yes -- it seems unlikely that GCC tests depend on cryptographic strength of the random nummber generator. If a PRNG is used then the classic FORTRAN "random" function would serve. paul
On 24/03/21 13:22 +0000, Koning, Paul via Libstdc++ wrote: > > >> On Mar 24, 2021, at 4:59 AM, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> >> On 24/03/21 03:53 -0300, Alexandre Oliva wrote: >>> >>> On target systems that don't support any random_device, not even the >>> default one, >> >> It should be impossible to have no random_device. > >Not true; deeply embedded systems might not have one. Among GCC platforms, pdp11 doesn't have one, and at least some vax platforms probably don't either. I'm talking about the C++ std::random_device type, not the /dev/random block device. >> As a fallback a >> pseudo random number generator should be used. > >Presumably yes -- it seems unlikely that GCC tests depend on cryptographic strength of the random nummber generator. If a PRNG is used then the classic FORTRAN "random" function would serve. Libstdc++ already contains several PNRGs, we don't need to use Fortran's.
On 24/03/21 11:27 +0000, Jonathan Wakely wrote: >On 24/03/21 07:33 -0300, Alexandre Oliva wrote: >>On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: >> >>>It should be impossible to have no random_device. As a fallback a >>>pseudo random number generator should be used. >> >>>If the default constructor throws then that suggests your target is >>>misconfigured. Why isn't the mt19937 PRNG being used? >> >>This is an x86_64-vx7r2 target, it has USE_RDRAND and USE_RDSEED both >>enabled as the only RAND backends. AFAICT both of them fail their cpuid >>tests, so _M_init falls through to the default, and throws. >> >>I suppose we need to cover the case in which all of the compile-time >>presumed-available random backends turn out to not be available at >>run-time, and introduce an MT19937 dynamic fallback in the following >>default: block, no? > >Hmm, that would be tricky because it's currently a static decision >made during preprocessing. We have no disciminator to tell us at >runtime that the _M_mt member of the union is active: > > union > { > struct > { > void* _M_file; > result_type (*_M_func)(void*); > int _M_fd; > }; > mt19937 _M_mt; > }; > >Does vxworks provide any platform-specific source of randomness, like >Linux getrandom(2) or BSD arc4random(3) or Windows rand_s? If yes, we >should add support for that (in the next stage 1). > >But even if it does, we should still have a dynamic fallback at >runtime. I have a patch coming to do that ... Attached (the patch is generated with -b to ignore changes in whitespace, for ease of reading the diff). This works for me on x86_64-linux and powerpc64le-linux, and also on x86_64-linux when I kluge the config macros so that the new code path gets used. Does this work for VxWorks? The problem this fixes is a regression. Before GCC 10 VxWorks would have always used the mt19937 code. Now it tries to use rdrand/rdseed instructions and fails if the hardware doesn't have them. This patch should make it work again (using a different PRNG, and a crappy seed but that's still better than mt19937 with the same seed every time).
On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > This works for me on x86_64-linux and powerpc64le-linux, and also on > x86_64-linux when I kluge the config macros so that the new code path > gets used. Does this work for VxWorks? Thanks. I (trivially) backported it to apply on our gcc-10 tree, and tested that on x86_64-vx7r2, and I confirm it works there too. However, I suspect there's a series of typos in the patch. You appear to be using the 'which' enum variable for bit testing, but with '|' rather than '&'. Unless I'm missing something in my reading of the modified code, this may cause a backend different from that requested by the token to be selected, but it doesn't look like we have any test that detects this problem.
On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > Does vxworks provide any platform-specific source of randomness, like > Linux getrandom(2) or BSD arc4random(3) or Windows rand_s? If yes, we > should add support for that (in the next stage 1). There appears to be a randNumGenCtl syscall that appears to be relevant to that end, and randAdd to seed and randBytes to obtain random bytes in vxRandLib. I couldn't find documentation on how to use it, but there seems to be some code using it in openssl. Sorry, I don't know a lot about vxworks.
On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > diff --git a/libstdc++-v3/testsuite/util/testsuite_random.h b/libstdc++-v3/testsuite/util/testsuite_random.h > index 0b670bfb771..c8323078492 100644 > --- a/libstdc++-v3/testsuite/util/testsuite_random.h > +++ b/libstdc++-v3/testsuite/util/testsuite_random.h > @@ -197,6 +197,19 @@ namespace __gnu_test > } > #endif > + // Check whether TOKEN can construct a std::random_device successfully. > + inline bool > + random_device_available(const std::string& token) noexcept > + { > + try { > + std::random_device dev(token); > + return true; > + } catch (...) { > + std::printf("random_device(\"%s\") not available\n", token); Another nit: I'm seeing line noise (do people still use this term? :-) in libstdc++.log where unavailable random_device tokens should appear. I suspect the token has to be converted to a C string in the call above.
On 25/03/21 08:00 -0300, Alexandre Oliva wrote: >On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > >> Does vxworks provide any platform-specific source of randomness, like >> Linux getrandom(2) or BSD arc4random(3) or Windows rand_s? If yes, we >> should add support for that (in the next stage 1). > >There appears to be a randNumGenCtl syscall that appears to be relevant >to that end, and randAdd to seed and randBytes to obtain random bytes in >vxRandLib. I couldn't find documentation on how to use it, but there >seems to be some code using it in openssl. Sorry, I don't know a lot >about vxworks. Thanks! We could look into that in stage 1.
On 25/03/21 08:03 -0300, Alexandre Oliva wrote: >On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > >> diff --git a/libstdc++-v3/testsuite/util/testsuite_random.h b/libstdc++-v3/testsuite/util/testsuite_random.h >> index 0b670bfb771..c8323078492 100644 >> --- a/libstdc++-v3/testsuite/util/testsuite_random.h >> +++ b/libstdc++-v3/testsuite/util/testsuite_random.h >> @@ -197,6 +197,19 @@ namespace __gnu_test >> } >> #endif > >> + // Check whether TOKEN can construct a std::random_device successfully. >> + inline bool >> + random_device_available(const std::string& token) noexcept >> + { >> + try { >> + std::random_device dev(token); >> + return true; >> + } catch (...) { >> + std::printf("random_device(\"%s\") not available\n", token); > >Another nit: I'm seeing line noise (do people still use this term? :-) >in libstdc++.log where unavailable random_device tokens should appear. >I suspect the token has to be converted to a C string in the call above. Doh, the function took a const char* then I changed it to std::string because some of the callers wanted to pass a std::string. I'll fix it in my branch.
On 25/03/21 07:17 -0300, Alexandre Oliva wrote: >On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > >> This works for me on x86_64-linux and powerpc64le-linux, and also on >> x86_64-linux when I kluge the config macros so that the new code path >> gets used. Does this work for VxWorks? > >Thanks. I (trivially) backported it to apply on our gcc-10 tree, and >tested that on x86_64-vx7r2, and I confirm it works there too. > >However, I suspect there's a series of typos in the patch. You appear >to be using the 'which' enum variable for bit testing, but with '|' >rather than '&'. Oops, that's what I get for a last-minute rewrite without proper testing. I originally had: if (which == blah || which == any) and then borked it in an attempt to use & instead. I'll fix that locally too. >Unless I'm missing something in my reading of the modified code, this >may cause a backend different from that requested by the token to be >selected, but it doesn't look like we have any test that detects this >problem. I don't see how it's possible to detect, without something like a modified system that replaces /dev/random with a non-random source so you can verify that std::random_device("/dev/random") and std::random_device("rdseed") don't use the same source. I suppose we could check the number of open file descriptors as a proxy for "something is reading /dev/random", but there's no way to check that std::random_device(tok1) and std::random_device(tok2) use different sources of randomness.
On 25/03/21 11:57 +0000, Jonathan Wakely wrote: >On 25/03/21 07:17 -0300, Alexandre Oliva wrote: >>On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: >> >>>This works for me on x86_64-linux and powerpc64le-linux, and also on >>>x86_64-linux when I kluge the config macros so that the new code path >>>gets used. Does this work for VxWorks? >> >>Thanks. I (trivially) backported it to apply on our gcc-10 tree, and >>tested that on x86_64-vx7r2, and I confirm it works there too. >> >>However, I suspect there's a series of typos in the patch. You appear >>to be using the 'which' enum variable for bit testing, but with '|' >>rather than '&'. > >Oops, that's what I get for a last-minute rewrite without proper >testing. I originally had: > > if (which == blah || which == any) > >and then borked it in an attempt to use & instead. > >I'll fix that locally too. Here's what I've pushed to trunk. Tested x86_64-linux, powerpc64le-linux, x86_64-w64-mingw.
On Thu, 25 Mar 2021 at 11:38, Jonathan Wakely wrote: > On 25/03/21 08:00 -0300, Alexandre Oliva wrote: > >On Mar 24, 2021, Jonathan Wakely <jwakely@redhat.com> wrote: > > > >> Does vxworks provide any platform-specific source of randomness, like > >> Linux getrandom(2) or BSD arc4random(3) or Windows rand_s? If yes, we > >> should add support for that (in the next stage 1). > > > >There appears to be a randNumGenCtl syscall that appears to be relevant > >to that end, and randAdd to seed and randBytes to obtain random bytes in > >vxRandLib. I couldn't find documentation on how to use it, but there > >seems to be some code using it in openssl. Sorry, I don't know a lot > >about vxworks. > > Thanks! We could look into that in stage 1. > Maybe something like this, completely untested and not ready to commit. diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc index 70fd520077a..eb092cd0950 100644 --- a/libstdc++-v3/src/c++11/random.cc +++ b/libstdc++-v3/src/c++11/random.cc @@ -75,6 +75,14 @@ # include <unistd.h> #endif +#if __has_include(<_vxworks-versions.h>) +# include <_vxworks-versions.h> +# if _VXWORKS_MAJOR_GE(7) && __has_include(<randomNumGen.h>) +# include <randomNumGen.h> +# define USE_RANDBYTES 1 +# endif +#endif + #if defined _GLIBCXX_USE_CRT_RAND_S || defined _GLIBCXX_USE_DEV_RANDOM \ || _GLIBCXX_HAVE_GETENTROPY // The OS provides a source of randomness we can use. @@ -221,6 +229,29 @@ namespace std _GLIBCXX_VISIBILITY(default) } #endif +#if USE_RANDBYTES + unsigned int + __vx_randBytes(void*) + { + int retries = 10; + unsigned int val; + auto bytes = reinterpret_cast<unsigned char*>(&val); + while (retries-- > 0) + { + RANDOM_NUM_GEN_STATUS status = randStatus(); + if (status == RANDOM_NUM_GEN_ENOUGH_ENTROPY + || status == RANDOM_NUM_GEN_MAX_ENTROPY) + { + if (randBytes(bytes, sizeof(val)) == OK) + return val; + } + else + taskDelay(5); + } + std::__throw_runtime_error(__N("random_device: randBytes failed")); + } +#endif + #ifdef USE_LCG // TODO: use this to seed std::mt19937 engine too. unsigned @@ -271,6 +302,7 @@ namespace std _GLIBCXX_VISIBILITY(default) enum Which : unsigned { device_file = 1, prng = 2, rand_s = 4, getentropy = 8, arc4random = 16, rdseed = 64, rdrand = 128, darn = 256, rndr = 512, + randBytes = 1024, any = 0xffff }; @@ -326,6 +358,11 @@ namespace std _GLIBCXX_VISIBILITY(default) return getentropy; #endif +#if USE_RANDBYTES + if (func == __vx_randBytes) + return randBytes; +#endif + #ifdef USE_LCG if (func == &__lcg) return prng; @@ -506,6 +543,14 @@ namespace std _GLIBCXX_VISIBILITY(default) } #endif // _GLIBCXX_HAVE_GETENTROPY +#if USE_RANDBYTES + if (which & randBytes) + { + _M_func = __vx_randBytes; + return; + } +#endif + #ifdef _GLIBCXX_USE_DEV_RANDOM if (which & device_file) { @@ -666,6 +711,8 @@ namespace std _GLIBCXX_VISIBILITY(default) case rand_s: case prng: return 0.0; + case randBytes: // XXX is this a real source of entropy or PRNG? + return 0.0; case device_file: // handled below break;
diff --git a/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc b/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc index defb8d58c586a..105ae0ba87743 100644 --- a/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc +++ b/libstdc++-v3/testsuite/26_numerics/random/random_device/cons/token.cc @@ -1,4 +1,5 @@ // { dg-do run { target c++11 } } +// { dg-require-effective-target random_device } // { dg-require-cstdint "" } // // 2008-11-24 Edward M. Smith-Rowland <3dw4rd@verizon.net>
On target systems that don't support any random_device, not even the default one, other random_device constructor tests are disabled by dg-require-effective-target random_device. The token.cc also exercises the default constructor, in a way that doesn't expect an exception to be raised, but it's not guarded by the same requirement. Other potentially-raising ctors in token.cc expect exceptions and handle them, but the ("default")-constructed one does not, so the program terminates and the test fails without exercising the other constructor variants. This patch arranges to disable the test altogether when the random_device feature is not available. A reasonable alternative would be to install a std::runtime_error handler around the test01 body, so that we exercise at least the exception raising, but then test03 would have to be relaxed, since without even "default", it likely wouldn't meet the tested requirement there. Regstrapped on x86_64-linux-gnu and cross-tested for x86_64-vx7r2 along with other patches, mostly for the testsuite. Ok to install? for libstdc++-v3/ChangeLog * testsuite/26_numerics/random/random_device/cons/token.cc: Require effective target feature random_device. --- .../26_numerics/random/random_device/cons/token.cc | 1 + 1 file changed, 1 insertion(+)