libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target

Message ID 1855720815.793351.1626790336484.JavaMail.zimbra@kalray.eu
State New
Headers show
Series
  • libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target
Related show

Commit Message

Ian Lance Taylor via Gcc-patches July 20, 2021, 2:12 p.m.
This fixes an incorrect invocation of gdb on remote targets where DejaGNU would try to run host's gdb in remote target simulator.
gdb-test skips the testing when target is remote or non native but the gdb version check function does not.

libstdc++-v3/ChangeLog:
        * testsuite/lib/gdb-test.exp (gdb_batch_check): Exit if non native or remote target.
commit 0c4ae4ff46b1d7633f1e06f57d348b5817b8f640
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jul 20 12:35:37 2021

    libstdc++: Add more tests for filesystem::create_directory [PR101510]
    
    Signed-off-by: Jonathan Wakely <jwakely@redhat.com>

    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/101510
            * src/c++17/fs_ops.cc (create_dir): Adjust whitespace.
            * testsuite/27_io/filesystem/operations/create_directory.cc:
            Test creating directory with name of existing symlink to
            directory.
            * testsuite/experimental/filesystem/operations/create_directory.cc:
            Likewise.

Comments

Ian Lance Taylor via Gcc-patches July 21, 2021, 3:02 p.m. | #1
With the correct patch attached, sorry for the incorrect previous one !

Marc

----- Original Message -----
> From: "gcc-patches" <gcc-patches@gcc.gnu.org>

> To: "gcc-patches" <gcc-patches@gcc.gnu.org>, "libstdc++" <libstdc++@gcc.gnu.org>

> Cc: "Luc Michel" <lmichel@kalray.eu>

> Sent: Tuesday, July 20, 2021 4:12:16 PM

> Subject: [NEWS]  libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target


> This fixes an incorrect invocation of gdb on remote targets where DejaGNU would

> try to run host's gdb in remote target simulator.

> gdb-test skips the testing when target is remote or non native but the gdb

> version check function does not.

> 

> libstdc++-v3/ChangeLog:

>        * testsuite/lib/gdb-test.exp (gdb_batch_check): Exit if non native or remote

