Re: [PATCH 1/1] gdb, testsuite: Handle unused compiler option fdiagnostics-color=never.

[Keith Seitz <keiths@redhat.com>]

Hi,

Thanks for the patch! More clang compatibility is always good.

On 3/22/24 06:55, Abdul Basit Ijaz wrote:
> From: "Ijaz, Abdul B" <abdul.b.ijaz@intel.com>
> 
> The 'univeral_compile_options' in gdb.exp file only verifies the support
> of '-fdiagnostics-color=never' for the "C" source file.  So while running
> tests with assembly source file (.s), many of them are not able to run
> on icx/clang compilers because '-fdiagnostics-color=never' option is not
> supported.  After this change, this function is split into multiple
> functions to check the support for different type of sources individually.

I like this approach.

> Before this change, in the case of clang and ICX compiler, this error is
> shown for assembly source files:
> 
> '''
> icx -fdiagnostics-color=never -Wno-unknown-warning-option -fno-pie -c -O0 -o
> amd64-entry-value0.o gdb/testsuite/gdb.arch/amd64-entry-value.s (timeout = 300)
> 
> icx: warning: argument unused during compilation: '-fdiagnostics-color=never'
> [-Wunused-command-line-argument]
> 
> gdb compile failed, icx: warning: argument unused during compilation:
> '-fdiagnostics-color=never' [-Wunused-command-line-argument]
> 
> UNTESTED: gdb.arch/amd64-entry-value.exp: failed to prepare
> '''
> 
> Similarly this error is shown for the clang compiler:
> 
> '''
> clang  -fdiagnostics-color=never -Wno-unknown-warning-option -fno-pie -c -O0
> -o amd64-entry-value0.o gdb/testsuite/gdb.arch/amd64-entry-value.s
> 
> clang: warning: argument unused during compilation:
>   '-fdiagnostics-color=never' [-Wunused-command-line-argument]
> '''
> 
> 2024-03-22 Ijaz, Abdul B <abdul.b.ijaz@intel.com>_

