{"thread":{"id":"56045","subject":"[PATCH] khash: clarify that allocations never fail","startedAt":"2021-07-03T10:06:10Z","lastAt":"2021-07-04T10:11:07Z","messageCount":12,"participants":["René Scharfe","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429106","messageId":"d13b50fd-5944-0bbe-d28e-8232a2932598@web.de","threadId":"56045","inReplyTo":null,"subject":"[PATCH] khash: clarify that allocations never fail","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-07-03T10:05:46Z","receivedAt":"2021-07-03T10:06:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We use our standard allocation functions and macros (xcalloc,\nALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\nthe program on error, so code that's using them doesn't have to handle\nallocation failures.  Make this behavior explicit by replacing the code\nthat handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n khash.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/khash.h b/khash.h\nindex 21c2095216..84ff7230b6 100644\n--- a/khash.h\n+++ b/khash.h\n@@ -126,7 +126,7 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n-\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n+\t\t\t\tif (!new_flags) BUG(\"ALLOC_ARRAY failed\");\t\t\t\t\\\n \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n@@ -181,10 +181,10 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\tif (h->n_occupied >= h->upper_bound) { /* update the hash table */ \\\n \t\t\tif (h->n_buckets > (h->size<<1)) {\t\t\t\t\t\t\t\\\n \t\t\t\tif (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear \"deleted\" elements */ \\\n-\t\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\\\n+\t\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\\\n \t\t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\t\t} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \\\n-\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\t\\\n+\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\t\\\n \t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\t} /* TODO: to implement automatically shrinking; resize() already support shrinking */ \\\n \t\t{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n--\n2.32.0\n"},{"id":"429107","messageId":"YOA+IibZw6C9mkdF@coredump.intra.peff.net","threadId":"56045","inReplyTo":"d13b50fd-5944-0bbe-d28e-8232a2932598@web.de","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-03T10:38:26Z","receivedAt":"2021-07-03T10:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 03, 2021 at 12:05:46PM +0200, René Scharfe wrote:\n\n> We use our standard allocation functions and macros (xcalloc,\n> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n> the program on error, so code that's using them doesn't have to handle\n> allocation failures.  Make this behavior explicit by replacing the code\n> that handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n\nSeems like a good idea.\n\nWe're very sloppy about checking the \"ret\" field from kh_put_* for\nerrors (it's a tri-state for \"already existed\", \"newly added\", or\n\"error\"). I think that's not a problem because as you show here, we\ncan't actually hit the error case. This makes that much more obvious.\n\nTwo nits if we wanted to go further:\n\n> diff --git a/khash.h b/khash.h\n> index 21c2095216..84ff7230b6 100644\n> --- a/khash.h\n> +++ b/khash.h\n> @@ -126,7 +126,7 @@ static const double __ac_HASH_UPPER = 0.77;\n>  \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n>  \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n>  \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n> -\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n> +\t\t\t\tif (!new_flags) BUG(\"ALLOC_ARRAY failed\");\t\t\t\t\\\n\nI converted this in b32fa95fd8 (convert trivial cases to ALLOC_ARRAY,\n2016-02-22), but left the now-obsolete error-check.\n\nBut a few lines below...\n\n>  \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n>  \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n>  \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n\nThese REALLOC_ARRAY() calls are in the same boat. You dropped the error\ncheck in 2756ca4347 (use REALLOC_ARRAY for changing the allocation size\nof arrays, 2014-09-16).\n\nShould we make the two match? I'd probably do so by making the former\nmatch the latter, and just drop the conditional and BUG entirely.\n\n> @@ -181,10 +181,10 @@ static const double __ac_HASH_UPPER = 0.77;\n>  \t\tif (h->n_occupied >= h->upper_bound) { /* update the hash table */ \\\n>  \t\t\tif (h->n_buckets > (h->size<<1)) {\t\t\t\t\t\t\t\\\n>  \t\t\t\tif (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear \"deleted\" elements */ \\\n> -\t\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\\\n> +\t\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\\\n>  \t\t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \\\n> -\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\t\\\n> +\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\t\\\n\nAfter the first hunk, does kh_resize_*() ever return anything but 0? If\nnot, can we drop its return entirely, making it more clear that it's not\nexpected to fail? Both for human readers, but also for the compiler\n(which could then alert us at compile-time if we missed any error\ncases).\n\n-Peff\n"},{"id":"429108","messageId":"YOA/iSCcnzB9f3ax@coredump.intra.peff.net","threadId":"56045","inReplyTo":"YOA+IibZw6C9mkdF@coredump.intra.peff.net","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-03T10:44:25Z","receivedAt":"2021-07-03T10:44:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 03, 2021 at 06:38:27AM -0400, Jeff King wrote:\n\n> On Sat, Jul 03, 2021 at 12:05:46PM +0200, René Scharfe wrote:\n> \n> > We use our standard allocation functions and macros (xcalloc,\n> > ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n> > the program on error, so code that's using them doesn't have to handle\n> > allocation failures.  Make this behavior explicit by replacing the code\n> > that handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n> \n> Seems like a good idea.\n> \n> We're very sloppy about checking the \"ret\" field from kh_put_* for\n> errors (it's a tri-state for \"already existed\", \"newly added\", or\n> \"error\"). I think that's not a problem because as you show here, we\n> can't actually hit the error case. This makes that much more obvious.\n\nActually a quad-state, looking at the code (it distinguishes \"in table\nbut deleted\", though I don't think I've ever seen a case where that is\nuseful).\n\n> Two nits if we wanted to go further:\n\nIn patch form (mostly because I was curious if I was missing any cases;\nI'd probably squash it in with yours):\n\ndiff --git a/khash.h b/khash.h\nindex 84ff7230b6..fad486c966 100644\n--- a/khash.h\n+++ b/khash.h\n@@ -74,7 +74,7 @@ static const double __ac_HASH_UPPER = 0.77;\n \tvoid kh_destroy_##name(kh_##name##_t *h);\t\t\t\t\t\\\n \tvoid kh_clear_##name(kh_##name##_t *h);\t\t\t\t\t\t\\\n \tkhint_t kh_get_##name(const kh_##name##_t *h, khkey_t key); \\\n-\tint kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets); \\\n+\tvoid kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets); \\\n \tkhint_t kh_put_##name(kh_##name##_t *h, khkey_t key, int *ret); \\\n \tvoid kh_del_##name(kh_##name##_t *h, khint_t x);\n \n@@ -116,7 +116,7 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\t\treturn __ac_iseither(h->flags, i)? h->n_buckets : i;\t\t\\\n \t\t} else return 0;\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n-\tSCOPE int kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets) \\\n+\tSCOPE void kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets) \\\n \t{ /* This function uses 0.25*n_buckets bytes of working space instead of [sizeof(key_t+val_t)+.25]*n_buckets. */ \\\n \t\tkhint32_t *new_flags = NULL;\t\t\t\t\t\t\t\t\t\t\\\n \t\tkhint_t j = 1;\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n@@ -173,18 +173,15 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\t\th->n_occupied = h->size;\t\t\t\t\t\t\t\t\t\\\n \t\t\th->upper_bound = (khint_t)(h->n_buckets * __ac_HASH_UPPER + 0.5); \\\n \t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n-\t\treturn 0;\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \tSCOPE khint_t kh_put_##name(kh_##name##_t *h, khkey_t key, int *ret) \\\n \t{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\tkhint_t x;\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\tif (h->n_occupied >= h->upper_bound) { /* update the hash table */ \\\n \t\t\tif (h->n_buckets > (h->size<<1)) {\t\t\t\t\t\t\t\\\n-\t\t\t\tif (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear \"deleted\" elements */ \\\n-\t\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\\\n-\t\t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n-\t\t\t} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \\\n-\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\t\\\n+\t\t\t\tkh_resize_##name(h, h->n_buckets - 1); /* clear \"deleted\" elements */ \\\n+\t\t\t} else { \\\n+\t\t\t\tkh_resize_##name(h, h->n_buckets + 1); /* expand the hash table */ \\\n \t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\t} /* TODO: to implement automatically shrinking; resize() already support shrinking */ \\\n \t\t{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n"},{"id":"429115","messageId":"87v95r8w86.fsf@evledraar.gmail.com","threadId":"56045","inReplyTo":"YOA+IibZw6C9mkdF@coredump.intra.peff.net","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-03T11:35:45Z","receivedAt":"2021-07-03T11:37:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jul 03 2021, Jeff King wrote:\n\n> On Sat, Jul 03, 2021 at 12:05:46PM +0200, René Scharfe wrote:\n>\n>> We use our standard allocation functions and macros (xcalloc,\n>> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n>> the program on error, so code that's using them doesn't have to handle\n>> allocation failures.  Make this behavior explicit by replacing the code\n>> that handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n>\n> Seems like a good idea.\n>\n> We're very sloppy about checking the \"ret\" field from kh_put_* for\n> errors (it's a tri-state for \"already existed\", \"newly added\", or\n> \"error\"). I think that's not a problem because as you show here, we\n> can't actually hit the error case. This makes that much more obvious.\n>\n> Two nits if we wanted to go further:\n>\n>> diff --git a/khash.h b/khash.h\n>> index 21c2095216..84ff7230b6 100644\n>> --- a/khash.h\n>> +++ b/khash.h\n>> @@ -126,7 +126,7 @@ static const double __ac_HASH_UPPER = 0.77;\n>>  \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n>>  \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n>>  \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n>> -\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n>> +\t\t\t\tif (!new_flags) BUG(\"ALLOC_ARRAY failed\");\t\t\t\t\\\n>\n> I converted this in b32fa95fd8 (convert trivial cases to ALLOC_ARRAY,\n> 2016-02-22), but left the now-obsolete error-check.\n>\n> But a few lines below...\n>\n>>  \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n>>  \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n>>  \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n>\n> These REALLOC_ARRAY() calls are in the same boat. You dropped the error\n> check in 2756ca4347 (use REALLOC_ARRAY for changing the allocation size\n> of arrays, 2014-09-16).\n>\n> Should we make the two match? I'd probably do so by making the former\n> match the latter, and just drop the conditional and BUG entirely.\n\nYes, I don't see why we should be guarding theis anymore than we do\nxmalloc() or other x*() functions in various places (which is what it\nresolves to).\n\nIf anything we might consider renaming it via coccinelle to\nXALLOC_ARRAY(), XREALLOC_ARRAY() etc. to make it clear that they handle\nany errors themselves.\n"},{"id":"429119","messageId":"5cc24582-ed96-453a-fbad-d3fb83c8f949@web.de","threadId":"56045","inReplyTo":"87v95r8w86.fsf@evledraar.gmail.com","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-07-03T12:56:50Z","receivedAt":"2021-07-03T12:57:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.07.21 um 13:35 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Jul 03 2021, Jeff King wrote:\n>\n>> On Sat, Jul 03, 2021 at 12:05:46PM +0200, René Scharfe wrote:\n>>\n>>> We use our standard allocation functions and macros (xcalloc,\n>>> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n>>> the program on error, so code that's using them doesn't have to handle\n>>> allocation failures.  Make this behavior explicit by replacing the code\n>>> that handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n>>\n>> Seems like a good idea.\n>>\n>> We're very sloppy about checking the \"ret\" field from kh_put_* for\n>> errors (it's a tri-state for \"already existed\", \"newly added\", or\n>> \"error\"). I think that's not a problem because as you show here, we\n>> can't actually hit the error case. This makes that much more obvious.\n>>\n>> Two nits if we wanted to go further:\n>>\n>>> diff --git a/khash.h b/khash.h\n>>> index 21c2095216..84ff7230b6 100644\n>>> --- a/khash.h\n>>> +++ b/khash.h\n>>> @@ -126,7 +126,7 @@ static const double __ac_HASH_UPPER = 0.77;\n>>>  \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n>>>  \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n>>>  \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n>>> -\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n>>> +\t\t\t\tif (!new_flags) BUG(\"ALLOC_ARRAY failed\");\t\t\t\t\\\n>>\n>> I converted this in b32fa95fd8 (convert trivial cases to ALLOC_ARRAY,\n>> 2016-02-22), but left the now-obsolete error-check.\n>>\n>> But a few lines below...\n>>\n>>>  \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n>>>  \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n>>>  \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n>>\n>> These REALLOC_ARRAY() calls are in the same boat. You dropped the error\n>> check in 2756ca4347 (use REALLOC_ARRAY for changing the allocation size\n>> of arrays, 2014-09-16).\n>>\n>> Should we make the two match? I'd probably do so by making the former\n>> match the latter, and just drop the conditional and BUG entirely.\n>\n> Yes, I don't see why we should be guarding theis anymore than we do\n> xmalloc() or other x*() functions in various places (which is what it\n> resolves to).\n\nAgreed.\n\n> If anything we might consider renaming it via coccinelle to\n> XALLOC_ARRAY(), XREALLOC_ARRAY() etc. to make it clear that they handle\n> any errors themselves.\n\nI don't think there's any confusion in our internal code about the macros'\nhandling of allocation errors.\n\nThe following semantic patch finds a leery xmalloc() caller in\ncompat/mmap.c, though:\n\n@@\nexpression PTR, SIZE, SIZE2;\n@@\n(\n  PTR = xmalloc(SIZE);\n|\n  PTR = xcalloc(SIZE, SIZE2);\n|\n  PTR = xrealloc(SIZE);\n|\n  ALLOC_ARRAY(PTR, SIZE);\n|\n  CALLOC_ARRAY(PTR, SIZE);\n|\n  REALLOC_ARRAY(PTR, SIZE);\n)\n  if (\n- PTR == NULL\n+ 0\n  ) {...}\n\nRené\n"},{"id":"429120","messageId":"c25f0c5a-ccb7-a490-bd7d-90704478a896@web.de","threadId":"56045","inReplyTo":"YOA+IibZw6C9mkdF@coredump.intra.peff.net","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-07-03T12:57:00Z","receivedAt":"2021-07-03T12:57:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.07.21 um 12:38 schrieb Jeff King:\n> On Sat, Jul 03, 2021 at 12:05:46PM +0200, René Scharfe wrote:\n>\n>> We use our standard allocation functions and macros (xcalloc,\n>> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n>> the program on error, so code that's using them doesn't have to handle\n>> allocation failures.  Make this behavior explicit by replacing the code\n>> that handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n>\n> Seems like a good idea.\n>\n> We're very sloppy about checking the \"ret\" field from kh_put_* for\n> errors (it's a tri-state for \"already existed\", \"newly added\", or\n> \"error\"). I think that's not a problem because as you show here, we\n> can't actually hit the error case. This makes that much more obvious.\n>\n> Two nits if we wanted to go further:\n>\n>> diff --git a/khash.h b/khash.h\n>> index 21c2095216..84ff7230b6 100644\n>> --- a/khash.h\n>> +++ b/khash.h\n>> @@ -126,7 +126,7 @@ static const double __ac_HASH_UPPER = 0.77;\n>>  \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n>>  \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n>>  \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n>> -\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n>> +\t\t\t\tif (!new_flags) BUG(\"ALLOC_ARRAY failed\");\t\t\t\t\\\n>\n> I converted this in b32fa95fd8 (convert trivial cases to ALLOC_ARRAY,\n> 2016-02-22), but left the now-obsolete error-check.\n>\n> But a few lines below...\n>\n>>  \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n>>  \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n>>  \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n>\n> These REALLOC_ARRAY() calls are in the same boat. You dropped the error\n> check in 2756ca4347 (use REALLOC_ARRAY for changing the allocation size\n> of arrays, 2014-09-16).\n>\n> Should we make the two match? I'd probably do so by making the former\n> match the latter, and just drop the conditional and BUG entirely.\n\nYeah, makes sense, thank you.\n\n>\n>> @@ -181,10 +181,10 @@ static const double __ac_HASH_UPPER = 0.77;\n>>  \t\tif (h->n_occupied >= h->upper_bound) { /* update the hash table */ \\\n>>  \t\t\tif (h->n_buckets > (h->size<<1)) {\t\t\t\t\t\t\t\\\n>>  \t\t\t\tif (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear \"deleted\" elements */ \\\n>> -\t\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\\\n>> +\t\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\\\n>>  \t\t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n>>  \t\t\t} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \\\n>> -\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\t\\\n>> +\t\t\t\tBUG(\"kh_resize_\" #name \" failed\");\t\t\t\t\t\t\\\n>\n> After the first hunk, does kh_resize_*() ever return anything but 0? If\n> not, can we drop its return entirely, making it more clear that it's not\n> expected to fail? Both for human readers, but also for the compiler\n> (which could then alert us at compile-time if we missed any error\n> cases).\n\nGood idea.  Both type of changes make syncing with upstream a bit\nharder, but even though the return type change bleeds into the caller,\nthe overall change affects only a small area.\n\nRené\n"},{"id":"429121","messageId":"3778fb28-ed19-e90e-216a-d29d72305155@web.de","threadId":"56045","inReplyTo":"d13b50fd-5944-0bbe-d28e-8232a2932598@web.de","subject":"[PATCH v2] khash: clarify that allocations never fail","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-07-03T12:57:30Z","receivedAt":"2021-07-03T12:57:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We use our standard allocation functions and macros (xcalloc,\nALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\nthe program on error instead, so code that's using them doesn't have to\nhandle allocation failures.  Make this behavior explicit by turning\nkh_resize_ into a void function and removing the related unreachable\nerror handling code.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1:\n- Removed BUG calls.\n- Made kh_resize_ void instead.\n\n khash.h | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/khash.h b/khash.h\nindex 21c2095216..cb79bf8856 100644\n--- a/khash.h\n+++ b/khash.h\n@@ -74,7 +74,7 @@ static const double __ac_HASH_UPPER = 0.77;\n \tvoid kh_destroy_##name(kh_##name##_t *h);\t\t\t\t\t\\\n \tvoid kh_clear_##name(kh_##name##_t *h);\t\t\t\t\t\t\\\n \tkhint_t kh_get_##name(const kh_##name##_t *h, khkey_t key); \\\n-\tint kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets); \\\n+\tvoid kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets); \\\n \tkhint_t kh_put_##name(kh_##name##_t *h, khkey_t key, int *ret); \\\n \tvoid kh_del_##name(kh_##name##_t *h, khint_t x);\n\n@@ -116,7 +116,7 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\t\treturn __ac_iseither(h->flags, i)? h->n_buckets : i;\t\t\\\n \t\t} else return 0;\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n-\tSCOPE int kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets) \\\n+\tSCOPE void kh_resize_##name(kh_##name##_t *h, khint_t new_n_buckets) \\\n \t{ /* This function uses 0.25*n_buckets bytes of working space instead of [sizeof(key_t+val_t)+.25]*n_buckets. */ \\\n \t\tkhint32_t *new_flags = NULL;\t\t\t\t\t\t\t\t\t\t\\\n \t\tkhint_t j = 1;\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n@@ -126,7 +126,6 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n-\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n@@ -173,18 +172,15 @@ static const double __ac_HASH_UPPER = 0.77;\n \t\t\th->n_occupied = h->size;\t\t\t\t\t\t\t\t\t\\\n \t\t\th->upper_bound = (khint_t)(h->n_buckets * __ac_HASH_UPPER + 0.5); \\\n \t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n-\t\treturn 0;\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \tSCOPE khint_t kh_put_##name(kh_##name##_t *h, khkey_t key, int *ret) \\\n \t{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\tkhint_t x;\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\tif (h->n_occupied >= h->upper_bound) { /* update the hash table */ \\\n \t\t\tif (h->n_buckets > (h->size<<1)) {\t\t\t\t\t\t\t\\\n-\t\t\t\tif (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear \"deleted\" elements */ \\\n-\t\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\\\n-\t\t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n-\t\t\t} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \\\n-\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\t\\\n+\t\t\t\tkh_resize_##name(h, h->n_buckets - 1); /* clear \"deleted\" elements */ \\\n+\t\t\t} else { \\\n+\t\t\t\tkh_resize_##name(h, h->n_buckets + 1); /* expand the hash table */ \\\n \t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n \t\t} /* TODO: to implement automatically shrinking; resize() already support shrinking */ \\\n \t\t{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n--\n2.32.0\n"},{"id":"429123","messageId":"87h7hb8s0o.fsf@evledraar.gmail.com","threadId":"56045","inReplyTo":"5cc24582-ed96-453a-fbad-d3fb83c8f949@web.de","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-03T13:05:28Z","receivedAt":"2021-07-03T13:07:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jul 03 2021, René Scharfe wrote:\n\n> Am 03.07.21 um 13:35 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Sat, Jul 03 2021, Jeff King wrote:\n>>\n>>> On Sat, Jul 03, 2021 at 12:05:46PM +0200, René Scharfe wrote:\n>>>\n>>>> We use our standard allocation functions and macros (xcalloc,\n>>>> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n>>>> the program on error, so code that's using them doesn't have to handle\n>>>> allocation failures.  Make this behavior explicit by replacing the code\n>>>> that handles allocation errors in kh_resize_ and kh_put_ with BUG calls.\n>>>\n>>> Seems like a good idea.\n>>>\n>>> We're very sloppy about checking the \"ret\" field from kh_put_* for\n>>> errors (it's a tri-state for \"already existed\", \"newly added\", or\n>>> \"error\"). I think that's not a problem because as you show here, we\n>>> can't actually hit the error case. This makes that much more obvious.\n>>>\n>>> Two nits if we wanted to go further:\n>>>\n>>>> diff --git a/khash.h b/khash.h\n>>>> index 21c2095216..84ff7230b6 100644\n>>>> --- a/khash.h\n>>>> +++ b/khash.h\n>>>> @@ -126,7 +126,7 @@ static const double __ac_HASH_UPPER = 0.77;\n>>>>  \t\t\tif (h->size >= (khint_t)(new_n_buckets * __ac_HASH_UPPER + 0.5)) j = 0;\t/* requested size is too small */ \\\n>>>>  \t\t\telse { /* hash table size to be changed (shrink or expand); rehash */ \\\n>>>>  \t\t\t\tALLOC_ARRAY(new_flags, __ac_fsize(new_n_buckets)); \\\n>>>> -\t\t\t\tif (!new_flags) return -1;\t\t\t\t\t\t\t\t\\\n>>>> +\t\t\t\tif (!new_flags) BUG(\"ALLOC_ARRAY failed\");\t\t\t\t\\\n>>>\n>>> I converted this in b32fa95fd8 (convert trivial cases to ALLOC_ARRAY,\n>>> 2016-02-22), but left the now-obsolete error-check.\n>>>\n>>> But a few lines below...\n>>>\n>>>>  \t\t\t\tmemset(new_flags, 0xaa, __ac_fsize(new_n_buckets) * sizeof(khint32_t)); \\\n>>>>  \t\t\t\tif (h->n_buckets < new_n_buckets) {\t/* expand */\t\t\\\n>>>>  \t\t\t\t\tREALLOC_ARRAY(h->keys, new_n_buckets); \\\n>>>\n>>> These REALLOC_ARRAY() calls are in the same boat. You dropped the error\n>>> check in 2756ca4347 (use REALLOC_ARRAY for changing the allocation size\n>>> of arrays, 2014-09-16).\n>>>\n>>> Should we make the two match? I'd probably do so by making the former\n>>> match the latter, and just drop the conditional and BUG entirely.\n>>\n>> Yes, I don't see why we should be guarding theis anymore than we do\n>> xmalloc() or other x*() functions in various places (which is what it\n>> resolves to).\n>\n> Agreed.\n>\n>> If anything we might consider renaming it via coccinelle to\n>> XALLOC_ARRAY(), XREALLOC_ARRAY() etc. to make it clear that they handle\n>> any errors themselves.\n>\n> I don't think there's any confusion in our internal code about the macros'\n> handling of allocation errors.\n>\n> The following semantic patch finds a leery xmalloc() caller in\n> compat/mmap.c, though:\n>\n> @@\n> expression PTR, SIZE, SIZE2;\n> @@\n> (\n>   PTR = xmalloc(SIZE);\n> |\n>   PTR = xcalloc(SIZE, SIZE2);\n> |\n>   PTR = xrealloc(SIZE);\n> |\n>   ALLOC_ARRAY(PTR, SIZE);\n> |\n>   CALLOC_ARRAY(PTR, SIZE);\n> |\n>   REALLOC_ARRAY(PTR, SIZE);\n> )\n>   if (\n> - PTR == NULL\n> + 0\n>   ) {...}\n>\n> René\n\nGood catch, a bug as old as 730d48a2ef8 ([PATCH] If NO_MMAP is defined,\nfake mmap() and munmap(), 2005-10-08).\n\nIt would be nice to have that coccinelle patch as a follow-up\npatch. Perhaps along with changing that \"0\" to something that's simply a\nsyntax error, or just:\n\n    if (0 /* always false due to (implicit?) x*() function call above */)\n"},{"id":"429155","messageId":"YOF46CU+SSDRE51/@coredump.intra.peff.net","threadId":"56045","inReplyTo":"5cc24582-ed96-453a-fbad-d3fb83c8f949@web.de","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-04T09:01:28Z","receivedAt":"2021-07-04T09:01:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 03, 2021 at 02:56:50PM +0200, René Scharfe wrote:\n\n> > If anything we might consider renaming it via coccinelle to\n> > XALLOC_ARRAY(), XREALLOC_ARRAY() etc. to make it clear that they handle\n> > any errors themselves.\n> \n> I don't think there's any confusion in our internal code about the macros'\n> handling of allocation errors.\n\nAgreed.\n\n> The following semantic patch finds a leery xmalloc() caller in\n> compat/mmap.c, though:\n> \n> @@\n> expression PTR, SIZE, SIZE2;\n> @@\n> (\n>   PTR = xmalloc(SIZE);\n> |\n>   PTR = xcalloc(SIZE, SIZE2);\n> |\n>   PTR = xrealloc(SIZE);\n> |\n>   ALLOC_ARRAY(PTR, SIZE);\n> |\n>   CALLOC_ARRAY(PTR, SIZE);\n> |\n>   REALLOC_ARRAY(PTR, SIZE);\n> )\n>   if (\n> - PTR == NULL\n> + 0\n>   ) {...}\n\nIMHO that should not be using xmalloc() at all. It is a replacement for\nsystem mmap, which can fail with ENOMEM, and we should be able to do the\nsame. Using xmalloc here is probably losing an opportunity to close\nanother pack window to free up memory for a new one.\n\nI doubt it matters that much in practice (most systems are not even\ncompiling or using this code, and it would only matter in a tight memory\nsituation). But as a general rule, I think compat/ wrappers should\nbehave as much like (sensible) system equivalents as possible.\n\n-Peff\n"},{"id":"429158","messageId":"YOF55hIKhRrwrZah@coredump.intra.peff.net","threadId":"56045","inReplyTo":"3778fb28-ed19-e90e-216a-d29d72305155@web.de","subject":"Re: [PATCH v2] khash: clarify that allocations never fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-04T09:05:42Z","receivedAt":"2021-07-04T09:05:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 03, 2021 at 02:57:30PM +0200, René Scharfe wrote:\n\n> We use our standard allocation functions and macros (xcalloc,\n> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate\n> the program on error instead, so code that's using them doesn't have to\n> handle allocation failures.  Make this behavior explicit by turning\n> kh_resize_ into a void function and removing the related unreachable\n> error handling code.\n\nThanks, this looks good to me.\n\n>  \tSCOPE khint_t kh_put_##name(kh_##name##_t *h, khkey_t key, int *ret) \\\n>  \t{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n>  \t\tkhint_t x;\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n>  \t\tif (h->n_occupied >= h->upper_bound) { /* update the hash table */ \\\n>  \t\t\tif (h->n_buckets > (h->size<<1)) {\t\t\t\t\t\t\t\\\n> -\t\t\t\tif (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear \"deleted\" elements */ \\\n> -\t\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\\\n> -\t\t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n> -\t\t\t} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \\\n> -\t\t\t\t*ret = -1; return h->n_buckets;\t\t\t\t\t\t\t\\\n> +\t\t\t\tkh_resize_##name(h, h->n_buckets - 1); /* clear \"deleted\" elements */ \\\n> +\t\t\t} else { \\\n> +\t\t\t\tkh_resize_##name(h, h->n_buckets + 1); /* expand the hash table */ \\\n>  \t\t\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n\nI had to read this over again carefully, because the second \"else-if\"\nmakes the conversion less obvious. I think the original would have been\neasier to read as:\n\n  if (something) {\n    if (do_option_one())\n        ...error...\n  } else {\n    if (do_option_two())\n        ...error...\n  }\n\ninstead of:\n\n  if (something) {\n    if (do_option_one())\n        ...error...\n  } else if (do_option_two())\n        ...error...\n  }\n\nAll of which is to say the conversion here is correct, and I think as a\nbonus the resulting code is easier to follow. ;)\n\n-Peff\n"},{"id":"429159","messageId":"b2c9b072-b2ac-625b-4f0f-9ba1ca3321d7@web.de","threadId":"56045","inReplyTo":"YOF46CU+SSDRE51/@coredump.intra.peff.net","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-07-04T09:41:23Z","receivedAt":"2021-07-04T09:41:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.07.21 um 11:01 schrieb Jeff King:\n> On Sat, Jul 03, 2021 at 02:56:50PM +0200, René Scharfe wrote:\n>\n>> The following semantic patch finds a leery xmalloc() caller in\n>> compat/mmap.c, though:\n>>\n>> @@\n>> expression PTR, SIZE, SIZE2;\n>> @@\n>> (\n>>   PTR = xmalloc(SIZE);\n>> |\n>>   PTR = xcalloc(SIZE, SIZE2);\n>> |\n>>   PTR = xrealloc(SIZE);\n>> |\n>>   ALLOC_ARRAY(PTR, SIZE);\n>> |\n>>   CALLOC_ARRAY(PTR, SIZE);\n>> |\n>>   REALLOC_ARRAY(PTR, SIZE);\n>> )\n>>   if (\n>> - PTR == NULL\n>> + 0\n>>   ) {...}\n>\n\nBtw. the found code is:\n\n\tstart = xmalloc(length);\n\tif (start == NULL) {\n\t\terrno = ENOMEM;\n\t\treturn MAP_FAILED;\n\t}\n\nstart cannot be NULL, so the check is dead code.\n\n> IMHO that should not be using xmalloc() at all. It is a replacement for\n> system mmap, which can fail with ENOMEM, and we should be able to do the\n> same. Using xmalloc here is probably losing an opportunity to close\n> another pack window to free up memory for a new one.\n\nDo you mean using malloc(3) directly instead of xmalloc() would no\nlonger try to release pack windows?  xmalloc() hasn't done that anymore\nsince 9827d4c185 (packfile: drop release_pack_memory(), 2019-08-12).\n\nxmalloc() still brings support for zero-sized allocations on platforms\nwhose malloc(3) doesn't like them, and it enforces GIT_ALLOC_LIMIT.\nmmap() is supposed give up with EINVAL if the length is 0, so the\nfirst feature is not actually helping.  And GIT_ALLOC_LIMIT is not\ndocumented and only useful for testing, right?\n\n> I doubt it matters that much in practice (most systems are not even\n> compiling or using this code, and it would only matter in a tight memory\n> situation). But as a general rule, I think compat/ wrappers should\n> behave as much like (sensible) system equivalents as possible.\n\nMakes sense.\n\nRené\n"},{"id":"429161","messageId":"YOGJOO6l6bIsISkT@coredump.intra.peff.net","threadId":"56045","inReplyTo":"b2c9b072-b2ac-625b-4f0f-9ba1ca3321d7@web.de","subject":"Re: [PATCH] khash: clarify that allocations never fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-04T10:11:04Z","receivedAt":"2021-07-04T10:11:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 04, 2021 at 11:41:23AM +0200, René Scharfe wrote:\n\n> Btw. the found code is:\n> \n> \tstart = xmalloc(length);\n> \tif (start == NULL) {\n> \t\terrno = ENOMEM;\n> \t\treturn MAP_FAILED;\n> \t}\n> \n> start cannot be NULL, so the check is dead code.\n\nYep, so it's doubly silly.\n\n> > IMHO that should not be using xmalloc() at all. It is a replacement for\n> > system mmap, which can fail with ENOMEM, and we should be able to do the\n> > same. Using xmalloc here is probably losing an opportunity to close\n> > another pack window to free up memory for a new one.\n> \n> Do you mean using malloc(3) directly instead of xmalloc() would no\n> longer try to release pack windows?  xmalloc() hasn't done that anymore\n> since 9827d4c185 (packfile: drop release_pack_memory(), 2019-08-12).\n\nNo, I meant that by using xmalloc() here, if the allocation fails, we'll\nimmediately die(). Whereas the caller of mmap() could get the ENOMEM\nerror, then unmap an in-use pack window and try again.\n\nHowever, I forgot that we don't actually do that (yet). We unmap windows\nif we go over our own packed_git_window_size counter, but not when mmap\nfails. Eric posted a patch recently to change that, though (at which\npoint the die() in xmalloc() would be working against us).\n\n(Actually, we did _try_ to do something like that in xmmap prior to\n9827d4c185, but I don't think it actually worked since it was based on\nour own internal limit, and not what the OS would allow).\n\n> xmalloc() still brings support for zero-sized allocations on platforms\n> whose malloc(3) doesn't like them, and it enforces GIT_ALLOC_LIMIT.\n> mmap() is supposed give up with EINVAL if the length is 0, so the\n> first feature is not actually helping.  And GIT_ALLOC_LIMIT is not\n> documented and only useful for testing, right?\n\nYeah, I think failing on a 0-length mmap is OK, since that's what real\nsystems do (and this is a wrapper). Our xmmap_gently() handles this,\nwhich is the right spot.\n\nI don't think of GIT_ALLOC_LIMIT as something we're committed to\npublicly supporting, though I have (ab)used it once or twice myself.\nHowever, I'm not sure if it makes sense here. True, on a system without\nmmap it is a heap allocation, but to me it's conceptually very different\nthan a normal allocation (and on a system with mmap, we have no problem\nat all requesting arbitrarily large maps). If we did want to put a limit\nhere, I think we'd want to do it at the xmmap layer, using a separate\nvariable.\n\n-Peff\n"}]}