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

Simon Marchi
2017-03-10T15:49:25Z

On 17-03-09 12:22 PM, Andreas Arnez wrote:
>> 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.

It's very possible, I just had this thought while reading the patch, I haven't actually tried.

> 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.

IMO it never hurt to split up big functions in smaller, more manageable parts...

> (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.
>

I don't the idea behind that rule. I thought it was just for readability,
to make it clear that the variable is a pointer without having to refer to the
declaration. Perhaps some older timers could shed some light on that :).

Simon