Re: [PATCH v4 3/3] Aarch64: Fix segfault when casting dummy calls

[Pedro Alves <palves at redhat dot com>]

On 10/31/2018 11:18 AM, Alan Hayward wrote:

> +++ b/gdb/testsuite/gdb.base/infcall-across-obj.exp
> @@ -0,0 +1,134 @@
> +# This testcase is part of GDB, the GNU debugger.
> +# Copyright 2018 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 function calls on functions in another object.
> +# See gdb/22736 

Add missing period.

This comment misses the most important aspect of the testcase/bugfix,
which is to call a C++ function with no debug info.  Please extend the
comment to stress that.

The "in another object" thing is really a red herring; there's nothing
special about it, and there are other tests that cover that already.
Putting the called-function in a separate object is just so that the
main objfile is always compiled with debug info, so that GDB knows the
program is a C++ program.

(The only reason the infcall tests in gdb.base/nodebug.exp don't exercise
this is that that testcase is C.  That's why I had suggested
gdb.cp/nodebug-infcall.exp previously.  Anyway, naming not that important,
I guess.)

> +
> +if [target_info exists gdb,cannot_call_functions] {
> +    unsupported "this target can not call functions"
> +    continue
> +}
> +
> +# Only test c++ if we are able. Always use c

Double space after period.  Add missing period in second
sentence.

> +if { [skip_cplus_tests] || [get_compiler_info "c++"] } {
> +    set lang {c}
> +} else {
> +    set lang {c c++}
> +}
> +
> +set main_basename infcall-across-obj-main
> +set lib_basename infcall-across-obj-lib
> +standard_testfile ${main_basename}.c ${lib_basename}.c
> +
> +set mainsrc "${srcdir}/${subdir}/${srcfile}"
> +set libsrc "${srcdir}/${subdir}/${srcfile2}"
> +
> +# Build both source files to objects using language LANG. Use SYMBOLS to build
> +# the with either debug symbols or without - but always build the main file

"build the with" is missing a word?

> +# with debug. Then make function calls across the files.

Double space after periods please, twice.

> +
> +proc build_and_run_test { lang symbols } {
> +
> +    global main_basename lib_basename mainsrc libsrc binfile testfile
> +    global gdb_prompt
> +
> +    if { $symbols == "debug" } {
> +    	set debug_flags "debug"
> +    } else {
> +        set debug_flags ""
> +    }

Mind space vs tabs above.  Double check the rest of the patch too, please.

> +
> +    # Setup directory.
> +
> +    set dir "$lang-$symbols"
> +    remote_exec build "rm -rf [standard_output_file ${dir}]"
> +    remote_exec build "mkdir -p [standard_output_file ${dir}]"
> +
> +    # Compile both files to objects, then link together.
> +
> +    set main_flags "$lang debug"
> +    set lib_flags "$lang $debug_flags"
> +    set main_o [standard_output_file ${dir}/${main_basename}.o]
> +    set lib_o [standard_output_file ${dir}/${lib_basename}.o]
> +    set binfile [standard_output_file ${dir}/${testfile}]
> +
> +    if { [gdb_compile $mainsrc $main_o object ${main_flags}] != "" } {
> +        untested "failed to compile main file to object"
> +        return -1
> +    }
> +
> +    if { [gdb_compile $libsrc $lib_o object ${lib_flags}] != "" } {
> +        untested "failed to compile secondary file to object"
> +        return -1
> +    }
> +
> +    if { [gdb_compile "$main_o $lib_o" ${binfile} executable ""] != "" } {
> +        untested "failed to compile"
> +        return -1
> +    }
> +
> +    # Startup and run to main.
> +
> +    clean_restart $binfile
> +
> +    if ![runto_main] then {
> +        fail "can't run to main"
> +        return
> +    }
> +
> +    # Function call with cast.
> +
> +    set test "p (int)foo()"
> +    gdb_test_multiple $test $test {
> +        -re " = 1\r\n$gdb_prompt " {
> +	    pass $test
> +        }
> +	default { 
> +	    fail $test
> +	}
> +    }

No need for the "default" case.  gdb_test_multiple already
issues a FAIL in that case.  And if you remove that, a
plain gdb_test instead should work:

    gdb_test "p (int)foo()" " = 1"

Unless the missing $ anchor was on purpose?

> +
> +    # Function call without cast. Will error if there are no debug symbols.

Double-space after periods.

> +
> +    set test "p foo()"
> +    gdb_test_multiple $test $test {
> +        -re " = 1\r\n$gdb_prompt " {
> +	    if { $symbols == "debug" } {
> +	        pass $test
> +	    } else {
> +	    	fail $test
> +	    }

You can write

  gdb_assert { $symbols == "debug" }

instead of the if/else.

> +        }
> +        -re "has unknown return type; cast the call to its declared return type\r\n$gdb_prompt " {
> +	    if { $symbols != "debug" } {
> +	        pass $test
> +	    } else {
> +	    	fail $test
> +	    }

Ditto.

> +        }
> +	default { 
> +	    fail $test
> +	}

Drop the default case.

> +    }
> +
> +}
> +
> +foreach_with_prefix l $lang {
> +    foreach_with_prefix s {debug nodebug} {
> +        build_and_run_test $l $s
> +    }
> +}
> +
> 

OK with those fixed.  Please post what you end up pushing.

Thanks,
Pedro Alves