diff mbox series

gdb/compile: Use std::filesystem::remove_all in cleanup

Message ID 20240303164756.870514-1-lancelot.six@amd.com
State New
Headers
Series gdb/compile: Use std::filesystem::remove_all in cleanup |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Lancelot SIX March 3, 2024, 4:47 p.m. UTC 
  In a previous review, I noticed that some code in gdb/compile/compile.c
could use c++17's `std::filesystem::remove_all` instead of using some
`system ("rm -rf ...");`.

This patch implements this.

Note that I use the noexcept overload of std::filesystem::remove_all and
explicitly check for an error code.  This means that this code called
during the cleanup procedure cannot throw, and does not risk preventing
other cleanup functions to be called.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31420
Change-Id: If5668bf3e15e66c020e5c3b4fa999f861690e4cf
---
 gdb/compile/compile.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)


base-commit: 90f8d97c8efa75f7f019b868eca9c626bc35203d

Comments

Tom Tromey March 8, 2024, 4:27 p.m. UTC | #1 
>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes:

Lancelot> In a previous review, I noticed that some code in gdb/compile/compile.c
Lancelot> could use c++17's `std::filesystem::remove_all` instead of using some
Lancelot> `system ("rm -rf ...");`.

Lancelot> This patch implements this.

Lancelot> Note that I use the noexcept overload of std::filesystem::remove_all and
Lancelot> explicitly check for an error code.  This means that this code called
Lancelot> during the cleanup procedure cannot throw, and does not risk preventing
Lancelot> other cleanup functions to be called.

Lancelot> Tested on x86_64-linux.

Lancelot> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31420
Lancelot> Change-Id: If5668bf3e15e66c020e5c3b4fa999f861690e4cf

Thank you.  I think this is ok.

There might be some risk that there's a compiler that implements C++17
but not std::filesystem.  If this happens I suppose we can either
consider simply rejecting that compiler, or alternatively, revert this
patch.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Lancelot SIX April 3, 2024, 12:56 p.m. UTC | #2 
> Thank you.  I think this is ok.
> 
> There might be some risk that there's a compiler that implements C++17
> but not std::filesystem.  If this happens I suppose we can either
> consider simply rejecting that compiler, or alternatively, revert this
> patch.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

I have just pushed this patch.  If it turns out that this causes any 
issue for some configuration, I'll revert the patch.

Best,
Lancelot.
Tom de Vries April 3, 2024, 2:37 p.m. UTC | #3 
On 3/8/24 17:27, Tom Tromey wrote:
>>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes:
> 
> Lancelot> In a previous review, I noticed that some code in gdb/compile/compile.c
> Lancelot> could use c++17's `std::filesystem::remove_all` instead of using some
> Lancelot> `system ("rm -rf ...");`.
> 
> Lancelot> This patch implements this.
> 
> Lancelot> Note that I use the noexcept overload of std::filesystem::remove_all and
> Lancelot> explicitly check for an error code.  This means that this code called
> Lancelot> during the cleanup procedure cannot throw, and does not risk preventing
> Lancelot> other cleanup functions to be called.
> 
> Lancelot> Tested on x86_64-linux.
> 
> Lancelot> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31420
> Lancelot> Change-Id: If5668bf3e15e66c020e5c3b4fa999f861690e4cf
> 
> Thank you.  I think this is ok.
> 
> There might be some risk that there's a compiler that implements C++17
> but not std::filesystem.  If this happens I suppose we can either
> consider simply rejecting that compiler, or alternatively, revert this
> patch.

This broke the build for me with gcc 7.5.0, which apparently falls into 
the category of "mostly implements C++17 but not std::filesystem".

