[v3,3/3] Aarch64: Fix segfault when casting dummy calls

  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.

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
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.

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.

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
  

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
  

> On 19 Oct 2018, at 12:35, Pedro Alves <palves@redhat.com> wrote:

> 


<snip>

>> 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?


By common I just meant not-aarch64-specific. I guess what I really meant
was any target that supported shared libraries.

> 

> 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.

> 


I could only get the bug to expose itself when using a .so

If I do the following using current HEAD then the bug is not present:

g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline
g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline
g++ condbreak-solib-main.o condbreak-solib-lib.o

It causes the type of the return value to be detected as
TYPE_CODE_PTR->TYPE_CODE_INT.

Or was there another method you were thinking?



"Ok" for all the other comments (including 1/3 and 2/3).

As a side note, many of the GNU style issues I kept hitting in the tests
are due to me relying too much on check_GNU_style.sh (from gcc) which
ignores any files in the testsuite directory. Tools are great, until
they aren’t :)


Alan.
  

On 10/23/2018 05:08 PM, Alan Hayward wrote:
> 
> 
>> On 19 Oct 2018, at 12:35, Pedro Alves <palves@redhat.com> wrote:
>>
> 
> <snip>
> 
>>> 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?
> 
> By common I just meant not-aarch64-specific. I guess what I really meant
> was any target that supported shared libraries.

I see.  (Suggest clarifying it in the commit log then.)

> 
>>
>> 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.
>>
> 
> I could only get the bug to expose itself when using a .so
> 
> If I do the following using current HEAD then the bug is not present:
> 
> g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline
> g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline
> g++ condbreak-solib-main.o condbreak-solib-lib.o
> 
> It causes the type of the return value to be detected as
> TYPE_CODE_PTR->TYPE_CODE_INT.

Huh.  That's really strange.  Where is that pointer coming from?

What does "ptype cmp3" say for you?

(gdb) b foo if (int)cmp3("abc") == 1
Breakpoint 1 at 0x40048b
(gdb) ptype cmp3
type = <unknown return type> ()
(gdb) whatis cmp3
type = <text variable, no debug info>


I wonder if what you're looking at is actually the malloc call?
GDB will call malloc to allocate space for the "abc" string in
the inferior.  Look at the backtrace, see where the call is coming
from.

Actually, because of that, I think it would be better (more focused)
to pass in a variable instead of "abc".  You already have the
unused variable "word" there:

const char *word = "stuff";

So:

 (gdb) b foo if (int)cmp3(word) == 1

but please rename it to something else, because I tried that
locally and there's another symbol called "word"
in glibc's localeinfo.h, and gdb picks that one up.

Also, is there a reason for the test to be a C++ program?
If there's none, it'd be better to make it a C program, again
to expose it to more targets.

Thanks,
Pedro Alves
  

> On 24 Oct 2018, at 16:14, Pedro Alves <palves@redhat.com> wrote:

> 

>> 

>>> 

>>> 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.

>>> 

>> 

>> I could only get the bug to expose itself when using a .so

>> 

>> If I do the following using current HEAD then the bug is not present:

>> 

>> g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline

>> g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline

>> g++ condbreak-solib-main.o condbreak-solib-lib.o

>> 

>> It causes the type of the return value to be detected as

>> TYPE_CODE_PTR->TYPE_CODE_INT.

> 

> Huh.  That's really strange.  Where is that pointer coming from?

> 

> What does "ptype cmp3" say for you?

> 

> (gdb) b foo if (int)cmp3("abc") == 1

> Breakpoint 1 at 0x40048b

> (gdb) ptype cmp3

> type = <unknown return type> ()

> (gdb) whatis cmp3

> type = <text variable, no debug info>

> 


Yes, I get the same.

Sounds to me like there might still be an issue in gdb? Or at least
a difference in the way the type is calculated.
This on it’s own is still a good fix, so I’ll send a new version.


> I wonder if what you're looking at is actually the malloc call?

> GDB will call malloc to allocate space for the "abc" string in

> the inferior.  Look at the backtrace, see where the call is coming

> from.



