diff mbox

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

Message ID 3c5ce0a0-72e5-4460-5555-ad2214866260@imgtec.com
State New, archived
Headers

Commit Message

Doug Gilmore April 13, 2017, 6:56 p.m. UTC 
  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(-)

Comments

Luis Machado April 14, 2017, 3:32 p.m. UTC | #1 
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
Simon Marchi April 22, 2017, 2:15 a.m. UTC | #2 
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
Doug Gilmore April 25, 2017, 6:16 p.m. UTC | #3 
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==
Simon Marchi April 27, 2017, 7:46 p.m. UTC | #4 
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
diff mbox

Patch

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