[resend; the original had an outdated address for Thomas, and I would definitely like his blessing before removing XDL_FAST_HASH].
On Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote: > Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64, > even if we are cross-compiling for a different architecture. Check > the __x86_64__ compiler macro instead. > > In addition to fixing the cross compilation bug, this is needed to > pass the Debian build reproducibility test > (https://tests.reproducible-builds.org/debian/index_variations.html). I don't think this is a good approach to fix it. Right now XDL_FAST_HASH is a Makefile knob that can be turned by the user, and can be used either to explicitly enable or explicitly disable the feature. With your patch, building with "make XDL_FAST_HASH=Yes" will still explicitly enable it, but "make XDL_FAST_HASH=" would no longer disable it (because even if unset, the compiler would turn it on when it sees __x86_64__). And being able to turn it off is important; more on that in a second. So I think if we wanted to auto-detect based on __x86_64__, we'd probably need to be able to set it to "auto" or something, and then #if defined(XDL_FAST_HASH_AUTO) && __x86_64__ #define XDL_FAST_HASH #endif or something. However, I think this might be the tip of the iceberg. There are lots of Makefile knobs whose defaults are tweaked based on uname output. This one caught you because you are cross-compiling across architectures, but in theory you could cross-compile for FreeBSD from Linux, or whatever. So I suspect a better strategy in general is to just override the uname_* variables when cross-compiling. All that being said, I actually think an easier fix for this particular case might be to drop XDL_FAST_HASH entirely. It computes the hashes slightly faster, but its collision characteristics are much worse. About 2 years ago I ran across a pathological diff that ran over 100x slower with XDL_FAST_HASH: http://public-inbox.org/git/20141222041944.ga...@peff.net/ The discussion veered into whether we should have a randomized hash secured against DoS attacks. I played around with some alternatives, but never found anything quite as fast for the "normal" case. And having disabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for me. I'd be happy if somebody wanted to investigate other hash functions further. But barring that, I think we should drop XDL_FAST_HASH (or at the very least stop turning it on by default) in the meantime. It's just not a good tradeoff. -Peff
