This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 3/3] Aarch64: Fix segfault when casting dummy calls
On 10/11/2018 03:49 PM, Alan Hayward wrote:
> Prevent the following causing a segfault on aarch64:
> (gdb) b foo if (int)strcmp(name,"abc") == 0
> (gdb) run
>
> This is because to aarch64_push_dummy_call determines the return type
> of the function and then does not check for null pointer.
>
> A null pointer for the return type means the call has no debug
> information. For the code to get here, then either there has been an
> error or the call has been cast - but we do not have this information
> available.
That "to get there, then either" is confusing, because assuming
"there" is the place that causes a crash, if there was an error,
we wouldn't ever get there.
I suggest this instead:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A null pointer for the return type means the call has no debug
information. For the code to get here, then the call must have
been cast, otherwise we'd error out sooner. In the case of a
no-debug-info call cast, the return type is the type the user
had cast the call to, but we do not have information
available here.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> In addition, aarch64_push_dummy_call does not check if the function is
> an IFUNC.
>
> However, aarch64_push_dummy_call only requires the return value in order
s/return value/return type/
> to calculate lang_struct_return. This information is available in the
> return_method enum. The fix is to simply use this instead.
>
> Add common test case using a shared library to ensure FUNC resolving.
What does "common" mean here?
And what does "to ensure FUNC resolving" mean too, btw?
AFAICT, the only reason to use a shared library is to
compile it with or without debug information, right?
Come to think of it, you could instead eliminate the
shared library and compile a separate .o file instead, right?
That would simplify the testcase a bit and expose it to more
targets.
>
> gdb/ChangeLog:
>
> 2018-10-11 Alan Hayward <alan.hayward@arm.com>
>
> PR gdb/22736:
> * aarch64-tdep.c (aarch64_push_dummy_call): Remove
> lang_struct_return code.
Indenting seems off here. Make sure to indent the
like starting with lang_struct_return with a tab.
>
> gdb/testsuite/ChangeLog:
>
> 2018-10-11 Alan Hayward <alan.hayward@arm.com>
>
> PR gdb/22736:
> * gdb.base/condbreak-solib-lib.cc: New test.
> * gdb.base/condbreak-solib-main.cc: New test.
> * gdb.base/condbreak-solib.exp: New file.
> ---
> gdb/aarch64-tdep.c | 30 +---
> gdb/testsuite/gdb.base/condbreak-solib-lib.cc | 22 +++
> .../gdb.base/condbreak-solib-main.cc | 39 +++++
> gdb/testsuite/gdb.base/condbreak-solib.exp | 136 ++++++++++++++++++
> 4 files changed, 200 insertions(+), 27 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/condbreak-solib-lib.cc
> create mode 100644 gdb/testsuite/gdb.base/condbreak-solib-main.cc
> create mode 100644 gdb/testsuite/gdb.base/condbreak-solib.exp
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 63771dc21d..e541e45f61 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1519,9 +1519,6 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> {
> int argnum;
> struct aarch64_call_info info;
> - struct type *func_type;
> - struct type *return_type;
> - int lang_struct_return;
>
> memset (&info, 0, sizeof (info));
>
> @@ -1543,35 +1540,14 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> If the language code decides to pass in memory we want to move
> the pointer inserted as the initial argument from the argument
> list and into X8, the conventional AArch64 struct return pointer
> - register.
> -
> - This is slightly awkward, ideally the flag "lang_struct_return"
> - would be passed to the targets implementation of push_dummy_call.
> - Rather that change the target interface we call the language code
> - directly ourselves. */
> -
> - func_type = check_typedef (value_type (function));
> -
> - /* Dereference function pointer types. */
> - if (TYPE_CODE (func_type) == TYPE_CODE_PTR)
> - func_type = TYPE_TARGET_TYPE (func_type);
> -
> - gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC
> - || TYPE_CODE (func_type) == TYPE_CODE_METHOD);
> -
> - /* If language_pass_by_reference () returned true we will have been
> - given an additional initial argument, a hidden pointer to the
> - return slot in memory. */
> - return_type = TYPE_TARGET_TYPE (func_type);
> - lang_struct_return = language_pass_by_reference (return_type);
> + register. */
>
> /* Set the return address. For the AArch64, the return breakpoint
> is always at BP_ADDR. */
> regcache_cooked_write_unsigned (regcache, AARCH64_LR_REGNUM, bp_addr);
>
> - /* If we were given an initial argument for the return slot because
> - lang_struct_return was true, lose it. */
> - if (lang_struct_return)
> + /* If we were given an initial argument for the return slot, lose it. */
> + if (return_method == return_method_hidden_param)
> {
> args++;
> nargs--;
> diff --git a/gdb/testsuite/gdb.base/condbreak-solib-lib.cc b/gdb/testsuite/gdb.base/condbreak-solib-lib.cc
> new file mode 100644
> index 0000000000..96dfe2182e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/condbreak-solib-lib.cc
> @@ -0,0 +1,22 @@
> +/* 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/>. */
> +
> +int
> +cmp3 (char *name)
As mentioned in the previous review, use "const char *" here?
> +{
> + return name[0] == '3';
> +}
> diff --git a/gdb/testsuite/gdb.base/condbreak-solib-main.cc b/gdb/testsuite/gdb.base/condbreak-solib-main.cc
> new file mode 100644
> index 0000000000..10f2e4d474
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/condbreak-solib-main.cc
> @@ -0,0 +1,39 @@
> +/* 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/>. */
> +
> +extern int cmp3 (char *name);
And here.
> +
> +const char *word = "stuff";
> +
> +int
> +foo ()
And please do write the void, like:
foo (void)
(void) and () are not the same in C.
> +{
> + return 1;
> +}
> +
> +int
> +bar ()
Ditto.
> +{
> + return 1;
> +}
> +
> +int
> +main (void)
> +{
> + foo ();
> + bar ();
> +}
> diff --git a/gdb/testsuite/gdb.base/condbreak-solib.exp b/gdb/testsuite/gdb.base/condbreak-solib.exp
> new file mode 100644
> index 0000000000..38169ff81a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/condbreak-solib.exp
> @@ -0,0 +1,136 @@
> +# 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 conditional breakpoints on functions in shared libraries.
Please expand this to mention no debug info.
> +# See gdb/22736
> +
> +if { [skip_cplus_tests] || [skip_shlib_tests] } {
> + return 0
> +}
> +
> +global target_size
> +set target_size TARGET_UNKNOWN
> +if {[is_lp64_target]} {
> + set target_size TARGET_LP64
> +} elseif {[is_ilp32_target]} {
> + set target_size TARGET_ILP32
> +} else {
> + return 0
> +}
Is this above actually used? I mean, the target_size variable?
> +
> +set main_basename condbreak-solib-main
> +set lib_basename condbreak-solib-lib
> +standard_testfile $main_basename.cc $lib_basename.cc
> +
> +if [get_compiler_info "c++"] {
> + return -1
> +}
> +
> +set libsrc "${srcdir}/${subdir}/${srcfile2}"
> +
> +
> +# Run to main. Set breakpoint at bar. Set conditional breakpoint CBREAK.
> +# Continue running. Test that program stops at function STOP, prefixed
> +# with the text PREFIX.
Double space after periods, please. Same for other comments below.
Missing "the": "Test that the program".
> +
> +proc break_and_run { cbreak stop prefix} {
Space after "prefix" to match the space before "cbreak".
Or the other way around, as long as consistent.
> +
> + if ![runto_main] then {
> + fail "can't run to main"
> + return
> + }
This should be within the with_test_prefix block too.
> +
> + with_test_prefix $cbreak {
> + gdb_test "b bar" "Breakpoint .*"
> + gdb_test "b $cbreak" "Breakpoint .*"
> + gdb_test "c" ".*${prefix}.*Breakpoint .* $stop .*"
> + }
> +}
> +
> +# Compile binary linked against shared library containing cmp3, using TYPE
> +# to build the library as either DEBUG or NODEBUG. Then test conditional
> +# breakpoints that make a call into the library.
> +
> +proc cond_break_test { type } {
> +
> + global srcdir subdir srcfile srcfile2 binfile testfile
> + global target_size main_basename lib_basename libsrc
> +
> + switch -regexp -- $type {
> + "^debug" {
> + set dir "debug"
> + set debug_flags "debug"
> + }
> + "^nodebug" {
> + set dir "nodebug"
> + set debug_flags ""
> + }
> + }
> +
> + remote_exec build "rm -rf [standard_output_file ${dir}]"
> + remote_exec build "mkdir [standard_output_file ${dir}]"
> +
> + set lib_so [standard_output_file ${dir}/${lib_basename}.so]
> + set lib_syms [shlib_symbol_file ${dir}/${lib_so}]
> + set binfile [standard_output_file ${dir}/${testfile}]
> +
> + # Compile a shared library containing cmp3.
> +
> + set lib_flags "c++ ldflags=-Wl,-Bsymbolic $debug_flags"
> +
> + if {[gdb_compile_shlib $libsrc $lib_so ${lib_flags}] != ""} {
> + untested "failed to compile shared library"
> + return -1
> + }
> +
> + # Compile the main test linked against the shared library.
> +
> + set bin_flags "debug shlib=${lib_so}"
> +
> + if { [gdb_compile $srcdir/$subdir/${srcfile} ${binfile} executable ${bin_flags}] != "" } {
> + untested "failed to compile"
> + return -1
> + }
> +
> + clean_restart $binfile
The untested calls, and the clean_restart should be under the
with_test_prefix too. But see further below.
> +
> + with_test_prefix $type {
> +
> + # Conditional break with casted function call that fails the condition.
> + break_and_run "foo if (int)cmp3(\"abc\") == 1" "bar" "Continuing.\r\n\r\n"
> +
> + # Conditional break with casted function call that passes the condition.
> + break_and_run "foo if (int)cmp3(\"345\") == 1" "foo" "Continuing.\r\n\r\n"
> +
> + # Conditional break with function call (no cast).
> + switch -regexp -- $type {
> + "^debug" {
> + #Same results as conditional break with casted function.
> + break_and_run "foo if cmp3(\"abc\") == 1" "bar" "Continuing.\r\n\r\n"
> + break_and_run "foo if cmp3(\"345\") == 1" "foo" "Continuing.\r\n\r\n"
Odd indentation above.
> + }
> + "^nodebug" {
> + #Call will error due to lack of debug information, causing the condition to pass.
> + break_and_run "foo if cmp3(\"abc\") == 1" "foo" "Continuing.\r\nError in testing breakpoint condition"
> + break_and_run "foo if cmp3(\"345\") == 1" "foo" "Continuing.\r\nError in testing breakpoint condition"
> + }
> + }
> + }
> +}
> +
> +cond_break_test debug
> +cond_break_test nodebug
Instead of with_test_prefix, you can write instead here:
foreach_with_prefix type {debug nodebug} {
cond_break_test $type
}
Thanks,
Pedro Alves