Re: [PATCH 06/13] reftable: (de)serialization for the polymorphic record type.
- From
Jeff King <peff@peff.net>
- Date
- Sep 24, 2020, 07:21 UTC
- Message-ID
- <20200924072151.GC1851751@coredump.intra.peff.net>
- In-Reply-To
- <CAFQ2z_M9uBc+VArOVXg-hwTY8bu-gQQSL9JC6TJ5MuTCtxx=EQ@mail.gmail.com>
On Mon, Sep 21, 2020 at 03:13:05PM +0200, Han-Wen Nienhuys wrote:
Show 7 quoted lines
> The problem is actually triggered by the Git-provided put_be64()
> (which appears unused in the Git source code). The definition
>
> #define put_be64(p, v) do { *(uint64_t *)(p) = htonll(v); } while (0)
>
> is trying to reinterpret a char* as a uint64_t* , which is illegal in
> strict aliasing rules?Ah, thanks for finding that. I think this resolves the mystery I had around aliasing warnings with put_be32() in f39ad38410 (fast-export: use local array to store anonymized oid, 2020-06-25). I got confused because when I looked at the implementation of put_be32(), I stupidly looked at the NO_ALIGNED_LOADS fallback code, which indeed does not alias. But the one we use on x86 does.
I thought this would be OK because C allows aliasing through "char *" pointers. But I think it may only go the other way. I.e., you can type-pun a uint64_t as a char, but not the other way around. But then why doesn't basically every use of put_be32() and put_be64() complain?
Show 13 quoted lines
> I originally had
>
> +void put_be64(uint8_t *out, uint64_t v)
> +{
> + int i = sizeof(uint64_t);
> + while (i--) {
> + out[i] = (uint8_t)(v & 0xff);
> + v >>= 8;
> + }
> +}
>
> in my reftable library, which is portable. Is there a reason for the
> magic with htonll and friends?Presumably it was thought to be faster. This comes originally from the block-sha1 code in 660231aa97 (block-sha1: support for architectures with memory alignment restrictions, 2009-08-12). I don't know how it compares in practice, and especially these days.
Our fallback routines are similar to an unrolled version of what you wrote above.
-Peff