Re: [PATCH 1/2] Catch exceptions thrown from gdbarch_skip_prologue

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Yao,

Thanks for the patch.  It look good to me, I just noted a few nits:

On 2017-07-19 13:27, Yao Qi wrote:
> PR 21555 is caused by the exception during the prologue analysis when 
> re-set
> a breakpoint.
> 
> (gdb) bt
>  #0  memory_error_message (err=TARGET_XFER_E_IO, gdbarch=0x153db50,
> memaddr=93824992233232) at ../../binutils-gdb/gdb/corefile.c:192
>  #1  0x00000000005718ed in memory_error (err=TARGET_XFER_E_IO,
> memaddr=memaddr@entry=93824992233232) at
> ../../binutils-gdb/gdb/corefile.c:220
>  #2  0x00000000005719d6 in read_memory_object
> (object=object@entry=TARGET_OBJECT_CODE_MEMORY,
> memaddr=93824992233232, memaddr@entry=1,
> myaddr=myaddr@entry=0x7fffffffd0a0 "P\333S\001", len=len@entry=1) at
> ../../binutils-gdb/gdb/corefile.c:259
>  #3  0x0000000000571c6e in read_code (len=1, myaddr=0x7fffffffd0a0
> "P\333S\001", memaddr=<optimized out>) at
> ../../binutils-gdb/gdb/corefile.c:287
>  #4  read_code_unsigned_integer (memaddr=memaddr@entry=93824992233232,
> len=len@entry=1, byte_order=byte_order@entry=BFD_ENDIAN_LITTLE)
>                   at ../../binutils-gdb/gdb/corefile.c:362
>  #5  0x000000000041d4a0 in amd64_analyze_prologue
> (gdbarch=gdbarch@entry=0x153db50, pc=pc@entry=93824992233232,
> current_pc=current_pc@entry=18446744073709551615,
> cache=cache@entry=0x7fffffffd1e0) at
> ../../binutils-gdb/gdb/amd64-tdep.c:2310
>  #6  0x000000000041e404 in amd64_skip_prologue (gdbarch=0x153db50,
> start_pc=93824992233232) at ../../binutils-gdb/gdb/amd64-tdep.c:2459
>  #7  0x000000000067bfb0 in skip_prologue_sal
> (sal=sal@entry=0x7fffffffd4e0) at ../../binutils-gdb/gdb/symtab.c:3628
>  #8  0x000000000067c4d8 in find_function_start_sal
> (sym=sym@entry=0x1549960, funfirstline=1) at
> ../../binutils-gdb/gdb/symtab.c:3501
>  #9  0x000000000060999d in symbol_to_sal
> (result=result@entry=0x7fffffffd5f0, funfirstline=<optimized out>,
> sym=sym@entry=0x1549960) at ../../binutils-gdb/gdb/linespec.c:3860
> ....
>  #16 0x000000000054b733 in location_to_sals (b=b@entry=0x15792d0,
> location=0x157c230, search_pspace=search_pspace@entry=0x1148120,
> found=found@entry=0x7fffffffdc64) at
> ../../binutils-gdb/gdb/breakpoint.c:14211
>  #17 0x000000000054c1f5 in breakpoint_re_set_default (b=0x15792d0) at
> ../../binutils-gdb/gdb/breakpoint.c:14301
>  #18 0x00000000005412a9 in breakpoint_re_set_one
> (bint=bint@entry=0x15792d0) at
> ../../binutils-gdb/gdb/breakpoint.c:14412
> 
> This problem can be fixed by
> 
>  - either each prologue analyzer doesn't throw exception,
>  - or catch the exception thrown from gdbarch_skip_prologue,
> 
> I choose the latter because the former needs to fix *every* prologue
> analyzer to not throw exception.
> 
> This error can be reproduced by changing reread.exp.  The test 
> reread.exp
> has already test that breakpoint can be reset correctly after the
> executable is re-read.  This patch extends this test by compiling test 
> c
> file with and without -fPIE.
> 
> (gdb) run ^M
> The program being debugged has been started already.^M
> Start it from the beginning? (y or n) y^M
> x86_64/gdb/testsuite/outputs/gdb.base/reread/reread' has changed;
> re-reading symbols.
> Error in re-setting breakpoint 1: Cannot access memory at address
> 0x555555554790^M
> Error in re-setting breakpoint 2: Cannot access memory at address
> 0x555555554790^M
> Starting program:
> /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/reread/reread
> ^M
> This is foo^M
> [Inferior 1 (process 27720) exited normally]^M
> (gdb) FAIL: gdb.base/reread.exp: opts= "-fPIE" "ldflags=-pie" : run to
> foo() second time (the program exited)
> 
> This patch doesn't re-indent the code, to keep the patch simple.
> Regression tested on x86_64-linux (ubuntu xenial), and verified that
> it fixes some fails on Debian/stretch.

