Re: [PATCH 6/6] reflog-walk: stop using fake parents

From: Jeff King <peff@peff.net>
Date: Jul 10, 2017, 11:20 UTC
Message-ID: <20170710112000.vmxscs7fpop3c6pq@sigill.intra.peff.net>
In-Reply-To: <20170710094255.GA8170@untitled>

On Mon, Jul 10, 2017 at 09:42:55AM +0000, Eric Wong wrote:

Show 7 quoted lines

> > Thanks, I was able to reproduce with CFLAGS=-m32.
> 
> No problem and thanks for the quick response!  Latest pu is
> fine for me.
> 
> To notice similar errors sooner, I wonder if we should have a
> test target for 64-bit users to test with -m32 enabled, somehow.

It's "just"

  make CFLAGS=-m32 test

once you are setup for building 32-bit binaries. But that setup can be a bit tricky. On Debian, I have multi-arch set up, and then I have to "apt-get install zlib1g-dev:i386" etc for all the dependencies. Which isn't _too_ bad except that libcurl4-openssl-dev conflicts between the amd64 and i386 versions due to /usr/bin/curl-config. So I have to install the i386 package, do the -m32 build, and then reinstall the amd64 one to go back to my regular builds.

At any rate, the 32-bit thing in this instance[1] mostly just tickled the bug in a different way. Testing with either ASan or valgrind also detected the problem on 64-bit machines, and people (including me) do run those periodically. In this case the bug was years old. The reason it wasn't caught earlier is that there wasn't test coverage, and my series added it for other reasons. So you really _did_ catch it as soon as it hit pu, which seems pretty reasonable.

-Peff

[1] There are bugs that really are 32-bit-only, of course, and
    instrumenting tools wouldn't catch those. I think we had a bug a
    while back where the code mistook sizeof(uint64_t*) for
    sizeof(uint64_t). They're the same on 64-bit platforms, but not on
    32-bit ones. I do think such bugs are relatively rare, though.