[PATCH] gdb/testsuite: dont test dprintf to stderr in gdb.mi/mi-dprintf.exp

[PATCH] gdb/testsuite: dont test dprintf to stderr in gdb.mi/mi-dprintf.exp

[Bruno Larsen]

As mentioned in commit 3f5bbc3e2075ef5061a815c73fdc277218489f22, some
compilers such as clang don't add debug information about stderr by
default, leaving it to external debug packages. However, different to
that commit, there seems to be no simple way to test if stderr is present
without introducing a failure, so instead this commit just disables
tests that rely on stderr when clang is detected
---
 gdb/testsuite/gdb.mi/mi-dprintf.exp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-dprintf.exp b/gdb/testsuite/gdb.mi/mi-dprintf.exp
index e40fa6121fa..735f7fc234e 100644
--- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
+++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
@@ -127,7 +127,6 @@ proc mi_continue_dprintf {args} {
 mi_continue_dprintf "gdb"
 
 # The "call" style depends on having I/O functions available, so test.
-
 if ![target_info exists gdb,noinferiorio] {
 
     # Now switch styles and rerun; in the absence of redirection the
@@ -136,9 +135,12 @@ if ![target_info exists gdb,noinferiorio] {
     mi_gdb_test "set dprintf-style call" ".*" "mi set dprintf style to call"
     mi_continue_dprintf "call"
 
-    mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
-    mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
-    mi_continue_dprintf "fprintf"
+    # Clang does not add information about stderr, so skip these tests if needed.
+    if ![test_compiler_info clang*] {
+	mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
+	mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
+	mi_continue_dprintf "fprintf"
+    }
 }
 
 set target_can_dprintf 0
-- 
2.41.0

Re: [PATCH] gdb/testsuite: dont test dprintf to stderr in gdb.mi/mi-dprintf.exp

[Andrew Burgess]

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> As mentioned in commit 3f5bbc3e2075ef5061a815c73fdc277218489f22, some
> compilers such as clang don't add debug information about stderr by
> default, leaving it to external debug packages. However, different to
> that commit, there seems to be no simple way to test if stderr is present
> without introducing a failure,

I don't understand what you mean here.  In the previous commit, you just
print stderr and check GDB's reply.  Why no adopt the same approach for
this test?

I revised this test to use this proc:

  # Check for stderr.
  proc mi_gdb_is_stderr_available {} {
      set has_stderr_symbol false
      gdb_test_multiple "-data-evaluate-expression stderr" "stderr symbol check" {
  	-re "\\^error,msg=\"'stderr' has unknown type; cast it to its declared type\"\r\n$::mi_gdb_prompt$" {
  	    # Default value of false is fine.
  	}
  	-re "$::mi_gdb_prompt$" {
  	    set has_stderr_symbol true
  	}
      }
      return $has_stderr_symbol
  }
  
  set has_stderr_symbol [mi_gdb_is_stderr_available]

And then replaced your:

 if ![test_compiler_info clang*]

with:

 if {$::has_stderr_symbol}

And (when testing with Clang) I don't see any failures.

I think, in general, we should avoid as much as possible placing
specific compiler checks into test scripts unless we can make a _really_
strong argument that a special behaviour will _only_ happen for this one
compiler.

In this case, if a compiler check really was the _only_ way to solve
this problem we would still be better off placing the check into a new
proc in lib/*.exp somewhere -- but in this case I don't think that's
necessary, we should be able to check for stderr.

Also, I think the new stderr checking proc should be placed into
lib/mi-support.exp.

Thanks,
Andrew


>                                 so instead this commit just disables
> tests that rely on stderr when clang is detected


> ---
>  gdb/testsuite/gdb.mi/mi-dprintf.exp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.mi/mi-dprintf.exp b/gdb/testsuite/gdb.mi/mi-dprintf.exp
> index e40fa6121fa..735f7fc234e 100644
> --- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
> +++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
> @@ -127,7 +127,6 @@ proc mi_continue_dprintf {args} {
>  mi_continue_dprintf "gdb"
>  
>  # The "call" style depends on having I/O functions available, so test.
> -
>  if ![target_info exists gdb,noinferiorio] {
>  
>      # Now switch styles and rerun; in the absence of redirection the
> @@ -136,9 +135,12 @@ if ![target_info exists gdb,noinferiorio] {
>      mi_gdb_test "set dprintf-style call" ".*" "mi set dprintf style to call"
>      mi_continue_dprintf "call"
>  
> -    mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
> -    mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
> -    mi_continue_dprintf "fprintf"
> +    # Clang does not add information about stderr, so skip these tests if needed.
> +    if ![test_compiler_info clang*] {
> +	mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
> +	mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
> +	mi_continue_dprintf "fprintf"
> +    }
>  }
>  
>  set target_can_dprintf 0
> -- 
> 2.41.0

Re: [PATCH] gdb/testsuite: dont test dprintf to stderr in gdb.mi/mi-dprintf.exp

[Bruno Larsen]

On 21/07/2023 11:12, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> As mentioned in commit 3f5bbc3e2075ef5061a815c73fdc277218489f22, some
>> compilers such as clang don't add debug information about stderr by
>> default, leaving it to external debug packages. However, different to
>> that commit, there seems to be no simple way to test if stderr is present
>> without introducing a failure,
> I don't understand what you mean here.  In the previous commit, you just
> print stderr and check GDB's reply.  Why no adopt the same approach for
> this test?
>
> I revised this test to use this proc:
>
>    # Check for stderr.
>    proc mi_gdb_is_stderr_available {} {
>        set has_stderr_symbol false
>        gdb_test_multiple "-data-evaluate-expression stderr" "stderr symbol check" {
>    	-re "\\^error,msg=\"'stderr' has unknown type; cast it to its declared type\"\r\n$::mi_gdb_prompt$" {
>    	    # Default value of false is fine.
>    	}
>    	-re "$::mi_gdb_prompt$" {
>    	    set has_stderr_symbol true
>    	}
>        }
>        return $has_stderr_symbol
>    }
>    
>    set has_stderr_symbol [mi_gdb_is_stderr_available]
>
> And then replaced your:
>
>   if ![test_compiler_info clang*]
>
> with:
>
>   if {$::has_stderr_symbol}
>
> And (when testing with Clang) I don't see any failures.
>
> I think, in general, we should avoid as much as possible placing
> specific compiler checks into test scripts unless we can make a _really_
> strong argument that a special behaviour will _only_ happen for this one
> compiler.
>
> In this case, if a compiler check really was the _only_ way to solve
> this problem we would still be better off placing the check into a new
> proc in lib/*.exp somewhere -- but in this case I don't think that's
> necessary, we should be able to check for stderr.
>
> Also, I think the new stderr checking proc should be placed into
> lib/mi-support.exp.

Oh this makes sense. I looked into lib/mi-support.exp and couldn't see 
anything that used the gdb_test_multiple machinery, so I assumed it 
wouldn't work.

I agree with everything you said, it makes a lot of sense. I'll 
incorporate the changes and send a v2 soon, thanks for the review!

-- 
Cheers,
Bruno

>
> Thanks,
> Andrew
>
>
>>                                  so instead this commit just disables
>> tests that rely on stderr when clang is detected
>
>> ---
>>   gdb/testsuite/gdb.mi/mi-dprintf.exp | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.mi/mi-dprintf.exp b/gdb/testsuite/gdb.mi/mi-dprintf.exp
>> index e40fa6121fa..735f7fc234e 100644
>> --- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
>> +++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
>> @@ -127,7 +127,6 @@ proc mi_continue_dprintf {args} {
>>   mi_continue_dprintf "gdb"
>>   
>>   # The "call" style depends on having I/O functions available, so test.
>> -
>>   if ![target_info exists gdb,noinferiorio] {
>>   
>>       # Now switch styles and rerun; in the absence of redirection the
>> @@ -136,9 +135,12 @@ if ![target_info exists gdb,noinferiorio] {
>>       mi_gdb_test "set dprintf-style call" ".*" "mi set dprintf style to call"
>>       mi_continue_dprintf "call"
>>   
>> -    mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
>> -    mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
>> -    mi_continue_dprintf "fprintf"
>> +    # Clang does not add information about stderr, so skip these tests if needed.
>> +    if ![test_compiler_info clang*] {
>> +	mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
>> +	mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
>> +	mi_continue_dprintf "fprintf"
>> +    }
>>   }
>>   
>>   set target_can_dprintf 0
>> -- 
>> 2.41.0