[PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Lancelot SIX]

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(-)

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;
 }

base-commit: 90f8d97c8efa75f7f019b868eca9c626bc35203d
-- 
2.34.1

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Tom Tromey]

>>>>> "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

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Lancelot SIX]

> 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.

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Tom de Vries]

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

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Lancelot SIX]

> 
> 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.

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Tom de Vries]

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

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Andrew Pinski]

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
>
>

Re: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup

[Lancelot SIX]

>> 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.