With the nodebug and debug shared library version:
(I need to run to main before I can set breakpoint on cmp3, otherwise
"Function "cmp3" not defined.”)

But with all versions, when stopped at cmp3, I get:
(gdb) bt
#0  0x00000000004005d4 in cmp3(char const*) ()
#1  <function called from gdb>
#2  0x00000000004005a4 in foo() ()
#3  0x00000000004005bc in main ()

> 

> Actually, because of that, I think it would be better (more focused)

> to pass in a variable instead of "abc".  You already have the

> unused variable "word" there:

> 

> const char *word = "stuff";

> 

> So:

> 

> (gdb) b foo if (int)cmp3(word) == 1

> 

> but please rename it to something else, because I tried that

> locally and there's another symbol called "word"

> in glibc's localeinfo.h, and gdb picks that one up.


Will do.
Using this still causes the bug in HEAD, so that’s ok.


> 

> Also, is there a reason for the test to be a C++ program?

> If there's none, it'd be better to make it a C program, again

> to expose it to more targets.



Switching to C causes the bug to vanish.
This is because C++ uses gnuv3_pass_by_reference(), and the
segfault happens inside there, whereas C uses
default_pass_by_reference(), which just returns 0.

I’ll expand the test to cover both C and C++.


Alan.
  

On 10/29/2018 11:58 AM, Alan Hayward wrote:
> 
> 
>> On 24 Oct 2018, at 16:14, Pedro Alves <palves@redhat.com> wrote:
>>
>>>
>>>>
>>>> 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.
>>>>
>>>
>>> I could only get the bug to expose itself when using a .so
>>>
>>> If I do the following using current HEAD then the bug is not present:
>>>
>>> g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline
>>> g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline
>>> g++ condbreak-solib-main.o condbreak-solib-lib.o
>>>
>>> It causes the type of the return value to be detected as
>>> TYPE_CODE_PTR->TYPE_CODE_INT.
>>
>> Huh.  That's really strange.  Where is that pointer coming from?
>>
>> What does "ptype cmp3" say for you?
>>
>> (gdb) b foo if (int)cmp3("abc") == 1
>> Breakpoint 1 at 0x40048b
>> (gdb) ptype cmp3
>> type = <unknown return type> ()
>> (gdb) whatis cmp3
>> type = <text variable, no debug info>
>>
> 
> Yes, I get the same.
> 
> Sounds to me like there might still be an issue in gdb? Or at least
> a difference in the way the type is calculated.
> This on it’s own is still a good fix, so I’ll send a new version.

I think we should learn the answer to the "where is that pointer
coming from" question.  I'm really not understanding why the
shared library makes a difference.

> 
> 
>> I wonder if what you're looking at is actually the malloc call?
>> GDB will call malloc to allocate space for the "abc" string in
>> the inferior.  Look at the backtrace, see where the call is coming
>> from.
> 
> 
> With the nodebug and debug shared library version:
> (I need to run to main before I can set breakpoint on cmp3, otherwise
> "Function "cmp3" not defined.”)
> 
> But with all versions, when stopped at cmp3, I get:
> (gdb) bt
> #0  0x00000000004005d4 in cmp3(char const*) ()
> #1  <function called from gdb>
> #2  0x00000000004005a4 in foo() ()
> #3  0x00000000004005bc in main ()


That's a backtrace in the inferior.  I meant instead for you to set
a breakpoint on gdb's aarch64_push_dummy_call, figure out where the
TYPE_CODE_PTR->TYPE_CODE_INT pointer comes from.  
With "b foo if (int)cmp3("abc") == 1", gdb will do two
infcalls, one malloc call to allocate space for "abc"
string, and then the call to cmp3.

Thanks,
Pedro Alves

> 
>>
>> Actually, because of that, I think it would be better (more focused)
>> to pass in a variable instead of "abc".  You already have the
>> unused variable "word" there:
>>
>> const char *word = "stuff";
>>
>> So:
>>
>> (gdb) b foo if (int)cmp3(word) == 1
>>
>> but please rename it to something else, because I tried that
>> locally and there's another symbol called "word"
>> in glibc's localeinfo.h, and gdb picks that one up.
> 
> Will do.
> Using this still causes the bug in HEAD, so that’s ok.
> 
> 
>>
>> Also, is there a reason for the test to be a C++ program?
>> If there's none, it'd be better to make it a C program, again
>> to expose it to more targets.
> 
> 
> Switching to C causes the bug to vanish.
> This is because C++ uses gnuv3_pass_by_reference(), and the
> segfault happens inside there, whereas C uses
> default_pass_by_reference(), which just returns 0.
> 
> I’ll expand the test to cover both C and C++.
  

> On 29 Oct 2018, at 12:38, Pedro Alves <palves@redhat.com> wrote:

> 

> On 10/29/2018 11:58 AM, Alan Hayward wrote:

>> 

>> 

>>> On 24 Oct 2018, at 16:14, Pedro Alves <palves@redhat.com> wrote:

>>> 

>>>> 

>>>>> 

>>>>> 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.

>>>>> 

>>>> 

>>>> I could only get the bug to expose itself when using a .so

>>>> 

>>>> If I do the following using current HEAD then the bug is not present:

>>>> 

>>>> g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline

>>>> g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline

>>>> g++ condbreak-solib-main.o condbreak-solib-lib.o

>>>> 

>>>> It causes the type of the return value to be detected as

>>>> TYPE_CODE_PTR->TYPE_CODE_INT.

>>> 

>>> Huh.  That's really strange.  Where is that pointer coming from?

>>> 

>>> What does "ptype cmp3" say for you?

>>> 

>>> (gdb) b foo if (int)cmp3("abc") == 1

>>> Breakpoint 1 at 0x40048b

>>> (gdb) ptype cmp3

>>> type = <unknown return type> ()

>>> (gdb) whatis cmp3

>>> type = <text variable, no debug info>

>>> 

>> 

>> Yes, I get the same.

>> 

>> Sounds to me like there might still be an issue in gdb? Or at least

>> a difference in the way the type is calculated.

>> This on it’s own is still a good fix, so I’ll send a new version.

> 

> I think we should learn the answer to the "where is that pointer

> coming from" question.  I'm really not understanding why the

> shared library makes a difference.

> 

>> 

>> 

>>> I wonder if what you're looking at is actually the malloc call?

>>> GDB will call malloc to allocate space for the "abc" string in

>>> the inferior.  Look at the backtrace, see where the call is coming

>>> from.

>> 

>> 

>> With the nodebug and debug shared library version:

>> (I need to run to main before I can set breakpoint on cmp3, otherwise

>> "Function "cmp3" not defined.”)

>> 

>> But with all versions, when stopped at cmp3, I get:

>> (gdb) bt

>> #0  0x00000000004005d4 in cmp3(char const*) ()

>> #1  <function called from gdb>

>> #2  0x00000000004005a4 in foo() ()

>> #3  0x00000000004005bc in main ()

> 

> 

> That's a backtrace in the inferior.  I meant instead for you to set

> a breakpoint on gdb's aarch64_push_dummy_call, figure out where the

> TYPE_CODE_PTR->TYPE_CODE_INT pointer comes from.  

> With "b foo if (int)cmp3("abc") == 1", gdb will do two

> infcalls, one malloc call to allocate space for "abc"

> string, and then the call to cmp3.

> 


A-ha! Now I understand why I get two calls into _push_dummy_call.

So to answer your question, the TYPE_CODE_PTR->TYPE_CODE_INT is the malloc call.

Then the next call to _push_dummy_call has a return type of 0, as expected.
This doesn’t segfault because it goes into language_pass_by_reference which
routes to default_pass_by_reference. The same as the C shared library version.


I’ve updated the test so it does {c,c++}*{debug nodebug}.
I can also update it to do both shared lib and non shared lib too. That should
cover everything.


Alan.
  

On 10/29/2018 02:56 PM, Alan Hayward wrote:

> A-ha! Now I understand why I get two calls into _push_dummy_call.
> 
> So to answer your question, the TYPE_CODE_PTR->TYPE_CODE_INT is the malloc call.
> 
> Then the next call to _push_dummy_call has a return type of 0, as expected.
> This doesn’t segfault because it goes into language_pass_by_reference which
> routes to default_pass_by_reference. The same as the C shared library version.
> 
> 
> I’ve updated the test so it does {c,c++}*{debug nodebug}.
> I can also update it to do both shared lib and non shared lib too. That should
> cover everything.
But still, why do you see a difference between shared library and non-shared
library?

Thanks,
Pedro Alves
  

> On 29 Oct 2018, at 18:13, Pedro Alves <palves@redhat.com> wrote:

> 

> On 10/29/2018 02:56 PM, Alan Hayward wrote:

> 

>> A-ha! Now I understand why I get two calls into _push_dummy_call.

>> 

>> So to answer your question, the TYPE_CODE_PTR->TYPE_CODE_INT is the malloc call.

>> 

>> Then the next call to _push_dummy_call has a return type of 0, as expected.

>> This doesn’t segfault because it goes into language_pass_by_reference which

>> routes to default_pass_by_reference. The same as the C shared library version.

>> 

>> 

>> I’ve updated the test so it does {c,c++}*{debug nodebug}.

>> I can also update it to do both shared lib and non shared lib too. That should

>> cover everything.

> But still, why do you see a difference between shared library and non-shared

> library?


In all cases the function type is the same.

The difference is because with c++ && shared library, the code ends up in 
gnuv3_pass_by_reference(), which means it’s using the GNU G++ Version 3 ABI,
whereas with any other options (non shared or c) it ends up in
default_pass_by_reference().

Looking at the doc for GNU G++ Version 3 ABI:
https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
The library needs to be linked against libstdc++.so to use it.

A quick ldd shows only the c++ .so is linked against it.


Alan.
  

On 10/30/2018 11:13 AM, Alan Hayward wrote:

>> On 29 Oct 2018, at 18:13, Pedro Alves <palves@redhat.com> wrote:
>>
>> On 10/29/2018 02:56 PM, Alan Hayward wrote:
>>
>>> A-ha! Now I understand why I get two calls into _push_dummy_call.
>>>
>>> So to answer your question, the TYPE_CODE_PTR->TYPE_CODE_INT is the malloc call.
>>>
>>> Then the next call to _push_dummy_call has a return type of 0, as expected.
>>> This doesn’t segfault because it goes into language_pass_by_reference which
>>> routes to default_pass_by_reference. The same as the C shared library version.
>>>
>>>
>>> I’ve updated the test so it does {c,c++}*{debug nodebug}.
>>> I can also update it to do both shared lib and non shared lib too. That should
>>> cover everything.
>> But still, why do you see a difference between shared library and non-shared
>> library?
> 
> In all cases the function type is the same.
> 
> The difference is because with c++ && shared library, the code ends up in 
> gnuv3_pass_by_reference(), which means it’s using the GNU G++ Version 3 ABI,
> whereas with any other options (non shared or c) it ends up in
> default_pass_by_reference().

The function is the same, and should have been compiled using the calling
convention irrespective of whether it is linked into the main program,
or linked into the separate library.  Right?

So, either I'm missing something, or in one of the cases (shared
vs non-shared), we're calling the function incorrectly (along
with anything else that depends on call ABI), no?

What am I missing?

What does:

 (gdb) show cp-abi 
 The currently selected C++ ABI is "auto" (currently "gnu-v3").

show for you, in the shared and non-shared cases?

/me pokes a bit.

OK, I see what it is.

You've compiled the _main_ .cc without debug info as well:

 g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline
 g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline
 g++ condbreak-solib-main.o condbreak-solib-lib.o

And if you do that, the program ends up with no debug info at
all, and so GDB has no clue that this is a C++ program:

 (gdb) start
 Temporary breakpoint 1 at 0x4004c1
 Starting program: /tmp/a.out 
 
 Temporary breakpoint 1, 0x00000000004004c1 in main ()
 (gdb) show language 
 The current source language is "auto; currently c".
 (gdb)


If you compile (only) the main.cc with debug info, like this:

 - g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline
 + g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline -g

then GDB will know that the program is a C++ program.  And you'd
still be calling a cmp3 function that has no debug info,
and should thus trigger the bug.



So when we call cmp3 with GDB's language set to C, we land
in default_pass_by_reference:

 (gdb) show language 
 The current source language is "auto; currently c".
 (gdb) p (int) cmp3(word)

 Thread 1 "gdb" hit Breakpoint 3, default_pass_by_reference (type=0x1b5a230) at src/gdb/language.c:669
 669       return 0;
 (top-gdb) c
 Continuing.

When the language is set to C++, we end up in gnuv3_pass_by_reference:

 (gdb) set language c++ 
 (gdb) p (int) cmp3(word)

 Thread 1 "gdb" hit Breakpoint 4, gnuv3_pass_by_reference (type=0x1b33290) at src/gdb/gnu-v3-abi.c:1255
 1255      type = check_typedef (type);
 (top-gdb) 

And this is because language_pass_by_reference uses the
current language, instead of the symbol's language (arguably a bug):

 (top-gdb) bt
 #0  0x000000000065be91 in gnuv3_pass_by_reference(type*) (type=0x1b33290)
     at src/gdb/gnu-v3-abi.c:1255
 #1  0x0000000000543e2a in cp_pass_by_reference(type*) (type=0x1b33290) at src/gdb/cp-abi.c:229
 #2  0x00000000006cc09b in language_pass_by_reference(type*) (type=0x1b33290)
     at src/gdb/language.c:660
 #3  0x000000000045a27a in default_return_in_first_hidden_param_p(gdbarch*, type*) (gdbarch=0x1b316b0, type=0x1b33290)
     at src/gdb/arch-utils.c:861
 #4  0x0000000000640a86 in gdbarch_return_in_first_hidden_param_p(gdbarch*, type*) (gdbarch=0x1b316b0, type=0x1b33290)
     at src/gdb/gdbarch.c:2739
 #5  0x00000000006a1011 in call_function_by_hand_dummy(value*, type*, int, value**, void (*)(void*, int), void*) (function=0x1b44730, default_return_type=0x1b33290, nargs=1, args=0x7fffffffc128, dummy_dtor=0x0, dummy_dtor_data=0x0)
     at src/gdb/infcall.c:881



So that's the real difference.  Shared vs non-shared is just
a kind of a red herring.  If you don't have debug info for
libstdc++, for example, then probably GDB won't know that the
no-debug-info program is a C++ program either.

So please adjust your test to eliminate use of the shared
library, and build just the cmp3 source file without
debug info.

> Looking at the doc for GNU G++ Version 3 ABI:
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> The library needs to be linked against libstdc++.so to use it.
> 
> A quick ldd shows only the c++ .so is linked against it.

That wouldn't make much sense.  The whole program is using the
same compiler/call/mangling ABI, certainly, which is what
matters here.

Thanks,
Pedro Alves
  

> On 30 Oct 2018, at 16:31, Pedro Alves <palves@redhat.com> wrote:

> 

> On 10/30/2018 11:13 AM, Alan Hayward wrote:

> 

>>> On 29 Oct 2018, at 18:13, Pedro Alves <palves@redhat.com> wrote:

>>> 

>>> On 10/29/2018 02:56 PM, Alan Hayward wrote:

>>> 

>>>> A-ha! Now I understand why I get two calls into _push_dummy_call.

>>>> 

>>>> So to answer your question, the TYPE_CODE_PTR->TYPE_CODE_INT is the malloc call.

>>>> 

>>>> Then the next call to _push_dummy_call has a return type of 0, as expected.

>>>> This doesn’t segfault because it goes into language_pass_by_reference which

>>>> routes to default_pass_by_reference. The same as the C shared library version.

>>>> 

>>>> 

>>>> I’ve updated the test so it does {c,c++}*{debug nodebug}.

>>>> I can also update it to do both shared lib and non shared lib too. That should

>>>> cover everything.

>>> But still, why do you see a difference between shared library and non-shared

>>> library?

>> 

>> In all cases the function type is the same.

>> 

>> The difference is because with c++ && shared library, the code ends up in 

>> gnuv3_pass_by_reference(), which means it’s using the GNU G++ Version 3 ABI,

>> whereas with any other options (non shared or c) it ends up in

>> default_pass_by_reference().

> 

> The function is the same, and should have been compiled using the calling

> convention irrespective of whether it is linked into the main program,

> or linked into the separate library.  Right?

> 

> So, either I'm missing something, or in one of the cases (shared

> vs non-shared), we're calling the function incorrectly (along

