On 07/22/2011 10:16 AM, Mike Frysinger wrote:

Mike, thanks for the review.

>> +#if defined __DSBT__
>> +static int
> 
> rather than being tied to the exec format that *gdbserver* is being
> built as, shouldnt this be bound to the ptrace defines being available
> ?  how abut using "#ifdef PTRACE_GETDSBT" ?
> 

Yeah, that makes sense.  Done.

>> +linux_read_loadmap (const char *annex, CORE_ADDR offset,
>> +                          unsigned char *myaddr, unsigned int len)
> 
> indentation looks wrong wrt GNU style.  needs tabs and spaces to
> properly align it.
> 

Fixed.

>> +  int addr = (strcmp (annex, "exec") == 0 ? (int) PTRACE_GETDSBT_EXEC :
>> +	      (strcmp (annex, "interp") == 0 ? (int) PTRACE_GETDSBT_INTERP :
>> +	       -1));
>> ...
>> +  if (addr == -1)
>> +    return -1;
> 
> style with add init is off here too.  but seems like it'd be cleaner
> to have this be a series of if statements incorporated into the -1
> check to make the -1 value unnecessary.
> 

Agreed.  Fixed.

>> +  copy_length = actual_length - offset < len ? actual_length - offset : len;
> 
> does gdb really not have min/max helpers ?
> 

gdb has, but gdbserver doesn't.  I'll leave the code here.  A follow-up
patch can move min/max helper into common/ and use them in gdbserver.

>> +  ptrace (PTRACE_GETDSBT, pid, addr, &data);
> 
> what if it fails ?
> 

We can return -1 if it fails.  Done in new patch.

>> +    {"fdpic", handle_qxfer_fdpic}
> 
> style is broken.  needs space after "{", space before "}", and comma after "}"
> 

Fixed.

>> +  /* Load maps for FDPIC systems.  */
>> +  TARGET_OBJECT_FDPIC
> 
> i think this needs a trailing comma

Fixed.

-- 
Yao (齐尧)