{"thread":{"id":"44581","subject":"[PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64","startedAt":"2016-12-01T03:04:21Z","lastAt":"2016-12-01T04:30:34Z","messageCount":4,"participants":["Anders Kaseorg","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"306665","messageId":"alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu","threadId":"44581","inReplyTo":null,"subject":"[PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-12-01T03:04:07Z","receivedAt":"2016-12-01T03:04:21Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,\neven if we are cross-compiling for a different architecture.  Check\nthe __x86_64__ compiler macro instead.\n\nIn addition to fixing the cross compilation bug, this is needed to\npass the Debian build reproducibility test\n(https://tests.reproducible-builds.org/debian/index_variations.html).\n\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n---\n config.mak.uname | 5 -----\n xdiff/xutils.c   | 4 ++++\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b232908f8..2831a68c3 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -1,10 +1,8 @@\n # Platform specific Makefile tweaks based on uname detection\n \n uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')\n-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')\n uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')\n uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')\n-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')\n uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')\n \n ifdef MSVC\n@@ -17,9 +15,6 @@ endif\n # because maintaining the nesting to match is a pain.  If\n # we had \"elif\" things would have been much nicer...\n \n-ifeq ($(uname_M),x86_64)\n-\tXDL_FAST_HASH = YesPlease\n-endif\n ifeq ($(uname_S),OSF1)\n \t# Need this for u_short definitions et al\n \tBASIC_CFLAGS += -D_OSF_SOURCE\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 027192a1c..f46351fe4 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -264,6 +264,10 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n \treturn ha;\n }\n \n+#ifdef __x86_64__\n+#define XDL_FAST_HASH\n+#endif\n+\n #ifdef XDL_FAST_HASH\n \n #define REPEAT_BYTE(x)  ((~0ul / 0xff) * (x))\n-- \n2.11.0\n\n"},{"id":"306666","messageId":"20161201035645.mv7c7lr6rnsxokll@sigill.intra.peff.net","threadId":"44581","inReplyTo":"alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu","subject":"Re: [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T03:56:45Z","receivedAt":"2016-12-01T03:56:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote:\n\n> Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,\n> even if we are cross-compiling for a different architecture.  Check\n> the __x86_64__ compiler macro instead.\n> \n> In addition to fixing the cross compilation bug, this is needed to\n> pass the Debian build reproducibility test\n> (https://tests.reproducible-builds.org/debian/index_variations.html).\n\nI don't think this is a good approach to fix it. Right now XDL_FAST_HASH\nis a Makefile knob that can be turned by the user, and can be used\neither to explicitly enable or explicitly disable the feature.\n\nWith your patch, building with \"make XDL_FAST_HASH=Yes\" will still\nexplicitly enable it, but \"make XDL_FAST_HASH=\" would no longer disable\nit (because even if unset, the compiler would turn it on when it sees\n__x86_64__).\n\nAnd being able to turn it off is important; more on that in a second.\n\nSo I think if we wanted to auto-detect based on __x86_64__, we'd\nprobably need to be able to set it to \"auto\" or something, and then\n\n  #if defined(XDL_FAST_HASH_AUTO) && __x86_64__\n  #define XDL_FAST_HASH\n  #endif\n\nor something.\n\nHowever, I think this might be the tip of the iceberg. There are lots of\nMakefile knobs whose defaults are tweaked based on uname output. This\none caught you because you are cross-compiling across architectures, but\nin theory you could cross-compile for FreeBSD from Linux, or whatever.\n\nSo I suspect a better strategy in general is to just override the\nuname_* variables when cross-compiling.\n\n\nAll that being said, I actually think an easier fix for this particular\ncase might be to drop XDL_FAST_HASH entirely. It computes the hashes\nslightly faster, but its collision characteristics are much worse. About\n2 years ago I ran across a pathological diff that ran over 100x slower\nwith XDL_FAST_HASH:\n\n  http://public-inbox.org/git/20141222041944.GA441@peff.net/\n\nThe discussion veered into whether we should have a randomized hash\nsecured against DoS attacks. I played around with some alternatives, but\nnever found anything quite as fast for the \"normal\" case. And having\ndisabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for\nme.\n\nI'd be happy if somebody wanted to investigate other hash functions\nfurther. But barring that, I think we should drop XDL_FAST_HASH (or at\nthe very least stop turning it on by default) in the meantime. It's just\nnot a good tradeoff.\n\n-Peff\n"},{"id":"306667","messageId":"20161201035914.kftxb4vqmzcqed5r@sigill.intra.peff.net","threadId":"44581","inReplyTo":"alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu","subject":"Re: [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T03:59:14Z","receivedAt":"2016-12-01T03:59:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[resend; the original had an outdated address for Thomas, and I would\n definitely like his blessing before removing XDL_FAST_HASH].\n\nOn Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote:\n\n> Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,\n> even if we are cross-compiling for a different architecture.  Check\n> the __x86_64__ compiler macro instead.\n> \n> In addition to fixing the cross compilation bug, this is needed to\n> pass the Debian build reproducibility test\n> (https://tests.reproducible-builds.org/debian/index_variations.html).\n\nI don't think this is a good approach to fix it. Right now XDL_FAST_HASH\nis a Makefile knob that can be turned by the user, and can be used\neither to explicitly enable or explicitly disable the feature.\n\nWith your patch, building with \"make XDL_FAST_HASH=Yes\" will still\nexplicitly enable it, but \"make XDL_FAST_HASH=\" would no longer disable\nit (because even if unset, the compiler would turn it on when it sees\n__x86_64__).\n\nAnd being able to turn it off is important; more on that in a second.\n\nSo I think if we wanted to auto-detect based on __x86_64__, we'd\nprobably need to be able to set it to \"auto\" or something, and then\n\n  #if defined(XDL_FAST_HASH_AUTO) && __x86_64__\n  #define XDL_FAST_HASH\n  #endif\n\nor something.\n\nHowever, I think this might be the tip of the iceberg. There are lots of\nMakefile knobs whose defaults are tweaked based on uname output. This\none caught you because you are cross-compiling across architectures, but\nin theory you could cross-compile for FreeBSD from Linux, or whatever.\n\nSo I suspect a better strategy in general is to just override the\nuname_* variables when cross-compiling.\n\n\nAll that being said, I actually think an easier fix for this particular\ncase might be to drop XDL_FAST_HASH entirely. It computes the hashes\nslightly faster, but its collision characteristics are much worse. About\n2 years ago I ran across a pathological diff that ran over 100x slower\nwith XDL_FAST_HASH:\n\n  http://public-inbox.org/git/20141222041944.GA441@peff.net/\n\nThe discussion veered into whether we should have a randomized hash\nsecured against DoS attacks. I played around with some alternatives, but\nnever found anything quite as fast for the \"normal\" case. And having\ndisabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for\nme.\n\nI'd be happy if somebody wanted to investigate other hash functions\nfurther. But barring that, I think we should drop XDL_FAST_HASH (or at\nthe very least stop turning it on by default) in the meantime. It's just\nnot a good tradeoff.\n\n-Peff\n"},{"id":"306672","messageId":"alpine.DEB.2.10.1611302329010.20145@buzzword-bingo.mit.edu","threadId":"44581","inReplyTo":"20161201035914.kftxb4vqmzcqed5r@sigill.intra.peff.net","subject":"[PATCH] xdiff: Do not enable XDL_FAST_HASH by default","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-12-01T04:30:11Z","receivedAt":"2016-12-01T04:30:34Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Although XDL_FAST_HASH computes hashes slightly faster on some\narchitectures, its collision characteristics are much worse, resulting\nin some pathological diffs running over 100x slower\n(http://public-inbox.org/git/20141222041944.GA441@peff.net/).\n\nFurthermore, it was being enabled when ‘uname -m’ returns x86_64, even\nif we are cross-compiling for a different architecture.  This mistake\nwas also causing the Debian build reproducibility test to fail\n(https://tests.reproducible-builds.org/debian/index_variations.html).\nFuture architecture-specific definitions should be based on compiler\nmacros such as __x86_64__ rather than uname.\n\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n---\n\n[Oops, also resending for Thomas’s new email address.  Sorry for the \nspam.]\n\nOn Wed, 30 Nov 2016, Jeff King wrote:\n> However, I think this might be the tip of the iceberg. There are lots of\n> Makefile knobs whose defaults are tweaked based on uname output. This\n> one caught you because you are cross-compiling across architectures, but\n> in theory you could cross-compile for FreeBSD from Linux, or whatever.\n> \n> So I suspect a better strategy in general is to just override the\n> uname_* variables when cross-compiling.\n\nThe specific case of an i386 userspace on an x86_64 kernel is important \nindependently of the general cross compilation problem (in fact, the words \n“cross compilation” may not even really apply here).  And I don’t think \none should have to manually tweak the build for this setup, especially \nsince the compiler already has the needed information.\n\n> All that being said, I actually think an easier fix for this particular\n> case might be to drop XDL_FAST_HASH entirely.\n\nWorks for me.\n\nAnders\n\n Makefile         | 1 -\n config.mak.uname | 5 -----\n 2 files changed, 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex f53fcc90d..c237d4f91 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -341,7 +341,6 @@ all::\n # Define XDL_FAST_HASH to use an alternative line-hashing method in\n # the diff algorithm.  It gives a nice speedup if your processor has\n # fast unaligned word loads.  Does NOT work on big-endian systems!\n-# Enabled by default on x86_64.\n #\n # Define GIT_USER_AGENT if you want to change how git identifies itself during\n # network interactions.  The default is \"git/$(GIT_VERSION)\".\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b232908f8..2831a68c3 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -1,10 +1,8 @@\n # Platform specific Makefile tweaks based on uname detection\n \n uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')\n-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')\n uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')\n uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')\n-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')\n uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')\n \n ifdef MSVC\n@@ -17,9 +15,6 @@ endif\n # because maintaining the nesting to match is a pain.  If\n # we had \"elif\" things would have been much nicer...\n \n-ifeq ($(uname_M),x86_64)\n-\tXDL_FAST_HASH = YesPlease\n-endif\n ifeq ($(uname_S),OSF1)\n \t# Need this for u_short definitions et al\n \tBASIC_CFLAGS += -D_OSF_SOURCE\n-- \n2.11.0\n\n"}]}