> with anything else that depends on call ABI), no?

> 

> What am I missing?

> 

> What does:

> 

> (gdb) show cp-abi 

> The currently selected C++ ABI is "auto" (currently "gnu-v3").

> 

> show for you, in the shared and non-shared cases?

> 

> /me pokes a bit.

> 

> OK, I see what it is.

> 

> You've compiled the _main_ .cc without debug info as well:

> 

> g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline

> g++ -c condbreak-solib-lib.cc -o condbreak-solib-lib.o -fno-inline

> g++ condbreak-solib-main.o condbreak-solib-lib.o

> 

> And if you do that, the program ends up with no debug info at

> all, and so GDB has no clue that this is a C++ program:

> 

> (gdb) start

> Temporary breakpoint 1 at 0x4004c1

> Starting program: /tmp/a.out 

> 

> Temporary breakpoint 1, 0x00000000004004c1 in main ()

> (gdb) show language 

> The current source language is "auto; currently c".

> (gdb)

> 

> 

> If you compile (only) the main.cc with debug info, like this:

> 

> - g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline

> + g++ -c condbreak-solib-main.cc -o condbreak-solib-main.o -fno-inline -g

> 

> then GDB will know that the program is a C++ program.  And you'd

> still be calling a cmp3 function that has no debug info,

