public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Cleanup testsuite temporary files
@ 2022-10-02 14:43 Andrew Burgess
  2022-10-02 14:43 ` [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-02 14:43 UTC (permalink / raw)
  To: gdb-patches

I spotted that we have a couple of temporary files being written to
the build/gdb/testsuite/ directory, not to the per test script output
directory.

The following two patches fix the issues I spotted.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb/testsuite: avoid creating files in gdb/testsuite directory
  gdb/testsuite: avoid temporary file in gdb/testsuite

 gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp | 3 ++-
 gdb/testsuite/gdb.gdb/unittest.exp               | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.25.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-02 14:43 [PATCH 0/2] Cleanup testsuite temporary files Andrew Burgess
@ 2022-10-02 14:43 ` Andrew Burgess
  2022-10-03 11:12   ` Lancelot SIX
  2022-10-02 14:43 ` [PATCH 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite Andrew Burgess
  2022-10-04 14:20 ` [PATCHv2 0/2] Cleanup testsuite temporary files Andrew Burgess
  2 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-10-02 14:43 UTC (permalink / raw)
  To: gdb-patches

I spotted that the test gdb.dwarf2/dw2-using-debug-str.exp was
creating an output file (called debug_str_section) in the root
build/gdb/testsuite directory instead of using the
build/gdb/testsuite/output/gdb.dwarf2/dw2-using-debug-str/ directory.

This is a result of not using standard_output_file in the test
script.

With this commit the file is now placed in the expected output
directory.  The test still passes for me.
---
 gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
index d27554f2f89..729961c99b5 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
@@ -105,7 +105,8 @@ gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
 # cc-with-dwz-m.exp and cc-with-gnu-debuglink.exp.  Handle this by
 # skipping the remainder of the test-case.
 set debug_str_section "${binfile}-debug-str"
-set args "--dump-section .debug_str=debug_str_section $binfile"
+set debug_str_file [standard_output_file "debug_str_section"]
+set args "--dump-section .debug_str=${debug_str_file} $binfile"
 set result [remote_exec host "[gdb_find_objcopy] $args"]
 set status [lindex $result 0]
 set output [lindex $result 1]
-- 
2.25.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite
  2022-10-02 14:43 [PATCH 0/2] Cleanup testsuite temporary files Andrew Burgess
  2022-10-02 14:43 ` [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
@ 2022-10-02 14:43 ` Andrew Burgess
  2022-10-04 14:20 ` [PATCHv2 0/2] Cleanup testsuite temporary files Andrew Burgess
  2 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-02 14:43 UTC (permalink / raw)
  To: gdb-patches

I spotted that the gdb.gdb/unittest.exp script causes a temporary file
inserters_extractors-2.txt to be created in build/gdb/testsuite/
instead of in build/gdb/testsuite/output/gdb.gdb/unittest/.

This is because some of the 'maint selftest' tests create temporary
files in GDB's current directory, specifically, the two source files:

  gdb/unittests/basic_string_view/inserters/wchar_t/2.cc
  gdb/unittests/basic_string_view/inserters/char/2.cc

both create a temporary file called inserters_extractors-2.txt, though
we only run the second of these as part of GDB's selftests.

I propose that in the gdb.gdb/unittest.exp we change GDB's current
working directory to be the output directory for the specific test,
that way the temporary file will be created in the correct place.

After this change all the tests continue to pass, and the temporary
file is no longer left in gdb/testsuite/.
---
 gdb/testsuite/gdb.gdb/unittest.exp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/testsuite/gdb.gdb/unittest.exp b/gdb/testsuite/gdb.gdb/unittest.exp
index 2967b994cc3..a60cd70a259 100644
--- a/gdb/testsuite/gdb.gdb/unittest.exp
+++ b/gdb/testsuite/gdb.gdb/unittest.exp
@@ -40,6 +40,14 @@ proc run_selftests { binfile } {
 	clean_restart ${binfile}
     }
 
+    # Some of the selftests create temporary files in GDB's current
+    # directory.  So, while running the selftests, switch to the
+    # test's output directory to avoid leaving clutter in the
+    # gdb/testsuite root directory.
+    set dir [standard_output_file ""]
+    gdb_test "cd $dir" "Working directory $dir\\." \
+	"switch to test output directory"
+
     set enabled 1
     set test "maintenance selftest"
     gdb_test_multiple $test $test {
-- 
2.25.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-02 14:43 ` [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
@ 2022-10-03 11:12   ` Lancelot SIX
  2022-10-03 16:06     ` Andrew Burgess
  0 siblings, 1 reply; 14+ messages in thread
From: Lancelot SIX @ 2022-10-03 11:12 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

Hi Andrew,

On Sun, Oct 02, 2022 at 03:43:56PM +0100, Andrew Burgess via Gdb-patches wrote:
> I spotted that the test gdb.dwarf2/dw2-using-debug-str.exp was
> creating an output file (called debug_str_section) in the root
> build/gdb/testsuite directory instead of using the
> build/gdb/testsuite/output/gdb.dwarf2/dw2-using-debug-str/ directory.
> 
> This is a result of not using standard_output_file in the test
> script.
> 
> With this commit the file is now placed in the expected output
> directory.  The test still passes for me.
> ---
>  gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
> index d27554f2f89..729961c99b5 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
> @@ -105,7 +105,8 @@ gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
>  # cc-with-dwz-m.exp and cc-with-gnu-debuglink.exp.  Handle this by
>  # skipping the remainder of the test-case.
>  set debug_str_section "${binfile}-debug-str"
> -set args "--dump-section .debug_str=debug_str_section $binfile"
> +set debug_str_file [standard_output_file "debug_str_section"]
> +set args "--dump-section .debug_str=${debug_str_file} $binfile"

Just above your change, there is the following line:

  set debug_str_section "${binfile}-debug-str"

I believe that the original intent was to use this as output file name,
but the '$' was use in the set args line.

It looks to me that the change should be:

  -set args "--dump-section .debug_str=debug_str_section $binfile"
  +set args "--dump-section .debug_str=$debug_str_section $binfile"

If you prefer your change, the `set debug_str_section` line should be
removed.

Best,
Lancelot.
>  set result [remote_exec host "[gdb_find_objcopy] $args"]
>  set status [lindex $result 0]
>  set output [lindex $result 1]
> -- 
> 2.25.4
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-03 11:12   ` Lancelot SIX
@ 2022-10-03 16:06     ` Andrew Burgess
  2022-10-03 18:51       ` Pedro Alves
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-10-03 16:06 UTC (permalink / raw)
  To: Lancelot SIX; +Cc: gdb-patches

Lancelot SIX <lsix@lancelotsix.com> writes:

> Hi Andrew,
>
> On Sun, Oct 02, 2022 at 03:43:56PM +0100, Andrew Burgess via Gdb-patches wrote:
>> I spotted that the test gdb.dwarf2/dw2-using-debug-str.exp was
>> creating an output file (called debug_str_section) in the root
>> build/gdb/testsuite directory instead of using the
>> build/gdb/testsuite/output/gdb.dwarf2/dw2-using-debug-str/ directory.
>> 
>> This is a result of not using standard_output_file in the test
>> script.
>> 
>> With this commit the file is now placed in the expected output
>> directory.  The test still passes for me.
>> ---
>>  gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
>> index d27554f2f89..729961c99b5 100644
>> --- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
>> +++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
>> @@ -105,7 +105,8 @@ gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
>>  # cc-with-dwz-m.exp and cc-with-gnu-debuglink.exp.  Handle this by
>>  # skipping the remainder of the test-case.
>>  set debug_str_section "${binfile}-debug-str"
>> -set args "--dump-section .debug_str=debug_str_section $binfile"
>> +set debug_str_file [standard_output_file "debug_str_section"]
>> +set args "--dump-section .debug_str=${debug_str_file} $binfile"
>
> Just above your change, there is the following line:
>
>   set debug_str_section "${binfile}-debug-str"
>
> I believe that the original intent was to use this as output file name,
> but the '$' was use in the set args line.
>
> It looks to me that the change should be:
>
>   -set args "--dump-section .debug_str=debug_str_section $binfile"
>   +set args "--dump-section .debug_str=$debug_str_section $binfile"
>
> If you prefer your change, the `set debug_str_section` line should be
> removed.

Good spot.

Updated patch below.

Thanks,
Andrew

---

commit 8e855f184d30f17a7bd0638f6cb8ee211be789e2
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sun Oct 2 15:18:15 2022 +0100

    gdb/testsuite: avoid creating files in gdb/testsuite directory
    
    I spotted that the test gdb.dwarf2/dw2-using-debug-str.exp was
    creating an output file called debug_str_section in the root
    build/gdb/testsuite directory instead of using the
    build/gdb/testsuite/output/gdb.dwarf2/dw2-using-debug-str/ directory.
    
    This appears to be caused by a missing '$' character.  We setup a
    variable debug_str_section which contains a path within the output
    directory, but then when we build the objcopy command we use
    'debug_str_section' without a '$' prefix, as a result, we create the
    debug_str_section file.
    
    This commit adds the missing '$', the file is now created in the
    output directory.

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
index d27554f2f89..4d1c49044d5 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
@@ -105,7 +105,7 @@ gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
 # cc-with-dwz-m.exp and cc-with-gnu-debuglink.exp.  Handle this by
 # skipping the remainder of the test-case.
 set debug_str_section "${binfile}-debug-str"
-set args "--dump-section .debug_str=debug_str_section $binfile"
+set args "--dump-section .debug_str=${debug_str_section} $binfile"
 set result [remote_exec host "[gdb_find_objcopy] $args"]
 set status [lindex $result 0]
 set output [lindex $result 1]


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-03 16:06     ` Andrew Burgess
@ 2022-10-03 18:51       ` Pedro Alves
  2022-10-04  8:55         ` Andrew Burgess
  2022-10-04  9:08         ` Andrew Burgess
  0 siblings, 2 replies; 14+ messages in thread
From: Pedro Alves @ 2022-10-03 18:51 UTC (permalink / raw)
  To: Andrew Burgess, Lancelot SIX; +Cc: gdb-patches

On 2022-10-03 5:06 p.m., Andrew Burgess via Gdb-patches wrote:
> Lancelot SIX <lsix@lancelotsix.com> writes:
> 

>> Just above your change, there is the following line:
>>
>>   set debug_str_section "${binfile}-debug-str"
>>
>> I believe that the original intent was to use this as output file name,
>> but the '$' was use in the set args line.
>>
>> It looks to me that the change should be:
>>
>>   -set args "--dump-section .debug_str=debug_str_section $binfile"
>>   +set args "--dump-section .debug_str=$debug_str_section $binfile"
>>
>> If you prefer your change, the `set debug_str_section` line should be
>> removed.
> 
> Good spot.
> 
> Updated patch below.

I'm glad you guys found this alternative approach.  I was going to suggest
to see if we could avoid changing directory, the "cd" approach IMO should be
avoided if possible.  The reason is that when you change gdb's directory to
the test's output dir, if GDB crashes and produces a core on teardown, then that core will
end up in the test's output directory, and thus won't be noticed by the spurious core
detection, i.e., won't be signaled in gdb.sum.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-03 18:51       ` Pedro Alves
@ 2022-10-04  8:55         ` Andrew Burgess
  2022-10-04  9:08         ` Andrew Burgess
  1 sibling, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-04  8:55 UTC (permalink / raw)
  To: Pedro Alves, Lancelot SIX; +Cc: gdb-patches

Pedro Alves <pedro@palves.net> writes:

> On 2022-10-03 5:06 p.m., Andrew Burgess via Gdb-patches wrote:
>> Lancelot SIX <lsix@lancelotsix.com> writes:
>> 
>
>>> Just above your change, there is the following line:
>>>
>>>   set debug_str_section "${binfile}-debug-str"
>>>
>>> I believe that the original intent was to use this as output file name,
>>> but the '$' was use in the set args line.
>>>
>>> It looks to me that the change should be:
>>>
>>>   -set args "--dump-section .debug_str=debug_str_section $binfile"
>>>   +set args "--dump-section .debug_str=$debug_str_section $binfile"
>>>
>>> If you prefer your change, the `set debug_str_section` line should be
>>> removed.
>> 
>> Good spot.
>> 
>> Updated patch below.
>
> I'm glad you guys found this alternative approach.  I was going to suggest
> to see if we could avoid changing directory, the "cd" approach IMO should be
> avoided if possible.  The reason is that when you change gdb's directory to
> the test's output dir, if GDB crashes and produces a core on teardown, then that core will
> end up in the test's output directory, and thus won't be noticed by the spurious core
> detection, i.e., won't be signaled in gdb.sum.

Unfortunately, there were two temporary file issues, the first of these
never used 'cd', and that's the one Lancelot commented on, and I've
updated.

The second patch, which is still on the table right now, uses 'cd'.

However, I'll take your comment as feedback on patch 2/2 and see if I
can come up with a better fix.

Thanks,
Andrew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-03 18:51       ` Pedro Alves
  2022-10-04  8:55         ` Andrew Burgess
@ 2022-10-04  9:08         ` Andrew Burgess
  2022-10-04 12:15           ` Pedro Alves
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-10-04  9:08 UTC (permalink / raw)
  To: Pedro Alves, Lancelot SIX; +Cc: gdb-patches

Pedro Alves <pedro@palves.net> writes:

> On 2022-10-03 5:06 p.m., Andrew Burgess via Gdb-patches wrote:
>> Lancelot SIX <lsix@lancelotsix.com> writes:
>> 
>
>>> Just above your change, there is the following line:
>>>
>>>   set debug_str_section "${binfile}-debug-str"
>>>
>>> I believe that the original intent was to use this as output file name,
>>> but the '$' was use in the set args line.
>>>
>>> It looks to me that the change should be:
>>>
>>>   -set args "--dump-section .debug_str=debug_str_section $binfile"
>>>   +set args "--dump-section .debug_str=$debug_str_section $binfile"
>>>
>>> If you prefer your change, the `set debug_str_section` line should be
>>> removed.
>> 
>> Good spot.
>> 
>> Updated patch below.
>
> I'm glad you guys found this alternative approach.  I was going to suggest
> to see if we could avoid changing directory, the "cd" approach IMO should be
> avoided if possible.  The reason is that when you change gdb's directory to
> the test's output dir, if GDB crashes and produces a core on teardown, then that core will
> end up in the test's output directory, and thus won't be noticed by the spurious core
> detection, i.e., won't be signaled in gdb.sum.

What if I added a mechanism to lib/gdb.exp that allowed for something
like:

  with_change_gdb_directory $some_directory {
    # A set of tests here...
  }

and had the with_change_gdb_directory proc check that GDB was still
running at the end of the block.

This way, when the test script ends, and GDB is shutdown, we will always
be back in the original directory, so a crash on teardown will be
spotted (via the coredump).

And if GDB crashes during the inner block, then yes, the coredump will
be in the "wrong" place, but we should be guaranteed to see a test
failure.

Would something like this be acceptable?

My other idea is to have 'maint selftest' take an extra argument like:

  (gdb) maint selftest --temp-directory /path/to/directory

which would then be used by the individual tests when creating temporary
files.

Thoughts?

Thanks,
Andrew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-04  9:08         ` Andrew Burgess
@ 2022-10-04 12:15           ` Pedro Alves
  0 siblings, 0 replies; 14+ messages in thread
From: Pedro Alves @ 2022-10-04 12:15 UTC (permalink / raw)
  To: Andrew Burgess, Lancelot SIX; +Cc: gdb-patches

On 2022-10-04 10:08 a.m., Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:

>> I'm glad you guys found this alternative approach.  I was going to suggest
>> to see if we could avoid changing directory, the "cd" approach IMO should be
>> avoided if possible.  The reason is that when you change gdb's directory to
>> the test's output dir, if GDB crashes and produces a core on teardown, then that core will
>> end up in the test's output directory, and thus won't be noticed by the spurious core
>> detection, i.e., won't be signaled in gdb.sum.
> 
> What if I added a mechanism to lib/gdb.exp that allowed for something
> like:
> 
>   with_change_gdb_directory $some_directory {
>     # A set of tests here...
>   }
> 
> and had the with_change_gdb_directory proc check that GDB was still
> running at the end of the block.
> 
> This way, when the test script ends, and GDB is shutdown, we will always
> be back in the original directory, so a crash on teardown will be
> spotted (via the coredump).
> 
> And if GDB crashes during the inner block, then yes, the coredump will
> be in the "wrong" place, but we should be guaranteed to see a test
> failure.
> 
> Would something like this be acceptable?

Yes, I think that is sufficient in practice.  I'm happy with that.

Thanks,
Pedro Alves

> 
> My other idea is to have 'maint selftest' take an extra argument like:
> 
>   (gdb) maint selftest --temp-directory /path/to/directory
> 
> which would then be used by the individual tests when creating temporary
> files.
> 
> Thoughts?
> 
> Thanks,
> Andrew
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCHv2 0/2] Cleanup testsuite temporary files
  2022-10-02 14:43 [PATCH 0/2] Cleanup testsuite temporary files Andrew Burgess
  2022-10-02 14:43 ` [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
  2022-10-02 14:43 ` [PATCH 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite Andrew Burgess
@ 2022-10-04 14:20 ` Andrew Burgess
  2022-10-04 14:20   ` [PATCHv2 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
                     ` (2 more replies)
  2 siblings, 3 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-04 14:20 UTC (permalink / raw)
  To: gdb-patches

I spotted that we have a couple of temporary files being written to
the build/gdb/testsuite/ directory, not to the per test script output
directory.

The following two patches fix the issues I spotted.

Changes since v1:

  - Patch #1 updated inline with Lancelot's suggestion,

  - Patch #2 updated inline with Pedro's suggestion.

---

Andrew Burgess (2):
  gdb/testsuite: avoid creating files in gdb/testsuite directory
  gdb/testsuite: avoid temporary file in gdb/testsuite (unittest.exp)

 .../gdb.dwarf2/dw2-using-debug-str.exp        |   2 +-
 gdb/testsuite/gdb.gdb/unittest.exp            |  46 +++++---
 gdb/testsuite/lib/gdb.exp                     | 110 ++++++++++++++++++
 3 files changed, 138 insertions(+), 20 deletions(-)

-- 
2.25.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCHv2 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory
  2022-10-04 14:20 ` [PATCHv2 0/2] Cleanup testsuite temporary files Andrew Burgess
@ 2022-10-04 14:20   ` Andrew Burgess
  2022-10-04 14:20   ` [PATCHv2 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite (unittest.exp) Andrew Burgess
  2022-10-14 16:28   ` [PATCHv2 0/2] Cleanup testsuite temporary files Tom Tromey
  2 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-04 14:20 UTC (permalink / raw)
  To: gdb-patches

I spotted that the test gdb.dwarf2/dw2-using-debug-str.exp was
creating an output file called debug_str_section in the root
build/gdb/testsuite directory instead of using the
build/gdb/testsuite/output/gdb.dwarf2/dw2-using-debug-str/ directory.

This appears to be caused by a missing '$' character.  We setup a
variable debug_str_section which contains a path within the output
directory, but then when we build the objcopy command we use
'debug_str_section' without a '$' prefix, as a result, we create the
debug_str_section file.

This commit adds the missing '$', the file is now created in the
output directory.
---
 gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
index d27554f2f89..4d1c49044d5 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
@@ -105,7 +105,7 @@ gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
 # cc-with-dwz-m.exp and cc-with-gnu-debuglink.exp.  Handle this by
 # skipping the remainder of the test-case.
 set debug_str_section "${binfile}-debug-str"
-set args "--dump-section .debug_str=debug_str_section $binfile"
+set args "--dump-section .debug_str=${debug_str_section} $binfile"
 set result [remote_exec host "[gdb_find_objcopy] $args"]
 set status [lindex $result 0]
 set output [lindex $result 1]
-- 
2.25.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCHv2 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite (unittest.exp)
  2022-10-04 14:20 ` [PATCHv2 0/2] Cleanup testsuite temporary files Andrew Burgess
  2022-10-04 14:20   ` [PATCHv2 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
@ 2022-10-04 14:20   ` Andrew Burgess
  2022-10-14 16:28   ` [PATCHv2 0/2] Cleanup testsuite temporary files Tom Tromey
  2 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-04 14:20 UTC (permalink / raw)
  To: gdb-patches

I spotted that the gdb.gdb/unittest.exp script causes a temporary file
inserters_extractors-2.txt to be created in build/gdb/testsuite/
instead of in build/gdb/testsuite/output/gdb.gdb/unittest/.

This is because some of the 'maint selftest' tests create temporary
files in GDB's current directory, specifically, the two source files:

  gdb/unittests/basic_string_view/inserters/wchar_t/2.cc
  gdb/unittests/basic_string_view/inserters/char/2.cc

both create a temporary file called inserters_extractors-2.txt, though
we only run the second of these as part of GDB's selftests.

I initially proposed just using GDB's 'cd' command in unittest.exp to
switch to the test output directory before running the selftests,
however, Pedro pointed out that there was a risk here that, if GDB
crashed during shutdown, the generated core file would be left in the
test output directory rather than in the testsuite directory.  As a
result, our clever core file spotting logic would fail to spot the
core file and alert the user.

Instead, I propose this slightly more involved solution.  I've added a
new with_gdb_cwd directory proc, used like this:

  with_gdb_cwd $directory {
    # Tests here...
  }

The new proc temporarily switches to $directory and then runs the
tests within the block.  After running the tests the previous current
working directory is restored.

Additionally, after switching back to the previous cwd, we check that
GDB is still responsive.  This means that if GDB crashed immediately
prior to restoring the previous directory, and left the core file in
the wrong place, then the responsiveness check will fail, and a FAIL
will be emitted, this should be enough to alert the user that
something has gone wrong.

With this commit in place the unittest.exp script now leaves its
temporary file in the test output directory.
---
 gdb/testsuite/gdb.gdb/unittest.exp |  46 +++++++-----
 gdb/testsuite/lib/gdb.exp          | 110 +++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 19 deletions(-)

diff --git a/gdb/testsuite/gdb.gdb/unittest.exp b/gdb/testsuite/gdb.gdb/unittest.exp
index 2967b994cc3..fdae7a24632 100644
--- a/gdb/testsuite/gdb.gdb/unittest.exp
+++ b/gdb/testsuite/gdb.gdb/unittest.exp
@@ -40,26 +40,34 @@ proc run_selftests { binfile } {
 	clean_restart ${binfile}
     }
 
+    # Some of the selftests create temporary files in GDB's current
+    # directory.  So, while running the selftests, switch to the
+    # test's output directory to avoid leaving clutter in the
+    # gdb/testsuite root directory.
+    set dir [standard_output_file ""]
     set enabled 1
-    set test "maintenance selftest"
-    gdb_test_multiple $test $test {
-	-re ".*Running selftest \[^\n\r\]+\." {
-	    # The selftests can take some time to complete.  To prevent
-	    # timeout spot the 'Running ...' lines going past, so long as
-	    # these are produced quickly enough then the overall test will
-	    # not timeout.
-	    exp_continue
-	}
-	-re "Ran ($decimal) unit tests, ($decimal) failed\r\n$gdb_prompt $" {
-	    set num_ran $expect_out(1,string)
-	    set num_failed $expect_out(2,string)
-	    gdb_assert "$num_ran > 0" "$test, ran some tests"
-	    gdb_assert "$num_failed == 0" "$test, failed none"
-	}
-	-re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
-	    unsupported $test
-	    set num_ran 0
-	    set enabled 0
+    set num_ran 0
+    with_gdb_cwd $dir {
+	set test "maintenance selftest"
+	gdb_test_multiple $test $test {
+	    -re ".*Running selftest \[^\n\r\]+\." {
+		# The selftests can take some time to complete.  To prevent
+		# timeout spot the 'Running ...' lines going past, so long as
+		# these are produced quickly enough then the overall test will
+		# not timeout.
+		exp_continue
+	    }
+	    -re "Ran ($decimal) unit tests, ($decimal) failed\r\n$gdb_prompt $" {
+		set num_ran $expect_out(1,string)
+		set num_failed $expect_out(2,string)
+		gdb_assert "$num_ran > 0" "$test, ran some tests"
+		gdb_assert "$num_failed == 0" "$test, failed none"
+	    }
+	    -re "Selftests have been disabled for this build.\r\n$gdb_prompt $" {
+		unsupported $test
+		set num_ran 0
+		set enabled 0
+	    }
 	}
     }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 432ed5e34ca..5a2b176afec 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2761,6 +2761,116 @@ proc with_cwd { dir body } {
     }
 }
 
+# Use GDB's 'cd' command to switch to DIR.  Return true if the switch
+# was successful, otherwise, call perror and return false.
+
+proc gdb_cd { dir } {
+    set new_dir ""
+    gdb_test_multiple "cd $dir" "" {
+	-re "^cd \[^\r\n\]+\r\n" {
+	    exp_continue
+	}
+
+	-re "^Working directory (\[^\r\n\]+)\\.\r\n" {
+	    set new_dir $expect_out(1,string)
+	    exp_continue
+	}
+
+	-re "^$::gdb_prompt $" {
+	    if { $new_dir == "" || $new_dir != $dir } {
+		perror "failed to switch to $dir"
+		return false
+	    }
+	}
+    }
+
+    return true
+}
+
+# Use GDB's 'pwd' command to figure out the current working directory.
+# Return the directory as a string.  If we can't figure out the
+# current working directory, then call perror, and return the empty
+# string.
+
+proc gdb_pwd { } {
+    set dir ""
+    gdb_test_multiple "pwd" "" {
+	-re "^pwd\r\n" {
+	    exp_continue
+	}
+
+	-re "^Working directory (\[^\r\n\]+)\\.\r\n" {
+	    set dir $expect_out(1,string)
+	    exp_continue
+	}
+
+	-re "^$::gdb_prompt $" {
+	}
+    }
+
+    if { $dir == "" } {
+	perror "failed to read GDB's current working directory"
+    }
+
+    return $dir
+}
+
+# Similar to the with_cwd proc, this proc runs BODY with the current
+# working directory changed to CWD.
+#
+# Unlike with_cwd, the directory change here is done within GDB
+# itself, so GDB must be running before this proc is called.
+
+proc with_gdb_cwd { dir body } {
+    set saved_dir [gdb_pwd]
+    if { $saved_dir == "" } {
+	return
+    }
+
+    verbose -log "Switching to directory $dir (saved CWD: $saved_dir)."
+    if ![gdb_cd $dir] {
+	return
+    }
+
+    set code [catch {uplevel 1 $body} result]
+
+    verbose -log "Switching back to $saved_dir."
+    if ![gdb_cd $saved_dir] {
+	return
+    }
+
+    # Check that GDB is still alive.  If GDB crashed in the above code
+    # then any corefile will have been left in DIR, not the root
+    # testsuite directory.  As a result the corefile will not be
+    # brought to the users attention.  Instead, if GDB crashed, then
+    # this check should cause a FAIL, which should be enough to alert
+    # the user.
+    set saw_result false
+    gdb_test_multiple "p 123" "" {
+	-re "p 123\r\n" {
+	    exp_continue
+	}
+
+	-re "^\\\$$::decimal = 123\r\n" {
+	    set saw_result true
+	    exp_continue
+	}
+
+	-re "^$::gdb_prompt $" {
+	    if { !$saw_result } {
+		fail "check gdb is alive in with_gdb_cwd"
+	    }
+	}
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+	return -code $code $result
+    }
+}
+
 # Run tests in BODY with GDB prompt and variable $gdb_prompt set to
 # PROMPT.  When BODY is finished, restore GDB prompt and variable
 # $gdb_prompt.
-- 
2.25.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 0/2] Cleanup testsuite temporary files
  2022-10-04 14:20 ` [PATCHv2 0/2] Cleanup testsuite temporary files Andrew Burgess
  2022-10-04 14:20   ` [PATCHv2 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
  2022-10-04 14:20   ` [PATCHv2 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite (unittest.exp) Andrew Burgess
@ 2022-10-14 16:28   ` Tom Tromey
  2022-10-19 11:20     ` Andrew Burgess
  2 siblings, 1 reply; 14+ messages in thread
From: Tom Tromey @ 2022-10-14 16:28 UTC (permalink / raw)
  To: Andrew Burgess via Gdb-patches

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> I spotted that we have a couple of temporary files being written to
Andrew> the build/gdb/testsuite/ directory, not to the per test script output
Andrew> directory.

These both look good to me, thank you.

Tom

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 0/2] Cleanup testsuite temporary files
  2022-10-14 16:28   ` [PATCHv2 0/2] Cleanup testsuite temporary files Tom Tromey
@ 2022-10-19 11:20     ` Andrew Burgess
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-19 11:20 UTC (permalink / raw)
  To: Tom Tromey, Andrew Burgess via Gdb-patches

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> I spotted that we have a couple of temporary files being written to
> Andrew> the build/gdb/testsuite/ directory, not to the per test script output
> Andrew> directory.
>
> These both look good to me, thank you.

Thanks, I've pushed these.

Andrew


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-10-19 11:20 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-02 14:43 [PATCH 0/2] Cleanup testsuite temporary files Andrew Burgess
2022-10-02 14:43 ` [PATCH 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
2022-10-03 11:12   ` Lancelot SIX
2022-10-03 16:06     ` Andrew Burgess
2022-10-03 18:51       ` Pedro Alves
2022-10-04  8:55         ` Andrew Burgess
2022-10-04  9:08         ` Andrew Burgess
2022-10-04 12:15           ` Pedro Alves
2022-10-02 14:43 ` [PATCH 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite Andrew Burgess
2022-10-04 14:20 ` [PATCHv2 0/2] Cleanup testsuite temporary files Andrew Burgess
2022-10-04 14:20   ` [PATCHv2 1/2] gdb/testsuite: avoid creating files in gdb/testsuite directory Andrew Burgess
2022-10-04 14:20   ` [PATCHv2 2/2] gdb/testsuite: avoid temporary file in gdb/testsuite (unittest.exp) Andrew Burgess
2022-10-14 16:28   ` [PATCHv2 0/2] Cleanup testsuite temporary files Tom Tromey
2022-10-19 11:20     ` Andrew Burgess

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