As we can see here ( https://gcc.gnu.org/gcc-8/changes.html#cxx ) 
support was added in gcc 8.

Thanks,
- Tom
Lancelot SIX April 3, 2024, 2:47 p.m. UTC | #4 
> 
> This broke the build for me with gcc 7.5.0, which apparently falls into
> the category of "mostly implements C++17 but not std::filesystem".
> 

Sorry about that, I have sent 
https://sourceware.org/pipermail/gdb-patches/2024-April/207760.html to 
revert that change.  At least, we know it is best to stay away from 
std::filesystem for now.

Best,
Lancelot.
Tom de Vries April 3, 2024, 2:51 p.m. UTC | #5 
On 4/3/24 16:47, Lancelot SIX wrote:
>>
>> This broke the build for me with gcc 7.5.0, which apparently falls into
>> the category of "mostly implements C++17 but not std::filesystem".
>>
> 
> Sorry about that, I have sent 
> https://sourceware.org/pipermail/gdb-patches/2024-April/207760.html to 
> revert that change.  At least, we know it is best to stay away from 
> std::filesystem for now.
> 

I wonder if there is a case for conditionally enabling this.  That would 
ensure that the work is not lost, as well as used when using gcc >=8.

Thanks,
- Tom
Andrew Pinski April 3, 2024, 5:08 p.m. UTC | #6 
On Wed, Apr 3, 2024 at 7:51 AM Tom de Vries <tdevries@suse.de> wrote:
>
> On 4/3/24 16:47, Lancelot SIX wrote:
> >>
> >> This broke the build for me with gcc 7.5.0, which apparently falls into
> >> the category of "mostly implements C++17 but not std::filesystem".
> >>
> >
> > Sorry about that, I have sent
> > https://sourceware.org/pipermail/gdb-patches/2024-April/207760.html to
> > revert that change.  At least, we know it is best to stay away from
> > std::filesystem for now.
> >
>
> I wonder if there is a case for conditionally enabling this.  That would
> ensure that the work is not lost, as well as used when using gcc >=8.

Let me add even though support was added for GCC 8, the functions are
in included in libstdc++fs.a rather than the standard
libstdc++.so/libstdc++.a.
It was added to libstdc++ in GCC 9. So if you add this conditionally,
you either need to do a feature test that does a link too or have one
that adds libstdc++fs.a if not found in the standard library.

Thanks,
Andrew

>
> Thanks,
> - Tom
>
>
Lancelot SIX April 4, 2024, 7:40 a.m. UTC | #7 
>> I wonder if there is a case for conditionally enabling this.  That would
>> ensure that the work is not lost, as well as used when using gcc >=8.
> 
> Let me add even though support was added for GCC 8, the functions are
> in included in libstdc++fs.a rather than the standard
> libstdc++.so/libstdc++.a.
> It was added to libstdc++ in GCC 9. So if you add this conditionally,
> you either need to do a feature test that does a link too or have one
> that adds libstdc++fs.a if not found in the standard library.
> 
> Thanks,
> Andrew
> 
Thanks for the input.

We did discuss briefly using "-libstdc++fs" on IRC, thanks for the 
precision regarding where it is required. As gcc-7.5 seems to still be 
used to build gdb, it looks like we will stay away from std::filesystem 
for now.

Thanks,
Lancelot.
diff mbox series

Patch

diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 27cff2553ee..45452f19abb 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -40,7 +40,9 @@ 
 #include "osabi.h"
 #include "gdbsupport/gdb_wait.h"
 #include "valprint.h"
+#include <filesystem>
 #include <optional>
+#include <system_error>
 #include "gdbsupport/gdb_unlinker.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/scoped_ignore_signal.h"
@@ -450,15 +452,11 @@  get_compile_file_tempdir (void)
   tempdir_name = xstrdup (tempdir_name);
   add_final_cleanup ([] ()
     {
-      char *zap;
-      int wstat;
-
-      gdb_assert (startswith (tempdir_name, TMP_PREFIX));
-      zap = concat ("rm -rf ", tempdir_name, (char *) NULL);
-      wstat = system (zap);
-      if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0)
-	warning (_("Could not remove temporary directory %s"), tempdir_name);
-      XDELETEVEC (zap);
+      std::error_code error;
+      if (std::filesystem::remove_all (tempdir_name, error)
+	  == static_cast<std::uintmax_t> (-1))
+	warning (_("Could not remove temporary directory %s (%s)"),
+		 tempdir_name, error.message ().c_str ());
     });
   return tempdir_name;
 }