Was this (apparently) stray line the start of a ChangeLog entry? [We
don't require those anymore.]

> ---
>   gdb/testsuite/lib/gdb.exp | 48 +++++++++++++++++++++++++++++++--------
>   1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index a0c4855ffc5..4f19162b3ce 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5018,17 +5018,9 @@ proc gdb_wrapper_init { args } {
>   }
>   
>   # Determine options that we always want to pass to the compiler.
> -gdb_caching_proc universal_compile_options {} {
> -    set me "universal_compile_options"
> +proc universal_compile_options {src obj} {
>       set options {}
>   
> -    set src [standard_temp_file ccopts.c]
> -    set obj [standard_temp_file ccopts.o]
> -
> -    gdb_produce_source $src {
> -	int foo(void) { return 0; }
> -    }
> -
>       # Try an option for disabling colored diagnostics.  Some compilers
>       # yield colored diagnostics by default (when run from a tty) unless
>       # such an option is specified.
> @@ -5038,6 +5030,23 @@ gdb_caching_proc universal_compile_options {} {
>   	# Seems to have worked; use the option.
>   	lappend options $opt
>       }
> +
> +    return $options
> +}
> +
> +# Determine options that we always want to pass to the C compiler.
> +gdb_caching_proc universal_compile_options_c {} {
> +    set me "universal_compile_options_c"
> +
> +    set src [standard_temp_file ccopts.c]
> +    set obj [standard_temp_file ccopts.o]
> +
> +    gdb_produce_source $src {
> +	int foo(void) { int arr[100]={0}; return 0; }

This patch changes this from "int foo(void) { return 0; }".
I don't see that documented why that was necessary?

> +    }
> +
> +    set options [universal_compile_options $src $obj]
> +
>       file delete $src
>       file delete $obj
>   
> @@ -5045,6 +5054,21 @@ gdb_caching_proc universal_compile_options {} {
>       return $options
>   }
>   
> +# Determine options that we always want to pass to the compiler for
> +# assembly source files with the extension ".s".
> +gdb_caching_proc universal_compile_options_assembly {} {
> +    set me "universal_compile_options_assembly"
> +
> +    set obj [standard_temp_file csymbol.o]
> +    set src "$::srcdir/gdb.arch/amd64-entry-value.s"

I don't think this is going to work anywhere except on x86.
In fact, I tried this on aarch64, and (with below issue
fixed), the log contains a bunch of errors (with "-v -v"
verbosity to DejaGNU):

Executing on host: clang    -fdiagnostics-color=never -c -o 
/root/test-fsf-master/gdb/build-aarch64-redhat-linux-gnu/gdb/testsuite/temp/69953/csymbol.o 
/root/test-fsf-master/gdb/build-aarch64-redhat-linux-gnu/gdb/testsuite/../../../gdb/testsuite/gdb.arch/amd64-entry-value.s 
    (timeout = 300)
builtin_spawn -ignore SIGHUP clang -fdiagnostics-color=never -c -o 
/root/test-fsf-master/gdb/build-aarch64-redhat-linux-gnu/gdb/testsuite/temp/69953/csymbol.o 
/root/test-fsf-master/gdb/build-aarch64-redhat-linux-gnu/gdb/testsuite/../../../gdb/testsuite/gdb.arch/amd64-entry-value.s
pid is 70084 -70084
clang: warning: argument unused during compilation: 
'-fdiagnostics-color=never'
[-Wunused-command-line-argument]
/root/test-fsf-master/gdb/build-aarch64-redhat-linux-gnu/gdb/testsuite/../../../gdb/testsuite/gdb.arch/amd64-entry-value.s:37:16: 
error: unexpected token in argument list
         movl    $0, _ZL1v(%rip)
                          ^
/root/test-fsf-master/gdb/build-aarch64-redhat-linux-gnu/gdb/testsuite/../../../gdb/testsuite/gdb.arch/amd64-entry-value.s:57:12: 
error: unexpected token in argument list
         addsd   .LC0(%rip), %xmm0
                     ^
[8000+ lines deleted]

[I note that the output contained a bunch of terminal escape codes
(which I've deleted for clarity).]

I think something more along the lines of using gdb_produce_source is
needed, where we attempt to output really generic asm (if possible?).
I wonder if we could do something along the lines of

    gdb_produce_source $src {
            .text
            .globl main
            .type main, @function
        main:
             nop
    }

Warning: untested. This does compile with both gcc and clang on aarch64
and x86_64 (with -z noexecstack), but that's all I've attempted to
verify.

Failing that, there is a relatively simple fallback for checking
for clang specifically: "test_compiler_info {clang-*}".
That might be safer than trying to write some generic asm code?

> +
> +    set options [universal_compile_options $src $obj]
> +    file delete $obj
> +
> +    verbose "$me:  returning $options" 2
> +    return $options

I really appreciate the verbose statements. Helped me understand
several of the questions I have!

> +}
> +
>   # Compile the code in $code to a file based on $name, using the flags
>   # $compile_flag as well as debug, nowarning and quiet  (unless otherwise
>   # specified in default_compile_flags).
> @@ -5224,7 +5248,11 @@ proc gdb_compile {source dest type options} {
>       if {[lsearch -exact $options rust] != -1} {
>   	# -fdiagnostics-color is not a rustcc option.
>       } else {
> -	set new_options [universal_compile_options]
> +	if {[string match *.s $source] != 0} {

I don't think this is sufficient. A bunch of test cases use ".S",
such as every aarch64 test using assembler. Perhaps add
"-nocase"?

> +	    set new_options [universal_compile_options_assembly]
> +	} else {
> +	    set new_options [universal_compile_options_c]
> +	}
>       }
>   
>       # C/C++ specific settings.

Thanks again for the patch!

Keith