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

[Luis Machado <lgustavo@codesourcery.com>]

On 04/13/2016 10:00 AM, Pedro Alves wrote:
> On 04/12/2016 08:24 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 ?? ()
>
> The warning output is missing here, right?
>

It is, because i did not update it after the patch. :-)

Should be fixed in v3.

>> (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)
>>
>
>>   void
>> @@ -142,6 +161,7 @@ exec_file_locate_attach (int pid, int from_tty)
>>   {
>>     char *exec_file, *full_exec_path = NULL;
>>     struct cleanup *old_chain;
>> +  struct gdb_exception prev_err;
>>
>>     /* Do nothing if we already have an executable filename.  */
>>     exec_file = (char *) get_exec_file (0);
>> @@ -182,9 +202,46 @@ 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 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.
>> +
>> +     Even without a symbol file, 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_ERROR)
>> +    {
>> +      if (err.message != NULL)
>> +	warning ("%s", err.message);
>> +
>> +      prev_err = err;
>> +
>> +      /* Save message so it doesn't get trashed by the catch problem
>
> "the catch problem below" should be either the "the caught
> problem below", or "the catch below", I think.
>

Yeah, i got things mixed up when coming up with it. "the catch below" is 
what i wanted to say.

>> +	 below.  */
>> +      prev_err.message = xstrdup (err.message);
>> +    }
>> +  END_CATCH
>> +
>> +  TRY
>> +    {
>> +      symbol_file_add_main (full_exec_path, from_tty);
>> +    }
>> +  CATCH (err, RETURN_MASK_ERROR)
>> +    {
>> +      if (!exception_print_same (prev_err, err))
>> +	warning ("%s", err.message);
>> +    }
>> +  END_CATCH
>>
>> +  xfree ((void *) prev_err.message);
>
> This will reference an uninitialized prev_err.message if the
> first TRY doesn't throw.  Initialize it with:
>
>   struct gdb_exception prev_err = exception_none;
>
>

Done.

> Also, it will leak prev_err.message on Ctrl-C/QUIT, which is
> not caught by RETURN_MASK_ERROR.  prev_err.message needs to be
> freed with a cleanup.  Say:
>
>    struct gdb_exception prev_err = exception_none;
>
>    old_chain = make_cleanup (xfree, full_exec_path);
>
>    make_cleanup (free_current_contents, &prev_err.message);
>
>    TRY
>      {
>         ...
>      }
>    CATCH (err, RETURN_MASK_ERROR)
>      {
>        if (err.message != NULL)
> 	warning ("%s", err.message);
>
>        prev_err = err;
>        prev_err.message = xstrdup (err.message);
>      }
>     ...
>
>    do_cleanups (old_chain);
>

Thanks. I've fixed these. I'll get v3 out once the testcase gets reviewed.

> Thanks,
> Pedro Alves
>