On Thu, May 3, 2018 at 4:04 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > I wrote: >> Andrew Gierth <and...@tao11.riddles.org.uk> writes: >>> Isn't there a hidden assumption about endianness there?
Right, thanks. >> Yeah, I was wondering about that too, but does anyone actually run >> ARMs big-endian? I think all the distros I follow dropped big endian support in recent years, but that's no excuse. > After a bit more thought ... we could remove the magic constant, > along with any assumptions about endianness, by doing it like this: > > result = (pg_comp_crc32c_armv8(0, &data, sizeof(data)) == > pg_comp_crc32c_sb8(0, &data, sizeof(data))); > > Of course we'd eat a few more cycles during the test, but it's hard > to see that mattering. I was thinking of proposing this: - uint64 data = 42; + uint64 data = UINT64CONST(0x4242424242424242); ... and: - result = (pg_comp_crc32c_armv8(0, &data, sizeof(data)) == 0xdd439b0d); + result = (pg_comp_crc32c_armv8(0, &data, sizeof(data)) == 0x54eb145b); No strong preference though. > It strikes me also that, at least for debugging purposes, it's seriously > awful that you can't tell from outside what result this function got. > Certainly the outcome that "pg_comp_crc32c_armv8 executed but returned > the wrong answer" is not something that ought to go unremarked. > We could elog the results, but I'm not very sure what log level is > appropriate --- also, I doubt we want another log entry from every > launched process, so how to prevent that? I don't think *broken* CPUs are something we need to handle, are they? Hmm, I suppose there might be a hypothetical operating system or ARM chip that somehow doesn't raise SIGILL and returns garbage, and then it's nice to fall back to the software version (unless you're 1/2^32 unlucky). For debugging I was just putting a temporary elog in. Leaving one in there at one of the DEBUG levels seems reasonable though. -- Thomas Munro http://www.enterprisedb.com