Re: [PATCH v4 08/23] ewah: compressed bitmap implementation

From: Jeff King <peff@peff.net>
Date: Jan 23, 2014, 20:38 UTC
Message-ID: <20140123203830.GA4365@sigill.intra.peff.net>
In-Reply-To: <20140123200311.GA31920@sigill.intra.peff.net>

On Thu, Jan 23, 2014 at 03:03:11PM -0500, Jeff King wrote:

Show 14 quoted lines

> > My main worry about the patches is that they will probably run into
> > an analagous problem to the one that v1.7.12-rc0~1^2~2 (block-sha1:
> > avoid pointer conversion that violates alignment constraints,
> > 2012-07-22) solved.  By casting the pointer to (uint32_t *) we are
> > telling the compiler it is 32-bit aligned (C99 section 6.3.2.3).
> 
> Yeah, maybe. We go via memcpy, which takes a "void *", so that part is
> good. However, the new code looks like:
> 
>   foo = align_ntohl(*(uint32_t *)ptr);
> 
> I think this probably works in practice because align_ntohl is inlined,
> and any sane compiler will never actually load the variable. If we
> change the signature of align_ntohl, we can do this:

Actually, it is a little trickier than that. We actually take the address in the macro. So even without inlining, we end up casting to void. I still think this:

Show 6 quoted lines

>   uint32_t align_ntohl(void *ptr)
>   {
>           uint32_t x;
>           memcpy(x, ptr, sizeof(x));
>           return ntohl(x);
>   }

is a little more obvious, though. It does mean that everybody has to pass a pointer, though, and on platforms where non-aligned reads are OK, we do the cast ourselves. That means that:

  foo = align_ntohl(&bar);

will not be able to do any type-checking for "bar" (say, when we are pulling "bar" straight out of a packed struct). I don't know how much we care.

-Peff