I wouldn't mind if you indented the code in this patch.  Many diff 
viewers are good at highlighting what changed in a line, so it's obvious 
when it's only whitespaces.  Otherwise, here's always git diff/show -w 
for those who like looking at raw diffs.

> 
> gdb:
> 
> 2017-07-18  Yao Qi  <yao.qi@linaro.org>
> 
> 	PR gdb/21555
> 	* arch-utils.c (gdbarch_skip_prologue_noexcept): New function.
> 	* arch-utils.h (gdbarch_skip_prologue_noexcept): Declare.
> 	* infrun.c: Include arch-utils.h
> 	(handle_step_into_function): Call gdbarch_skip_prologue_noexcept.
> 	(handle_step_into_function_backward): Likewise.
> 	* symtab.c (skip_prologue_sal): Likewise.
> 
> gdb/testsuite:
> 
> 2017-07-18  Yao Qi  <yao.qi@linaro.org>
> 
> 	PR gdb/21555
> 	* gdb.base/reread.exp: Wrap the whole test with two kinds of
> 	compilation flags, with -fPIE and without -fPIE.
> ---
>  gdb/arch-utils.c                  | 19 +++++++++++++++++++
>  gdb/arch-utils.h                  |  3 +++
>  gdb/infrun.c                      |  9 +++++----
>  gdb/symtab.c                      |  3 ++-
>  gdb/testsuite/gdb.base/reread.exp | 12 +++++++++---
>  5 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 2ae3413..2f63b80 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -985,6 +985,25 @@ default_print_insn (bfd_vma memaddr,
> disassemble_info *info)
>    return (*disassemble_fn) (memaddr, info);
>  }
> 
> +/* Wrapper to gdbarch_skip_prologue, but doesn't throw exception.  
> Catch
> +   exception thrown from gdbarch_skip_prologue, and return PC.  */

Could you explain in the comment what is returned if an exception is 
caught by the wrapper?

The comment should probably go in the .h, with a "See arch-utils.h" 
comment here.

