{"thread":{"id":"52968","subject":"[PATCH] commit-slab: clarify slabname##_peek()'s return value","startedAt":"2020-03-10T15:31:03Z","lastAt":"2020-03-11T16:07:54Z","messageCount":6,"participants":["SZEDER Gábor","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393001","messageId":"20200310153049.3482-1-szeder.dev@gmail.com","threadId":"52968","inReplyTo":null,"subject":"[PATCH] commit-slab: clarify slabname##_peek()'s return value","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-03-10T15:30:49Z","receivedAt":"2020-03-10T15:31:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since 862e730ec1 (commit-slab: introduce slabname##_peek()\nfunction, 2015-05-14) the slabname##_peek() function is documented as:\n\n  This function is similar to indegree_at(), but it will return NULL\n  until a call to indegree_at() was made for the commit.\n\nThis, however, is usually not the case.  If indegree_at() allocates\nmemory, then it will do so not only for the single commit it got as\nparameter, but it will allocate a whole new, ~512kB slab.  Later on,\nif any other commit's 'index' field happens to point into an already\nallocated slab, then indegree_peek() for such a commit will return a\nvalid non-NULL pointer, pointing to a zero-initialized location in the\nslab, even if no indegree_at() call has been made for that commit yet.\n\nUpdate slabname##_peek()'s documentation to clarify this.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n commit-slab.h | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex 69bf0c807c..05b3f2804e 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -24,7 +24,12 @@\n  * - int *indegree_peek(struct indegree *, struct commit *);\n  *\n  *   This function is similar to indegree_at(), but it will return NULL\n- *   until a call to indegree_at() was made for the commit.\n+ *   if the location to store the data associated with the given commit\n+ *   has not been allocated yet.\n+ *   Note that the location to store the data might have already been\n+ *   allocated even if no indegree_at() call has been made for that commit\n+ *   yet; in this case this function returns a pointer to a\n+ *   zero-initialized location.\n  *\n  * - void init_indegree(struct indegree *);\n  *   void init_indegree_with_stride(struct indegree *, int);\n-- \n2.26.0.rc0.366.gb2d359bb99\n\n"},{"id":"393013","messageId":"20200310175446.GB549010@coredump.intra.peff.net","threadId":"52968","inReplyTo":"20200310153049.3482-1-szeder.dev@gmail.com","subject":"Re: [PATCH] commit-slab: clarify slabname##_peek()'s return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-10T17:54:46Z","receivedAt":"2020-03-10T17:54:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 10, 2020 at 04:30:49PM +0100, SZEDER Gábor wrote:\n\n> Ever since 862e730ec1 (commit-slab: introduce slabname##_peek()\n> function, 2015-05-14) the slabname##_peek() function is documented as:\n> \n>   This function is similar to indegree_at(), but it will return NULL\n>   until a call to indegree_at() was made for the commit.\n> \n> This, however, is usually not the case.  If indegree_at() allocates\n> memory, then it will do so not only for the single commit it got as\n> parameter, but it will allocate a whole new, ~512kB slab.  Later on,\n> if any other commit's 'index' field happens to point into an already\n> allocated slab, then indegree_peek() for such a commit will return a\n> valid non-NULL pointer, pointing to a zero-initialized location in the\n> slab, even if no indegree_at() call has been made for that commit yet.\n> \n> Update slabname##_peek()'s documentation to clarify this.\n\nYeah, I agree the existing documentation is misleading. Your update\nlooks good to me.\n\nI thought at first we might simply be able to say:\n\n  This function is similar to indegree_at(), but it will avoid\n  allocating new slab memory (so its result is suitable only for\n  reading, not writing).\n\nBut I think it's worth mentioning that the caller needs to handle both\nNULL or a possible zero-initialized value, as your patch does.\n\nI also wondered if we could make life easier for the caller by\ncollapsing these cases. I.e., always returning a zero-initialized value,\nand never NULL. All of the callers do something like:\n\n  struct blame_origin *get_blame_suspects(struct commit *commit)\n  {                               \n          struct blame_origin **result;\n                  \n          result = blame_suspects_peek(&blame_suspects, commit);\n  \n          return result ? *result : NULL;\n  }\n\nall of which could be turned into a single blame_suspects_peek() call if\nit just consistently returned a zero-initialized value (it's a little\nconfusing in this example because we're storing pointers, so the\nzero-initialized value is _also_ NULL, but it's a different type).\n\nBut that would get a bit awkward, because peek() returns a pointer, not\na value (as it should, because the type we're storing may be a compound\ntype, which we generally avoid passing or returning by value).  So we'd\nactually need to return a pointer to a zero-initialized dummy value. Not\nimpossible, but getting a bit odd.\n\n-Peff\n"},{"id":"393014","messageId":"20200310180748.GC549010@coredump.intra.peff.net","threadId":"52968","inReplyTo":"20200310175446.GB549010@coredump.intra.peff.net","subject":"Re: [PATCH] commit-slab: clarify slabname##_peek()'s return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-10T18:07:48Z","receivedAt":"2020-03-10T18:07:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 10, 2020 at 01:54:46PM -0400, Jeff King wrote:\n\n> I also wondered if we could make life easier for the caller by\n> collapsing these cases. I.e., always returning a zero-initialized value,\n> and never NULL.\n> [...]\n> But that would get a bit awkward, because peek() returns a pointer, not\n> a value (as it should, because the type we're storing may be a compound\n> type, which we generally avoid passing or returning by value).  So we'd\n> actually need to return a pointer to a zero-initialized dummy value. Not\n> impossible, but getting a bit odd.\n\nI was a little curious how it would look. Code-wise it's not too bad,\nbut if anybody ever wrote to the dummy entry, that would cause confusing\nbugs. Possibly peek() could return a pointer-to-const.\n\nThe code savings in the callers are not all that much (I didn't convert\nall sites, but you can see that it just saves a few lines in each case).\n\nSo it's probably not worth the churn, but if anybody is really excited\nabout it, I can polish it into a real patch.\n\ndiff --git a/blame.c b/blame.c\nindex 29770e5c81..3ae42a1edd 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -15,11 +15,7 @@ static struct blame_suspects blame_suspects;\n \n struct blame_origin *get_blame_suspects(struct commit *commit)\n {\n-\tstruct blame_origin **result;\n-\n-\tresult = blame_suspects_peek(&blame_suspects, commit);\n-\n-\treturn result ? *result : NULL;\n+\treturn *blame_suspects_peek(&blame_suspects, commit);\n }\n \n static void set_blame_suspects(struct commit *commit, struct blame_origin *origin)\ndiff --git a/commit-slab-decl.h b/commit-slab-decl.h\nindex adc7b46c83..4b951e7b2f 100644\n--- a/commit-slab-decl.h\n+++ b/commit-slab-decl.h\n@@ -13,6 +13,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n \tunsigned stride;\t\t\t\t\t\t\\\n \tunsigned slab_count;\t\t\t\t\t\t\\\n \telemtype **slab;\t\t\t\t\t\t\\\n+\telemtype dummy;\t\t\t\t\t\t\t\\\n }\n \n /*\ndiff --git a/commit-slab-impl.h b/commit-slab-impl.h\nindex 5c0eb91a5d..505c21599a 100644\n--- a/commit-slab-impl.h\n+++ b/commit-slab-impl.h\n@@ -22,6 +22,7 @@ scope void init_ ##slabname## _with_stride(struct slabname *s,\t\t\\\n \ts->slab_size = COMMIT_SLAB_SIZE / elem_size;\t\t\t\\\n \ts->slab_count = 0;\t\t\t\t\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n+\tmemset(&s->dummy, 0, sizeof(elemtype));\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n scope void init_ ##slabname(struct slabname *s)\t\t\t\t\\\n@@ -50,15 +51,15 @@ scope elemtype *slabname## _at_peek(struct slabname *s,\t\t\t\\\n \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n \t\tunsigned int i;\t\t\t\t\t\t\\\n \t\tif (!add_if_missing)\t\t\t\t\t\\\n-\t\t\treturn NULL;\t\t\t\t\t\\\n+\t\t\treturn &s->dummy;\t\t\t\t\\\n \t\tREALLOC_ARRAY(s->slab, nth_slab + 1);\t\t\t\\\n \t\tfor (i = s->slab_count; i <= nth_slab; i++)\t\t\\\n \t\t\ts->slab[i] = NULL;\t\t\t\t\\\n \t\ts->slab_count = nth_slab + 1;\t\t\t\t\\\n \t}\t\t\t\t\t\t\t\t\\\n \tif (!s->slab[nth_slab]) {\t\t\t\t\t\\\n \t\tif (!add_if_missing)\t\t\t\t\t\\\n-\t\t\treturn NULL;\t\t\t\t\t\\\n+\t\t\treturn &s->dummy;\t\t\t\t\\\n \t\ts->slab[nth_slab] = xcalloc(s->slab_size,\t\t\\\n \t\t\t\t\t    sizeof(**s->slab) * s->stride);\t\t\\\n \t}\t\t\t\t\t\t\t\t\\\ndiff --git a/commit.c b/commit.c\nindex a6cfa41a4e..49d86435a5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -289,11 +289,6 @@ const void *get_cached_commit_buffer(struct repository *r, const struct commit *\n {\n \tstruct commit_buffer *v = buffer_slab_peek(\n \t\tr->parsed_objects->buffer_slab, commit);\n-\tif (!v) {\n-\t\tif (sizep)\n-\t\t\t*sizep = 0;\n-\t\treturn NULL;\n-\t}\n \tif (sizep)\n \t\t*sizep = v->size;\n \treturn v->buffer;\n"},{"id":"393015","messageId":"xmqqftegjcne.fsf@gitster.c.googlers.com","threadId":"52968","inReplyTo":"20200310175446.GB549010@coredump.intra.peff.net","subject":"Re: [PATCH] commit-slab: clarify slabname##_peek()'s return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-10T18:19:33Z","receivedAt":"2020-03-10T18:19:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But that would get a bit awkward, because peek() returns a pointer, not\n> a value (as it should, because the type we're storing may be a compound\n> type, which we generally avoid passing or returning by value).  So we'd\n> actually need to return a pointer to a zero-initialized dummy value. Not\n> impossible, but getting a bit odd.\n\nDo we have a guarantee that callers of the peek only look at, never\ntouch, the location?  As long as we make it return a \"const *\", it\nmight be OK, but a quick look at commit-slab.h tells me that we do\nnot say \"const\".\n"},{"id":"393025","messageId":"xmqqblp4jcc2.fsf@gitster.c.googlers.com","threadId":"52968","inReplyTo":"xmqqftegjcne.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-slab: clarify slabname##_peek()'s return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-10T18:26:21Z","receivedAt":"2020-03-10T18:26:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> But that would get a bit awkward, because peek() returns a pointer, not\n>> a value (as it should, because the type we're storing may be a compound\n>> type, which we generally avoid passing or returning by value).  So we'd\n>> actually need to return a pointer to a zero-initialized dummy value. Not\n>> impossible, but getting a bit odd.\n>\n> Do we have a guarantee that callers of the peek only look at, never\n> touch, the location?  As long as we make it return a \"const *\", it\n> might be OK, but a quick look at commit-slab.h tells me that we do\n> not say \"const\".\n\nAh, should have read the other message ;-)\n"},{"id":"393073","messageId":"20200311160732.GA26869@coredump.intra.peff.net","threadId":"52968","inReplyTo":"20200310180748.GC549010@coredump.intra.peff.net","subject":"Re: [PATCH] commit-slab: clarify slabname##_peek()'s return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-11T16:07:32Z","receivedAt":"2020-03-11T16:07:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 10, 2020 at 02:07:49PM -0400, Jeff King wrote:\n\n> I was a little curious how it would look. Code-wise it's not too bad,\n> but if anybody ever wrote to the dummy entry, that would cause confusing\n> bugs. Possibly peek() could return a pointer-to-const.\n\nI was also curious about this, but it gets ugly really quickly. So I\nthink this is a dead end, but read on if you're interested.\n\nThe first issue is that implicit const conversions are tricky around\npointers-to-pointers. So the actual commit-slab change ends up with\nthis:\n\ndiff --git a/commit-slab-decl.h b/commit-slab-decl.h\nindex 4b951e7b2f..2f67b08aa6 100644\n--- a/commit-slab-decl.h\n+++ b/commit-slab-decl.h\n@@ -35,7 +35,7 @@ void init_ ##slabname(struct slabname *s);\t\t\t\t\\\n void clear_ ##slabname(struct slabname *s);\t\t\t\t\\\n elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \\\n elemtype *slabname## _at(struct slabname *s, const struct commit *c);\t\\\n-elemtype *slabname## _peek(struct slabname *s, const struct commit *c)\n+const elemtype *slabname## _peek(struct slabname *s, const struct commit *c)\n \n #define define_shared_commit_slab(slabname, elemtype) \\\n \tdeclare_commit_slab(slabname, elemtype); \\\ndiff --git a/commit-slab-impl.h b/commit-slab-impl.h\nindex 505c21599a..f2d6ef97a2 100644\n--- a/commit-slab-impl.h\n+++ b/commit-slab-impl.h\n@@ -72,10 +72,14 @@ scope elemtype *slabname## _at(struct slabname *s,\t\t\t\\\n \treturn slabname##_at_peek(s, c, 1);\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-scope elemtype *slabname## _peek(struct slabname *s,\t\t\t\\\n+scope const elemtype *slabname## _peek(struct slabname *s,\t\t\\\n \t\t\t\t\t     const struct commit *c)\t\\\n {\t\t\t\t\t\t\t\t\t\\\n-\treturn slabname##_at_peek(s, c, 0);\t\t\t\t\\\n+\t/*\t\t\t\t\t\t\t\t\\\n+\t * The cast is necessary here to handle pointer elemtypes.\t\\\n+\t * E.g., (int **) cannot implicitly become (const int **).\t\\\n+\t */\t\t\t\t\t\t\t\t\\\n+\treturn (const elemtype *)slabname##_at_peek(s, c, 0);\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n struct slabname\n\nGross, but at least it's contained. The bigger problem, though, is that\nmany of the callers _do_ want to peek and then modify the result if it\nexists and isn't set to zero. So in blame, for example, we might try to\ndo:\n\ndiff --git a/blame.c b/blame.c\nindex 3ae42a1edd..ef0526c100 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -13,7 +13,7 @@\n define_commit_slab(blame_suspects, struct blame_origin *);\n static struct blame_suspects blame_suspects;\n \n-struct blame_origin *get_blame_suspects(struct commit *commit)\n+const struct blame_origin *get_blame_suspects(struct commit *commit)\n {\n \treturn *blame_suspects_peek(&blame_suspects, commit);\n }\n@@ -24,11 +24,12 @@ static void set_blame_suspects(struct commit *commit, struct blame_origin *origi\n }\n \n void blame_origin_decref(struct blame_origin *o)\n {\n \tif (o && --o->refcnt <= 0) {\n-\t\tstruct blame_origin *p, *l = NULL;\n+\t\tconst struct blame_origin *p;\n+\t\tstruct blame_origin *l = NULL;\n \t\tif (o->previous)\n \t\t\tblame_origin_decref(o->previous);\n \t\tfree(o->file.ptr);\n \t\t/* Should be present exactly once in commit chain */\n \t\tfor (p = get_blame_suspects(o->commit); p; l = p, p = p->next) {\n\nBut that loop at the end of the context won't work, because we're using\na non-const pointer \"l\" to iterate. And that \"l\" has to be non-const,\nbecause we actually modify its next pointer.\n\nAnd here's an even more straightforward example, with an actual solution\nto make it compile:\n\ndiff --git a/commit.c b/commit.c\nindex 49d86435a5..ee36a47db2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -319,17 +319,19 @@ void repo_unuse_commit_buffer(struct repository *r,\n \t\t\t      const struct commit *commit,\n \t\t\t      const void *buffer)\n {\n-\tstruct commit_buffer *v = buffer_slab_peek(\n+\tconst struct commit_buffer *v = buffer_slab_peek(\n \t\tr->parsed_objects->buffer_slab, commit);\n \tif (!(v && v->buffer == buffer))\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit)\n {\n-\tstruct commit_buffer *v = buffer_slab_peek(\n+\tconst struct commit_buffer *cv = buffer_slab_peek(\n \t\tpool->buffer_slab, commit);\n-\tif (v) {\n+\tif (cv->buffer) {\n+\t\t/* we know this isn't a dummy because it has a real buffer */\n+\t\tstruct commit_buffer *v = (struct commit_buffer *)cv;\n \t\tFREE_AND_NULL(v->buffer);\n \t\tv->size = 0;\n \t}\n\nThe first hunk is straight-forward, because we really are treating the\nslab value as read-only. But in the second we're trying to free the\nvalue it points to.  Using peek there makes sense, since we wouldn't\nwant to allocate a slab entry just to see that it points to no buffer at\nall. And the code is correct even without const, because the writing we\ndo is a noop in the case that the buffer struct was already empty. But\nC's type system doesn't know that, of course.\n\nSo the workarounds we have to do here and elsewhere are much worse than\nthe few lines of \"if (v)\" that we got rid of by simplifying peek().\n\n-Peff\n"}]}