Re: [PATCH 2/4] [nto] Fixes for nto procfs.

Pedro Alves
2015-10-20T13:21:53Z

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