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

[Doug Gilmore <Doug dot Gilmore at imgtec dot com>]

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