git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] fast-export: avoid NULL pointer arithmetic

From
René Scharfe <l.s.r@web.de>
Date
May 10, 2018, 19:47 UTC
Message-ID
<654fac2a-8dca-7bee-2bab-a3986aa7e52d@web.de>
In-Reply-To
<xmqqo9hniy1v.fsf@gitster-ct.c.googlers.com>
Am 10.05.2018 um 12:51 schrieb Junio C Hamano:
Show 23 quoted lines
> René Scharfe <l.s.r@web.de> writes:
> 
>> The standard says about uintptr_t that "any valid pointer to void can
>> be converted to this type, then converted back to pointer to void, and
>> the result will compare equal to the original pointer".  So void * ->
>> uintptr_t -> void * is a proper roundtrip, but that doesn't imply that
>> casting arbitrary uintptr_t values to void * would be lossless.
>>
>> I don't know an architecture where this would bite us, but I wonder if
>> there is a cleaner way.  Perhaps changing the type of the decoration
>> member of struct decoration_entry in decorate.h to uintptr_t?
> 
> In order to ensure "void * -> uintptr_t -> void *" roundtrip holds,
> the implementation would guarantee that uintptr_t is wider than
> void*, so what you suggest technically makes sense.  We should be
> able to store any pointer in the field.  And we should be able to
> store any value of an unsigned integral type that is narrower than
> uintptr_t.
> 
> But it somehow feels backwards in spirit to me, as the reason why we
> use "void *" there in the decoration field is because we expect that
> we'd have a pointer to some struture most of the time, and we have
> to occasionally store a small integer there.

Yes, fast-export seems to be the only place that stores an integer as a decoration.

Show 14 quoted lines
>  So I'd naively expect
> that
> 
> 	uint32_t mark = 23;
> 	de->decoration = (void *)mark;
> 
> would be a good way to store mark #23 in the field and
> 
> 	uint32_t mark;
> 	mark = (typeof(mark))de->decoration;
> 
> would be a good way to read it off of the "void *" field.  Of
> course, this assume that (void *) is at least as wide as 32-bit and
> it also ignores the standard ;-)

Right, it looks deceptively good and works fine if memory is flat and valid values for pointers are in a contiguous range starting at zero. The standard allows for other models as well, though.

Show 9 quoted lines
> This is an unrelated tangent but the mark-to-ptr() and ptr-to-mark()
> implementations feel wasteful, especially when we worry about 32-bit
> archs.  A naive platform implementation of
> 
> 	(uint32_t *)mark - (uint32_t *)NULL;
> 
> would be ((uintptr_t)mark) / 4, i.e. the de->decoration field will
> always have two LSB clear and only utilize top 30-bit to represent
> the value of mark.

That's right, but I don't see what's naive about it, or how a 32-bit architecture could avoid wasting those two bits.

Using struct decorate in fast-export has the benefit of not requiring separate allocations for individual entries. Switching to struct hashmap would require individual allocations. Adding a custom clone of decorate with a uint32_t payload would be an option.

René
Previous: Junio C HamanoNext: Junio C Hamano
Message 5 of 16 in “fast-export: avoid NULL pointer arithmetic”
  1. fast-export: avoid NULL pointer arithmeticRené Scharfe, May 9, 2018
  2. Johannes SchindelinMay 9, 2018
  3. René ScharfeMay 10, 2018
  4. Junio C HamanoMay 10, 2018
  5. René ScharfeMay 10, 2018
  6. Junio C HamanoMay 11, 2018
  7. Junio C HamanoMay 11, 2018
  8. Duy NguyenMay 11, 2018
  9. Jeff KingMay 11, 2018
  10. Duy NguyenMay 11, 2018
  11. Duy NguyenMay 11, 2018
  12. Jeff KingMay 11, 2018
  13. René ScharfeMay 12, 2018
  14. René ScharfeMay 12, 2018
  15. Junio C HamanoMay 14, 2018
  16. René ScharfeMay 15, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.