* [PATCH] libstdc++: testsuite: avoid predictable mkstemp @ 2022-06-22 6:04 Alexandre Oliva 2022-06-22 9:16 ` Jonathan Wakely 0 siblings, 1 reply; 10+ messages in thread From: Alexandre Oliva @ 2022-06-22 6:04 UTC (permalink / raw) To: gcc-patches, libstdc++ This patch was originally meant to reduce the likelihood that nonexistent_path() returns the same pathname for from and to. It was prompted by a target system with a non-random implementation of mkstemp, that returns a predictable sequence of filenames and selects the first one that isn't already taken. That turned out not to be enough: nonexistent_path adds a suffix to the filename chosen by mkstemp and removes the file it created, so mkstemp may very well insist on the same basename, and the case that doesn't use mkstemp doesn't even check whether the file already exists. Anyway, by the time I realized this wasn't enough, I'd already implemented some of the changes, and I figured I might as well contribute them, even though they don't really solve any problem, and even if they did, they'd be just a partial solution. Regstrapped on x86_64-linux-gnu, also tested with a cross to aarch64-rtems6. Ok to install? for libstdc++-v3/ChangeLog * testsuite/27_io/filesystem/operations/copy.cc (test02): Select TO after creating FROM. (test03, test04): Likewise. * testsuite/experimental/filesystem/operations/copy.cc (test02, test03, test04): Likewise. --- .../testsuite/27_io/filesystem/operations/copy.cc | 7 ++++--- .../experimental/filesystem/operations/copy.cc | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc index b936e04493b5c..f3081f4b64ebc 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc @@ -73,7 +73,6 @@ test02() const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec; ec = bad_ec; @@ -81,6 +80,7 @@ test02() VERIFY( !ec ); VERIFY( fs::exists(from) ); + auto to = __gnu_test::nonexistent_path(); ec = bad_ec; fs::copy(from, to, fs::copy_options::skip_symlinks, ec); VERIFY( !ec ); @@ -117,12 +117,13 @@ void test03() { auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); // test empty file std::ofstream{from}; VERIFY( fs::exists(from) ); VERIFY( fs::file_size(from) == 0 ); + + auto to = __gnu_test::nonexistent_path(); fs::copy(from, to); VERIFY( fs::exists(to) ); VERIFY( fs::file_size(to) == 0 ); @@ -145,11 +146,11 @@ test04() { const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec; create_directories(from/"a/b/c"); + auto to = __gnu_test::nonexistent_path(); { __gnu_test::scoped_file f(to); copy(from, to, ec); diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc index 5cd6b483c269b..ca38328c5da15 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc @@ -73,7 +73,6 @@ test02() #endif auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec, bad = std::make_error_code(std::errc::invalid_argument); ec = bad; @@ -81,6 +80,7 @@ test02() VERIFY( !ec ); VERIFY( fs::exists(from) ); + auto to = __gnu_test::nonexistent_path(); ec = bad; fs::copy(from, to, fs::copy_options::skip_symlinks, ec); VERIFY( !ec ); @@ -116,12 +116,13 @@ void test03() { auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); // test empty file std::ofstream{from.c_str()}; VERIFY( fs::exists(from) ); VERIFY( fs::file_size(from) == 0 ); + + auto to = __gnu_test::nonexistent_path(); fs::copy(from, to); VERIFY( fs::exists(to) ); VERIFY( fs::file_size(to) == 0 ); @@ -143,11 +144,11 @@ void test04() { auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec; create_directories(from/"a/b/c"); + auto to = __gnu_test::nonexistent_path(); { __gnu_test::scoped_file f(to); copy(from, to, ec); -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about <https://stallmansupport.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-06-22 6:04 [PATCH] libstdc++: testsuite: avoid predictable mkstemp Alexandre Oliva @ 2022-06-22 9:16 ` Jonathan Wakely 2022-06-23 11:39 ` Alexandre Oliva 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Wakely @ 2022-06-22 9:16 UTC (permalink / raw) To: Alexandre Oliva; +Cc: gcc Patches, libstdc++ On Wed, 22 Jun 2022 at 07:05, Alexandre Oliva via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > > This patch was originally meant to reduce the likelihood that > nonexistent_path() returns the same pathname for from and to. > > It was prompted by a target system with a non-random implementation of > mkstemp, that returns a predictable sequence of filenames and selects > the first one that isn't already taken. > > That turned out not to be enough: nonexistent_path adds a suffix to > the filename chosen by mkstemp and removes the file it created, so > mkstemp may very well insist on the same basename, and the case that > doesn't use mkstemp doesn't even check whether the file already > exists. > > Anyway, by the time I realized this wasn't enough, I'd already > implemented some of the changes, and I figured I might as well > contribute them, even though they don't really solve any problem, and > even if they did, they'd be just a partial solution. > > Regstrapped on x86_64-linux-gnu, also tested with a cross to > aarch64-rtems6. Ok to install? OK ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-06-22 9:16 ` Jonathan Wakely @ 2022-06-23 11:39 ` Alexandre Oliva 2022-06-23 16:36 ` Jonathan Wakely 0 siblings, 1 reply; 10+ messages in thread From: Alexandre Oliva @ 2022-06-23 11:39 UTC (permalink / raw) To: Jonathan Wakely; +Cc: gcc Patches, libstdc++ On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > On Wed, 22 Jun 2022 at 07:05, Alexandre Oliva via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: >> It was prompted by a target system with a non-random implementation of >> mkstemp, that returns a predictable sequence of filenames and selects >> the first one that isn't already taken. > OK And here's the patch that enabled me to stop worrying about the above. Regstrapped on x86_64-linux-gnu, also tested with a cross to aarch64-rtems6. Ok to install? __gnu_test::nonexistent_path: Always include counter in filename returned From: Joel Brobecker <brobecker@adacore.com> We have noticed that, on RTEMS, a small number of testscases are failing because two calls to this method return the same filename. This happens for instance in 27_io/filesystem/operations/copy_file.cc where it does: auto from = __gnu_test::nonexistent_path(); auto to = __gnu_test::nonexistent_path(); We tracked this issue down to the fact that the implementation of mkstemp on that system appears to use a very predictable algorithm for chosing the name of the temporary file, where the same filename appears to be tried in the same order, regardless of past calls. So, as long as the file gets deleted after a call to mkstemp (something we do here in our nonexistent_path method), the next call to mkstemps ends up returning the same filename, causing the collision we se above. This commit enhances the __gnu_test::nonexistent_path method to introduce in the filename being returned a counter which gets incremented at every call of this method. libstdc++-v3/ChangeLog: * testsuite/util/testsuite_fs.h (__gnu_test::nonexistent_path): Always include a counter in the filename returned. --- libstdc++-v3/testsuite/util/testsuite_fs.h | 31 ++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h index 037d9ffc0f429..206ea67779003 100644 --- a/libstdc++-v3/testsuite/util/testsuite_fs.h +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h @@ -38,9 +38,9 @@ namespace test_fs = std::experimental::filesystem; #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L #include <stdlib.h> // mkstemp -#else -#include <random> // std::random_device +#include <cstring> // strcpy #endif +#include <random> // std::random_device #if defined(__MINGW32__) || defined(__MINGW64__) \ || !defined (_GLIBCXX_HAVE_SYMLINK) @@ -125,8 +125,32 @@ namespace __gnu_test file.erase(0, pos+1); test_fs::path p; + // A counter, starting from a random value, to be included as part + // of the filename being returned, and incremented each time + // this method is used. It allows us to ensure that two calls + // to this method can never return the same filename, something + // testcases do when they need multiple non-existent filenames + // for their purposes. + static unsigned counter = std::random_device{}(); + #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L - char tmp[] = "filesystem-test.XXXXXX"; + // Use mkstemp to determine the name of a file which does not exist yet. + // + // Note that we have seen on some systems (such as RTEMS, for instance) + // that mkstemp behaves very predictably, causing it to always try + // the same sequence of file names. In other words, if we call mkstemp + // with a pattern, delete the file it created (which is what we do, here), + // and call mkstemp with the same pattern again, it returns the same + // filename once more. While most implementations introduce a degree + // of randomness, it is not mandated by the standard, and this is why + // we include a counter in the template passed to mkstemp. + std::string mkstemp_template ("filesystem-test."); + mkstemp_template.append(std::to_string (counter++)); + mkstemp_template.append(".XXXXXX"); + + char tmp[mkstemp_template.length() + 1]; + std::strcpy (tmp, mkstemp_template.c_str()); + int fd = ::mkstemp(tmp); if (fd == -1) throw test_fs::filesystem_error("mkstemp failed", @@ -141,7 +165,6 @@ namespace __gnu_test if (file.length() > 64) file.resize(64); char buf[128]; - static unsigned counter = std::random_device{}(); #if _GLIBCXX_USE_C99_STDIO std::snprintf(buf, 128, #else -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about <https://stallmansupport.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-06-23 11:39 ` Alexandre Oliva @ 2022-06-23 16:36 ` Jonathan Wakely 2022-06-27 9:31 ` Alexandre Oliva 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Wakely @ 2022-06-23 16:36 UTC (permalink / raw) To: Alexandre Oliva, Joel Brobecker; +Cc: gcc Patches, libstdc++ [-- Attachment #1: Type: text/plain, Size: 4690 bytes --] On Thu, 23 Jun 2022 at 12:39, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > On Wed, 22 Jun 2022 at 07:05, Alexandre Oliva via Libstdc++ > > <libstdc++@gcc.gnu.org> wrote: > > >> It was prompted by a target system with a non-random implementation of > >> mkstemp, that returns a predictable sequence of filenames and selects > >> the first one that isn't already taken. > > > OK > > And here's the patch that enabled me to stop worrying about the above. > Regstrapped on x86_64-linux-gnu, also tested with a cross to > aarch64-rtems6. Ok to install? > > > __gnu_test::nonexistent_path: Always include counter in filename returned > > From: Joel Brobecker <brobecker@adacore.com> > > We have noticed that, on RTEMS, a small number of testscases are > failing because two calls to this method return the same filename. > This happens for instance in 27_io/filesystem/operations/copy_file.cc > where it does: > > auto from = __gnu_test::nonexistent_path(); > auto to = __gnu_test::nonexistent_path(); > > We tracked this issue down to the fact that the implementation of > mkstemp on that system appears to use a very predictable algorithm > for chosing the name of the temporary file, where the same filename > appears to be tried in the same order, regardless of past calls. > So, as long as the file gets deleted after a call to mkstemp (something > we do here in our nonexistent_path method), the next call to mkstemps > ends up returning the same filename, causing the collision we se above. > > This commit enhances the __gnu_test::nonexistent_path method to > introduce in the filename being returned a counter which gets > incremented at every call of this method. > > libstdc++-v3/ChangeLog: > > * testsuite/util/testsuite_fs.h (__gnu_test::nonexistent_path): > Always include a counter in the filename returned. > --- > libstdc++-v3/testsuite/util/testsuite_fs.h | 31 ++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h > index 037d9ffc0f429..206ea67779003 100644 > --- a/libstdc++-v3/testsuite/util/testsuite_fs.h > +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h > @@ -38,9 +38,9 @@ namespace test_fs = std::experimental::filesystem; > > #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L > #include <stdlib.h> // mkstemp > -#else > -#include <random> // std::random_device > +#include <cstring> // strcpy > #endif > +#include <random> // std::random_device > > #if defined(__MINGW32__) || defined(__MINGW64__) \ > || !defined (_GLIBCXX_HAVE_SYMLINK) > @@ -125,8 +125,32 @@ namespace __gnu_test > file.erase(0, pos+1); > > test_fs::path p; > + // A counter, starting from a random value, to be included as part > + // of the filename being returned, and incremented each time > + // this method is used. It allows us to ensure that two calls > + // to this method can never return the same filename, something > + // testcases do when they need multiple non-existent filenames > + // for their purposes. > + static unsigned counter = std::random_device{}(); > + > #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L > - char tmp[] = "filesystem-test.XXXXXX"; > + // Use mkstemp to determine the name of a file which does not exist yet. > + // > + // Note that we have seen on some systems (such as RTEMS, for instance) > + // that mkstemp behaves very predictably, causing it to always try > + // the same sequence of file names. In other words, if we call mkstemp > + // with a pattern, delete the file it created (which is what we do, here), > + // and call mkstemp with the same pattern again, it returns the same > + // filename once more. While most implementations introduce a degree > + // of randomness, it is not mandated by the standard, and this is why > + // we include a counter in the template passed to mkstemp. > + std::string mkstemp_template ("filesystem-test."); > + mkstemp_template.append(std::to_string (counter++)); > + mkstemp_template.append(".XXXXXX"); > + > + char tmp[mkstemp_template.length() + 1]; > + std::strcpy (tmp, mkstemp_template.c_str()); std::string::copy can be used to copy a string into a buffer, but there's no reason to use another buffer anyway. The attached makes this a bit more efficient, and makes more of the code common to the mkstemp and non-mkstmp branches. I'll wait to hear back from you before pushing it (since it has Joel's name on the patch). [-- Attachment #2: patch.txt --] [-- Type: text/plain, Size: 4808 bytes --] commit 833337d861e3126f6304e8bc9d8e743022a21424 Author: Joel Brobecker <brobecker@adacore.com> Date: Thu Jun 23 13:12:12 2022 libstdc++: testsuite: avoid predicable mkstemp We have noticed that, on RTEMS, a small number of testscases are failing because two calls to this method return the same filename. This happens for instance in 27_io/filesystem/operations/copy_file.cc where it does: auto from = __gnu_test::nonexistent_path(); auto to = __gnu_test::nonexistent_path(); We tracked this issue down to the fact that the implementation of mkstemp on that system appears to use a very predictable algorithm for chosing the name of the temporary file, where the same filename appears to be tried in the same order, regardless of past calls. So, as long as the file gets deleted after a call to mkstemp (something we do here in our nonexistent_path method), the next call to mkstemps ends up returning the same filename, causing the collision we se above. This commit enhances the __gnu_test::nonexistent_path method to introduce in the filename being returned a counter which gets incremented at every call of this method. Co-authored-by: Jonathan Wakely <jwakely@redhat.com> libstdc++-v3/ChangeLog: * testsuite/util/testsuite_fs.h (__gnu_test::nonexistent_path): Always include a counter in the filename returned. diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h index 9358a04e56c..3a197c70a1e 100644 --- a/libstdc++-v3/testsuite/util/testsuite_fs.h +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h @@ -32,14 +32,14 @@ namespace test_fs = std::experimental::filesystem; #endif #include <algorithm> #include <fstream> +#include <random> // std::random_device #include <string> +#include <system_error> #include <cstdio> #include <unistd.h> // unlink, close, getpid, geteuid #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L #include <stdlib.h> // mkstemp -#else -#include <random> // std::random_device #endif namespace __gnu_test @@ -109,32 +109,51 @@ namespace __gnu_test if (pos != file.npos) file.erase(0, pos+1); + file.reserve(file.size() + 40); + file.insert(0, "filesystem-test."); + + // A counter, starting from a random value, to be included as part + // of the filename being returned, and incremented each time + // this function is used. It allows us to ensure that two calls + // to this function can never return the same filename, something + // testcases do when they need multiple non-existent filenames + // for their purposes. + static unsigned counter = std::random_device{}(); + file += '.'; + file += std::to_string(counter++); + file += '.'; + test_fs::path p; #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L - char tmp[] = "filesystem-test.XXXXXX"; - int fd = ::mkstemp(tmp); + + // Use mkstemp to determine the name of a file which does not exist yet. + // + // Note that we have seen on some systems (such as RTEMS, for instance) + // that mkstemp behaves very predictably, causing it to always try + // the same sequence of file names. In other words, if we call mkstemp + // with a pattern, delete the file it created (which is what we do, here), + // and call mkstemp with the same pattern again, it returns the same + // filename once more. While most implementations introduce a degree + // of randomness, it is not mandated by the standard, and this is why + // we also include a counter in the template passed to mkstemp. + file += "XXXXXX"; + int fd = ::mkstemp(&file[0]); if (fd == -1) throw test_fs::filesystem_error("mkstemp failed", std::error_code(errno, std::generic_category())); - ::unlink(tmp); + ::unlink(file.c_str()); ::close(fd); - if (!file.empty()) - file.insert(0, 1, '-'); - file.insert(0, tmp); - p = file; + p = std::move(file); #else if (file.length() > 64) file.resize(64); - char buf[128]; - static unsigned counter = std::random_device{}(); -#if _GLIBCXX_USE_C99_STDIO - std::snprintf(buf, 128, -#else - std::sprintf(buf, -#endif - "filesystem-test.%u.%lu-%s", counter++, (unsigned long) ::getpid(), - file.c_str()); - p = buf; + // The combination of random counter and PID should be unique for a given + // run of the testsuite. + file += std::to_string(::getpid()); + p = std::move(file); + if (test_fs::exists(p)) + throw test_fs::filesystem_error("Failed to generate unique pathname", p, + std::make_error_code(std::errc::file_exists)); #endif return p; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-06-23 16:36 ` Jonathan Wakely @ 2022-06-27 9:31 ` Alexandre Oliva 2022-06-27 10:18 ` Jonathan Wakely 2022-07-05 9:10 ` Alexandre Oliva 0 siblings, 2 replies; 10+ messages in thread From: Alexandre Oliva @ 2022-06-27 9:31 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Joel Brobecker, gcc Patches, libstdc++ On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > The attached makes this a bit more efficient, and makes more of the > code common to the mkstemp and non-mkstmp branches. I'll wait to hear > back from you before pushing it (since it has Joel's name on the > patch). Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it works for us. -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about <https://stallmansupport.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-06-27 9:31 ` Alexandre Oliva @ 2022-06-27 10:18 ` Jonathan Wakely 2022-07-05 9:10 ` Alexandre Oliva 1 sibling, 0 replies; 10+ messages in thread From: Jonathan Wakely @ 2022-06-27 10:18 UTC (permalink / raw) To: Alexandre Oliva; +Cc: Joel Brobecker, gcc Patches, libstdc++ On Mon, 27 Jun 2022 at 10:32, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > The attached makes this a bit more efficient, and makes more of the > > code common to the mkstemp and non-mkstmp branches. I'll wait to hear > > back from you before pushing it (since it has Joel's name on the > > patch). > > Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it > works for us. I've pushed it to trunk now. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-06-27 9:31 ` Alexandre Oliva 2022-06-27 10:18 ` Jonathan Wakely @ 2022-07-05 9:10 ` Alexandre Oliva 2022-07-05 9:16 ` Jonathan Wakely 1 sibling, 1 reply; 10+ messages in thread From: Alexandre Oliva @ 2022-07-05 9:10 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Joel Brobecker, gcc Patches, libstdc++ On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: >> The attached makes this a bit more efficient, and makes more of the >> code common to the mkstemp and non-mkstmp branches. I'll wait to hear >> back from you before pushing it (since it has Joel's name on the >> patch). > Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it > works for us. The bad news is that it broke on some other systems I didn't test back then. It turns out the type cast for the ::getpid result was not just because it was passed to printf before :-/ libstdc++: testsuite: cast getpid result On vxworks, in kernel mode, getpid's return type is a pointer type, so std::to_string on it fails overload resolution. Restore the type cast from the original patch that suggested adding the pid. Regstrapped on x86_64-linux-gnu, also tested on aarch64-rtems6.0 and ppc64-vx7r2. I'm going ahead and checking this in as obvious. Please let me know if you'd prefer this to be fixed in a different way. for libstdc++/ChangeLog * testsuite/util/testsuite_fs.h (nonexistent_path): Convert the getpid result to an integral type. --- libstdc++-v3/testsuite/util/testsuite_fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h index 908fcdbcaeed1..25f8f734dc792 100644 --- a/libstdc++-v3/testsuite/util/testsuite_fs.h +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h @@ -163,7 +163,7 @@ namespace __gnu_test file.resize(64); // The combination of random counter and PID should be unique for a given // run of the testsuite. - file += std::to_string(::getpid()); + file += std::to_string((unsigned long) ::getpid()); p = std::move(file); if (test_fs::exists(p)) throw test_fs::filesystem_error("Failed to generate unique pathname", p, -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about <https://stallmansupport.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-07-05 9:10 ` Alexandre Oliva @ 2022-07-05 9:16 ` Jonathan Wakely 2022-07-05 17:45 ` Alexandre Oliva 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Wakely @ 2022-07-05 9:16 UTC (permalink / raw) To: Alexandre Oliva; +Cc: libstdc++, gcc Patches, Joel Brobecker On Tue, 5 Jul 2022 at 10:10, Alexandre Oliva via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > > > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > >> The attached makes this a bit more efficient, and makes more of the > >> code common to the mkstemp and non-mkstmp branches. I'll wait to hear > >> back from you before pushing it (since it has Joel's name on the > >> patch). > > > Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it > > works for us. > > The bad news is that it broke on some other systems I didn't test back > then. It turns out the type cast for the ::getpid result was not just > because it was passed to printf before :-/ Ah, whoops. I did check that POSIX requires pid_t to be a signed integer type, but that doesn't mean it's always true for all GCC targets. > > > libstdc++: testsuite: cast getpid result > > On vxworks, in kernel mode, getpid's return type is a pointer type, so > std::to_string on it fails overload resolution. Restore the type cast > from the original patch that suggested adding the pid. > > Regstrapped on x86_64-linux-gnu, also tested on aarch64-rtems6.0 and > ppc64-vx7r2. I'm going ahead and checking this in as obvious. Please > let me know if you'd prefer this to be fixed in a different way. The cast itself is fine, but I'd like a comment like "N.B. pid_t is a pointer on vxworks" so I don't "simplify" it again. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-07-05 9:16 ` Jonathan Wakely @ 2022-07-05 17:45 ` Alexandre Oliva 2022-07-05 18:02 ` Jonathan Wakely 0 siblings, 1 reply; 10+ messages in thread From: Alexandre Oliva @ 2022-07-05 17:45 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc Patches, Joel Brobecker On Jul 5, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > The cast itself is fine, but I'd like a comment like "N.B. pid_t is a > pointer on vxworks" so I don't "simplify" it again. libstdc++: testsuite: why cast getpid result Add a comment next to the getpid call to explain why the typecast is needed. Regstrapped on x86_64-linux-gnu, will install later today if there's no objection. for libstdc++-v3/ChangeLog * testsuite/util/testsuite_fs (nonexistent_path): Explain why we need the typecast. --- libstdc++-v3/testsuite/util/testsuite_fs.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h index 25f8f734dc792..0e28385e99aca 100644 --- a/libstdc++-v3/testsuite/util/testsuite_fs.h +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h @@ -162,7 +162,8 @@ namespace __gnu_test if (file.length() > 64) file.resize(64); // The combination of random counter and PID should be unique for a given - // run of the testsuite. + // run of the testsuite. N.B. getpid() returns a pointer type on vxworks + // in kernel mode. file += std::to_string((unsigned long) ::getpid()); p = std::move(file); if (test_fs::exists(p)) -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about <https://stallmansupport.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp 2022-07-05 17:45 ` Alexandre Oliva @ 2022-07-05 18:02 ` Jonathan Wakely 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Wakely @ 2022-07-05 18:02 UTC (permalink / raw) To: Alexandre Oliva; +Cc: libstdc++, gcc Patches, Joel Brobecker On Tue, 5 Jul 2022 at 18:46, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jul 5, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > The cast itself is fine, but I'd like a comment like "N.B. pid_t is a > > pointer on vxworks" so I don't "simplify" it again. > > > libstdc++: testsuite: why cast getpid result > > Add a comment next to the getpid call to explain why the typecast is > needed. > > Regstrapped on x86_64-linux-gnu, will install later today if there's no > objection. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-07-05 18:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-22 6:04 [PATCH] libstdc++: testsuite: avoid predictable mkstemp Alexandre Oliva 2022-06-22 9:16 ` Jonathan Wakely 2022-06-23 11:39 ` Alexandre Oliva 2022-06-23 16:36 ` Jonathan Wakely 2022-06-27 9:31 ` Alexandre Oliva 2022-06-27 10:18 ` Jonathan Wakely 2022-07-05 9:10 ` Alexandre Oliva 2022-07-05 9:16 ` Jonathan Wakely 2022-07-05 17:45 ` Alexandre Oliva 2022-07-05 18:02 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).