Re: [PATCH 1/3] inf-ptrace: Do not stop memory transfers after a single word

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

Simon,

Thanks for your comments!

On Wed, Mar 08 2017, Simon Marchi wrote:

> On 17-03-06 11:00 AM, Andreas Arnez wrote:

[...]

>> This patch fixes the performance issue by attempting to fulfill the whole
>> transfer request in inf_ptrace_xfer_partial, using a loop around the
>> ptrace call.
>
> I think the idea is good.  The xfer partial interface says that the
> target should transfer up to len bytes.  Transferring 1 word at the
> time respects the contract, but we should try to be more efficient
> when possible.

Right, and I think the function now behaves more like you would expect
(https://en.wikipedia.org/wiki/Principle_of_least_astonishment).  Maybe
at some point we should also fix the discrepancy between fulfilling the
contract and still not working correctly, but that is another story.

[...]

>> +	unsigned chunk;
>
> "unsigned" -> "unsigned int"?

OK.

[...]

>> +	    /* Read the word, also when doing a partial word write.  */
>> +	    if (readbuf || chunk < sizeof (PTRACE_TYPE_RET))
>
> Use != NULL or == NULL when checking pointers.

OK.  (I thought I've seen patches that stopped following this rule after
the C++ transition.) (1)

[...]

> This is not a comment specifically about your patch, since that's how
> it was already, but maybe it would be a good time to address this.  I
> understand there's some level of overlap between the read and write
> (like the offset/skip computation), but I don't think that handling
> reading and writing in the same loop is very readable.  It just adds a
> bunch of branches and makes it hard to follow.  If that code was split
> in two functions (one for read, one for write), it would be way more
> straightforward.

That's probably a matter of taste.  Note that we do have separate
routines in gdbserver/linux-low.c that fulfill the equivalent function:
linux_read_memory() and linux_write_memory().  IMO they have even worse
readability *plus* suffer from heavy code duplication.  Maybe that's
just me, though.

Another thought that crossed my mind is whether we should extract the
whole peek/poke loop into a separate function instead of packing all the
logic under a case statement.  So far I didn't, because I wanted to keep
the bug fix small.

--
Andreas


(1) The GDB C/C++ coding standards provide a dubious explanation for the
"NULL Is Not Zero" rule
(https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#NULL_Is_Not_Zero):

  "Zero constant (0) is not interchangeable with a null pointer constant
  (NULL) anywhere. GCC does not give a warning for such interchange."

To me this seems to imply that the language does not support the
interchangeability.  But according to the C standard, it does:

  "An integer constant expression with the value 0, or such an
  expression cast to type void *, is called a null pointer constant."

C++ has a similar definition and specifies boolean conversion from
pointer types as well.  See also Stroustrup's style FAQ "should I use
NULL or 0?": http://www.stroustrup.com/bs_faq2.html#null

So maybe we want to support non-conforming compilers?  Or is this in
fact a GDB-specific style rule?  In either case we should adjust the
explanation, I think.