On Mon, Mar 26, 2012 at 18:37, Steve Kargl
<s...@troutmask.apl.washington.edu> wrote:
> On Mon, Mar 26, 2012 at 06:17:08PM +0300, Janne Blomqvist wrote:
>> currently in libgfortran we have two malloc() wrappers, get_mem and
>> internal_malloc_size, which abort the program if malloc fails.
>> internal_malloc_size does
>> if (size == 0)
>>     size = 1;
>> and then calls get_mem, which doesn't have such a zero-size check.
>> This is, however, a bug in get_mem, as malloc(0) is allowed to return
>> NULL, and in that case get_mem would incorrectly abort the program.
>> Thus having both these functions is unnecessary. The attached patch
>> combines these two functions into one.  As I couldn't decide between
>> get_mem and internal_malloc_size and didn't particularly like either
>> of the names, I named the combined function "xmalloc", as is common
>> for such functions to be called.
>> When developing the patch, I noticed that the implementations of the
>> intrinsics IALL, IANY, IPARITY, NORM2, and PARITY weren't being
>> regenerated from the m4 sources. The reason, it turned out, was that
>> the dependencies were specified incorrectly in Makefile.am. The patch
>> also fixes this.
>> Regtested on x86_64-unknown-linux-gnu. While the patch is large, it's
>> also mechanical, hence committed as obvious.
> The patch looks ok to me, but I do have two questions.
> 1) xmalloc is used in other parts of gcc.  Is there a
>   possibility of confusing the libgfortran version
>   with the other(s)?  Perhaps, a unique name is in
>   order, gfc_xmalloc().

I though about that, and ultimately decided against it, as the symbol
name is mangled anyway (__gfortrani_xmalloc IIRC), and so shouldn't
cause any linking problems. Also, I don't think it's worth worrying
about similar named source identifiers in other subdirectories in the
GCC tree; it's easy enough to find the implementation that is used in
libgfortran with (git) grep.

> 2) Unless I've misread the patch (which is always a
>   possibility), it seems that you've removed
>   GFC_CLEAR_MEMORY, which used calloc to zero the
>   allocated memory.  Is there a situation where
>   libgfortran needs/assumes the memory is zeroed
>   upon allocation?

GFC_CLEAR_MEMORY was a macro for debugging purposes. Today, with tools
like valgrind available, replacing malloc with calloc is quite a
inefficient debugging tool. And as it's only a one-line change to
replace the malloc call with calloc inside xmalloc, if somebody wants
to do that it's no more effort than uncommenting the GFC_CLEAR_MEMORY
macro definition was before.

And yes, we nowadays have a corresponding xcalloc for those cases
where the caller requires zeroed memory; before xcalloc the caller
used memset to explicitly zero the memory, so GFC_CLEAR_MEMORY was
never needed from a correctness standpoint.

Janne Blomqvist

Reply via email to