Re: [PATCH 1/2] Debugging without a binary (regression)

[Luis Machado <lgustavo@codesourcery.com>]

On 02/24/2016 10:06 PM, Pedro Alves wrote:
> On 02/24/2016 02:26 PM, Luis Machado wrote:
>> When we attempt to debug a process using GDBserver in standard remote mode
>> without a symbol file on GDB's end, we may run into an issue where GDB cuts
>> the connection attempt short due to an error. The error is caused by not
>> being able to open a symbol file, like so:
>>
>> --
>>
>> (gdb) set sysroot
>> (gdb) tar rem :2345
>> Remote debugging using :2345
>> /proc/23769/exe: Permission denied.
>> (gdb) i r
>> The program has no registers now.
>> (gdb)
>>
>> It should've been like this:
>>
>> (gdb) set sysroot
>> (gdb) tar rem :2345
>> Remote debugging using :2345
>> 0xf7ddb2d0 in ?? ()
>> (gdb) i r
>> eax            0x0  0
>> ecx            0x0  0
>> edx            0x0  0
>> ebx            0x0  0
>> esp            0xffffdfa0 0xffffdfa0
>> ebp            0x0  0x0
>> esi            0x0  0
>> edi            0x0  0
>> eip            0xf7ddb2d0 0xf7ddb2d0
>> eflags         0x200  [ IF ]
>> cs             0x33 51
>> ss             0x2b 43
>> ds             0x0  0
>> es             0x0  0
>> fs             0x0  0
>> gs             0x0  0
>> (gdb)
>>
>> This is caused by a couple of function calls within exec_file_locate_attach
>> that can potentially throw errors.
>>
>> The following patch guards both exec_file_attach and symbol_file_add_main to
>> prevent the errors from disrupting the connection process.
>>
>> There was also a case where native GDB tripped on this problem, but it was
>> mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d.
>>
>> Regression-tested on x86-64/Ubuntu.
>>
>> gdb/ChangeLog:
>>
>> 2016-02-24  Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* exec.c (exec_file_locate_attach): Guard a couple functions
>> 	that can throw errors.
>> ---
>>   gdb/exec.c | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/exec.c b/gdb/exec.c
>> index 90811c0..3f77b3d 100644
>> --- a/gdb/exec.c
>> +++ b/gdb/exec.c
>> @@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty)
>>
>>     old_chain = make_cleanup (xfree, full_exec_path);
>>
>> -  exec_file_attach (full_exec_path, from_tty);
>> -  symbol_file_add_main (full_exec_path, from_tty);
>> +  /* exec_file_attach and symbol_file_add_main may throw an error if the file
>> +     cannot be opened either locally or remotely.  This happens when, for
>> +     example, GDB connects to a GDBserver that is running on a different
>> +     filesystem and the sysroot is set to non-target-based (no "target:").
>
> The second sentence no longer makes sense after Gary's change.
>
> I think it should read:
>
>    This happens for example, when the file is first found in the local
>    sysroot (above), and then disappears (a TOCTOU race), or when it doesn't
>    exist in the target filesystem, or when the file does exist, but
>    is not readable.
>

Sounds good. Updated in the next version.

>> +
>> +     Then GDB will neither load the binary from the target nor be able to
>> +     load a binary from the local filesystem (it may not exist in the local
>> +     filesystem in the same path as in the remote filesystem).
>
> No longer makes sense either.
>

Updated as well.

>> +
>> +     Even without a binary, the remote-based debugging session should
>> +     continue normally instead of ending abruptly.  Hence we catch thrown
>> +     errors/exceptions in the following code.  */
>> +  TRY
>> +    {
>> +      exec_file_attach (full_exec_path, from_tty);
>> +    }
>> +  CATCH (err, RETURN_MASK_ALL)
>
> This should be RETURN_MASK_ERROR instead.  Silently swallowing ctrl-c is
> rarely the right thing to do, if ever.  A ctrl-c during the
> initial remote connection is supposed to bring down the connection
> immediately:

Fixed

>> +    {
>> +    }
>> +  END_CATCH
>> +
>> +  TRY
>> +    {
>> +      symbol_file_add_main (full_exec_path, from_tty);
>> +    }
>> +  CATCH (err, RETURN_MASK_ALL)
>> +    {
>> +    }
>
> Throwing is bad, but, should we warn, including
> the exception message?

That's what i originally did with a single try/catch block, but i 
noticed with two of them we are prone to having two warnings of the same 
kind, one after the other. So i ended up leaving them empty.

For example:

warning: 
/home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: 
Permission denied.^M
warning: 
/home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: 
Permission denied.^M

Shouldn't we go back to a single block? Checking if both exceptions' 
messages match seems a bit too much.