[1/2] 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 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(-)
  

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
>
  

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]
  

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.
  

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
  

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
  

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
>