Re: [PATCH] Makefile: describe XMALLOC_POISON

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2016, 18:23 UTC
Message-ID: <xmqqwprdnxte.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1452704204-1928-1-git-send-email-kuleshovmail@gmail.com>

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

Show 9 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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