[PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging.

[PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

[-- Attachment #1: Type: text/plain, Size: 89 bytes --]

This is a fix to a problem that was introduced with commit g3e29f34.

OK to apply?

Doug

[-- Attachment #2: 0001-mips-Fix-PR-21337-v1-segfault-when-re-reading-symbol.patch --]
[-- Type: text/x-patch, Size: 1406 bytes --]

From 6746b67149f1158c072317e0f1a095d682e112aa Mon Sep 17 00:00:00 2001
From: Doug Gilmore <doug.gilmore@imgtec.com>
Date: Fri, 31 Mar 2017 15:41:06 -0700
Subject: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with
 remote debugging.

gdb/

2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>

	* symfile.c (reread_symbols): Fix PR 21337.
---
 gdb/symfile.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 8b79508..290b18b 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2592,6 +2592,12 @@ reread_symbols (void)
 	  /* Free the obstacks for non-reusable objfiles.  */
 	  psymbol_bcache_free (objfile->psymbol_cache);
 	  objfile->psymbol_cache = psymbol_bcache_init ();
+
+	  /* Notify objfiles that we've modified objfile sections, which now
+	     needs to be done early to ensure that, for the MIPS target,
+	     find_pc_section won't access stale data.  PR 21337.  */
+	  objfiles_changed ();
+
 	  obstack_free (&objfile->objfile_obstack, 0);
 	  objfile->sections = NULL;
 	  objfile->compunit_symtabs = NULL;
@@ -2678,9 +2684,6 @@ reread_symbols (void)
     {
       int ix;
 
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
-- 
1.9.1

Re: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 03/31/2017 04:04 PM, Doug Gilmore wrote:
> This is a fix to a problem that was introduced with commit g3e29f34.
>
> OK to apply?
>
> Doug
>
Could a global maintainer review the patch attached to:

https://www.sourceware.org/ml/gdb-patches/2017-03/msg00559.html

when they have the chance?  Note that patch involves changing code
that is not MIPS specific.

Thanks,

Doug

Re: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging.

[Luis Machado]

On 03/31/2017 06:04 PM, Doug Gilmore wrote:
> This is a fix to a problem that was introduced with commit g3e29f34.
>

Which commit is that? I couldn't find it in the git tree.

Incidentally, i have a local patch that does pretty much the same thing, 
but a little further down compared to yours. It notifies gdb of objfile 
changes right before the call to read_symbols (...).

But my patch doesn't remove the original call to objfiles_changed (...). 
What is the rationale behind that change?

> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 8b79508..290b18b 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2592,6 +2592,12 @@ reread_symbols (void)
>  	  /* Free the obstacks for non-reusable objfiles.  */
>  	  psymbol_bcache_free (objfile->psymbol_cache);
>  	  objfile->psymbol_cache = psymbol_bcache_init ();
> +
> +	  /* Notify objfiles that we've modified objfile sections, which now
> +	     needs to be done early to ensure that, for the MIPS target,
> +	     find_pc_section won't access stale data.  PR 21337.  */

I think the PR number is not needed. After all the bug will be gone with 
this fix.

Re: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 04/12/2017 11:52 AM, Luis Machado wrote:
> On 03/31/2017 06:04 PM, Doug Gilmore wrote:
>> This is a fix to a problem that was introduced with commit g3e29f34.
>>
> 
> Which commit is that? I couldn't find it in the git tree.
$ git log -n 1 3e29f34 | head
commit 3e29f34a4eef29f5b159749ccb1efb8867b2e651
Author: Maciej W. Rozycki <macro@codesourcery.com>
Date:   Fri Dec 12 13:31:53 2014 +0000

    MIPS: Keep the ISA bit in compressed code addresses
    
    1. Background information
    
    The MIPS architecture, as originally designed and implemented in
    mid-1980s has a uniform instruction word size that is 4 bytes, naturally
$ 
> 
> Incidentally, i have a local patch that does pretty much the same
> thing, but a little further down compared to yours. It notifies gdb
> of objfile changes right before the call to read_symbols (...).
That location is fine too, I just put the call at the point that
the data actually becomes stale.
> 
> But my patch doesn't remove the original call to objfiles_changed
> (...). What is the rationale behind that change?
I removed it just because would always be an duplicate call.
> 
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index 8b79508..290b18b 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -2592,6 +2592,12 @@ reread_symbols (void)
>>        /* Free the obstacks for non-reusable objfiles.  */
>>        psymbol_bcache_free (objfile->psymbol_cache);
>>        objfile->psymbol_cache = psymbol_bcache_init ();
>> +
>> +      /* Notify objfiles that we've modified objfile sections, which now
>> +         needs to be done early to ensure that, for the MIPS target,
>> +         find_pc_section won't access stale data.  PR 21337.  */
> 
> I think the PR number is not needed. After all the bug will be gone with this fix.
Your right, people can just run "git log -p" to recover that information.

I'll update the patch accordingly.

Thanks,

Doug

[PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

I updated and rebased the patch per Luis's comments in:

https://www.sourceware.org/ml/gdb-patches/2017-04/msg00361.html

which I attached.  Could a global maintainer review it when they have
the chance?  The problem is only exposed on MIPS, however the patch
involves changing code that is not MIPS specific.

Thanks,

Doug

gdb/

2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>

	* symfile.c (reread_symbols): Fix PR 21337.
---
 gdb/symfile.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 846aabe..d57563d 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2576,6 +2576,12 @@ reread_symbols (void)
 	  /* Free the obstacks for non-reusable objfiles.  */
 	  psymbol_bcache_free (objfile->psymbol_cache);
 	  objfile->psymbol_cache = psymbol_bcache_init ();
+
+	  /* Notify objfiles that we've modified objfile sections, which now
+	     needs to be done early to ensure that, for the MIPS target,
+	     find_pc_section won't access stale data.  */
+	  objfiles_changed ();
+
 	  obstack_free (&objfile->objfile_obstack, 0);
 	  objfile->sections = NULL;
 	  objfile->compunit_symtabs = NULL;
@@ -2660,9 +2666,6 @@ reread_symbols (void)
 
   if (!new_objfiles.empty ())
     {
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
-- 
1.9.1

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Luis Machado]

On 04/13/2017 01:56 PM, Doug Gilmore wrote:
> I updated and rebased the patch per Luis's comments in:
>
> https://www.sourceware.org/ml/gdb-patches/2017-04/msg00361.html
>
> which I attached.  Could a global maintainer review it when they have
> the chance?  The problem is only exposed on MIPS, however the patch
> involves changing code that is not MIPS specific.
>
> Thanks,
>
> Doug
>
> gdb/
>
> 2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>
>
> 	* symfile.c (reread_symbols): Fix PR 21337.
> ---
>  gdb/symfile.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 846aabe..d57563d 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2576,6 +2576,12 @@ reread_symbols (void)
>  	  /* Free the obstacks for non-reusable objfiles.  */
>  	  psymbol_bcache_free (objfile->psymbol_cache);
>  	  objfile->psymbol_cache = psymbol_bcache_init ();
> +
> +	  /* Notify objfiles that we've modified objfile sections, which now
> +	     needs to be done early to ensure that, for the MIPS target,
> +	     find_pc_section won't access stale data.  */
> +	  objfiles_changed ();
> +
>  	  obstack_free (&objfile->objfile_obstack, 0);
>  	  objfile->sections = NULL;
>  	  objfile->compunit_symtabs = NULL;

This looks reasonable to me. I have no further comments.

Thanks,
Luis

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Simon Marchi]

On 2017-04-13 14:56, Doug Gilmore wrote:
> I updated and rebased the patch per Luis's comments in:
> 
> https://www.sourceware.org/ml/gdb-patches/2017-04/msg00361.html
> 
> which I attached.  Could a global maintainer review it when they have
> the chance?  The problem is only exposed on MIPS, however the patch
> involves changing code that is not MIPS specific.
> 
> Thanks,
> 
> Doug
> 
> gdb/
> 
> 2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>
> 
> 	* symfile.c (reread_symbols): Fix PR 21337.
> ---
>  gdb/symfile.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 846aabe..d57563d 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2576,6 +2576,12 @@ reread_symbols (void)
>  	  /* Free the obstacks for non-reusable objfiles.  */
>  	  psymbol_bcache_free (objfile->psymbol_cache);
>  	  objfile->psymbol_cache = psymbol_bcache_init ();
> +
> +	  /* Notify objfiles that we've modified objfile sections, which now
> +	     needs to be done early to ensure that, for the MIPS target,
> +	     find_pc_section won't access stale data.  */
> +	  objfiles_changed ();
> +
>  	  obstack_free (&objfile->objfile_obstack, 0);
>  	  objfile->sections = NULL;
>  	  objfile->compunit_symtabs = NULL;
> @@ -2660,9 +2666,6 @@ reread_symbols (void)
> 
>    if (!new_objfiles.empty ())
>      {
> -      /* Notify objfiles that we've modified objfile sections.  */
> -      objfiles_changed ();
> -
>        clear_symtab_users (0);
> 
>        /* clear_objfile_data for each objfile was called before freeing 
> it and

I don't have the required knowledge to review this properly, but I have 
a question.  From the attachment in Bugzilla, the backtrace where the 
crash happens is:

==19949==    at 0x64D827: bsearch_cmp(void const*, void const*) 
(objfiles.c:1415)
==19949==    by 0x559D247: bsearch (stdlib-bsearch.h:33)
==19949==    by 0x64D9E3: find_pc_section(unsigned long) 
(objfiles.c:1462)
==19949==    by 0x643BDE: lookup_minimal_symbol_by_pc(unsigned long) 
(minsyms.c:785)
==19949==    by 0x40852B: mips_pc_is_mips(unsigned long) 
(mips-tdep.c:1183)
==19949==    by 0x4086EA: mips_adjust_dwarf2_addr(unsigned long) 
(mips-tdep.c:1271)
==19949==    by 0x5E0F98: gdbarch_adjust_dwarf2_addr(gdbarch*, unsigned 
long) (gdbarch.c:3369)
==19949==    by 0x5A24E5: read_attribute_value(die_reader_specs const*, 
attribute*, unsigned int, long, unsigned char const*) 
(dwarf2read.c:16570)
==19949==    by 0x5A2E2E: read_attribute(die_reader_specs const*, 
attribute*, attr_abbrev*, unsigned char const*) (dwarf2read.c:16796)
==19949==    by 0x59FA76: read_full_die_1(die_reader_specs const*, 
die_info**, unsigned char const*, int*, int) (dwarf2read.c:15537)
==19949==    by 0x59FAEB: read_full_die(die_reader_specs const*, 
die_info**, unsigned char const*, int*) (dwarf2read.c:15556)
==19949==    by 0x587B9A: init_cutu_and_read_dies(dwarf2_per_cu_data*, 
abbrev_table*, int, int, void (*)(die_reader_specs const*, unsigned char 
const*, die_info*, int, void*), void*) (dwarf2read.c:5710)


I'd be curious to see the rest of that backtrace to understand better 
when/why it blows up.  It looks like you can use --num-callers with 
valgrind, or simply use GDB :).

Simon

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 04/21/2017 07:15 PM, Simon Marchi wrote:
> On 2017-04-13 14:56, Doug Gilmore wrote:
>> I updated and rebased the patch per Luis's comments in:
>>
>> https://www.sourceware.org/ml/gdb-patches/2017-04/msg00361.html
>>
>> which I attached.  Could a global maintainer review it when they have
>> the chance?  The problem is only exposed on MIPS, however the patch
>> involves changing code that is not MIPS specific.
>>
>> Thanks,
>>
>> Doug
>>
>> gdb/
>>
>> 2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>
>>
>>     * symfile.c (reread_symbols): Fix PR 21337.
>> ---
>>  gdb/symfile.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index 846aabe..d57563d 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -2576,6 +2576,12 @@ reread_symbols (void)
>>        /* Free the obstacks for non-reusable objfiles.  */
>>        psymbol_bcache_free (objfile->psymbol_cache);
>>        objfile->psymbol_cache = psymbol_bcache_init ();
>> +
>> +      /* Notify objfiles that we've modified objfile sections, which now
>> +         needs to be done early to ensure that, for the MIPS target,
>> +         find_pc_section won't access stale data.  */
>> +      objfiles_changed ();
>> +
>>        obstack_free (&objfile->objfile_obstack, 0);
>>        objfile->sections = NULL;
>>        objfile->compunit_symtabs = NULL;
>> @@ -2660,9 +2666,6 @@ reread_symbols (void)
>>
>>    if (!new_objfiles.empty ())
>>      {
>> -      /* Notify objfiles that we've modified objfile sections.  */
>> -      objfiles_changed ();
>> -
>>        clear_symtab_users (0);
>>
>>        /* clear_objfile_data for each objfile was called before freeing it and
> 
> I don't have the required knowledge to review this properly, but I have a question.  From the attachment in Bugzilla, the backtrace where the crash happens is:
> 
> ==19949==    at 0x64D827: bsearch_cmp(void const*, void const*) (objfiles.c:1415)
> ==19949==    by 0x559D247: bsearch (stdlib-bsearch.h:33)
> ==19949==    by 0x64D9E3: find_pc_section(unsigned long) (objfiles.c:1462)
> ==19949==    by 0x643BDE: lookup_minimal_symbol_by_pc(unsigned long) (minsyms.c:785)
> ==19949==    by 0x40852B: mips_pc_is_mips(unsigned long) (mips-tdep.c:1183)
> ==19949==    by 0x4086EA: mips_adjust_dwarf2_addr(unsigned long) (mips-tdep.c:1271)
> ==19949==    by 0x5E0F98: gdbarch_adjust_dwarf2_addr(gdbarch*, unsigned long) (gdbarch.c:3369)
> ==19949==    by 0x5A24E5: read_attribute_value(die_reader_specs const*, attribute*, unsigned int, long, unsigned char const*) (dwarf2read.c:16570)
> ==19949==    by 0x5A2E2E: read_attribute(die_reader_specs const*, attribute*, attr_abbrev*, unsigned char const*) (dwarf2read.c:16796)
> ==19949==    by 0x59FA76: read_full_die_1(die_reader_specs const*, die_info**, unsigned char const*, int*, int) (dwarf2read.c:15537)
> ==19949==    by 0x59FAEB: read_full_die(die_reader_specs const*, die_info**, unsigned char const*, int*) (dwarf2read.c:15556)
> ==19949==    by 0x587B9A: init_cutu_and_read_dies(dwarf2_per_cu_data*, abbrev_table*, int, int, void (*)(die_reader_specs const*, unsigned char const*, die_info*, int, void*), void*) (dwarf2read.c:5710)
> 
> 
> I'd be curious to see the rest of that backtrace to understand better when/why it blows up.  It looks like you can use --num-callers with valgrind, or simply use GDB :).
> 
> Simon
> 
Sorry for the delay, here is the trace under "valgrind --num-callers=111".

Note that I marked the critical call points in reread_symbols.  In
reread_symbols at symfile.c:2631 read_symbols is called.  Via the call
to read_symbols, bsearch_cmp is eventually called, which "reads"
memory that was free'd memory via the call to _obstack_free that was
called just above in reread_symbols at symfile.c:2579.

The patch moves the call to objfiles_changed to the point obstack_free
is called, so that free'd memory will not be referenced.

Trace attached.

Thanks,

Doug

(gdb) qrun
`/home/dgilmore/tmp/h' has changed; re-reading symbols.
==8380== Invalid read of size 8
==8380==    at 0x653949: bsearch_cmp(void const*, void const*) (objfiles.c:1415)
==8380==    by 0x559D247: bsearch (stdlib-bsearch.h:33)
==8380==    by 0x653B05: find_pc_section(unsigned long) (objfiles.c:1462)
==8380==    by 0x649D00: lookup_minimal_symbol_by_pc(unsigned long) (minsyms.c:785)
==8380==    by 0x40852B: mips_pc_is_mips(unsigned long) (mips-tdep.c:1183)
==8380==    by 0x4086EA: mips_adjust_dwarf2_addr(unsigned long) (mips-tdep.c:1271)
==8380==    by 0x5E55C2: gdbarch_adjust_dwarf2_addr(gdbarch*, unsigned long) (gdbarch.c:3417)
==8380==    by 0x5A3EB1: read_attribute_value(die_reader_specs const*, attribute*, unsigned int, long, unsigned char const*) (dwarf2read.c:16620)
==8380==    by 0x5A4823: read_attribute(die_reader_specs const*, attribute*, attr_abbrev*, unsigned char const*) (dwarf2read.c:16846)
==8380==    by 0x5A13BC: read_full_die_1(die_reader_specs const*, die_info**, unsigned char const*, int*, int) (dwarf2read.c:15585)
==8380==    by 0x5A1431: read_full_die(die_reader_specs const*, die_info**, unsigned char const*, int*) (dwarf2read.c:15604)
==8380==    by 0x5890B5: init_cutu_and_read_dies(dwarf2_per_cu_data*, abbrev_table*, int, int, void (*)(die_reader_specs const*, unsigned char const*, die_info*, int, void*), void*) (dwarf2read.c:5753)
==8380==    by 0x58A48F: process_psymtab_comp_unit(dwarf2_per_cu_data*, int, language) (dwarf2read.c:6265)
==8380==    by 0x58B0D0: dwarf2_build_psymtabs_hard(objfile*) (dwarf2read.c:6658)
==8380==    by 0x5859F8: dwarf2_build_psymtabs(objfile*) (dwarf2read.c:4407)
==8380==    by 0x498C32: read_psyms(objfile*) (elfread.c:1290)
==8380==    by 0x6703D8: require_partial_symbols(objfile*, int) (psymtab.c:87)
==8380==    by 0x6ADB21: read_symbols(objfile*, enum_flags<symfile_add_flag>) (symfile.c:883)
==8380==    by 0x6B1706: reread_symbols() (symfile.c:2631)   <<<  note _obstack_free called at reread_symbols() (symfile.c:2579)
==8380==    by 0x4389A5: remote_open_1(char const*, int, target_ops*, int) (remote.c:5021)
==8380==    by 0x437AD8: remote_open(char const*, int) (remote.c:4370)
==8380==    by 0x6D546E: open_target(char*, int, cmd_list_element*) (target.c:359)
==8380==    by 0x4662CC: do_sfunc(cmd_list_element*, char*, int) (cli-decode.c:122)
==8380==    by 0x4692F1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1887)
==8380==    by 0x6E76BF: execute_command(char*, int) (top.c:674)
==8380==    by 0x46DF03: execute_control_command(command_line*) (cli-script.c:494)
==8380==    by 0x46DD74: execute_user_command(cmd_list_element*, char*) (cli-script.c:423)
==8380==    by 0x6E7606: execute_command(char*, int) (top.c:664)
==8380==    by 0x5C392C: command_handler(char*) (event-top.c:590)
==8380==    by 0x5C3CF1: command_line_handler(char*) (event-top.c:780)
==8380==    by 0x5C32D2: gdb_rl_callback_handler(char*) (event-top.c:213)
==8380==    by 0x76C1DF: rl_callback_read_char (callback.c:220)
==8380==    by 0x5C31EB: gdb_rl_callback_read_char_wrapper_noexcept() (event-top.c:175)
==8380==    by 0x5C3261: gdb_rl_callback_read_char_wrapper(void*) (event-top.c:192)
==8380==    by 0x5C37DB: stdin_event_handler(int, void*) (event-top.c:518)
==8380==    by 0x5C1E60: handle_file_event(file_handler*, int) (event-loop.c:733)
==8380==    by 0x5C23EC: gdb_wait_for_event(int) (event-loop.c:859)
==8380==    by 0x5C1231: gdb_do_one_event() (event-loop.c:322)
==8380==    by 0x5C12E2: start_event_loop() (event-loop.c:371)
==8380==    by 0x6352DC: captured_command_loop(void*) (main.c:325)
==8380==    by 0x5C4CA6: catch_errors(int (*)(void*), void*, char const*, return_mask) (exceptions.c:236)
==8380==    by 0x6366C4: captured_main(void*) (main.c:1150)
==8380==    by 0x6366ED: gdb_main(captured_main_args*) (main.c:1160)
==8380==    by 0x4066C3: main (gdb.c:32)
==8380==  Address 0x5aaca00 is 288 bytes inside a block of size 4,064 free'd
==8380==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8380==    by 0x545CD3: xfree(void*) (common-utils.c:100)
==8380==    by 0x81C903: _obstack_free (obstack.c:280)
==8380==    by 0x6B1143: reread_symbols() (symfile.c:2579)    <<<
==8380==    by 0x4389A5: remote_open_1(char const*, int, target_ops*, int) (remote.c:5021)
==8380==    by 0x437AD8: remote_open(char const*, int) (remote.c:4370)
==8380==    by 0x6D546E: open_target(char*, int, cmd_list_element*) (target.c:359)
==8380==    by 0x4662CC: do_sfunc(cmd_list_element*, char*, int) (cli-decode.c:122)
==8380==    by 0x4692F1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1887)
==8380==    by 0x6E76BF: execute_command(char*, int) (top.c:674)
==8380==    by 0x46DF03: execute_control_command(command_line*) (cli-script.c:494)
==8380==    by 0x46DD74: execute_user_command(cmd_list_element*, char*) (cli-script.c:423)
==8380==    by 0x6E7606: execute_command(char*, int) (top.c:664)
==8380==    by 0x5C392C: command_handler(char*) (event-top.c:590)
==8380==    by 0x5C3CF1: command_line_handler(char*) (event-top.c:780)
==8380==    by 0x5C32D2: gdb_rl_callback_handler(char*) (event-top.c:213)
==8380==    by 0x76C1DF: rl_callback_read_char (callback.c:220)
==8380==    by 0x5C31EB: gdb_rl_callback_read_char_wrapper_noexcept() (event-top.c:175)
==8380==    by 0x5C3261: gdb_rl_callback_read_char_wrapper(void*) (event-top.c:192)
==8380==    by 0x5C37DB: stdin_event_handler(int, void*) (event-top.c:518)
==8380==    by 0x5C1E60: handle_file_event(file_handler*, int) (event-loop.c:733)
==8380==    by 0x5C23EC: gdb_wait_for_event(int) (event-loop.c:859)
==8380==    by 0x5C1231: gdb_do_one_event() (event-loop.c:322)
==8380==    by 0x5C12E2: start_event_loop() (event-loop.c:371)
==8380==    by 0x6352DC: captured_command_loop(void*) (main.c:325)
==8380==    by 0x5C4CA6: catch_errors(int (*)(void*), void*, char const*, return_mask) (exceptions.c:236)
==8380==    by 0x6366C4: captured_main(void*) (main.c:1150)
==8380==    by 0x6366ED: gdb_main(captured_main_args*) (main.c:1160)
==8380==    by 0x4066C3: main (gdb.c:32)
==8380== 

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Simon Marchi]

On 2017-04-25 14:16, Doug Gilmore wrote:
> Sorry for the delay, here is the trace under "valgrind 
> --num-callers=111".
> 
> Note that I marked the critical call points in reread_symbols.  In
> reread_symbols at symfile.c:2631 read_symbols is called.  Via the call
> to read_symbols, bsearch_cmp is eventually called, which "reads"
> memory that was free'd memory via the call to _obstack_free that was
> called just above in reread_symbols at symfile.c:2579.
> 
> The patch moves the call to objfiles_changed to the point obstack_free
> is called, so that free'd memory will not be referenced.
> 
> Trace attached.
> 
> Thanks,
> 
> Doug
> 
> (gdb) qrun
> `/home/dgilmore/tmp/h' has changed; re-reading symbols.
> ==8380== Invalid read of size 8
> ==8380==    at 0x653949: bsearch_cmp(void const*, void const*) 
> (objfiles.c:1415)
> ==8380==    by 0x559D247: bsearch (stdlib-bsearch.h:33)
> ==8380==    by 0x653B05: find_pc_section(unsigned long) 
> (objfiles.c:1462)
> ==8380==    by 0x649D00: lookup_minimal_symbol_by_pc(unsigned long)
> (minsyms.c:785)
> ==8380==    by 0x40852B: mips_pc_is_mips(unsigned long) 
> (mips-tdep.c:1183)
> ==8380==    by 0x4086EA: mips_adjust_dwarf2_addr(unsigned long)
> (mips-tdep.c:1271)
> ==8380==    by 0x5E55C2: gdbarch_adjust_dwarf2_addr(gdbarch*, unsigned
> long) (gdbarch.c:3417)
> ==8380==    by 0x5A3EB1: read_attribute_value(die_reader_specs const*,
> attribute*, unsigned int, long, unsigned char const*)
> (dwarf2read.c:16620)
> ==8380==    by 0x5A4823: read_attribute(die_reader_specs const*,
> attribute*, attr_abbrev*, unsigned char const*) (dwarf2read.c:16846)
> ==8380==    by 0x5A13BC: read_full_die_1(die_reader_specs const*,
> die_info**, unsigned char const*, int*, int) (dwarf2read.c:15585)
> ==8380==    by 0x5A1431: read_full_die(die_reader_specs const*,
> die_info**, unsigned char const*, int*) (dwarf2read.c:15604)
> ==8380==    by 0x5890B5: init_cutu_and_read_dies(dwarf2_per_cu_data*,
> abbrev_table*, int, int, void (*)(die_reader_specs const*, unsigned
> char const*, die_info*, int, void*), void*) (dwarf2read.c:5753)
> ==8380==    by 0x58A48F:
> process_psymtab_comp_unit(dwarf2_per_cu_data*, int, language)
> (dwarf2read.c:6265)
> ==8380==    by 0x58B0D0: dwarf2_build_psymtabs_hard(objfile*)
> (dwarf2read.c:6658)
> ==8380==    by 0x5859F8: dwarf2_build_psymtabs(objfile*) 
> (dwarf2read.c:4407)
> ==8380==    by 0x498C32: read_psyms(objfile*) (elfread.c:1290)
> ==8380==    by 0x6703D8: require_partial_symbols(objfile*, int) 
> (psymtab.c:87)
> ==8380==    by 0x6ADB21: read_symbols(objfile*,
> enum_flags<symfile_add_flag>) (symfile.c:883)
> ==8380==    by 0x6B1706: reread_symbols() (symfile.c:2631)   <<<  note
> _obstack_free called at reread_symbols() (symfile.c:2579)
> ==8380==    by 0x4389A5: remote_open_1(char const*, int, target_ops*,
> int) (remote.c:5021)
> ==8380==    by 0x437AD8: remote_open(char const*, int) (remote.c:4370)
> ==8380==    by 0x6D546E: open_target(char*, int, cmd_list_element*)
> (target.c:359)
> ==8380==    by 0x4662CC: do_sfunc(cmd_list_element*, char*, int)
> (cli-decode.c:122)
> ==8380==    by 0x4692F1: cmd_func(cmd_list_element*, char*, int)
> (cli-decode.c:1887)
> ==8380==    by 0x6E76BF: execute_command(char*, int) (top.c:674)
> ==8380==    by 0x46DF03: execute_control_command(command_line*)
> (cli-script.c:494)
> ==8380==    by 0x46DD74: execute_user_command(cmd_list_element*,
> char*) (cli-script.c:423)
> ==8380==    by 0x6E7606: execute_command(char*, int) (top.c:664)
> ==8380==    by 0x5C392C: command_handler(char*) (event-top.c:590)
> ==8380==    by 0x5C3CF1: command_line_handler(char*) (event-top.c:780)
> ==8380==    by 0x5C32D2: gdb_rl_callback_handler(char*) 
> (event-top.c:213)
> ==8380==    by 0x76C1DF: rl_callback_read_char (callback.c:220)
> ==8380==    by 0x5C31EB: gdb_rl_callback_read_char_wrapper_noexcept()
> (event-top.c:175)
> ==8380==    by 0x5C3261: gdb_rl_callback_read_char_wrapper(void*)
> (event-top.c:192)
> ==8380==    by 0x5C37DB: stdin_event_handler(int, void*) 
> (event-top.c:518)
> ==8380==    by 0x5C1E60: handle_file_event(file_handler*, int)
> (event-loop.c:733)
> ==8380==    by 0x5C23EC: gdb_wait_for_event(int) (event-loop.c:859)
> ==8380==    by 0x5C1231: gdb_do_one_event() (event-loop.c:322)
> ==8380==    by 0x5C12E2: start_event_loop() (event-loop.c:371)
> ==8380==    by 0x6352DC: captured_command_loop(void*) (main.c:325)
> ==8380==    by 0x5C4CA6: catch_errors(int (*)(void*), void*, char
> const*, return_mask) (exceptions.c:236)
> ==8380==    by 0x6366C4: captured_main(void*) (main.c:1150)
> ==8380==    by 0x6366ED: gdb_main(captured_main_args*) (main.c:1160)
> ==8380==    by 0x4066C3: main (gdb.c:32)
> ==8380==  Address 0x5aaca00 is 288 bytes inside a block of size 4,064 
> free'd
> ==8380==    at 0x4C2BDEC: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==8380==    by 0x545CD3: xfree(void*) (common-utils.c:100)
> ==8380==    by 0x81C903: _obstack_free (obstack.c:280)
> ==8380==    by 0x6B1143: reread_symbols() (symfile.c:2579)    <<<
> ==8380==    by 0x4389A5: remote_open_1(char const*, int, target_ops*,
> int) (remote.c:5021)
> ==8380==    by 0x437AD8: remote_open(char const*, int) (remote.c:4370)
> ==8380==    by 0x6D546E: open_target(char*, int, cmd_list_element*)
> (target.c:359)
> ==8380==    by 0x4662CC: do_sfunc(cmd_list_element*, char*, int)
> (cli-decode.c:122)
> ==8380==    by 0x4692F1: cmd_func(cmd_list_element*, char*, int)
> (cli-decode.c:1887)
> ==8380==    by 0x6E76BF: execute_command(char*, int) (top.c:674)
> ==8380==    by 0x46DF03: execute_control_command(command_line*)
> (cli-script.c:494)
> ==8380==    by 0x46DD74: execute_user_command(cmd_list_element*,
> char*) (cli-script.c:423)
> ==8380==    by 0x6E7606: execute_command(char*, int) (top.c:664)
> ==8380==    by 0x5C392C: command_handler(char*) (event-top.c:590)
> ==8380==    by 0x5C3CF1: command_line_handler(char*) (event-top.c:780)
> ==8380==    by 0x5C32D2: gdb_rl_callback_handler(char*) 
> (event-top.c:213)
> ==8380==    by 0x76C1DF: rl_callback_read_char (callback.c:220)
> ==8380==    by 0x5C31EB: gdb_rl_callback_read_char_wrapper_noexcept()
> (event-top.c:175)
> ==8380==    by 0x5C3261: gdb_rl_callback_read_char_wrapper(void*)
> (event-top.c:192)
> ==8380==    by 0x5C37DB: stdin_event_handler(int, void*) 
> (event-top.c:518)
> ==8380==    by 0x5C1E60: handle_file_event(file_handler*, int)
> (event-loop.c:733)
> ==8380==    by 0x5C23EC: gdb_wait_for_event(int) (event-loop.c:859)
> ==8380==    by 0x5C1231: gdb_do_one_event() (event-loop.c:322)
> ==8380==    by 0x5C12E2: start_event_loop() (event-loop.c:371)
> ==8380==    by 0x6352DC: captured_command_loop(void*) (main.c:325)
> ==8380==    by 0x5C4CA6: catch_errors(int (*)(void*), void*, char
> const*, return_mask) (exceptions.c:236)
> ==8380==    by 0x6366C4: captured_main(void*) (main.c:1150)
> ==8380==    by 0x6366ED: gdb_main(captured_main_args*) (main.c:1160)
> ==8380==    by 0x4066C3: main (gdb.c:32)
> ==8380==

Hi Doug,

I've ran this in my head many times, but I still don't understand which 
field exactly is stale and causes the segfault.

According to the backtrace, the line of the crash is:

   if (pc < obj_section_addr (section))

That macro expands to

#define obj_section_addr(s)				      		\
   (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)		\
    + obj_section_offset (s))

which further expands to

#define obj_section_offset(s)						\
   (((s)->objfile->section_offsets)->offsets[gdb_bfd_section_index 
((s)->objfile->obfd, (s)->the_bfd_section)])


Could you point out which dereferencing operator/memory access causes 
the crash?  At first I thought it would be ->section_offsets, but that 
field is set properly before calling read_symbols:

	  /* We use the same section offsets as from last time.  I'm not
	     sure whether that is always correct for shared libraries.  */
	  objfile->section_offsets = (struct section_offsets *)
	    obstack_alloc (&objfile->objfile_obstack,
			   SIZEOF_N_SECTION_OFFSETS (num_offsets));
	  memcpy (objfile->section_offsets, offsets,
		  SIZEOF_N_SECTION_OFFSETS (num_offsets));
	  objfile->num_sections = num_offsets;

so it should not be the culprit...  The s variable itself should point 
to an instance of obj_section, contained in the 
objfile_pspace_info::sections array.  This one is allocated with 
XNEWVEC, so it shouldn't be affected by the fact that we clear the 
obstack.

The objfile object itself doesn't change address, so the pointers to it 
should still be valid...

I find the obj_section_addr and obj_section_offset very bad for 
readability and debuggability.  Could you change them for inline 
functions that are not one liners?  Then it will be obvious in the 
backtrace which operation causes the crash.

Thanks,

Simon

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

> ...
> Hi Doug,
> 
> I've ran this in my head many times, but I still don't understand which field exactly is stale and causes the segfault.
> 
> According to the backtrace, the line of the crash is:
> 
>   if (pc < obj_section_addr (section))
> 
> That macro expands to
> 
> #define obj_section_addr(s)                              \
>   (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)        \
>    + obj_section_offset (s))
> 
> which further expands to
> 
> #define obj_section_offset(s)                        \
>   (((s)->objfile->section_offsets)->offsets[gdb_bfd_section_index ((s)->objfile->obfd, (s)->the_bfd_section)])
> 
> 
> Could you point out which dereferencing operator/memory access causes the crash?  At first I thought it would be ->section_offsets, but that field is set properly before calling read_symbols:
> 
>       /* We use the same section offsets as from last time.  I'm not
>          sure whether that is always correct for shared libraries.  */
>       objfile->section_offsets = (struct section_offsets *)
>         obstack_alloc (&objfile->objfile_obstack,
>                SIZEOF_N_SECTION_OFFSETS (num_offsets));
>       memcpy (objfile->section_offsets, offsets,
>           SIZEOF_N_SECTION_OFFSETS (num_offsets));
>       objfile->num_sections = num_offsets;
> 
> so it should not be the culprit...  The s variable itself should point to an instance of obj_section, contained in the objfile_pspace_info::sections array.  This one is allocated with XNEWVEC, so it shouldn't be affected by the fact that we clear the obstack.
> 
> The objfile object itself doesn't change address, so the pointers to it should still be valid...
> 
> I find the obj_section_addr and obj_section_offset very bad for readability and debuggability.  Could you change them for inline functions that are not one liners?  Then it will be obvious in the backtrace which operation causes the crash.
> 
> Thanks,
> 
> Simon
Hi Simon,

After thinking about it my comment and code placement wasn't
particularly good.  Something along the line's of Luis's change
is better.

Does Luis's comment address the question you have?

If so, Luis: Should is it OK we incorporate your changes in the patch?

I attached a diff for the change.

Thanks,

Doug


diff --git a/gdb/symfile.c b/gdb/symfile.c
index 846aabe..aff4341 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2628,6 +2628,20 @@ reread_symbols (void)
 	  clear_complaints (&symfile_complaints, 1, 1);
 
 	  objfile->flags &= ~OBJF_PSYMTABS_READ;
+
+	  /* We are about to read new symbols and potentially also DWARF
+	     information.  Some targets may want to pass addresses read from
+	     DWARF DIE's through an adjustment function before saving them, like
+	     MIPS, which may call into "find_pc_section".  When called, that
+	     function will make use of per-objfile program space data.
+
+	     Since we discarded our section information above, we have dangling
+	     pointers in the per-objfile program space data structure.  Force
+	     GDB to update the section mapping information by letting it know
+	     the objfile has changed, making the dangling pointers point to
+	     correct data again.  */
+	  objfiles_changed ();
+
 	  read_symbols (objfile, 0);
 
 	  if (!objfile_has_symbols (objfile))
@@ -2660,9 +2674,6 @@ reread_symbols (void)
 
   if (!new_objfiles.empty ())
     {
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
-- 
1.9.1

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Luis Machado]

On 04/28/2017 06:44 PM, Doug Gilmore wrote:
>> ...
>>       /* We use the same section offsets as from last time.  I'm not
>>          sure whether that is always correct for shared libraries.  */
>>       objfile->section_offsets = (struct section_offsets *)
>>         obstack_alloc (&objfile->objfile_obstack,
>>                SIZEOF_N_SECTION_OFFSETS (num_offsets));
>>       memcpy (objfile->section_offsets, offsets,
>>           SIZEOF_N_SECTION_OFFSETS (num_offsets));
>>       objfile->num_sections = num_offsets;
>>
>> so it should not be the culprit...  The s variable itself should point to an instance of obj_section, contained in the objfile_pspace_info::sections array.  This one is allocated with XNEWVEC, so it shouldn't be affected by the fact that we clear the obstack.
>>
>> The objfile object itself doesn't change address, so the pointers to it should still be valid...
>>
>> I find the obj_section_addr and obj_section_offset very bad for readability and debuggability.  Could you change them for inline functions that are not one liners?  Then it will be obvious in the backtrace which operation causes the crash.
>>
>> Thanks,
>>
>> Simon
> Hi Simon,
>
> After thinking about it my comment and code placement wasn't
> particularly good.  Something along the line's of Luis's change
> is better.
>
> Does Luis's comment address the question you have?
>
> If so, Luis: Should is it OK we incorporate your changes in the patch?

Sure. I have no problems with that.

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Simon Marchi]

On 2017-04-28 19:44, Doug Gilmore wrote:
> Hi Simon,
> 
> After thinking about it my comment and code placement wasn't
> particularly good.  Something along the line's of Luis's change
> is better.
> 
> Does Luis's comment address the question you have?
> 
> If so, Luis: Should is it OK we incorporate your changes in the patch?
> 
> I attached a diff for the change.
> 
> Thanks,
> 
> Doug

Hi Doug,

The comment certainly helps, but in the commit log I'd like to see a 
more detailed list of events that leads to the crash.

Now that I look into it again, I think I understand.  The 
objfile_pspace_info::sections array/vector is a list of obj_section 
pointers (in C++ we'd probably use an std::vector<obj_section*>).  That 
list contains pointers to all the sections from all the objfiles sorted 
in order of increasing address.  They point directly to the sections 
allocated by the objfile in their obstacks (and accessible through 
objfile::sections).  So when the obstack is freed in reread_symbols, the 
sorted list contains stale pointers.  Is that it?

If that's what's happening, then I'm more convinced the fix is right.  
Is this behaviour caught by a test?  If not, could you write one?

> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 846aabe..aff4341 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2628,6 +2628,20 @@ reread_symbols (void)
>  	  clear_complaints (&symfile_complaints, 1, 1);
> 
>  	  objfile->flags &= ~OBJF_PSYMTABS_READ;
> +
> +	  /* We are about to read new symbols and potentially also DWARF
> +	     information.  Some targets may want to pass addresses read from
> +	     DWARF DIE's through an adjustment function before saving them, 
> like
> +	     MIPS, which may call into "find_pc_section".  When called, that
> +	     function will make use of per-objfile program space data.

If you are talking about objfile_pspace_info, it's not per-objfile.  
There's one instance of it per program space, so it's actually 
"objfiles-related per-program-space data".  It contains the sorted list 
of all sections of all objfiles loaded in the pspace.

> +
> +	     Since we discarded our section information above, we have 
> dangling
> +	     pointers in the per-objfile program space data structure.  Force

again

> +	     GDB to update the section mapping information by letting it know
> +	     the objfile has changed, making the dangling pointers point to
> +	     correct data again.  */
> +	  objfiles_changed ();
> +
>  	  read_symbols (objfile, 0);
> 
>  	  if (!objfile_has_symbols (objfile))
> @@ -2660,9 +2674,6 @@ reread_symbols (void)
> 
>    if (!new_objfiles.empty ())
>      {
> -      /* Notify objfiles that we've modified objfile sections.  */
> -      objfiles_changed ();
> -
>        clear_symtab_users (0);
> 
>        /* clear_objfile_data for each objfile was called before freeing 
> it and

Thanks,

Simon

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 04/28/17 18:41, Simon Marchi wrote:
> On 2017-04-28 19:44, Doug Gilmore wrote:
>> Hi Simon,
>>
>> After thinking about it my comment and code placement wasn't
>> particularly good.  Something along the line's of Luis's change
>> is better.
>>
>> Does Luis's comment address the question you have?
>>
>> If so, Luis: Should is it OK we incorporate your changes in the patch?
>>
>> I attached a diff for the change.
>>
>> Thanks,
>>
>> Doug
> 
> Hi Doug,
> 
> The comment certainly helps, but in the commit log I'd like to see a
> more detailed list of events that leads to the crash.
> 
> Now that I look into it again, I think I understand.  The
> objfile_pspace_info::sections array/vector is a list of obj_section
> pointers (in C++ we'd probably use an std::vector<obj_section*>).
> That list contains pointers to all the sections from all the
> objfiles sorted in order of increasing address.  They point directly
> to the sections allocated by the objfile in their obstacks (and
> accessible through objfile::sections).  So when the obstack is freed
> in reread_symbols, the sorted list contains stale pointers.  Is that
> it?
Right.
> 
> If that's what's happening, then I'm more convinced the fix is
> right.  Is this behaviour caught by a test?  If not, could you write
> one?
> ...
I'll need to take a look.  Last time I tried I it was more difficult
to expose the problem on the native build of GDB.

Thanks,

Doug

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Simon Marchi]

On 2017-04-29 13:12, Doug Gilmore wrote:
> On 04/28/17 18:41, Simon Marchi wrote:
>> On 2017-04-28 19:44, Doug Gilmore wrote:
>>> Hi Simon,
>>> 
>>> After thinking about it my comment and code placement wasn't
>>> particularly good.  Something along the line's of Luis's change
>>> is better.
>>> 
>>> Does Luis's comment address the question you have?
>>> 
>>> If so, Luis: Should is it OK we incorporate your changes in the 
>>> patch?
>>> 
>>> I attached a diff for the change.
>>> 
>>> Thanks,
>>> 
>>> Doug
>> 
>> Hi Doug,
>> 
>> The comment certainly helps, but in the commit log I'd like to see a
>> more detailed list of events that leads to the crash.
>> 
>> Now that I look into it again, I think I understand.  The
>> objfile_pspace_info::sections array/vector is a list of obj_section
>> pointers (in C++ we'd probably use an std::vector<obj_section*>).
>> That list contains pointers to all the sections from all the
>> objfiles sorted in order of increasing address.  They point directly
>> to the sections allocated by the objfile in their obstacks (and
>> accessible through objfile::sections).  So when the obstack is freed
>> in reread_symbols, the sorted list contains stale pointers.  Is that
>> it?
> Right.
>> 
>> If that's what's happening, then I'm more convinced the fix is
>> right.  Is this behaviour caught by a test?  If not, could you write
>> one?
>> ...
> I'll need to take a look.  Last time I tried I it was more difficult
> to expose the problem on the native build of GDB.

reread_symbols is called when using the run (run_command_1), attach 
(attach_post_wait which then calls setup_inferior) and load 
(load_command) commands.  So maybe something like this would reproduce 
it?

- compile test program
- launch gdb with test program
- touch test program
- run

Simon

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

>>> ...
>> I'll need to take a look.  Last time I tried I it was more difficult
>> to expose the problem on the native build of GDB.
> 
> reread_symbols is called when using the run (run_command_1), attach (attach_post_wait which then calls setup_inferior) and load (load_command) commands.  So maybe something like this would reproduce it?
> 
> - compile test program
> - launch gdb with test program
> - touch test program
> - run
> 
> Simon
> 
Hi Simon,

What I meant was that when I previously did tests with the native
build, for some reason the freed data was not being overwritten, or
possible written with the same data, at the time read_symbols was
called in reread_symbols.  Thus problem wasn't exposed before the
objfiles_changed is eventually called in reread_symbols.

I just did a test with a native build of gdb (7.9.1) and the problem
was exposed, so chances are it will be also be exposed with a ToT
build.

Doug

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 04/29/2017 10:14 PM, Doug Gilmore wrote:
>
> >>> ...
> >> I'll need to take a look.  Last time I tried I it was more difficult
> >> to expose the problem on the native build of GDB.
> >
> > reread_symbols is called when using the run (run_command_1), attach (attach_post_wait which then calls setup_inferior) and load (load_command) commands.  So maybe something like this would reproduce it?
> >
> > - compile test program
> > - launch gdb with test program
> > - touch test program
> > - run
> >
> > Simon
> >
> Hi Simon,
>
> What I meant was that when I previously did tests with the native
> build, for some reason the freed data was not being overwritten, or
> possible written with the same data, at the time read_symbols was
> called in reread_symbols.  Thus problem wasn't exposed before the
> objfiles_changed is eventually called in reread_symbols.
>
> I just did a test with a native build of gdb (7.9.1) and the problem
> was exposed, so chances are it will be also be exposed with a ToT
> build.
>
> Doug
>
It tooks some experimentation, but I found a quick, and it appears
reliable, way to reproduce the problem natively.

I have attached a new patch with commit with a detailed explanation
for the commit log entry.

OK to apply?

Thanks,

Doug

Fix PR 21337: segfault when re-reading symbols.

Fix issue exposed by commit 3e29f34.

The basic issue is that section data referenced through an objfile
pointer can also be referenced via the program-space data pointer,
although via a separate mapping mechanism, which is set up by
update_section_map.  Thus once section data attached to an objfile
pointer is released, the section map associated with the program-space
data pointer must be marked dirty to ensure that update_section_map is
called to prevent stale data being referenced.  For the matter at hand
this marking is being done via a call to objfiles_changed.

Before commit 3e29f34 objfiles_changed could be called after all of
the objfile pointers were processed in reread_symbols since section
data references via the program-space data pointer would not occur in
the calls of read_symbols performed by reread_symbols.

With commit 3e29f34 MIPS target specific calls to find_pc_section were
added to the code for DWARF information processing, which is called
via read_symbols.  Thus in reread_symbols the call to objfiles_changed
needs to be called before calling read_symbols, otherwise stale
section data can be referenced.

Thanks to Luis Machado for providing text for the main comment
associated with the change.

gdb/
2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>
    * symfile.c (reread_symbols): Fix PR 21337.

gdb/testsuite
2017-??-??  Doug Gilmore  <Doug Gilmore@rDoug.Gilmore@imgtec.com>
    PR gdb/r21337
    * gdb.base/pr21337.c: New file.
    * gdb.base/pr21337.exp: New file.
    * gdb.base/pr21337.gdb: New file.
---
 gdb/symfile.c                      | 20 +++++++++++--
 gdb/testsuite/gdb.base/pr21337.c   |  4 +++
 gdb/testsuite/gdb.base/pr21337.exp | 57 ++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/pr21337.gdb | 13 +++++++++
 4 files changed, 91 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/pr21337.c
 create mode 100644 gdb/testsuite/gdb.base/pr21337.exp
 create mode 100644 gdb/testsuite/gdb.base/pr21337.gdb

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 846aabe..0492f56 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2576,6 +2576,9 @@ reread_symbols (void)
       /* Free the obstacks for non-reusable objfiles.  */
       psymbol_bcache_free (objfile->psymbol_cache);
       objfile->psymbol_cache = psymbol_bcache_init ();
+
+      /* NB: after this call to obstack_free, objfiles_changed
+         will need to be called (see discussion below).  */
       obstack_free (&objfile->objfile_obstack, 0);
       objfile->sections = NULL;
       objfile->compunit_symtabs = NULL;
@@ -2628,6 +2631,20 @@ reread_symbols (void)
       clear_complaints (&symfile_complaints, 1, 1);
 
       objfile->flags &= ~OBJF_PSYMTABS_READ;
+
+      /* We are about to read new symbols and potentially also DWARF
+         information.  Some targets may want to pass addresses read from
+         DWARF DIE's through an adjustment function before saving them, like
+         MIPS, which may call into "find_pc_section".  When called, that
+         function will make use of per-objfile program space data.
+
+         Since we discarded our section information above, we have dangling
+         pointers in the per-objfile program space data structure.  Force
+         GDB to update the section mapping information by letting it know
+         the objfile has changed, making the dangling pointers point to
+         correct data again.  */
+      objfiles_changed ();
+
       read_symbols (objfile, 0);
 
       if (!objfile_has_symbols (objfile))
@@ -2660,9 +2677,6 @@ reread_symbols (void)
 
   if (!new_objfiles.empty ())
     {
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
diff --git a/gdb/testsuite/gdb.base/pr21337.c b/gdb/testsuite/gdb.base/pr21337.c
new file mode 100644
index 0000000..f8b643a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr21337.c
@@ -0,0 +1,4 @@
+int main()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/pr21337.exp b/gdb/testsuite/gdb.base/pr21337.exp
new file mode 100644
index 0000000..d7718b4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr21337.exp
@@ -0,0 +1,57 @@
+# Copyright 1998-2017 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/>.
+
+set prototypes 1
+
+# build the test case
+
+set testfile "pr21337"
+set srcfile ${testfile}.c
+# Cygwin needs $EXEEXT.
+set binfile [standard_output_file ${testfile}$EXEEXT]
+# set binfile ${testfile}
+
+set gdbfile [standard_output_file ${testfile}.gdb]
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } {
+    untested "failed to compile first testcase"
+    return -1
+}
+
+# Using the source command to read commands from a file is important,
+# otherwise section data is freed and reallocated using the same
+# memory locations and the bug is not exposed.
+
+set ifd [open "$srcdir/$subdir/${testfile}.gdb" r]
+set ofd [open $gdbfile w]
+while {[gets $ifd line] >= 0} {
+    regsub $testfile $line $binfile tmp
+    puts $ofd $tmp
+}
+close $ifd
+close $ofd
+
+gdb_start
+
+if [is_remote target] {
+    unsupported $test
+} else {
+    gdb_test "source $gdbfile" ".*source-command-completed.*" "source $testfile.gdb"
+    gdb_test "source $gdbfile" ".*source-command-completed.*" "source $testfile.gdb"
+}
+
+# End of tests.
+
+return 0
diff --git a/gdb/testsuite/gdb.base/pr21337.gdb b/gdb/testsuite/gdb.base/pr21337.gdb
new file mode 100644
index 0000000..42fac26
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr21337.gdb
@@ -0,0 +1,13 @@
+file pr21337
+shell sleep 1; touch pr21337
+run
+file
+file pr21337
+shell sleep 1; touch pr21337
+run
+file
+file pr21337
+shell sleep 1; touch pr21337
+run
+file
+p "source-command-completed"
-- 
1.9.1

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Simon Marchi]

Hi Doug,

On 2017-05-10 19:26, Doug Gilmore wrote:
> The basic issue is that section data referenced through an objfile
> pointer can also be referenced via the program-space data pointer,
> although via a separate mapping mechanism, which is set up by
> update_section_map.  Thus once section data attached to an objfile
> pointer is released, the section map associated with the program-space
> data pointer must be marked dirty to ensure that update_section_map is
> called to prevent stale data being referenced.  For the matter at hand
> this marking is being done via a call to objfiles_changed.
> 
> Before commit 3e29f34 objfiles_changed could be called after all of
> the objfile pointers were processed in reread_symbols since section
> data references via the program-space data pointer would not occur in
> the calls of read_symbols performed by reread_symbols.
> 
> With commit 3e29f34 MIPS target specific calls to find_pc_section were
> added to the code for DWARF information processing, which is called
> via read_symbols.  Thus in reread_symbols the call to objfiles_changed
> needs to be called before calling read_symbols, otherwise stale
> section data can be referenced.
> 
> Thanks to Luis Machado for providing text for the main comment
> associated with the change.

Thanks for the commit log.

> gdb/
> 2017-??-??  Doug Gilmore  <Doug.Gilmore@Doug.Gilmore@imgtec.com>

Your email address has twice the username part (in both ChangeLog 
entries).

>     * symfile.c (reread_symbols): Fix PR 21337.

This should state more precisely what actually changed:

         * symfile.c (reread_symbols): Call objfiles_changed just before 
read_symbols.

> 
> gdb/testsuite
> 2017-??-??  Doug Gilmore  <Doug Gilmore@rDoug.Gilmore@imgtec.com>
>     PR gdb/r21337

Spurious "r" before the PR number.

>     * gdb.base/pr21337.c: New file.
>     * gdb.base/pr21337.exp: New file.
>     * gdb.base/pr21337.gdb: New file.
> ---
>  gdb/symfile.c                      | 20 +++++++++++--
>  gdb/testsuite/gdb.base/pr21337.c   |  4 +++
>  gdb/testsuite/gdb.base/pr21337.exp | 57 
> ++++++++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/pr21337.gdb | 13 +++++++++
>  4 files changed, 91 insertions(+), 3 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/pr21337.c
>  create mode 100644 gdb/testsuite/gdb.base/pr21337.exp
>  create mode 100644 gdb/testsuite/gdb.base/pr21337.gdb
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 846aabe..0492f56 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2576,6 +2576,9 @@ reread_symbols (void)
>        /* Free the obstacks for non-reusable objfiles.  */
>        psymbol_bcache_free (objfile->psymbol_cache);
>        objfile->psymbol_cache = psymbol_bcache_init ();
> +
> +      /* NB: after this call to obstack_free, objfiles_changed
> +         will need to be called (see discussion below).  */
> 
>        obstack_free (&objfile->objfile_obstack, 0);
>        objfile->sections = NULL;
>        objfile->compunit_symtabs = NULL;
> @@ -2628,6 +2631,20 @@ reread_symbols (void)
>        clear_complaints (&symfile_complaints, 1, 1);
> 
>        objfile->flags &= ~OBJF_PSYMTABS_READ;
> +
> +      /* We are about to read new symbols and potentially also DWARF
> +         information.  Some targets may want to pass addresses read 
> from
> +         DWARF DIE's through an adjustment function before saving 
> them, like
> +         MIPS, which may call into "find_pc_section".  When called, 
> that
> +         function will make use of per-objfile program space data.
> +
> +         Since we discarded our section information above, we have 
> dangling
> +         pointers in the per-objfile program space data structure.  
> Force
> +         GDB to update the section mapping information by letting it 
> know
> +         the objfile has changed, making the dangling pointers point 
> to
> +         correct data again.  */
> +      objfiles_changed ();
> +
>        read_symbols (objfile, 0);
> 
>        if (!objfile_has_symbols (objfile))
> 
> @@ -2660,9 +2677,6 @@ reread_symbols (void)
> 
>    if (!new_objfiles.empty ())
>      {
> -      /* Notify objfiles that we've modified objfile sections.  */
> -      objfiles_changed ();
> -
>        clear_symtab_users (0);
> 
>        /* clear_objfile_data for each objfile was called before freeing 
> it and
> diff --git a/gdb/testsuite/gdb.base/pr21337.c 
> b/gdb/testsuite/gdb.base/pr21337.c
> new file mode 100644
> index 0000000..f8b643a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr21337.c
> @@ -0,0 +1,4 @@
> +int main()

Nit: to match the coding style of GDB:

int
main ()

> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/pr21337.exp
> b/gdb/testsuite/gdb.base/pr21337.exp
> new file mode 100644
> index 0000000..d7718b4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr21337.exp

Even though there are a few instances of this, we no longer name the 
test cases using the PR number.  Instead, let's try to find a relevant 
name.

Also, you seem to have based your test case on an old file that does not 
quite meet today's standards (especially the initial setup/build part).  
For an example of test that follows today's practices, look at 
gdb.base/step-over-exit.exp, for example.

> @@ -0,0 +1,57 @@
> +# Copyright 1998-2017 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/>.
> +
> +set prototypes 1

What does this do?

> +
> +# build the test case
> +
> +set testfile "pr21337"
> +set srcfile ${testfile}.c
> +# Cygwin needs $EXEEXT.
> +set binfile [standard_output_file ${testfile}$EXEEXT]
> +# set binfile ${testfile}

This should be replaced with a call to standard_testfile.

> +
> +set gdbfile [standard_output_file ${testfile}.gdb]
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" 
> executable {debug nowarnings}] != "" } {

No need for "nowarnings".

> +    untested "failed to compile first testcase"
> +    return -1
> +}
> +
> +# Using the source command to read commands from a file is important,
> +# otherwise section data is freed and reallocated using the same
> +# memory locations and the bug is not exposed.

I don't really know what's different when sourcing a file than typing 
commands.  But if your experience is that sourcing reproduces the crash 
reliably while sending commands does not, I'm fine with that.

> +set ifd [open "$srcdir/$subdir/${testfile}.gdb" r]
> +set ofd [open $gdbfile w]
> +while {[gets $ifd line] >= 0} {
> +    regsub $testfile $line $binfile tmp
> +    puts $ofd $tmp
> +}
> +close $ifd
> +close $ofd

Would it be simpler to have a procedure to generate the .gdb file with 
the right filename from the start?  Something like:

proc generate_cmd_file { } {
     set ofd [open $gdbfile w]

     puts $ofd "file ${binfile}"
     puts $ofd "shell sleep 1; touch ${binfile}"
     ...
}

> +
> +gdb_start

What is your intent in calling gdb_start?  I think what you want is just 
to start GDB itself, so that it can source the command file below.  If 
so, you should call clean_restart:

   clear_restart ${binfile}

Among other things, this handles the extended-remote target (calling 
"set remote exec-file"), which this test case should be able to support. 
  As in gdb.base/step-over-exit.exp, you can call prepare_for_testing 
which does the build + clean_restart for you.

> +
> +if [is_remote target] {

Since your test relies on being able to "run" a program, the right check 
would be [use_gdb_stub]:

   if [use_gdb_stub] {

> +    unsupported $test
> +} else {
> +    gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> $testfile.gdb"
> +    gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> $testfile.gdb"

Why is it needed to source the file twice?

> +}
> +
> +# End of tests.
> +
> +return 0

This comment and return statement are not necessary.

> diff --git a/gdb/testsuite/gdb.base/pr21337.gdb
> b/gdb/testsuite/gdb.base/pr21337.gdb
> new file mode 100644
> index 0000000..42fac26
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr21337.gdb
> @@ -0,0 +1,13 @@
> +file pr21337
> +shell sleep 1; touch pr21337
> +run
> +file
> +file pr21337
> +shell sleep 1; touch pr21337
> +run
> +file
> +file pr21337
> +shell sleep 1; touch pr21337
> +run
> +file
> +p "source-command-completed"

Thanks for adding a test case!

Simon

Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 05/11/2017 08:29 PM, Simon Marchi wrote:
> Hi Doug,
>
> On 2017-05-10 19:26, Doug Gilmore wrote:
> > The basic issue is that section data referenced through an objfile
> > pointer can also be referenced via the program-space data pointer,
> > although via a separate mapping mechanism, which is set up by
> > update_section_map.  Thus once section data attached to an objfile
> > pointer is released, the section map associated with the program-space
> > data pointer must be marked dirty to ensure that update_section_map is
> > called to prevent stale data being referenced.  For the matter at hand
> > this marking is being done via a call to objfiles_changed.
> >
> > Before commit 3e29f34 objfiles_changed could be called after all of
> > the objfile pointers were processed in reread_symbols since section
> > data references via the program-space data pointer would not occur in
> > the calls of read_symbols performed by reread_symbols.
> >
> > With commit 3e29f34 MIPS target specific calls to find_pc_section were
> > added to the code for DWARF information processing, which is called
> > via read_symbols.  Thus in reread_symbols the call to objfiles_changed
> > needs to be called before calling read_symbols, otherwise stale
> > section data can be referenced.
> >
> > Thanks to Luis Machado for providing text for the main comment
> > associated with the change.
>
> Thanks for the commit log.
>
>..
> Your email address has twice the username part (in both ChangeLog entries).
>
>..
> This should state more precisely what actually changed:
>
>         * symfile.c (reread_symbols): Call objfiles_changed just before read_symbols.
>
> Spurious "r" before the PR number.
>..
Hi Simon

Whoops, thanks will update accordingly.
>
> ..
> Nit: to match the coding style of GDB:
>
> int
> main ()
OK
> ...
> Also, you seem to have based your test case on an old file that does
> not quite meet today's standards (especially the initial setup/build
> part).  For an example of test that follows today's practices, look
> at gdb.base/step-over-exit.exp, for example.
Sounds good, I'll take a look.
>
> ...
> > +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } {
>
> No need for "nowarnings".
OK
> I don't really know what's different when sourcing a file than
> typing commands.  But if your experience is that sourcing reproduces
> the crash reliably while sending commands does not, I'm fine with
> that.
Right, it is really helpful in reproducing the issue reliably.
> ...

> Would it be simpler to have a procedure to generate the .gdb file with the right filename from the start?  Something like:
>
> proc generate_cmd_file { } {
>     set ofd [open $gdbfile w]
>
>     puts $ofd "file ${binfile}"
>     puts $ofd "shell sleep 1; touch ${binfile}"
>     ...
> }
OK
>
> > +
> > +gdb_start
> ...
>
> What is your intent in calling gdb_start?  I think what you want is
> just to start GDB itself, so that it can source the command file
> below.  If so, you should call clean_restart:
> clear_restart ${binfile}
OK
> Among other things, this handles the extended-remote target (calling
> "set remote exec-file"), which this test case should be able to
> support.  As in gdb.base/step-over-exit.exp, you can call
> prepare_for_testing which does the build + clean_restart for you.
OK
>
> > +
> > +if [is_remote target] {
>
> Since your test relies on being able to "run" a program, the right check would be [use_gdb_stub]:
>
>   if [use_gdb_stub] {
>
> > +    unsupported $test
> > +} else {
> > +    gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> > $testfile.gdb"
> > +    gdb_test "source $gdbfile" ".*source-command-completed.*" "source
> > $testfile.gdb"
>
> Why is it needed to source the file twice?
In some situations I have seen the failure to occur only on the second invocation.
>
> > +}
> > +
> > +# End of tests.
> > +
> > +return 0
>
> This comment and return statement are not necessary.
OK
>
> ..
>
> Thanks for adding a test case!
>
> Simon
Will update the patch per your comments.

Thanks,

Doug

Re: [PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 05/11/2017 08:29 PM, Simon Marchi wrote:
> Hi Doug,
> 
> On 2017-05-10 19:26, Doug Gilmore wrote:
>> The basic issue is that section data referenced through an objfile
>> ....
> 
> Thanks for the commit log.
> ...
> 
> Thanks for adding a test case!
> 
> Simon

Hi Simon,

I attached an updated patch, which also fails with
--target_board=native-extended-gdbserver, though I haven't come up
with a test for --target_board=native-gdbserver, so the test is
basically just a NOOP in that situation.

I think I resolved all of the issues.  Does this look OK?

Thanks,

Doug

Fix PR 21337: segfault when re-reading symbols.

Fix issue exposed by commit 3e29f34.

The basic issue is that section data referenced through an objfile
pointer can also be referenced via the program-space data pointer,
although via a separate mapping mechanism, which is set up by
update_section_map.  Thus once section data attached to an objfile
pointer is released, the section map associated with the program-space
data pointer must be marked dirty to ensure that update_section_map is
called to prevent stale data being referenced.  For the matter at hand
this marking is being done via a call to objfiles_changed.

Before commit 3e29f34 objfiles_changed could be called after all of
the objfile pointers were processed in reread_symbols since section
data references via the program-space data pointer would not occur in
the calls of read_symbols performed by reread_symbols.

With commit 3e29f34 MIPS target specific calls to find_pc_section were
added to the code for DWARF information processing, which is called
via read_symbols.  Thus in reread_symbols the call to objfiles_changed
needs to be called before calling read_symbols, otherwise stale
section data can be referenced.

Thanks to Luis Machado for providing text for the main comment
associated with the change.

gdb/
2017-??-??  Doug Gilmore <Doug.Gilmore@imgtec.com>
    PR gdb/21337.
    * symfile.c (reread_symbols): Call objfiles_changed just before
    read_symbols.

gdb/testsuite
2017-??-??  Doug Gilmore <Doug Gilmore@imgtec.com>
    PR gdb/21337
    * gdb.base/reread-readsym.exp: New file.
    * gdb.base/reread-readsym.c: New file.
---
 gdb/symfile.c                             | 20 ++++++++--
 gdb/testsuite/gdb.base/reread-readsym.c   | 22 +++++++++++
 gdb/testsuite/gdb.base/reread-readsym.exp | 61 +++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/reread-readsym.c
 create mode 100644 gdb/testsuite/gdb.base/reread-readsym.exp

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 846aabe..0492f56 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2576,6 +2576,9 @@ reread_symbols (void)
 	  /* Free the obstacks for non-reusable objfiles.  */
 	  psymbol_bcache_free (objfile->psymbol_cache);
 	  objfile->psymbol_cache = psymbol_bcache_init ();
+
+	  /* NB: after this call to obstack_free, objfiles_changed
+	     will need to be called (see discussion below).  */
 	  obstack_free (&objfile->objfile_obstack, 0);
 	  objfile->sections = NULL;
 	  objfile->compunit_symtabs = NULL;
@@ -2628,6 +2631,20 @@ reread_symbols (void)
 	  clear_complaints (&symfile_complaints, 1, 1);
 
 	  objfile->flags &= ~OBJF_PSYMTABS_READ;
+
+	  /* We are about to read new symbols and potentially also DWARF
+	     information.  Some targets may want to pass addresses read from
+	     DWARF DIE's through an adjustment function before saving them, like
+	     MIPS, which may call into "find_pc_section".  When called, that
+	     function will make use of per-objfile program space data.
+
+	     Since we discarded our section information above, we have dangling
+	     pointers in the per-objfile program space data structure.  Force
+	     GDB to update the section mapping information by letting it know
+	     the objfile has changed, making the dangling pointers point to
+	     correct data again.  */
+	  objfiles_changed ();
+
 	  read_symbols (objfile, 0);
 
 	  if (!objfile_has_symbols (objfile))
@@ -2660,9 +2677,6 @@ reread_symbols (void)
 
   if (!new_objfiles.empty ())
     {
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
diff --git a/gdb/testsuite/gdb.base/reread-readsym.c b/gdb/testsuite/gdb.base/reread-readsym.c
new file mode 100644
index 0000000..2fee696
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reread-readsym.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2017 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 (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/reread-readsym.exp b/gdb/testsuite/gdb.base/reread-readsym.exp
new file mode 100644
index 0000000..6cdb541
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reread-readsym.exp
@@ -0,0 +1,61 @@
+#   Copyright 2017 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/>.
+
+standard_testfile
+
+set gdbfile [standard_output_file ${testfile}.gdb]
+
+# Test rereading executable.  See PR gdb/21337.
+
+proc generate_cmd_file {gdbfile binfile} {
+    set ofd [open $gdbfile w]
+
+    puts $ofd "file ${binfile}"
+    puts $ofd "shell sleep 1; touch ${binfile}"
+    puts $ofd "run"
+    puts $ofd "file"
+    puts $ofd "file ${binfile}"
+    puts $ofd "shell sleep 1; touch ${binfile}"
+    puts $ofd "run"
+    puts $ofd "file"
+    puts $ofd "file ${binfile}"
+    puts $ofd "shell sleep 1; touch ${binfile}"
+    puts $ofd "run"
+    puts $ofd "file"
+    puts $ofd "p \"source-command-completed\""
+    close $ofd
+}
+
+if [use_gdb_stub] {
+    return 0
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+
+# Using the source command to read commands from a file is important,
+# otherwise section data is freed and reallocated using the same
+# memory locations and the bug is not exposed.
+generate_cmd_file $gdbfile $binfile
+
+gdb_test "source $gdbfile" ".*source-command-completed.*" \
+    "source $testfile.gdb"
+# Sometimes the failure only occurs on the second invocation.
+gdb_test "source $gdbfile" ".*source-command-completed.*" \
+    "source $testfile.gdb"
-- 
1.9.1

Re: [PING][PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

https://sourceware.org/ml/gdb-patches/2017-05/msg00360.html

Ping

Doug

Re: [PING^2][PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 06/06/2017 09:08 AM, Doug Gilmore wrote:
> https://sourceware.org/ml/gdb-patches/2017-05/msg00360.html
> 
> Ping
> 
> Doug
> 
Ping^2

Re: [PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging.

[Simon Marchi]

Hi Doug,

Good thing you pinged, I had completely missed it, sorry for the wait.

+gdb_test "source $gdbfile" ".*source-command-completed.*" \
+    "source $testfile.gdb"
+# Sometimes the failure only occurs on the second invocation.
+gdb_test "source $gdbfile" ".*source-command-completed.*" \
+    "source $testfile.gdb"

Tests should have unique names, so that when one fails, you know easily 
which one.  So, perhaps "source $testfile.gdb 1" and "source 
$testfile.gdb 2"?

The patch is ok with this fixed.

Thanks,

Simon

Re: [PATCH] Fix PR 21337 (v4): segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 06/25/2017 04:24 AM, Simon Marchi wrote:
> Hi Doug,
> 
> Good thing you pinged, I had completely missed it, sorry for the wait.
> 
> +gdb_test "source $gdbfile" ".*source-command-completed.*" \
> +    "source $testfile.gdb"
> +# Sometimes the failure only occurs on the second invocation.
> +gdb_test "source $gdbfile" ".*source-command-completed.*" \
> +    "source $testfile.gdb"
> 
> Tests should have unique names, so that when one fails, you know easily which one.  So, perhaps "source $testfile.gdb 1" and "source $testfile.gdb 2"?
> 
> The patch is ok with this fixed.
> 
> Thanks,
> 
> Simon
Hi Simon and Maciej,

Simon: I updated the tests per your request.  When I asked Maciej to
commit the patch for me, he noticed that the comment extended over the
soft 72 column limit of, so I reformatted it.  Also I corrected a typo
and format issues in the changelog entries that he noticed.

Thanks,

Doug

Fix PR 21337: segfault when re-reading symbols.

Fix issue exposed by commit 3e29f34.

The basic issue is that section data referenced through an objfile
pointer can also be referenced via the program-space data pointer,
although via a separate mapping mechanism, which is set up by
update_section_map.  Thus once section data attached to an objfile
pointer is released, the section map associated with the program-space
data pointer must be marked dirty to ensure that update_section_map is
called to prevent stale data being referenced.  For the matter at hand
this marking is being done via a call to objfiles_changed.

Before commit 3e29f34 objfiles_changed could be called after all of
the objfile pointers were processed in reread_symbols since section
data references via the program-space data pointer would not occur in
the calls of read_symbols performed by reread_symbols.

With commit 3e29f34 MIPS target specific calls to find_pc_section were
added to the code for DWARF information processing, which is called
via read_symbols.  Thus in reread_symbols the call to objfiles_changed
needs to be called before calling read_symbols, otherwise stale
section data can be referenced.

Thanks to Luis Machado for providing text for the main comment
associated with the change.

gdb/
2017-??-??  Doug Gilmore  <Doug.Gilmore@imgtec.com>
    PR gdb/21337.
    * symfile.c (reread_symbols): Call objfiles_changed just before
    read_symbols.

gdb/testsuite
2017-??-??  Doug Gilmore  <Doug.Gilmore@imgtec.com>
    PR gdb/21337
    * gdb.base/reread-readsym.exp: New file.
    * gdb.base/reread-readsym.c: New file.

diff --git a/gdb/symfile.c b/gdb/symfile.c
index aa53415..ce37390 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2587,6 +2587,9 @@ reread_symbols (void)
 	  /* Free the obstacks for non-reusable objfiles.  */
 	  psymbol_bcache_free (objfile->psymbol_cache);
 	  objfile->psymbol_cache = psymbol_bcache_init ();
+
+	  /* NB: after this call to obstack_free, objfiles_changed
+	     will need to be called (see discussion below).  */
 	  obstack_free (&objfile->objfile_obstack, 0);
 	  objfile->sections = NULL;
 	  objfile->compunit_symtabs = NULL;
@@ -2639,6 +2642,23 @@ reread_symbols (void)
 	  clear_complaints (&symfile_complaints, 1, 1);
 
 	  objfile->flags &= ~OBJF_PSYMTABS_READ;
+
+	  /* We are about to read new symbols and potentially also
+	     DWARF information.  Some targets may want to pass addresses
+	     read from DWARF DIE's through an adjustment function before
+	     saving them, like MIPS, which may call into
+	     "find_pc_section".  When called, that function will make
+	     use of per-objfile program space data.
+
+	     Since we discarded our section information above, we have
+	     dangling pointers in the per-objfile program space data
+	     structure.  Force GDB to update the section mapping
+	     information by letting it know the objfile has changed,
+	     making the dangling pointers point to correct data
+	     again.  */
+	     
+	  objfiles_changed ();
+
 	  read_symbols (objfile, 0);
 
 	  if (!objfile_has_symbols (objfile))
@@ -2671,9 +2691,6 @@ reread_symbols (void)
 
   if (!new_objfiles.empty ())
     {
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
-
       clear_symtab_users (0);
 
       /* clear_objfile_data for each objfile was called before freeing it and
diff --git a/gdb/testsuite/gdb.base/reread-readsym.c b/gdb/testsuite/gdb.base/reread-readsym.c
new file mode 100644
index 0000000..2fee696
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reread-readsym.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2017 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 (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/reread-readsym.exp b/gdb/testsuite/gdb.base/reread-readsym.exp
new file mode 100644
index 0000000..b69eaad
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reread-readsym.exp
@@ -0,0 +1,61 @@
+#   Copyright 2017 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/>.
+
+standard_testfile
+
+set gdbfile [standard_output_file ${testfile}.gdb]
+
+# Test rereading executable.  See PR gdb/21337.
+
+proc generate_cmd_file {gdbfile binfile} {
+    set ofd [open $gdbfile w]
+
+    puts $ofd "file ${binfile}"
+    puts $ofd "shell sleep 1; touch ${binfile}"
+    puts $ofd "run"
+    puts $ofd "file"
+    puts $ofd "file ${binfile}"
+    puts $ofd "shell sleep 1; touch ${binfile}"
+    puts $ofd "run"
+    puts $ofd "file"
+    puts $ofd "file ${binfile}"
+    puts $ofd "shell sleep 1; touch ${binfile}"
+    puts $ofd "run"
+    puts $ofd "file"
+    puts $ofd "p \"source-command-completed\""
+    close $ofd
+}
+
+if [use_gdb_stub] {
+    return 0
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+
+# Using the source command to read commands from a file is important,
+# otherwise section data is freed and reallocated using the same
+# memory locations and the bug is not exposed.
+generate_cmd_file $gdbfile $binfile
+
+gdb_test "source $gdbfile" ".*source-command-completed.*" \
+    "source $testfile.gdb 1"
+# Sometimes the failure only occurs on the second invocation.
+gdb_test "source $gdbfile" ".*source-command-completed.*" \
+    "source $testfile.gdb 2"
-- 
1.9.1

Re: [PATCH] Fix PR 21337 (v4): segfault when re-reading symbols with remote debugging.

[Doug Gilmore]

On 06/27/2017 10:28 AM, Doug Gilmore wrote:
> On 06/25/2017 04:24 AM, Simon Marchi wrote:
>> Hi Doug,
>>
>> Good thing you pinged, I had completely missed it, sorry for the wait.
>>
>> +gdb_test "source $gdbfile" ".*source-command-completed.*" \
>> +    "source $testfile.gdb"
>> +# Sometimes the failure only occurs on the second invocation.
>> +gdb_test "source $gdbfile" ".*source-command-completed.*" \
>> +    "source $testfile.gdb"
>>
>> Tests should have unique names, so that when one fails, you know easily which one.  So, perhaps "source $testfile.gdb 1" and "source $testfile.gdb 2"?
>>
>> The patch is ok with this fixed.
>>
>> Thanks,
>>
>> Simon
> Hi Simon and Maciej,
> 
> Simon: I updated the tests per your request.  When I asked Maciej to
> commit the patch for me, he noticed that the comment extended over the
> soft 72 column limit of, so I reformatted it.  Also I corrected a typo
> and format issues in the changelog entries that he noticed.
> 
> Thanks,
> 
> Doug
Also I forgot to mention per Maciej's request, that my work is covered
by a copyright assignment to FSF.

Doug

Re: [PATCH] Fix PR 21337 (v4): segfault when re-reading symbols with remote debugging.

[Maciej W. Rozycki]

On Tue, 27 Jun 2017, Doug Gilmore wrote:

> Also I forgot to mention per Maciej's request, that my work is covered
> by a copyright assignment to FSF.

 Thanks for clarifying it.  I have committed your change, having removed 
an issue that I didn't previously notice with trailing white space added 
to symfile.c (and which GIT complained about at patch application).

 Thank you for your contribution to the GDB project.

  Maciej