Re: [RFC][PATCH] fix gdb segv when objfile can't be opened

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-10-19 11:59, Mike Gulick wrote:
> I apologize for the improperly formatted patch -- I'm really struggling
> to get thunderbird to behave as I want.
>
> Here is an updated patch.  I would have sent it with git send-email, 
> but
> I could not figure out the proper way to add this preface before the
> patch (without it looking like part of the commit message).

Hi Mike,

Thanks, I was able to apply this version correctly.

If I have a short comment that's not meant to be in the commit message, 
I usually
include it in brackets like this:

  [Re-sending this patch because the first try was not formatted 
correctly.]

If it's longer you can always end it with a line "Actual commit 
message:".  Either way, it's not really a big deal, as long as it's 
clear.  You can use the --annotate option of git-send-email to edit the 
message before sending it.

> ---
> From 5dee04076518554e4baae864569d6f4faee9b685 Mon Sep 17 00:00:00 2001
> From: Mike Gulick <mgulick@mathworks.com>
> Date: Wed, 18 Oct 2017 16:04:27 -0400
> Subject: [PATCH] fix gdb segv when objfile can't be opened
>
> This fixes PR 16577.
>
> This patch changes gdb_bfd_map_section to issue a warning rather than 
> an
> error if it is unable to read the object file, and sets the size of the
> section/frame that it attempted to read to 0 on error.
>
> The description of gdb_bfd_map_section states that it will try to read
> or map the contents of the section SECT, and if successful, the section
> data is returned and *SIZE is set to the size of the section data.  
> This
> function was throwing an error and leaving *size as-is.  Setting the
> section size to 0 indicates to dwarf2_build_frame_info that there is no
> data to read, otherwise it will try to read from an invalid frame
> pointer.
>
> Changing the error to a warning allows this to be handled gracefully.
> Additionally, the error was clobbering the breakpoint output indicating
> the current frame (function name, arguments, source file, and line 
> number).
> E.g.
>
> Thread 3 "foo" hit Breakpoint 1, BFD: reopening
> /tmp/jna-1013829440/jna2973250704389291330.tmp: No such file or
> directory
>
> BFD: reopening /tmp/jna-1013829440/jna2973250704389291330.tmp: No such
> file or directory

For some reason, I am not able to reproduce the crash using the 
instructions in the bug report, and gdb master.

(gdb) up
#1  0x00007ffff78d525a in sleep () from /lib/x86_64-linux-gnu/libc.so.6
(gdb)
BFD: reopening ./badlib.so: No such file or directory

BFD: reopening ./badlib.so: No such file or directory

Can't read data for section '.eh_frame' in file './badlib.so'
(gdb)
Initial frame selected; you cannot go up.
(gdb)
Initial frame selected; you cannot go up.
(gdb)
Initial frame selected; you cannot go up.
(gdb) bt
#0  0x00007ffff78d52f0 in nanosleep () from 
/lib/x86_64-linux-gnu/libc.so.6
#1  0x00007ffff78d525a in sleep () from /lib/x86_64-linux-gnu/libc.so.6


Would you be able to create a test case to reproduce it?  We would need 
one to go in with the fix in the end anyway, and it's easier for 
reviewers if they can just run a test file rather than try to reproduce 
by hand.  You can start by copying an existing solib test, like 
gdb.base/solib-display.exp.  See here for more details about tests:

http://sourceware.org/gdb/wiki/TestingGDB
http://sourceware.org/gdb/wiki/GDBTestcaseCookbook

Don't hesitate to ask here or on IRC if you need assistance.

> (gdb)
>
> While the "BFD: reopening ..." messages will still appear interspersed 
> in the
> breakpoint output, the current frame info is now displayed:
>
> Thread 3 "foo" hit Breakpoint 1, BFD: reopening
> /tmp/jna-1013829440/jna1875755897659885075.tmp: No such file or
> directory
>
> BFD: reopening /tmp/jna-1013829440/jna1875755897659885075.tmp: No such
> file or directory
>
> warning: Can't read data for section '.eh_frame' in file
> '/tmp/jna-1013829440/jna1875755897659885075.tmp'
> do_something () at file.cpp:80
> 80	{
> (gdb)
> ---
>  gdb/gdb_bfd.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 29080b8..229f5ae 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -705,9 +705,15 @@ gdb_bfd_map_section (asection *sectp, 
> bfd_size_type *size)
>
>    data = NULL;
>    if (!bfd_get_full_section_contents (abfd, sectp, &data))
> -    error (_("Can't read data for section '%s' in file '%s'"),
> -	   bfd_get_section_name (abfd, sectp),
> -	   bfd_get_filename (abfd));
> +    {
> +      warning (_("Can't read data for section '%s' in file '%s'"),
> +	       bfd_get_section_name (abfd, sectp),
> +	       bfd_get_filename (abfd));
> +      /* Section is invalid -- set size to 0 and return NULL */
> +      descriptor->size = 0;
> +      *size = descriptor->size;
> +      return (const gdb_byte *) NULL;
> +    }
>    descriptor->data = data;
>
>   done:

I don't know if it is really this function's responsibility to clear 
*size in case of error, or it would be the callers responsibility to 
properly check for errors.  But if the function doesn't throw anymore, 
the comment in gdb_bfd.h should be updated accordingly.

Thanks,

Simon