> and should thus trigger the bug.

> 


Yes, I hit the bug this way.

Many thanks for looking through this.


> 

> 

> So when we call cmp3 with GDB's language set to C, we land

> in default_pass_by_reference:

> 

> (gdb) show language 

> The current source language is "auto; currently c".

> (gdb) p (int) cmp3(word)

> 

> Thread 1 "gdb" hit Breakpoint 3, default_pass_by_reference (type=0x1b5a230) at src/gdb/language.c:669

> 669       return 0;

> (top-gdb) c

> Continuing.

> 

> When the language is set to C++, we end up in gnuv3_pass_by_reference:

> 

> (gdb) set language c++ 

> (gdb) p (int) cmp3(word)

> 

> Thread 1 "gdb" hit Breakpoint 4, gnuv3_pass_by_reference (type=0x1b33290) at src/gdb/gnu-v3-abi.c:1255

> 1255      type = check_typedef (type);

> (top-gdb) 

> 

> And this is because language_pass_by_reference uses the

> current language, instead of the symbol's language (arguably a bug):

> 

> (top-gdb) bt

> #0  0x000000000065be91 in gnuv3_pass_by_reference(type*) (type=0x1b33290)

>     at src/gdb/gnu-v3-abi.c:1255

> #1  0x0000000000543e2a in cp_pass_by_reference(type*) (type=0x1b33290) at src/gdb/cp-abi.c:229

