* [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging. @ 2017-03-31 23:04 Doug Gilmore 2017-04-10 16:01 ` Doug Gilmore 2017-04-12 18:52 ` Luis Machado 0 siblings, 2 replies; 25+ messages in thread From: Doug Gilmore @ 2017-03-31 23:04 UTC (permalink / raw) To: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging. 2017-03-31 23:04 [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging Doug Gilmore @ 2017-04-10 16:01 ` Doug Gilmore 2017-04-12 18:52 ` Luis Machado 1 sibling, 0 replies; 25+ messages in thread From: Doug Gilmore @ 2017-04-10 16:01 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging. 2017-03-31 23:04 [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging Doug Gilmore 2017-04-10 16:01 ` Doug Gilmore @ 2017-04-12 18:52 ` Luis Machado 2017-04-12 21:42 ` Doug Gilmore 1 sibling, 1 reply; 25+ messages in thread From: Luis Machado @ 2017-04-12 18:52 UTC (permalink / raw) To: Doug Gilmore, gdb-patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging. 2017-04-12 18:52 ` Luis Machado @ 2017-04-12 21:42 ` Doug Gilmore 2017-04-13 18:56 ` [PATCH] Fix PR 21337 v2: " Doug Gilmore 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-04-12 21:42 UTC (permalink / raw) To: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-12 21:42 ` Doug Gilmore @ 2017-04-13 18:56 ` Doug Gilmore 2017-04-14 15:33 ` Luis Machado 2017-04-22 2:15 ` Simon Marchi 0 siblings, 2 replies; 25+ messages in thread From: Doug Gilmore @ 2017-04-13 18:56 UTC (permalink / raw) To: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-13 18:56 ` [PATCH] Fix PR 21337 v2: " Doug Gilmore @ 2017-04-14 15:33 ` Luis Machado 2017-04-22 2:15 ` Simon Marchi 1 sibling, 0 replies; 25+ messages in thread From: Luis Machado @ 2017-04-14 15:33 UTC (permalink / raw) To: Doug Gilmore, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-13 18:56 ` [PATCH] Fix PR 21337 v2: " Doug Gilmore 2017-04-14 15:33 ` Luis Machado @ 2017-04-22 2:15 ` Simon Marchi 2017-04-25 18:16 ` Doug Gilmore 1 sibling, 1 reply; 25+ messages in thread From: Simon Marchi @ 2017-04-22 2:15 UTC (permalink / raw) To: Doug Gilmore; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-22 2:15 ` Simon Marchi @ 2017-04-25 18:16 ` Doug Gilmore 2017-04-27 19:46 ` Simon Marchi 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-04-25 18:16 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches 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== ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-25 18:16 ` Doug Gilmore @ 2017-04-27 19:46 ` Simon Marchi 2017-04-28 23:44 ` Doug Gilmore 0 siblings, 1 reply; 25+ messages in thread From: Simon Marchi @ 2017-04-27 19:46 UTC (permalink / raw) To: Doug Gilmore; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-27 19:46 ` Simon Marchi @ 2017-04-28 23:44 ` Doug Gilmore 2017-04-29 1:13 ` Luis Machado 2017-04-29 1:42 ` Simon Marchi 0 siblings, 2 replies; 25+ messages in thread From: Doug Gilmore @ 2017-04-28 23:44 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches > ... > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-28 23:44 ` Doug Gilmore @ 2017-04-29 1:13 ` Luis Machado 2017-04-29 1:42 ` Simon Marchi 1 sibling, 0 replies; 25+ messages in thread From: Luis Machado @ 2017-04-29 1:13 UTC (permalink / raw) To: Doug Gilmore, Simon Marchi; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-28 23:44 ` Doug Gilmore 2017-04-29 1:13 ` Luis Machado @ 2017-04-29 1:42 ` Simon Marchi 2017-04-29 17:12 ` Doug Gilmore 1 sibling, 1 reply; 25+ messages in thread From: Simon Marchi @ 2017-04-29 1:42 UTC (permalink / raw) To: Doug Gilmore; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-29 1:42 ` Simon Marchi @ 2017-04-29 17:12 ` Doug Gilmore 2017-04-29 23:26 ` Simon Marchi 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-04-29 17:12 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-29 17:12 ` Doug Gilmore @ 2017-04-29 23:26 ` Simon Marchi 2017-04-30 5:14 ` Doug Gilmore 0 siblings, 1 reply; 25+ messages in thread From: Simon Marchi @ 2017-04-29 23:26 UTC (permalink / raw) To: Doug Gilmore; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-29 23:26 ` Simon Marchi @ 2017-04-30 5:14 ` Doug Gilmore 2017-05-10 23:26 ` Doug Gilmore 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-04-30 5:14 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches >>> ... >> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-04-30 5:14 ` Doug Gilmore @ 2017-05-10 23:26 ` Doug Gilmore 2017-05-12 3:29 ` Simon Marchi 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-05-10 23:26 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-05-10 23:26 ` Doug Gilmore @ 2017-05-12 3:29 ` Simon Marchi 2017-05-12 19:24 ` Doug Gilmore 2017-05-16 23:11 ` [PATCH] Fix PR 21337 (v3): " Doug Gilmore 0 siblings, 2 replies; 25+ messages in thread From: Simon Marchi @ 2017-05-12 3:29 UTC (permalink / raw) To: Doug Gilmore; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. 2017-05-12 3:29 ` Simon Marchi @ 2017-05-12 19:24 ` Doug Gilmore 2017-05-16 23:11 ` [PATCH] Fix PR 21337 (v3): " Doug Gilmore 1 sibling, 0 replies; 25+ messages in thread From: Doug Gilmore @ 2017-05-12 19:24 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging. 2017-05-12 3:29 ` Simon Marchi 2017-05-12 19:24 ` Doug Gilmore @ 2017-05-16 23:11 ` Doug Gilmore 2017-06-06 16:08 ` [PING][PATCH] " Doug Gilmore 2017-06-25 11:25 ` [PATCH] " Simon Marchi 1 sibling, 2 replies; 25+ messages in thread From: Doug Gilmore @ 2017-05-16 23:11 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PING][PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging. 2017-05-16 23:11 ` [PATCH] Fix PR 21337 (v3): " Doug Gilmore @ 2017-06-06 16:08 ` Doug Gilmore 2017-06-23 18:20 ` [PING^2][PATCH] " Doug Gilmore 2017-06-25 11:25 ` [PATCH] " Simon Marchi 1 sibling, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-06-06 16:08 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches https://sourceware.org/ml/gdb-patches/2017-05/msg00360.html Ping Doug ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PING^2][PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging. 2017-06-06 16:08 ` [PING][PATCH] " Doug Gilmore @ 2017-06-23 18:20 ` Doug Gilmore 0 siblings, 0 replies; 25+ messages in thread From: Doug Gilmore @ 2017-06-23 18:20 UTC (permalink / raw) To: Simon Marchi; +Cc: Luis Machado, gdb-patches On 06/06/2017 09:08 AM, Doug Gilmore wrote: > https://sourceware.org/ml/gdb-patches/2017-05/msg00360.html > > Ping > > Doug > Ping^2 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 (v3): segfault when re-reading symbols with remote debugging. 2017-05-16 23:11 ` [PATCH] Fix PR 21337 (v3): " Doug Gilmore 2017-06-06 16:08 ` [PING][PATCH] " Doug Gilmore @ 2017-06-25 11:25 ` Simon Marchi 2017-06-27 17:29 ` [PATCH] Fix PR 21337 (v4): " Doug Gilmore 1 sibling, 1 reply; 25+ messages in thread From: Simon Marchi @ 2017-06-25 11:25 UTC (permalink / raw) To: Doug Gilmore; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 (v4): segfault when re-reading symbols with remote debugging. 2017-06-25 11:25 ` [PATCH] " Simon Marchi @ 2017-06-27 17:29 ` Doug Gilmore 2017-06-27 21:35 ` Doug Gilmore 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-06-27 17:29 UTC (permalink / raw) To: Simon Marchi, Maciej W. Rozycki; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 (v4): segfault when re-reading symbols with remote debugging. 2017-06-27 17:29 ` [PATCH] Fix PR 21337 (v4): " Doug Gilmore @ 2017-06-27 21:35 ` Doug Gilmore 2017-06-28 2:01 ` Maciej W. Rozycki 0 siblings, 1 reply; 25+ messages in thread From: Doug Gilmore @ 2017-06-27 21:35 UTC (permalink / raw) To: Simon Marchi, Maciej W. Rozycki; +Cc: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Fix PR 21337 (v4): segfault when re-reading symbols with remote debugging. 2017-06-27 21:35 ` Doug Gilmore @ 2017-06-28 2:01 ` Maciej W. Rozycki 0 siblings, 0 replies; 25+ messages in thread From: Maciej W. Rozycki @ 2017-06-28 2:01 UTC (permalink / raw) To: Doug Gilmore; +Cc: Simon Marchi, Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-06-28 2:01 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-31 23:04 [PATCH] [mips] Fix PR 21337 v1: segfault when re-reading symbols with remote debugging Doug Gilmore 2017-04-10 16:01 ` Doug Gilmore 2017-04-12 18:52 ` Luis Machado 2017-04-12 21:42 ` Doug Gilmore 2017-04-13 18:56 ` [PATCH] Fix PR 21337 v2: " Doug Gilmore 2017-04-14 15:33 ` Luis Machado 2017-04-22 2:15 ` Simon Marchi 2017-04-25 18:16 ` Doug Gilmore 2017-04-27 19:46 ` Simon Marchi 2017-04-28 23:44 ` Doug Gilmore 2017-04-29 1:13 ` Luis Machado 2017-04-29 1:42 ` Simon Marchi 2017-04-29 17:12 ` Doug Gilmore 2017-04-29 23:26 ` Simon Marchi 2017-04-30 5:14 ` Doug Gilmore 2017-05-10 23:26 ` Doug Gilmore 2017-05-12 3:29 ` Simon Marchi 2017-05-12 19:24 ` Doug Gilmore 2017-05-16 23:11 ` [PATCH] Fix PR 21337 (v3): " Doug Gilmore 2017-06-06 16:08 ` [PING][PATCH] " Doug Gilmore 2017-06-23 18:20 ` [PING^2][PATCH] " Doug Gilmore 2017-06-25 11:25 ` [PATCH] " Simon Marchi 2017-06-27 17:29 ` [PATCH] Fix PR 21337 (v4): " Doug Gilmore 2017-06-27 21:35 ` Doug Gilmore 2017-06-28 2:01 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).