Re: [PATCH] khash: clarify that allocations never fail

Am 04.07.21 um 11:01 schrieb Jeff King:

On Sat, Jul 03, 2021 at 02:56:50PM +0200, René Scharfe wrote:

quoted

The following semantic patch finds a leery xmalloc() caller in
compat/mmap.c, though:

@@
expression PTR, SIZE, SIZE2;
@@
(
  PTR = xmalloc(SIZE);
|
  PTR = xcalloc(SIZE, SIZE2);
|
  PTR = xrealloc(SIZE);
|
  ALLOC_ARRAY(PTR, SIZE);
|
  CALLOC_ARRAY(PTR, SIZE);
|
  REALLOC_ARRAY(PTR, SIZE);
)
  if (
- PTR == NULL
+ 0
  ) {...}

Btw. the found code is:

	start = xmalloc(length);
	if (start == NULL) {
		errno = ENOMEM;
		return MAP_FAILED;
	}

start cannot be NULL, so the check is dead code.

IMHO that should not be using xmalloc() at all. It is a replacement for
system mmap, which can fail with ENOMEM, and we should be able to do the
same. Using xmalloc here is probably losing an opportunity to close
another pack window to free up memory for a new one.

Do you mean using malloc(3) directly instead of xmalloc() would no
longer try to release pack windows?  xmalloc() hasn't done that anymore
since 9827d4c185 (packfile: drop release_pack_memory(), 2019-08-12).

xmalloc() still brings support for zero-sized allocations on platforms
whose malloc(3) doesn't like them, and it enforces GIT_ALLOC_LIMIT.
mmap() is supposed give up with EINVAL if the length is 0, so the
first feature is not actually helping.  And GIT_ALLOC_LIMIT is not
documented and only useful for testing, right?

I doubt it matters that much in practice (most systems are not even
compiling or using this code, and it would only matter in a tight memory
situation). But as a general rule, I think compat/ wrappers should
behave as much like (sensible) system equivalents as possible.

Makes sense.

René