> #2  0x00000000006cc09b in language_pass_by_reference(type*) (type=0x1b33290)

>     at src/gdb/language.c:660

> #3  0x000000000045a27a in default_return_in_first_hidden_param_p(gdbarch*, type*) (gdbarch=0x1b316b0, type=0x1b33290)

>     at src/gdb/arch-utils.c:861

> #4  0x0000000000640a86 in gdbarch_return_in_first_hidden_param_p(gdbarch*, type*) (gdbarch=0x1b316b0, type=0x1b33290)

>     at src/gdb/gdbarch.c:2739

> #5  0x00000000006a1011 in call_function_by_hand_dummy(value*, type*, int, value**, void (*)(void*, int), void*) (function=0x1b44730, default_return_type=0x1b33290, nargs=1, args=0x7fffffffc128, dummy_dtor=0x0, dummy_dtor_data=0x0)

>     at src/gdb/infcall.c:881

> 

> 

> 

> So that's the real difference.  Shared vs non-shared is just

> a kind of a red herring.  If you don't have debug info for

> libstdc++, for example, then probably GDB won't know that the

> no-debug-info program is a C++ program either.

> 

> So please adjust your test to eliminate use of the shared

> library, and build just the cmp3 source file without

> debug info.


Will do.


> 

>> Looking at the doc for GNU G++ Version 3 ABI:

