Re: [PATCH 11/11] gdb/testsuite: disable gdb.cp/call-method-register.exp with clang

[Bruno Larsen <blarsen@redhat.com>]

On 27/10/2022 11:49, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> The test gdb.cp/call-method-register.exp assumes that the class will be
>> placed on a register. However, this keyword has been deprecated since
>> C++11, and clang does not feel the need to follow it. Since this test is
>> not usable without this working, this commit marks this test as
>> untested.
> As I understand it, the combination of register and asm, as used in the
> test source is a GCC extension, and so...
>
>> ---
>>   gdb/testsuite/gdb.cp/call-method-register.exp | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/gdb/testsuite/gdb.cp/call-method-register.exp b/gdb/testsuite/gdb.cp/call-method-register.exp
>> index a1e6498d66c..71d1f61f59f 100644
>> --- a/gdb/testsuite/gdb.cp/call-method-register.exp
>> +++ b/gdb/testsuite/gdb.cp/call-method-register.exp
>> @@ -26,6 +26,11 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
>>       return -1
>>   }
>>   
>> +if {[test_compiler_info clang-*-*]} {
>> +    untested "clang does not place the class in the register"
>> +    return
>> +}
> ... I think this would be better written as:
>
>    if {![test_compiler_info gcc-*-* c++]} {
>      untested "test relies on a gcc extension"
>      return
>    }
>
> However, I also noticed that this test is only currently supported for
> x86-64, i386, and ppc, as the test source itself needs a particular
> register to be selected for each architecture.
>
> I wondered if we could do better using the DWARF assembler.  Below is my
> proposal that would replace this patch.  What do you think?
I did think about this, but it felt like it wasn't worth it, especially 
given that we can't use the dwarf assembler with clang. That said, I 
just tried your patch and it works just fine, so it is a pretty good 
improvement. I only have a very minor inlined.
>
> Thanks,
> Andrew
>
> ---
>
> commit 613e0a042c3220f183e02c9c3cf76c68b4d7e453
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Thu Oct 27 10:15:09 2022 +0100
>
>      gdb/testsuite: port gdb.cp/call-method-register.exp to DWARF assembler
>      
>      The test gdb.cp/call-method-register.exp relies on a GCC extension,
>      that is combining the register keyword with the asm keyword to place a
>      variable into a known register.
>      
>      This test already suffers from requiring each architecture to pick a
>      register to use, currently the test only supports x86-64, i386, and
>      ppc64.  Plus we know that the test will only ever work with GCC.
>      
>      In this commit I add a guard to the call-method-register.exp script so
>      if the C++ compiler is not g++ then the test will be skipped.
>      
>      However, the main change in this commit is that I have added a
>      complete new test gdb.dwarf2/dw2-call-method-register.exp, this is a
>      copy of the original test but rewritten to use the DWARF assembler.
>      I've tested the new test on x86-64, aarch64, and ppc64le.
>      
>      I did consider removing the original test, however, I thought there
>      might be some value in retaining it, just so we can have some
>      validation that GDB can correctly handle GCC's register extension, the
>      test is pretty short, so doesn't take much time to run.
>
> diff --git a/gdb/testsuite/gdb.cp/call-method-register.exp b/gdb/testsuite/gdb.cp/call-method-register.exp
> index a1e6498d66c..2662d6b0891 100644
> --- a/gdb/testsuite/gdb.cp/call-method-register.exp
> +++ b/gdb/testsuite/gdb.cp/call-method-register.exp
> @@ -18,6 +18,19 @@
>   
>   if { [skip_cplus_tests] } { continue }
>   
> +# This test relies on a GCC extension to place a structure into a
> +# named register.  As a result this test will only work with GCC.  But
> +# also, only a few architectures have a register named.  Any
> +# architecture not explicitly supported in the source file will fail
> +# to compile.
> +#
> +# However, the test gdb.dwarf2/dw2-call-method-register.exp is a
> +# reimplementation of this test using the DWARF assembler, and should
> +# work for any architecture, with any compiler (that supports the
> +# DWARF assembler).  This test is retained mostly so we can track that
> +# nothing weird happens w.r.t. how GDB handles GCC's register extension.
> +if { ![test_compiler_info {gcc-*-*} c++] } { continue }
> +
>   load_lib "cp-support.exp"
>   
>   standard_testfile .cc
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-call-method-register.c b/gdb/testsuite/gdb.dwarf2/dw2-call-method-register.c
> new file mode 100644
> index 00000000000..d5328156c55
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-call-method-register.c
> @@ -0,0 +1,24 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int
> +main ()
> +{
> +  asm ("main_label: .globl main_label");
> +  return 0;
> +}
> +
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-call-method-register.exp b/gdb/testsuite/gdb.dwarf2/dw2-call-method-register.exp
> new file mode 100644
> index 00000000000..3b761698076
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-call-method-register.exp
> @@ -0,0 +1,127 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test callling a method on a variable that has been put in a register.
> +#
> +# We use the DWARF assembler to generate DWARF that says the global variable
> +# is located in a register.  We don't care which register we use as the
> +# value in the register is not important, we only care that the DWARF says
> +# the variable is in a register.  In fact, there is no variable in the
> +# source program at all!
> +
> +load_lib dwarf.exp
> +
> +# Check the DWARF assembler is supported.
> +if {![dwarf2_support]} { return }
> +
> +standard_testfile .c -dw.S
> +
> +# Compile and start the .c file so we can figure out pointer sizes.
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +
> +    # Get information about function main.
> +    set main_result \
> +	[function_range main ${::srcdir}/${::subdir}/${::srcfile}]
> +    set main_start [lindex $main_result 0]
> +    set main_length [lindex $main_result 1]
> +
> +    cu {} {
> +	compile_unit {
> +	    {DW_AT_language @DW_LANG_C_plus_plus}
> +	    {DW_AT_name $::srcfile}
> +	    {DW_AT_comp_dir /tmp}
> +	} {
> +	    declare_labels int_type_label struct_type_label \
> +		struct_ptr_type_label
> +            set ptr_size [get_sizeof "void *" 96]
> +
> +	    DW_TAG_subprogram {
> +		{name main}
> +		{low_pc $main_start addr}
> +		{high_pc $main_length data8}
> +		{DW_AT_type :$int_type_label}
> +	    }
> +
> +	    int_type_label: DW_TAG_base_type {
> +		{DW_AT_byte_size 4 DW_FORM_sdata}
> +		{DW_AT_encoding @DW_ATE_signed}
> +		{DW_AT_name int}
> +	    }
> +
> +	    struct_type_label: DW_TAG_structure_type {
> +		{DW_AT_byte_size 4 DW_FORM_sdata}
> +		{DW_AT_name small}
> +	    } {
> +		member {
> +		    {name xxx}
> +		    {type :$int_type_label}
> +		    {data_member_location 0 data1}
> +		}
> +		subprogram {
> +		    {name yyy}
> +		    {linkage_name _ZN5small3yyyEv}
Why did you decide to give the method a linkage name? It doesn't seem 
related to the error message and I think we should only leave important 
things in the tests.

Cheers,
Bruno

> +		    {external 1 flag_present}
> +		    {type :$int_type_label}
> +		    {data_member_location 0 data1}
> +		} {
> +		    formal_parameter {
> +			{type :$struct_ptr_type_label}
> +			{artificial 1 flag_present}
> +		    }
> +		}
> +	    }
> +
> +	    struct_ptr_type_label: DW_TAG_pointer_type {
> +		{DW_AT_byte_size $ptr_size DW_FORM_data1}
> +		{type :$struct_type_label}
> +	    }
> +
> +	    # This is where we place the variable into a register.  Just
> +	    # use DWARF register 0, whatever that might be.  See the
> +	    # comments at the start of the file for why we don't care
> +	    # about the choice of register.
> +            DW_TAG_variable {
> +                {DW_AT_name global_var}
> +                {DW_AT_type :$struct_type_label}
> +                {DW_AT_location {
> +                    DW_OP_reg0
> +                } SPECIAL_expr}
> +                {external 1 flag}
> +            }
> +	}
> +    }
> +}
> +
> +# Rebuild the test program, this time include the generated debug
> +# information.
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	  [list $srcfile $asm_file] {nodebug}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Try to call a method on a variable of structure type, however, the
> +# variable is located in a register.
> +gdb_test "print global_var.yyy ()" \
> +    "Address requested for identifier \"global_var\" which is in register .*" \
> +    "call method on register local"
>