{"thread":{"id":"48446","subject":"[PATCH] fast-export: avoid NULL pointer arithmetic","startedAt":"2018-05-09T21:06:54Z","lastAt":"2018-05-15T19:36:47Z","messageCount":16,"participants":["René Scharfe","Johannes Schindelin","Junio C Hamano","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347136","messageId":"d50a4d5d-3b99-453e-1b52-4e733453fb78@web.de","threadId":"48446","inReplyTo":null,"subject":"[PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-09T21:06:46Z","receivedAt":"2018-05-09T21:06:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Clang 6 reports the following warning, which is turned into an error in a\nDEVELOPER build:\n\n\tbuiltin/fast-export.c:162:28: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]\n\t\treturn ((uint32_t *)NULL) + mark;\n\t\t       ~~~~~~~~~~~~~~~~~~ ^\n\t1 error generated.\n\nThe compiler is correct, and the error message speaks for itself.  There\nis no need for any undefined operation -- just cast mark to void * or\nuint32_t after an intermediate cast to uintptr_t.  That encodes the\ninteger value into a pointer and later decodes it as intended.\n\nWhile at it remove an outdated comment -- intptr_t has been used since\nffe659f94d (parse-options: make some arguments optional, add callbacks),\ncommitted in October 2007.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/fast-export.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 530df12f05..fa556a3c93 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -156,15 +156,14 @@ static void anonymize_path(struct strbuf *out, const char *path,\n \t}\n }\n \n-/* Since intptr_t is C99, we do not use it here */\n-static inline uint32_t *mark_to_ptr(uint32_t mark)\n+static inline void *mark_to_ptr(uint32_t mark)\n {\n-\treturn ((uint32_t *)NULL) + mark;\n+\treturn (void *)(uintptr_t)mark;\n }\n \n static inline uint32_t ptr_to_mark(void * mark)\n {\n-\treturn (uint32_t *)mark - (uint32_t *)NULL;\n+\treturn (uint32_t)(uintptr_t)mark;\n }\n \n static inline void mark_object(struct object *object, uint32_t mark)\n-- \n2.17.0\n"},{"id":"347140","messageId":"nycvar.QRO.7.76.6.1805091743080.77@tvgsbejvaqbjf.bet","threadId":"48446","inReplyTo":"d50a4d5d-3b99-453e-1b52-4e733453fb78@web.de","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-09T21:43:30Z","receivedAt":"2018-05-09T21:43:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Wed, 9 May 2018, René Scharfe wrote:\n\n> Clang 6 reports the following warning, which is turned into an error in a\n> DEVELOPER build:\n> \n> \tbuiltin/fast-export.c:162:28: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]\n> \t\treturn ((uint32_t *)NULL) + mark;\n> \t\t       ~~~~~~~~~~~~~~~~~~ ^\n> \t1 error generated.\n> \n> The compiler is correct, and the error message speaks for itself.  There\n> is no need for any undefined operation -- just cast mark to void * or\n> uint32_t after an intermediate cast to uintptr_t.  That encodes the\n> integer value into a pointer and later decodes it as intended.\n> \n> While at it remove an outdated comment -- intptr_t has been used since\n> ffe659f94d (parse-options: make some arguments optional, add callbacks),\n> committed in October 2007.\n\nACK.\n\nThanks,\nDscho"},{"id":"347201","messageId":"99d443cd-e817-7db5-f758-bf4cf47f7c06@web.de","threadId":"48446","inReplyTo":"d50a4d5d-3b99-453e-1b52-4e733453fb78@web.de","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-10T09:24:01Z","receivedAt":"2018-05-10T09:24:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.05.2018 um 23:06 schrieb René Scharfe:\n> Clang 6 reports the following warning, which is turned into an error in a\n> DEVELOPER build:\n> \n> \tbuiltin/fast-export.c:162:28: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]\n> \t\treturn ((uint32_t *)NULL) + mark;\n> \t\t       ~~~~~~~~~~~~~~~~~~ ^\n> \t1 error generated.\n> \n> The compiler is correct, and the error message speaks for itself.  There\n> is no need for any undefined operation -- just cast mark to void * or\n> uint32_t after an intermediate cast to uintptr_t.  That encodes the\n> integer value into a pointer and later decodes it as intended.\n\nHaving thought about it a bit more I have to say: That seems to work,\nbut it's not portable.  \n\nThe standard says about uintptr_t that \"any valid pointer to void can\nbe converted to this type, then converted back to pointer to void, and\nthe result will compare equal to the original pointer\".  So void * ->\nuintptr_t -> void * is a proper roundtrip, but that doesn't imply that\ncasting arbitrary uintptr_t values to void * would be lossless.\n\nI don't know an architecture where this would bite us, but I wonder if\nthere is a cleaner way.  Perhaps changing the type of the decoration\nmember of struct decoration_entry in decorate.h to uintptr_t?\n\n> While at it remove an outdated comment -- intptr_t has been used since\n> ffe659f94d (parse-options: make some arguments optional, add callbacks),\n> committed in October 2007.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>   builtin/fast-export.c | 7 +++----\n>   1 file changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 530df12f05..fa556a3c93 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -156,15 +156,14 @@ static void anonymize_path(struct strbuf *out, const char *path,\n>   \t}\n>   }\n>   \n> -/* Since intptr_t is C99, we do not use it here */\n> -static inline uint32_t *mark_to_ptr(uint32_t mark)\n> +static inline void *mark_to_ptr(uint32_t mark)\n>   {\n> -\treturn ((uint32_t *)NULL) + mark;\n> +\treturn (void *)(uintptr_t)mark;\n>   }\n>   \n>   static inline uint32_t ptr_to_mark(void * mark)\n>   {\n> -\treturn (uint32_t *)mark - (uint32_t *)NULL;\n> +\treturn (uint32_t)(uintptr_t)mark;\n>   }\n>   \n>   static inline void mark_object(struct object *object, uint32_t mark)\n> \n"},{"id":"347209","messageId":"xmqqo9hniy1v.fsf@gitster-ct.c.googlers.com","threadId":"48446","inReplyTo":"99d443cd-e817-7db5-f758-bf4cf47f7c06@web.de","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T10:51:08Z","receivedAt":"2018-05-10T10:51:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> The standard says about uintptr_t that \"any valid pointer to void can\n> be converted to this type, then converted back to pointer to void, and\n> the result will compare equal to the original pointer\".  So void * ->\n> uintptr_t -> void * is a proper roundtrip, but that doesn't imply that\n> casting arbitrary uintptr_t values to void * would be lossless.\n>\n> I don't know an architecture where this would bite us, but I wonder if\n> there is a cleaner way.  Perhaps changing the type of the decoration\n> member of struct decoration_entry in decorate.h to uintptr_t?\n\nIn order to ensure \"void * -> uintptr_t -> void *\" roundtrip holds,\nthe implementation would guarantee that uintptr_t is wider than\nvoid*, so what you suggest technically makes sense.  We should be\nable to store any pointer in the field.  And we should be able to\nstore any value of an unsigned integral type that is narrower than\nuintptr_t.\n\nBut it somehow feels backwards in spirit to me, as the reason why we\nuse \"void *\" there in the decoration field is because we expect that\nwe'd have a pointer to some struture most of the time, and we have\nto occasionally store a small integer there.  So I'd naively expect\nthat\n\n\tuint32_t mark = 23;\n\tde->decoration = (void *)mark;\n\nwould be a good way to store mark #23 in the field and\n\n\tuint32_t mark;\n\tmark = (typeof(mark))de->decoration;\n\nwould be a good way to read it off of the \"void *\" field.  Of\ncourse, this assume that (void *) is at least as wide as 32-bit and\nit also ignores the standard ;-)\n\nThis is an unrelated tangent but the mark-to-ptr() and ptr-to-mark()\nimplementations feel wasteful, especially when we worry about 32-bit\narchs.  A naive platform implementation of\n\n\t(uint32_t *)mark - (uint32_t *)NULL;\n\nwould be ((uintptr_t)mark) / 4, i.e. the de->decoration field will\nalways have two LSB clear and only utilize top 30-bit to represent\nthe value of mark.\n"},{"id":"347301","messageId":"654fac2a-8dca-7bee-2bab-a3986aa7e52d@web.de","threadId":"48446","inReplyTo":"xmqqo9hniy1v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-10T19:47:56Z","receivedAt":"2018-05-10T19:48:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.05.2018 um 12:51 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> The standard says about uintptr_t that \"any valid pointer to void can\n>> be converted to this type, then converted back to pointer to void, and\n>> the result will compare equal to the original pointer\".  So void * ->\n>> uintptr_t -> void * is a proper roundtrip, but that doesn't imply that\n>> casting arbitrary uintptr_t values to void * would be lossless.\n>>\n>> I don't know an architecture where this would bite us, but I wonder if\n>> there is a cleaner way.  Perhaps changing the type of the decoration\n>> member of struct decoration_entry in decorate.h to uintptr_t?\n> \n> In order to ensure \"void * -> uintptr_t -> void *\" roundtrip holds,\n> the implementation would guarantee that uintptr_t is wider than\n> void*, so what you suggest technically makes sense.  We should be\n> able to store any pointer in the field.  And we should be able to\n> store any value of an unsigned integral type that is narrower than\n> uintptr_t.\n> \n> But it somehow feels backwards in spirit to me, as the reason why we\n> use \"void *\" there in the decoration field is because we expect that\n> we'd have a pointer to some struture most of the time, and we have\n> to occasionally store a small integer there.\n\nYes, fast-export seems to be the only place that stores an integer as\na decoration.\n\n>  So I'd naively expect\n> that\n> \n> \tuint32_t mark = 23;\n> \tde->decoration = (void *)mark;\n> \n> would be a good way to store mark #23 in the field and\n> \n> \tuint32_t mark;\n> \tmark = (typeof(mark))de->decoration;\n> \n> would be a good way to read it off of the \"void *\" field.  Of\n> course, this assume that (void *) is at least as wide as 32-bit and\n> it also ignores the standard ;-)\n\nRight, it looks deceptively good and works fine if memory is flat and\nvalid values for pointers are in a contiguous range starting at zero.\nThe standard allows for other models as well, though.\n\n> This is an unrelated tangent but the mark-to-ptr() and ptr-to-mark()\n> implementations feel wasteful, especially when we worry about 32-bit\n> archs.  A naive platform implementation of\n> \n> \t(uint32_t *)mark - (uint32_t *)NULL;\n> \n> would be ((uintptr_t)mark) / 4, i.e. the de->decoration field will\n> always have two LSB clear and only utilize top 30-bit to represent\n> the value of mark.\n\nThat's right, but I don't see what's naive about it, or how a 32-bit\narchitecture could avoid wasting those two bits.\n\n\nUsing struct decorate in fast-export has the benefit of not\nrequiring separate allocations for individual entries.  Switching to\nstruct hashmap would require individual allocations.  Adding a\ncustom clone of decorate with a uint32_t payload would be an option.\n\nRené\n"},{"id":"347325","messageId":"xmqqbmdnhr8b.fsf@gitster-ct.c.googlers.com","threadId":"48446","inReplyTo":"654fac2a-8dca-7bee-2bab-a3986aa7e52d@web.de","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-11T02:16:04Z","receivedAt":"2018-05-11T02:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> But it somehow feels backwards in spirit to me, as the reason why we\n>> use \"void *\" there in the decoration field is because we expect that\n>> we'd have a pointer to some struture most of the time, and we have\n>> to occasionally store a small integer there.\n>\n> Yes, fast-export seems to be the only place that stores an integer as\n> a decoration.\n\nWith the decoration subsystem that might be the case, but I think\nwe have other codepaths where \"void * .util\" field in the structure\nis used to store (void *)1, expecting that a normal allocation will\nnever yield a pointer that is indistinguishable from that value.\n\n> Using struct decorate in fast-export has the benefit of not\n> requiring separate allocations for individual entries.  Switching to\n> struct hashmap would require individual allocations.  Adding a\n> custom clone of decorate with a uint32_t payload would be an option.\n\nAs long as we know uint32_t is no wider than uintptr_t, your patch\nshould be safe, shouldn't it?\n"},{"id":"347333","messageId":"xmqq7eoahk49.fsf@gitster-ct.c.googlers.com","threadId":"48446","inReplyTo":"xmqqbmdnhr8b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-11T04:49:42Z","receivedAt":"2018-05-11T04:49:49Z","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> René Scharfe <l.s.r@web.de> writes:\n>\n>>> But it somehow feels backwards in spirit to me, as the reason why we\n>>> use \"void *\" there in the decoration field is because we expect that\n>>> we'd have a pointer to some struture most of the time, and we have\n>>> to occasionally store a small integer there.\n>>\n>> Yes, fast-export seems to be the only place that stores an integer as\n>> a decoration.\n>\n> With the decoration subsystem that might be the case, but I think\n> we have other codepaths where \"void * .util\" field in the structure\n> is used to store (void *)1, expecting that a normal allocation will\n> never yield a pointer that is indistinguishable from that value.\n\nI was looking at a different topic and noticed that bisect.c uses\ncommit->util (which is of type \"void *\") to always store an int (it\nnever stores a pointer and there is no mixing).  This one is equally\nunportable as fast-export after your fix.\n\n"},{"id":"347336","messageId":"CACsJy8AN4nssu1+x0x9Kmz1BB1aXO7_UBFCjpyULMeC5K-Fzvw@mail.gmail.com","threadId":"48446","inReplyTo":"xmqq7eoahk49.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T06:19:59Z","receivedAt":"2018-05-11T06:20:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 11, 2018 at 6:49 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> René Scharfe <l.s.r@web.de> writes:\n>>\n>>>> But it somehow feels backwards in spirit to me, as the reason why we\n>>>> use \"void *\" there in the decoration field is because we expect that\n>>>> we'd have a pointer to some struture most of the time, and we have\n>>>> to occasionally store a small integer there.\n>>>\n>>> Yes, fast-export seems to be the only place that stores an integer as\n>>> a decoration.\n>>\n>> With the decoration subsystem that might be the case, but I think\n>> we have other codepaths where \"void * .util\" field in the structure\n>> is used to store (void *)1, expecting that a normal allocation will\n>> never yield a pointer that is indistinguishable from that value.\n>\n> I was looking at a different topic and noticed that bisect.c uses\n> commit->util (which is of type \"void *\") to always store an int (it\n> never stores a pointer and there is no mixing).  This one is equally\n> unportable as fast-export after your fix.\n>\n\nIn both cases we should be able to use commit-slab instead of\ncommit->util. We could go even further and kill \"util\" pointer but\nthat's more work.\n-- \nDuy\n"},{"id":"347346","messageId":"20180511085634.GC22086@sigill.intra.peff.net","threadId":"48446","inReplyTo":"CACsJy8AN4nssu1+x0x9Kmz1BB1aXO7_UBFCjpyULMeC5K-Fzvw@mail.gmail.com","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-11T08:56:34Z","receivedAt":"2018-05-11T08:56:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 11, 2018 at 08:19:59AM +0200, Duy Nguyen wrote:\n\n> On Fri, May 11, 2018 at 6:49 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Junio C Hamano <gitster@pobox.com> writes:\n> >\n> >> René Scharfe <l.s.r@web.de> writes:\n> >>\n> >>>> But it somehow feels backwards in spirit to me, as the reason why we\n> >>>> use \"void *\" there in the decoration field is because we expect that\n> >>>> we'd have a pointer to some struture most of the time, and we have\n> >>>> to occasionally store a small integer there.\n> >>>\n> >>> Yes, fast-export seems to be the only place that stores an integer as\n> >>> a decoration.\n> >>\n> >> With the decoration subsystem that might be the case, but I think\n> >> we have other codepaths where \"void * .util\" field in the structure\n> >> is used to store (void *)1, expecting that a normal allocation will\n> >> never yield a pointer that is indistinguishable from that value.\n> >\n> > I was looking at a different topic and noticed that bisect.c uses\n> > commit->util (which is of type \"void *\") to always store an int (it\n> > never stores a pointer and there is no mixing).  This one is equally\n> > unportable as fast-export after your fix.\n> >\n> \n> In both cases we should be able to use commit-slab instead of\n> commit->util. We could go even further and kill \"util\" pointer but\n> that's more work.\n\nI would love it if we could kill the util pointer in favor of\ncommit-slab. Unfortunately the fast-export case is decorating non-commit\nobjects, too.\n\nI'm not sure how possible/easy it would be to have an \"object slab\".\nSome complications are:\n\n  1. It would increase the size of \"struct tree\" and \"struct blob\" by at\n     least 4 bytes (possibly 8, due to alignment) to store the slab\n     index. Commits would stay the same, but my experience is that most\n     repositories have 5-10 times as many trees/blobs as commits.\n\n     Some of that memory might be reclaimable. E.g., if we moved\n     tree->buffer and tree->size into a slab, callers which don't use\n     them would actually come out ahead (and more callers than you might\n     expect could do this, since many only need to look at the tree\n     contents inside a single function).\n\n  2. If we allocate the indices for all types as a single sequence, then\n     callers who only care about a specific type will have very sparse\n     slabs (so they'll over-allocate, and it will have poor cache\n     behavior).\n\n     It might be possible to do something more clever. E.g., give each\n     object type its own index counter, but then for a full object-slab,\n     store per-type slabs and dereference obj->type to know which slab\n     to look in.\n\n     There'd be some complication with any_object. We'd probably need an\n     OBJ_NONE slab, and then to allocate a type-specific index when the\n     type is assigned. There's already a central point for this in\n     object_as_type(). But we'd also have to migrate any\n     previously-stored slab data (stored when it was OBJ_NONE), which\n     implies having a global list of slabs.\n\nSo I dunno. It seems do-able but complicated.\n\n-Peff\n"},{"id":"347355","messageId":"CACsJy8CnJYsgDe11tK4JzH2sDRuLwgoUz=HCso5qOhEfyZNH5Q@mail.gmail.com","threadId":"48446","inReplyTo":"20180511085634.GC22086@sigill.intra.peff.net","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T13:11:46Z","receivedAt":"2018-05-11T13:12:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 11, 2018 at 10:56 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, May 11, 2018 at 08:19:59AM +0200, Duy Nguyen wrote:\n>\n>> On Fri, May 11, 2018 at 6:49 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> > Junio C Hamano <gitster@pobox.com> writes:\n>> >\n>> >> René Scharfe <l.s.r@web.de> writes:\n>> >>\n>> >>>> But it somehow feels backwards in spirit to me, as the reason why we\n>> >>>> use \"void *\" there in the decoration field is because we expect that\n>> >>>> we'd have a pointer to some struture most of the time, and we have\n>> >>>> to occasionally store a small integer there.\n>> >>>\n>> >>> Yes, fast-export seems to be the only place that stores an integer as\n>> >>> a decoration.\n>> >>\n>> >> With the decoration subsystem that might be the case, but I think\n>> >> we have other codepaths where \"void * .util\" field in the structure\n>> >> is used to store (void *)1, expecting that a normal allocation will\n>> >> never yield a pointer that is indistinguishable from that value.\n>> >\n>> > I was looking at a different topic and noticed that bisect.c uses\n>> > commit->util (which is of type \"void *\") to always store an int (it\n>> > never stores a pointer and there is no mixing).  This one is equally\n>> > unportable as fast-export after your fix.\n>> >\n>>\n>> In both cases we should be able to use commit-slab instead of\n>> commit->util. We could go even further and kill \"util\" pointer but\n>> that's more work.\n>\n> I would love it if we could kill the util pointer in favor of\n> commit-slab. Unfortunately the fast-export case is decorating non-commit\n> objects, too.\n\nRight. The \"util\" thing was a side discussion.\n\nBack to fast-export, can we just allocate a new int on heap and point\nit there? Allocating small pieces becomes quite cheap and fast with\nmem-pool.h and we can avoid this storing integer in pointer business.\n-- \nDuy\n"},{"id":"347356","messageId":"20180511133419.GA2170@duynguyen.home","threadId":"48446","inReplyTo":"CACsJy8CnJYsgDe11tK4JzH2sDRuLwgoUz=HCso5qOhEfyZNH5Q@mail.gmail.com","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T13:34:19Z","receivedAt":"2018-05-11T13:34:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 11, 2018 at 03:11:46PM +0200, Duy Nguyen wrote:\n> Back to fast-export, can we just allocate a new int on heap and point\n> it there? Allocating small pieces becomes quite cheap and fast with\n> mem-pool.h and we can avoid this storing integer in pointer business.\n\nSomething like this seems to work, but we use 4-ish more bytes per\nobject, or 100MB overhead on a repo with 25M objects. I think it's a\nreasonable trade off.\n\n-- 8< --\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 530df12f05..de593035b1 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -21,6 +21,7 @@\n #include \"quote.h\"\n #include \"remote.h\"\n #include \"blob.h\"\n+#include \"mem-pool.h\"\n \n static const char *fast_export_usage[] = {\n \tN_(\"git fast-export [rev-list-opts]\"),\n@@ -38,6 +39,7 @@ static struct string_list extra_refs = STRING_LIST_INIT_NODUP;\n static struct refspec *refspecs;\n static int refspecs_nr;\n static int anonymize;\n+static struct mem_pool int_pool = MEM_POOL_INIT(2 * 1024 * 1024);\n \n static int parse_opt_signed_tag_mode(const struct option *opt,\n \t\t\t\t     const char *arg, int unset)\n@@ -156,20 +158,22 @@ static void anonymize_path(struct strbuf *out, const char *path,\n \t}\n }\n \n-/* Since intptr_t is C99, we do not use it here */\n-static inline uint32_t *mark_to_ptr(uint32_t mark)\n+static inline uint32_t ptr_to_mark(void *mark)\n {\n-\treturn ((uint32_t *)NULL) + mark;\n-}\n-\n-static inline uint32_t ptr_to_mark(void * mark)\n-{\n-\treturn (uint32_t *)mark - (uint32_t *)NULL;\n+\tif (!mark)\n+\t\tBUG(\"not marked!\");\n+\treturn *(uint32_t *)mark;\n }\n \n static inline void mark_object(struct object *object, uint32_t mark)\n {\n-\tadd_decoration(&idnums, object, mark_to_ptr(mark));\n+\tuint32_t *ptr = lookup_decoration(&idnums, object);\n+\n+\tif (!ptr)\n+\t\tptr = mem_pool_alloc(&int_pool, sizeof(uint32_t));\n+\n+\t*ptr = mark;\n+\tadd_decoration(&idnums, object, ptr);\n }\n \n static inline void mark_next_object(struct object *object)\ndiff --git a/fast-import.c b/fast-import.c\nindex 34edf3fb8f..ce5ce2081f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -300,8 +300,7 @@ static int global_argc;\n static const char **global_argv;\n \n /* Memory pools */\n-static struct mem_pool fi_mem_pool =  {NULL, 2*1024*1024 -\n-\t\t\t\t       sizeof(struct mp_block), 0 };\n+static struct mem_pool fi_mem_pool = MEM_POOL_INIT(2*1024*1024);\n \n /* Atom management */\n static unsigned int atom_table_sz = 4451;\ndiff --git a/mem-pool.h b/mem-pool.h\nindex 829ad58ecf..bccbd3f224 100644\n--- a/mem-pool.h\n+++ b/mem-pool.h\n@@ -21,6 +21,8 @@ struct mem_pool {\n \tsize_t pool_alloc;\n };\n \n+#define MEM_POOL_INIT(block_size) { NULL, (block_size) - sizeof(struct mp_block), 0 }\n+\n /*\n  * Alloc memory from the mem_pool.\n  */\n-- 8< --\n"},{"id":"347369","messageId":"20180511174237.GA19670@sigill.intra.peff.net","threadId":"48446","inReplyTo":"20180511133419.GA2170@duynguyen.home","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-11T17:42:37Z","receivedAt":"2018-05-11T17:42:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 11, 2018 at 03:34:19PM +0200, Duy Nguyen wrote:\n\n> On Fri, May 11, 2018 at 03:11:46PM +0200, Duy Nguyen wrote:\n> > Back to fast-export, can we just allocate a new int on heap and point\n> > it there? Allocating small pieces becomes quite cheap and fast with\n> > mem-pool.h and we can avoid this storing integer in pointer business.\n> \n> Something like this seems to work, but we use 4-ish more bytes per\n> object, or 100MB overhead on a repo with 25M objects. I think it's a\n> reasonable trade off.\n\nI'm not sure I agree. 4 bytes per object certainly isn't the end of the\nworld, but what was the problem we were solving in the first place? Just\nthat we weren't comfortable with the round-trip from uintptr_t to void\nand back? Is this actually a problem on real platforms? If not, it seems\nsilly to incur a run-time cost.\n\n-Peff\n"},{"id":"347438","messageId":"80397e16-8667-e0cd-4049-aad453d35e6f@web.de","threadId":"48446","inReplyTo":"20180511174237.GA19670@sigill.intra.peff.net","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-12T08:45:16Z","receivedAt":"2018-05-12T08:45:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.05.2018 um 19:42 schrieb Jeff King:\n> On Fri, May 11, 2018 at 03:34:19PM +0200, Duy Nguyen wrote:\n> \n>> On Fri, May 11, 2018 at 03:11:46PM +0200, Duy Nguyen wrote:\n>>> Back to fast-export, can we just allocate a new int on heap and point\n>>> it there? Allocating small pieces becomes quite cheap and fast with\n>>> mem-pool.h and we can avoid this storing integer in pointer business.\n>>\n>> Something like this seems to work, but we use 4-ish more bytes per\n>> object, or 100MB overhead on a repo with 25M objects. I think it's a\n>> reasonable trade off.\n> \n> I'm not sure I agree. 4 bytes per object certainly isn't the end of the\n> world, but what was the problem we were solving in the first place? Just\n> that we weren't comfortable with the round-trip from uintptr_t to void\n> and back? Is this actually a problem on real platforms? If not, it seems\n> silly to incur a run-time cost.\n\nStoring integer values in pointers is a trick that seems to have worked\nso far for fast-export.  A portable way to avoid that trick without\nrequiring more memory would be to use a union.\n\nOr we could roll our own custom hash map, as I mused in an earlier post.\nThat would duplicate quite a bit of code; are there reusable pieces\nhidden within that could be extracted into common functions?\n\n---\n builtin/fast-export.c | 105 ++++++++++++++++++++++++++++++++----------\n 1 file changed, 81 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 530df12f05..627b0032f3 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -14,7 +14,6 @@\n #include \"diffcore.h\"\n #include \"log-tree.h\"\n #include \"revision.h\"\n-#include \"decorate.h\"\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"parse-options.h\"\n@@ -71,9 +70,65 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n \treturn 0;\n }\n \n-static struct decoration idnums;\n+struct object_mark_entry {\n+\tconst struct object *base;\n+\tuint32_t mark;\n+};\n+\n+struct object_marks {\n+\tunsigned int size;\n+\tunsigned int nr;\n+\tstruct object_mark_entry *entries;\n+};\n+\n+static struct object_marks idnums;\n static uint32_t last_idnum;\n \n+static unsigned int hash_obj(const struct object *obj, unsigned int n)\n+{\n+\treturn sha1hash(obj->oid.hash) % n;\n+}\n+\n+static void set_object_mark(struct object_marks *n, const struct object *base,\n+\t\t\t    uint32_t mark)\n+{\n+\tunsigned int size = n->size;\n+\tstruct object_mark_entry *entries = n->entries;\n+\tunsigned int j = hash_obj(base, size);\n+\n+\twhile (entries[j].base) {\n+\t\tif (entries[j].base == base) {\n+\t\t\tentries[j].mark = mark;\n+\t\t\treturn;\n+\t\t}\n+\t\tif (++j >= size)\n+\t\t\tj = 0;\n+\t}\n+\tentries[j].base = base;\n+\tentries[j].mark = mark;\n+\tn->nr++;\n+}\n+\n+static void grow_object_marks(struct object_marks *n)\n+{\n+\tunsigned int i;\n+\tunsigned int old_size = n->size;\n+\tstruct object_mark_entry *old_entries = n->entries;\n+\n+\tn->size = (old_size + 1000) * 3 / 2;\n+\tn->entries = xcalloc(n->size, sizeof(n->entries[0]));\n+\tn->nr = 0;\n+\n+\tfor (i = 0; i < old_size; i++) {\n+\t\tconst struct object *base = old_entries[i].base;\n+\t\tuint32_t mark = old_entries[i].mark;\n+\n+\t\tif (mark)\n+\t\t\tset_object_mark(n, base, mark);\n+\t}\n+\tfree(old_entries);\n+}\n+\n static int has_unshown_parent(struct commit *commit)\n {\n \tstruct commit_list *parent;\n@@ -156,20 +211,13 @@ static void anonymize_path(struct strbuf *out, const char *path,\n \t}\n }\n \n-/* Since intptr_t is C99, we do not use it here */\n-static inline uint32_t *mark_to_ptr(uint32_t mark)\n-{\n-\treturn ((uint32_t *)NULL) + mark;\n-}\n-\n-static inline uint32_t ptr_to_mark(void * mark)\n-{\n-\treturn (uint32_t *)mark - (uint32_t *)NULL;\n-}\n-\n static inline void mark_object(struct object *object, uint32_t mark)\n {\n-\tadd_decoration(&idnums, object, mark_to_ptr(mark));\n+\tunsigned int nr = idnums.nr + 1;\n+\n+\tif (nr > idnums.size * 2 / 3)\n+\t\tgrow_object_marks(&idnums);\n+\treturn set_object_mark(&idnums, object, mark);\n }\n \n static inline void mark_next_object(struct object *object)\n@@ -179,10 +227,21 @@ static inline void mark_next_object(struct object *object)\n \n static int get_object_mark(struct object *object)\n {\n-\tvoid *decoration = lookup_decoration(&idnums, object);\n-\tif (!decoration)\n+\tunsigned int j;\n+\n+\t/* nothing to lookup */\n+\tif (!idnums.size)\n \t\treturn 0;\n-\treturn ptr_to_mark(decoration);\n+\tj = hash_obj(object, idnums.size);\n+\tfor (;;) {\n+\t\tstruct object_mark_entry *ref = idnums.entries + j;\n+\t\tif (ref->base == object)\n+\t\t\treturn ref->mark;\n+\t\tif (!ref->base)\n+\t\t\treturn 0;\n+\t\tif (++j == idnums.size)\n+\t\t\tj = 0;\n+\t}\n }\n \n static void show_progress(void)\n@@ -897,8 +956,7 @@ static void handle_tags_and_duplicates(void)\n static void export_marks(char *file)\n {\n \tunsigned int i;\n-\tuint32_t mark;\n-\tstruct decoration_entry *deco = idnums.entries;\n+\tstruct object_mark_entry *entry = idnums.entries;\n \tFILE *f;\n \tint e = 0;\n \n@@ -907,15 +965,14 @@ static void export_marks(char *file)\n \t\tdie_errno(\"Unable to open marks file %s for writing.\", file);\n \n \tfor (i = 0; i < idnums.size; i++) {\n-\t\tif (deco->base && deco->base->type == 1) {\n-\t\t\tmark = ptr_to_mark(deco->decoration);\n-\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", mark,\n-\t\t\t\toid_to_hex(&deco->base->oid)) < 0) {\n+\t\tif (entry->base && entry->base->type == 1) {\n+\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", entry->mark,\n+\t\t\t\t    oid_to_hex(&entry->base->oid)) < 0) {\n \t\t\t    e = 1;\n \t\t\t    break;\n \t\t\t}\n \t\t}\n-\t\tdeco++;\n+\t\tentry++;\n \t}\n \n \te |= ferror(f);\n-- \n2.17.0\n"},{"id":"347439","messageId":"01bb1946-df79-6448-663b-21a85cd44312@web.de","threadId":"48446","inReplyTo":"80397e16-8667-e0cd-4049-aad453d35e6f@web.de","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-12T08:49:36Z","receivedAt":"2018-05-12T08:49:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.05.2018 um 10:45 schrieb René Scharfe:\n> Or we could roll our own custom hash map, as I mused in an earlier post.\n> That would duplicate quite a bit of code; are there reusable pieces\n> hidden within that could be extracted into common functions?\n\nAt least it would allow us to save four bytes of padding per object on\nx64 by using a separate array for the hash map values; not sure how that\nwould impact performance, though.\n\n---\n builtin/fast-export.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 627b0032f3..086fcaf9ea 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -72,13 +72,13 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n \n struct object_mark_entry {\n \tconst struct object *base;\n-\tuint32_t mark;\n };\n \n struct object_marks {\n \tunsigned int size;\n \tunsigned int nr;\n \tstruct object_mark_entry *entries;\n+\tuint32_t *marks;\n };\n \n static struct object_marks idnums;\n@@ -98,14 +98,14 @@ static void set_object_mark(struct object_marks *n, const struct object *base,\n \n \twhile (entries[j].base) {\n \t\tif (entries[j].base == base) {\n-\t\t\tentries[j].mark = mark;\n+\t\t\tn->marks[j] = mark;\n \t\t\treturn;\n \t\t}\n \t\tif (++j >= size)\n \t\t\tj = 0;\n \t}\n \tentries[j].base = base;\n-\tentries[j].mark = mark;\n+\tn->marks[j] = mark;\n \tn->nr++;\n }\n \n@@ -114,19 +114,22 @@ static void grow_object_marks(struct object_marks *n)\n \tunsigned int i;\n \tunsigned int old_size = n->size;\n \tstruct object_mark_entry *old_entries = n->entries;\n+\tuint32_t *old_marks = n->marks;\n \n \tn->size = (old_size + 1000) * 3 / 2;\n \tn->entries = xcalloc(n->size, sizeof(n->entries[0]));\n+\tn->marks = xcalloc(n->size, sizeof(n->marks[0]));\n \tn->nr = 0;\n \n \tfor (i = 0; i < old_size; i++) {\n \t\tconst struct object *base = old_entries[i].base;\n-\t\tuint32_t mark = old_entries[i].mark;\n+\t\tuint32_t mark = old_marks[i];\n \n \t\tif (mark)\n \t\t\tset_object_mark(n, base, mark);\n \t}\n \tfree(old_entries);\n+\tfree(old_marks);\n }\n \n static int has_unshown_parent(struct commit *commit)\n@@ -236,7 +239,7 @@ static int get_object_mark(struct object *object)\n \tfor (;;) {\n \t\tstruct object_mark_entry *ref = idnums.entries + j;\n \t\tif (ref->base == object)\n-\t\t\treturn ref->mark;\n+\t\t\treturn idnums.marks[j];\n \t\tif (!ref->base)\n \t\t\treturn 0;\n \t\tif (++j == idnums.size)\n@@ -966,7 +969,7 @@ static void export_marks(char *file)\n \n \tfor (i = 0; i < idnums.size; i++) {\n \t\tif (entry->base && entry->base->type == 1) {\n-\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", entry->mark,\n+\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", idnums.marks[i],\n \t\t\t\t    oid_to_hex(&entry->base->oid)) < 0) {\n \t\t\t    e = 1;\n \t\t\t    break;\n-- \n2.17.0\n\n"},{"id":"347562","messageId":"xmqqwow7c90d.fsf@gitster-ct.c.googlers.com","threadId":"48446","inReplyTo":"80397e16-8667-e0cd-4049-aad453d35e6f@web.de","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-14T01:37:38Z","receivedAt":"2018-05-14T01:37:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Storing integer values in pointers is a trick that seems to have worked\n> so far for fast-export.  A portable way to avoid that trick without\n> requiring more memory would be to use a union.\n>\n> Or we could roll our own custom hash map, as I mused in an earlier post.\n> That would duplicate quite a bit of code; are there reusable pieces\n> hidden within that could be extracted into common functions?\n\nHmm, this together with your follow-up does not look too bad, but it\ndoes introduce quite a lot of code that could be refactored, so I am\nnot sure if I really like it or not.\n\n>\n> ---\n>  builtin/fast-export.c | 105 ++++++++++++++++++++++++++++++++----------\n>  1 file changed, 81 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 530df12f05..627b0032f3 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -14,7 +14,6 @@\n>  #include \"diffcore.h\"\n>  #include \"log-tree.h\"\n>  #include \"revision.h\"\n> -#include \"decorate.h\"\n>  #include \"string-list.h\"\n>  #include \"utf8.h\"\n>  #include \"parse-options.h\"\n> @@ -71,9 +70,65 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n>  \treturn 0;\n>  }\n>  \n> -static struct decoration idnums;\n> +struct object_mark_entry {\n> +\tconst struct object *base;\n> +\tuint32_t mark;\n> +};\n> +\n> +struct object_marks {\n> +\tunsigned int size;\n> +\tunsigned int nr;\n> +\tstruct object_mark_entry *entries;\n> +};\n> +\n> +static struct object_marks idnums;\n>  static uint32_t last_idnum;\n>  \n> +static unsigned int hash_obj(const struct object *obj, unsigned int n)\n> +{\n> +\treturn sha1hash(obj->oid.hash) % n;\n> +}\n> +\n> +static void set_object_mark(struct object_marks *n, const struct object *base,\n> +\t\t\t    uint32_t mark)\n> +{\n> +\tunsigned int size = n->size;\n> +\tstruct object_mark_entry *entries = n->entries;\n> +\tunsigned int j = hash_obj(base, size);\n> +\n> +\twhile (entries[j].base) {\n> +\t\tif (entries[j].base == base) {\n> +\t\t\tentries[j].mark = mark;\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tif (++j >= size)\n> +\t\t\tj = 0;\n> +\t}\n> +\tentries[j].base = base;\n> +\tentries[j].mark = mark;\n> +\tn->nr++;\n> +}\n> +\n> +static void grow_object_marks(struct object_marks *n)\n> +{\n> +\tunsigned int i;\n> +\tunsigned int old_size = n->size;\n> +\tstruct object_mark_entry *old_entries = n->entries;\n> +\n> +\tn->size = (old_size + 1000) * 3 / 2;\n> +\tn->entries = xcalloc(n->size, sizeof(n->entries[0]));\n> +\tn->nr = 0;\n> +\n> +\tfor (i = 0; i < old_size; i++) {\n> +\t\tconst struct object *base = old_entries[i].base;\n> +\t\tuint32_t mark = old_entries[i].mark;\n> +\n> +\t\tif (mark)\n> +\t\t\tset_object_mark(n, base, mark);\n> +\t}\n> +\tfree(old_entries);\n> +}\n> +\n>  static int has_unshown_parent(struct commit *commit)\n>  {\n>  \tstruct commit_list *parent;\n> @@ -156,20 +211,13 @@ static void anonymize_path(struct strbuf *out, const char *path,\n>  \t}\n>  }\n>  \n> -/* Since intptr_t is C99, we do not use it here */\n> -static inline uint32_t *mark_to_ptr(uint32_t mark)\n> -{\n> -\treturn ((uint32_t *)NULL) + mark;\n> -}\n> -\n> -static inline uint32_t ptr_to_mark(void * mark)\n> -{\n> -\treturn (uint32_t *)mark - (uint32_t *)NULL;\n> -}\n> -\n>  static inline void mark_object(struct object *object, uint32_t mark)\n>  {\n> -\tadd_decoration(&idnums, object, mark_to_ptr(mark));\n> +\tunsigned int nr = idnums.nr + 1;\n> +\n> +\tif (nr > idnums.size * 2 / 3)\n> +\t\tgrow_object_marks(&idnums);\n> +\treturn set_object_mark(&idnums, object, mark);\n>  }\n>  \n>  static inline void mark_next_object(struct object *object)\n> @@ -179,10 +227,21 @@ static inline void mark_next_object(struct object *object)\n>  \n>  static int get_object_mark(struct object *object)\n>  {\n> -\tvoid *decoration = lookup_decoration(&idnums, object);\n> -\tif (!decoration)\n> +\tunsigned int j;\n> +\n> +\t/* nothing to lookup */\n> +\tif (!idnums.size)\n>  \t\treturn 0;\n> -\treturn ptr_to_mark(decoration);\n> +\tj = hash_obj(object, idnums.size);\n> +\tfor (;;) {\n> +\t\tstruct object_mark_entry *ref = idnums.entries + j;\n> +\t\tif (ref->base == object)\n> +\t\t\treturn ref->mark;\n> +\t\tif (!ref->base)\n> +\t\t\treturn 0;\n> +\t\tif (++j == idnums.size)\n> +\t\t\tj = 0;\n> +\t}\n>  }\n>  \n>  static void show_progress(void)\n> @@ -897,8 +956,7 @@ static void handle_tags_and_duplicates(void)\n>  static void export_marks(char *file)\n>  {\n>  \tunsigned int i;\n> -\tuint32_t mark;\n> -\tstruct decoration_entry *deco = idnums.entries;\n> +\tstruct object_mark_entry *entry = idnums.entries;\n>  \tFILE *f;\n>  \tint e = 0;\n>  \n> @@ -907,15 +965,14 @@ static void export_marks(char *file)\n>  \t\tdie_errno(\"Unable to open marks file %s for writing.\", file);\n>  \n>  \tfor (i = 0; i < idnums.size; i++) {\n> -\t\tif (deco->base && deco->base->type == 1) {\n> -\t\t\tmark = ptr_to_mark(deco->decoration);\n> -\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", mark,\n> -\t\t\t\toid_to_hex(&deco->base->oid)) < 0) {\n> +\t\tif (entry->base && entry->base->type == 1) {\n> +\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", entry->mark,\n> +\t\t\t\t    oid_to_hex(&entry->base->oid)) < 0) {\n>  \t\t\t    e = 1;\n>  \t\t\t    break;\n>  \t\t\t}\n>  \t\t}\n> -\t\tdeco++;\n> +\t\tentry++;\n>  \t}\n>  \n>  \te |= ferror(f);\n"},{"id":"347708","messageId":"bcb24c9e-e10e-7f1b-6cbd-9cdd2508e795@web.de","threadId":"48446","inReplyTo":"xmqqwow7c90d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fast-export: avoid NULL pointer arithmetic","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-15T19:36:26Z","receivedAt":"2018-05-15T19:36:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.05.2018 um 03:37 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> Storing integer values in pointers is a trick that seems to have worked\n>> so far for fast-export.  A portable way to avoid that trick without\n>> requiring more memory would be to use a union.\n>>\n>> Or we could roll our own custom hash map, as I mused in an earlier post.\n>> That would duplicate quite a bit of code; are there reusable pieces\n>> hidden within that could be extracted into common functions?\n> \n> Hmm, this together with your follow-up does not look too bad, but it\n> does introduce quite a lot of code that could be refactored, so I am\n> not sure if I really like it or not.\n\nPutting keys and values into separate arrays probably causes stores and\nlookups to hit (at least) two cache lines instead of just one.  Not sure\nhow much of an impact that has on the overall performance (probably not\nmuch), but we'd need at least a perf test for that.\n\nAnd we have enough hash map implementations already.\n\nCasting should be good enough for now, to avoid the compiler warning.\n\nRené\n"}]}