>> https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html

>> The library needs to be linked against libstdc++.so to use it.

>> 

>> A quick ldd shows only the c++ .so is linked against it.

> 

> That wouldn't make much sense.  The whole program is using the

> same compiler/call/mangling ABI, certainly, which is what

> matters here.

> 

> Thanks,

> Pedro Alves
  

On 10/30/2018 05:09 PM, Alan Hayward wrote:
> 
> 
>> On 30 Oct 2018, at 16:31, Pedro Alves <palves@redhat.com> wrote:

>> So that's the real difference.  Shared vs non-shared is just
>> a kind of a red herring.  If you don't have debug info for
>> libstdc++, for example, then probably GDB won't know that the
>> no-debug-info program is a C++ program either.
>>
>> So please adjust your test to eliminate use of the shared
>> library, and build just the cmp3 source file without
>> debug info.
> 
> Will do.

Thinking a little bit more, the conditional breakpoint is also
kind of a red herring here.  What really matters is the infcall.

So the testcase could just do:

  (gdb) p (int) cmp3(word)

I'd simplify the testcase a little bit in that direction,
and call it gdb.cp/nodebug-infcall.exp, for example.

Note, the testcase should be gated with a 

 if [target_info exists gdb,cannot_call_functions] {

check.

Thanks,
Pedro Alves