>         target.
diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
index af20c85e5a0..0ec9ac46c68 100644
--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -244,6 +244,8 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
 
 # Invoke gdb with a command and pattern-match the output.
 proc gdb_batch_check {command pattern} {
+    if { ![isnative] || [is_remote target] } { return 0 }
+
     set gdb_name $::env(GUALITY_GDB_NAME)
     set cmd "$gdb_name -nw -nx -quiet -batch -ex \"$command\""
     send_log "Spawning: $cmd\n"
Ian Lance Taylor via Gcc-patches July 21, 2021, 3:53 p.m. | #2
On Wed, 21 Jul 2021 at 16:02, Marc Poulhies via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>

> With the correct patch attached, sorry for the incorrect previous one !


Thanks for the patch. I agree we should skip the version checks, not
only the actual tests. But I wonder whether we want to do that in
xmethods.exp and prettyprinters.exp rather than in the gdb_batch_check
proc. Or maybe like this instead:

--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -280,6 +280,8 @@ proc gdb_batch_check {command pattern} {
# but not earlier versions.
# Return 1 if the version is ok, 0 otherwise.
proc gdb_version_check {} {
+    if { ![isnative] || [is_remote target] } { return 0 }
+
    return [gdb_batch_check "python print(gdb.lookup_global_symbol)" \
             "<built-in function lookup_global_symbol>"]
}
@@ -288,6 +290,8 @@ proc gdb_version_check {} {
# in a manner similar to the check for a version of gdb which supports the
# pretty-printer tests below.
proc gdb_version_check_xmethods {} {
+    if { ![isnative] || [is_remote target] } { return 0 }
+
    return [gdb_batch_check \
             "python import gdb.xmethod; print(gdb.xmethod.XMethod)" \
             "<class 'gdb\\.xmethod\\.XMethod'>"]



I don't think it really makes much difference, I'm just unsure what is
"cleaner" and more consistent with DG conventions and/or the rest of
the gdb-test.exp file.
Ian Lance Taylor via Gcc-patches July 22, 2021, 9:19 a.m. | #3
----- Original Message -----
> From: "Jonathan Wakely" <jwakely@redhat.com>

> To: "Marc Poulhies" <mpoulhies@kalrayinc.com>

> Cc: "libstdc++" <libstdc++@gcc.gnu.org>, "gcc-patches" <gcc-patches@gcc.gnu.org>, "Luc Michel" <lmichel@kalray.eu>

> Sent: Wednesday, July 21, 2021 5:53:52 PM

> Subject: Re: [NEWS] libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target


> Thanks for the patch. I agree we should skip the version checks, not

> only the actual tests. But I wonder whether we want to do that in

> xmethods.exp and prettyprinters.exp rather than in the gdb_batch_check

> proc. Or maybe like this instead:

> 

> I don't think it really makes much difference, I'm just unsure what is

> "cleaner" and more consistent with DG conventions and/or the rest of

> the gdb-test.exp file.


Here are a bit more information on the issue we are having.
While trying to understand why the testsuite is taking a long time to execute, we (Luc in Cc) discovered that the logs contain:

Spawning: gdb -nw -nx -quiet -batch -ex "python print(gdb.lookup_global_symbol)"
spawn -ignore SIGHUP kvx-mppa --unnamed-log --bootcluster=node0 --no-trap --no-gdb-attach --dcache-no-check -- gdb -nw -nx -quiet -batch -ex python print(gdb.lookup_global_symbol)
UNSUPPORTED: prettyprinters.exp
testcase /work1/mpoulhies/tools-csw/gcc/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp completed in 613 seconds

kvx-mppa being a simulator and gdb the host (x86) binary.
The strangest part being that running the command by hand will fail immediately, but when DG is running the tests, it waits until the timeout is reached: we don't understand this behavior, but we get several <defunct> processes probably waiting to be joined..

I don't have a strong opinion here as I'm really no DG expert. I find the filtering in gdb-test maybe more robust as it prevents any error like the above. Having the test in prettyprinters/xmethod allows for quicker exit (saves 15s here), but I don't see that as a strong argument.

Thanks for the review,

Marc

Patch

diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index 66207ae5e44..cec76446f06 100644
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -577,8 +577,7 @@  namespace
   {
     bool created = false;
 #ifdef _GLIBCXX_HAVE_SYS_STAT_H
-    posix::mode_t mode
-      = static_cast<std::underlying_type_t<fs::perms>>(perm);
+    posix::mode_t mode = static_cast<std::underlying_type_t<fs::perms>>(perm);
     if (posix::mkdir(p.c_str(), mode))
       {
 	const int err = errno;
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
index a0e50471275..256621481d7 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
@@ -54,6 +54,33 @@  test01()
   b = create_directory(p);
   VERIFY( !b );
 
+  auto f = p/"file";
+  std::ofstream{f} << "create file";
+  b = create_directory(f, ec);
+  VERIFY( ec == std::errc::file_exists );
+  VERIFY( !b );
+  try
+  {
+    create_directory(f);
+    VERIFY( false );
+  }
+  catch (const fs::filesystem_error& e)
+  {
+    VERIFY( e.code() == std::errc::file_exists );
+    VERIFY( e.path1() == f );
+  }
+
+  // PR libstdc++/101510 create_directory on an existing symlink to a directory
+  fs::create_directory(p/"dir");
+  auto link = p/"link";
+  fs::create_directory_symlink("dir", link);
+  ec = bad_ec;
+  b = fs::create_directory(link, ec);
+  VERIFY( !b );
+  VERIFY( !ec );
+  b = fs::create_directory(link);
+  VERIFY( !b );
+
   remove_all(p, ec);
 }
 
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
index ee2a74b8803..39f95b61a45 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
@@ -46,12 +46,40 @@  test01()
   VERIFY( exists(p) );
 
   // Test existing path (libstdc++/71036).
+  ec = make_error_code(std::errc::invalid_argument);
   b = create_directory(p, ec);
   VERIFY( !ec );
   VERIFY( !b );
   b = create_directory(p);
   VERIFY( !b );
 
+  auto f = p/"file";
+  std::ofstream{f} << "create file";
+  b = create_directory(f, ec);
+  VERIFY( ec == std::errc::file_exists );
+  VERIFY( !b );
+  try
+  {
+    create_directory(f);
+    VERIFY( false );
+  }
+  catch (const fs::filesystem_error& e)
+  {
+    VERIFY( e.code() == std::errc::file_exists );
+    VERIFY( e.path1() == f );
+  }
+
+  // PR libstdc++/101510 create_directory on an existing symlink to a directory
+  fs::create_directory(p/"dir");
+  auto link = p/"link";
+  fs::create_directory_symlink("dir", link);
+  ec = make_error_code(std::errc::invalid_argument);
+  b = fs::create_directory(link, ec);
+  VERIFY( !b );
+  VERIFY( !ec );
+  b = fs::create_directory(link);
+  VERIFY( !b );
+
   remove_all(p, ec);
 }