Re: [PATCH] Makefile: describe XMALLOC_POISON

[Junio C Hamano <gitster@pobox.com>]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> The do_xmalloc() functions may fill an allocated buffer with the
> known value (0xA5) for debugging if we will pass the XMALLOC_POISON
> option during build.
>
> This patch adds description of this option to the Makefile and
> adds it to BASIC_CFLAGS if it was provided.
>
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---

I am guessing that the message this one is a response to is
(incomplete) v1 that I shouldn't look at, and this is v2 that is
expected to be useful.

XMALLOC_POISON is not about "if you are debugging the xmalloc()",
though.  By filling the memory returned with a non-NUL byte, what we
get is to catch callers that depended on the region of memory being
NUL-filled (often happens in early in a short-lived program), so it
is about catching more bugs in the callers of xmalloc() [*1*].

>  Makefile | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index f3325de..3f942b5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -367,6 +367,10 @@ all::
>  # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
>  #
>  # Define HAVE_GETDELIM if your system has the getdelim() function.
> +#
> +# Define XMALLOC_POISON if you are debugging the xmalloc(). In a XMALLOC_POISON
> +# build, each allocated buffer by the xmalloc() will be in known state. After
> +# memory allocation, a buffer will be filled with '0xA5' values.

"will be in known state" is not an interesting bit, and I do not
think we want people relying on the exact contents of the
uninitialized bytes.

Personally, I feel that XMALLOC_POISON outlived its usefulness, with
the availability of --valgrind tests and other forms of checks
(including static analyzers) in the modern world.  While I do not
think it hurts to allow people who know what they are doing to say
"make XMALLOC_POISON=YesPlease", I suspect it would cause more harm
to have a wrong description on what it is about here than the new
makefile knob helps them.  Because of the above, this change is
somewhere between "Meh" and "Perhaps a bad idea" to me.

If you have a more compelling story to tell ("XMALLOC_POISON-enabled
build allowed me to hunt down this and that kind of bug because I
can scan the entire address space looking for regions filled with
0xA5 to do X") and description based on that story is in the log
message and also in the Makefile comment, on the other hand, my
above assessment may become vastly move positive, though.  During
the course of running the project since the XMALLOC_POISON was added
in April 2006, I didn't encounter any such interesting debugging
session that helped me myself.

Thanks.

>  
> +ifdef XMALLOC_POISON
> +	BASIC_CFLAGS += -DXMALLOC_POISON
> +endif
> +


[Footnote]

*1* Another reason for choosing 0xA5 is because it is 'odd' and more
likely to cause bus errors on architectures that do not allow
unaligned access when the uninitialized value is used as a pointer,
but its value is fairly limited.