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

Andreas Arnez
2017-03-09T17:22:21Z

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.