This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/4] [nto] Fixes for nto procfs.
- From: Pedro Alves <palves at redhat dot com>
- To: Aleksandar Ristovski <aristovski at qnx dot com>, gdb-patches at sourceware dot org
- Date: Tue, 20 Oct 2015 14:21:49 +0100
- Subject: Re: [PATCH 2/4] [nto] Fixes for nto procfs.
- Authentication-results: sourceware.org; auth=none
- References: <1444752074-878-1-git-send-email-aristovski at qnx dot com> <1444752074-878-3-git-send-email-aristovski at qnx dot com> <561FE53A dot 1050406 at redhat dot com> <56263704 dot 6000007 at qnx dot com>
On 10/20/2015 01:43 PM, Aleksandar Ristovski wrote:
> On 15-10-15 01:41 PM, Pedro Alves wrote:
>> It would have been nicer to see this split into a fix/theme
>> per patch, and add something to the commit log about each
>> fix. E.g., the aux bits could easily be a separate patch.
>>
>> Anyway, this is pretty isolated to NTO bits.
>
> I'm trying to catch up and submit local changes for previous ports.
That shouldn't prevent splitting up changes per logical units.
>
> While patches may not be minimalistic, I am trying to at least bring
> certain rounded-up improvement (e.g. having a debug session).
The procfs_pid_to_exec_file change here doesn't look like the sort
that would be necessary for plainly having a debug session,
for instance.
AFAICS, there are a few different changes here:
- Implementing procfs_pid_to_exec_file, so that "attach" can
work without having the user specify an executable.
- Some accommodations for different procfs paths.
- Reading TARGET_OBJECT_AUXV off the stack.
- Some minor fixes here and there.
My trouble, along with having these all mixed up in a single
patch, is that as is there no clue on why these changes
are necessary at all (either in comments or in the commit log).
>
> But I will try to make more granulated patches.
>
Thanks.
>>
>> LGTM with the nits below addressed.
>>
>> On 10/13/2015 05:01 PM, Aleksandar Ristovski wrote:
>>
>>> }
>>>
>>> do_cleanups (inner_cleanup);
>>> @@ -599,9 +612,40 @@ procfs_files_info (struct target_ops *ignore)
>>>
>>> printf_unfiltered ("\tUsing the running image of %s %s via %s.\n",
>>> inf->attach_flag ? "attached" : "child",
>>> - target_pid_to_str (inferior_ptid), nto_procfs_path);
>>> + target_pid_to_str (inferior_ptid),
>>> + nodestr ? nodestr : "local node");
>>
>> Write 'nodestr != NULL'.
>
> Done. Here and other places where pointer is used as a logical expression.
> ...
>>> + if (rd <= 0)
>>> + {
>>> + proc_path[0] = '\0';
>>> + return NULL;
>>> + }
>>> + else
>>> + proc_path[rd] = '\0';
>>> +
>>> + return proc_path;
>>
>> Either write:
>>
>> else
>> {
>> proc_path[rd] = '\0';
>> return proc_path;
>> }
>>
>> Or drop the "else".
>
> Dropped 'else'.
>
> ...
>>> +
>>> + if (!tempbuf)
>>> + return TARGET_XFER_E_IO;
>>
>> if (tempbuf == NULL)
>>
>> Can NTO's alloca really return NULL?
>
> Yes.
>
>
> Attached fixed version of the patch.
This looks better, thanks.
--
Pedro Alves