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

[Pedro Alves <palves at redhat 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