> +CORE_ADDR
> +gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, CORE_ADDR pc) 
> noexcept
> +{
> +  CORE_ADDR new_pc = pc;
> +
> +  TRY
> +    {
> +      new_pc = gdbarch_skip_prologue (gdbarch, new_pc);

I know it does the same thing, but I think it would be clearer to pass 
"pc" as a parameter to the function.

> +    }
> +  CATCH (ex, RETURN_MASK_ALL)
> +    {}
> +  END_CATCH
> +
> +  return new_pc;
> +}
> +
>  /* -Wmissing-prototypes */
>  extern initialize_file_ftype _initialize_gdbarch_utils;
> 
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 2aa9159..11c4afc 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -268,4 +268,7 @@ extern void default_guess_tracepoint_registers
> (struct gdbarch *gdbarch,
> 
>  extern int default_print_insn (bfd_vma memaddr, disassemble_info 
> *info);
> 
> +extern CORE_ADDR gdbarch_skip_prologue_noexcept (gdbarch *gdbarch,
> +						 CORE_ADDR pc) noexcept;
> +
>  #endif
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a68da6a..37ff015 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -66,6 +66,7 @@
>  #include "common/enum-flags.h"
>  #include "progspace-and-thread.h"
>  #include "common/gdb_optional.h"
> +#include "arch-utils.h"
> 
>  /* Prototypes for local functions */
> 
> @@ -7317,8 +7318,8 @@ handle_step_into_function (struct gdbarch 
> *gdbarch,
> 
>    cust = find_pc_compunit_symtab (stop_pc);
>    if (cust != NULL && compunit_language (cust) != language_asm)
> -    ecs->stop_func_start = gdbarch_skip_prologue (gdbarch,
> -						  ecs->stop_func_start);
> +    ecs->stop_func_start
> +      = gdbarch_skip_prologue_noexcept (gdbarch, 
> ecs->stop_func_start);
> 
>    stop_func_sal = find_pc_line (ecs->stop_func_start, 0);
>    /* Use the step_resume_break to step until the end of the prologue,
> @@ -7396,8 +7397,8 @@ handle_step_into_function_backward (struct
> gdbarch *gdbarch,
> 
>    cust = find_pc_compunit_symtab (stop_pc);
>    if (cust != NULL && compunit_language (cust) != language_asm)
> -    ecs->stop_func_start = gdbarch_skip_prologue (gdbarch,
> -						  ecs->stop_func_start);
> +    ecs->stop_func_start
> +      = gdbarch_skip_prologue_noexcept (gdbarch, 
> ecs->stop_func_start);
> 
>    stop_func_sal = find_pc_line (stop_pc, 0);
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index d4e107a..ccf31cc 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -64,6 +64,7 @@
>  #include "progspace-and-thread.h"
>  #include "common/gdb_optional.h"
>  #include "filename-seen-cache.h"
> +#include "arch-utils.h"
> 
>  /* Forward declarations for local functions.  */
> 
> @@ -3627,7 +3628,7 @@ skip_prologue_sal (struct symtab_and_line *sal)
>        if (gdbarch_skip_entrypoint_p (gdbarch))
>          pc = gdbarch_skip_entrypoint (gdbarch, pc);
>        if (skip)
> -	pc = gdbarch_skip_prologue (gdbarch, pc);
> +	pc = gdbarch_skip_prologue_noexcept (gdbarch, pc);
> 
>        /* For overlays, map pc back into its mapped VMA range.  */
>        pc = overlay_mapped_address (pc, section);
> diff --git a/gdb/testsuite/gdb.base/reread.exp
> b/gdb/testsuite/gdb.base/reread.exp
> index cc0f577..74bd20e 100644
> --- a/gdb/testsuite/gdb.base/reread.exp
> +++ b/gdb/testsuite/gdb.base/reread.exp
> @@ -15,6 +15,11 @@
> 
>  set prototypes 1
> 
> +# Build programs in PIE mode, to reproduce PR 21555.
> +foreach_with_prefix opts {
> +    { "" "" }
> +    { "-fPIE" "ldflags=-pie" } } {
> +
>  # build the first test case
> 
>  set testfile1 "reread1"
> @@ -22,7 +27,7 @@ set srcfile1 ${testfile1}.c
>  # Cygwin needs $EXEEXT.
>  set binfile1 [standard_output_file ${testfile1}$EXEEXT]
> 
> -if  { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}"
> executable {debug nowarnings}] != "" } {
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}"
> executable [list debug nowarnings additional_flags=[lindex $opts 0]
> [lindex $opts 1]]] != "" } {
>      untested "failed to compile first testcase"
>      return -1
>  }
> @@ -33,8 +38,8 @@ set testfile2 "reread2"
>  set srcfile2 ${testfile2}.c
>  set binfile2 [standard_output_file ${testfile2}$EXEEXT]
> 
> -if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}"
> executable {debug nowarnings}] != ""
> -      && [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}"
> executable {debug nowarnings additional_flags=-DNO_SECTIONS}] != ""} {
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}"
> executable [list debug nowarnings additional_flags=[lindex $opts 0]
> [lindex $opts 1]]] != ""
> +      && [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}"
> executable [list debug nowarnings "additional_flags=-DNO_SECTIONS
> [lindex $opts 0]" [lindex $opts 1]]] != ""} {

Could you split those really long lines?  It would perhaps help to 
define an intermediary variable for compilation opts.

>      untested "failed to compile second testcase"
>      return -1
>  }
> @@ -120,6 +125,7 @@ if [is_remote target] {
>      gdb_test "" "Breakpoint.* foo .* at .*:9.*" "second pass: run to
> foo() second time"
>  }
> 
> +}
>  # End of tests.
> 
>  return 0

Thanks,

Simon