[PATCH] Travis: also test on 32-bit Linux

[PATCH] Travis: also test on 32-bit Linux

[Johannes Schindelin]

When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.

This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.

Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.

A big thank you to Lars Schneider without whose help this patch would
not have happened.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/travis-32-bit-v1
Fetch-It-Via: git fetch https://github.com/dscho/git travis-32-bit-v1

 .travis.yml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f68..87d9e9051a6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,17 @@ env:
 
 matrix:
   include:
+    - env: Linux32
+      os: linux
+      compiler: clang
+      sudo: required
+      services:
+        - docker
+      before_install:
+        - docker pull daald/ubuntu32:xenial
+      before_script:
+      script:
+        - "sudo docker run -i -v \"${PWD}:/usr/src/git\" daald/ubuntu32:xenial /bin/bash -c \"linux32 --32bit i386 sh -c 'apt update && apt install -y build-essential libcurl4-openssl-dev libssl-dev libexpat-dev gettext python && cd /usr/src/git && DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\\\"--timer --jobs 3 --state=failed,slow,save\\\" GIT_TEST_OPTS=--verbose-log GIT_TEST_CLONE_2GB=YesPlease make -j2 test'\""
     - env: Documentation
       os: linux
       compiler: clang

base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
-- 
2.12.0.windows.1.3.g8a117c48243

Re: [PATCH] Travis: also test on 32-bit Linux

[Johannes Schindelin]

Hi,

On Tue, 28 Feb 2017, Johannes Schindelin wrote:

> When Git v2.9.1 was released, it had a bug that showed only on Windows
> and on 32-bit systems: our assumption that `unsigned long` can hold
> 64-bit values turned out to be wrong.
> 
> This could have been caught earlier if we had a Continuous Testing set
> up that includes a build and test run on 32-bit Linux.
> 
> Let's do this (and take care of the Windows build later). This patch
> asks Travis CI to install a Docker image with 32-bit libraries and then
> goes on to build and test Git using this 32-bit setup.

For the record, I first tested this with the LONG_IS_32BIT flag forced to
`true` so that the date tests must fail. They did fail as expected (search
for "not ok" in this output):

	https://travis-ci.org/git/git/jobs/206199002

(I actually cannot see the log right now, Travis seems to be under heavy
load...)

A much cleaned up version that does not force that LONG_IS_32BIT produced
this log:

	https://travis-ci.org/git/git/jobs/206228708

(Same here, my browser fails to load the log, probably because Travis
experiences quite high a load...)

Please note that this approach is not without problems. It would appear
that Travis currently has serious problems to even *reach* the Docker Hub,
and therefore cannot download the Docker image:

	https://travis-ci.org/git/git/jobs/206292947#L6

Ciao,
Johannes

Re: [PATCH] Travis: also test on 32-bit Linux

[Junio C Hamano]

On Tue, Feb 28, 2017 at 11:17 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> .... This patch
> asks Travis CI to install a Docker image with 32-bit libraries and then
> goes on to build and test Git using this 32-bit setup.
>
> A big thank you to Lars Schneider without whose help this patch would
> not have happened.

This has been in 'pu' for a few days, and
https://travis-ci.org/git/git/builds shows that we have
a new build job running successfully.

Good job ;-)

Thanks.

[PATCH v1] Travis: also test on 32-bit Linux

[Lars Schneider]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.

This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.

Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---

Thanks for the patch Dscho!

The patch looks good to me in general but I want to propose the following
changes:

(1) Move all the docker magic into a dedicated file "ci/run-linux-32-build.sh"
    This way people should be able to run this build on their local machines
    without TravisCI. However, I haven't tested this.

(2) The docker build command inherits the Git test environment variables.
    This way we use the same environment variables as in all other TravisCI
    builds (plus it would use *your* variables if you run it locally).

(3) Silence the apt update/git output as is it clutters the log.
    I did not silence stderr output!

(4) Remove (to my knowledge) superfluous "compiler: clang" in the Linux32 job.

I added my sign-off. I hope this is the right thing to do in this "I took your
patch and changed it to suggest an improvement" situation.

You can see a successful run here:
https://travis-ci.org/larsxschneider/git/jobs/206945950


One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr  0.00 sys +  0.00 cusr  0.00 csys ...
Has anyone an idea why that happens and how we can disable it?


Cheers,
Lars


Notes:
    Base Ref:
    Web-Diff: https://github.com/larsxschneider/git/commit/82995ed59c
    Checkout: git fetch https://github.com/larsxschneider/git travisci/linux32-v1 && git checkout 82995ed59c

 .travis.yml             |  9 +++++++++
 ci/run-linux32-build.sh | 21 +++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100755 ci/run-linux32-build.sh

diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f6..c8c789c437 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,15 @@ env:

 matrix:
   include:
+    - env: Linux32
+      os: linux
+      sudo: required
+      services:
+        - docker
+      before_install:
+        - docker pull daald/ubuntu32:xenial
+      before_script:
+      script: ci/run-linux32-build.sh
     - env: Documentation
       os: linux
       compiler: clang
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
new file mode 100755
index 0000000000..b892fbdc9e
--- /dev/null
+++ b/ci/run-linux32-build.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+#
+# Build and test Git in a docker container running a 32-bit Ubuntu Linux
+#
+
+set -e
+
+APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
+"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
+
+TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
+"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
+"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
+"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
+
+TEST_GIT_CMD="linux32 --32bit i386 sh -c "\
+"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
+
+sudo docker run \
+    --interactive --volume "${PWD}:/usr/src/git" \
+    daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"

base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
--
2.11.1

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Johannes Schindelin]

Hi Lars,

On Thu, 2 Mar 2017, Lars Schneider wrote:

> The patch looks good to me in general but I want to propose the following
> changes:

I know you are using your script to generate this mail, but I would have
liked to see v2 in the subject ;-)

> (1) Move all the docker magic into a dedicated file
> "ci/run-linux-32-build.sh" This way people should be able to run this
> build on their local machines without TravisCI. However, I haven't
> tested this.

I considered this, but there is serious overlap: the `docker pull` call
and the `docker run` call *have* to refer to the same image. It's very
easy for them to get out of sync if you have that information in two
files. Maybe make that an option of the script, defaulting to
daald/ubuntu32:xenial?

BTW speaking of Docker: it would be nicer if there was a Docker image that
already had the build-essentials installed, to save on startup time. But I
did not find any that was reasonably up-to-date.

> (2) The docker build command inherits the Git test environment
> variables.  This way we use the same environment variables as in all
> other TravisCI builds (plus it would use *your* variables if you run it
> locally).

Good!

> (3) Silence the apt update/git output as is it clutters the log.  I did
> not silence stderr output!

Also good!

> (4) Remove (to my knowledge) superfluous "compiler: clang" in the
> Linux32 job.

I copied that from one of your experimental .travis.yml patches ;-)

> I added my sign-off. I hope this is the right thing to do in this "I
> took your patch and changed it to suggest an improvement" situation.

Absolutely. Thank you for taking it from here.

> One thing that still bugs me: In the Linux32 environment prove adds the
> CPU times to every test run: ( 0.02 usr  0.00 sys +  0.00 cusr  0.00
> csys ...  Has anyone an idea why that happens and how we can disable it?

I have no idea.

> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> new file mode 100755
> index 0000000000..b892fbdc9e
> --- /dev/null
> +++ b/ci/run-linux32-build.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +#
> +# Build and test Git in a docker container running a 32-bit Ubuntu Linux
> +#
> +
> +set -e

Is this really necessary? I really like to avoid `set -e`, in particular
when we do pretty much everything in && chains anyway.

> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> +
> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> +
> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> +
> +sudo docker run \
> +    --interactive --volume "${PWD}:/usr/src/git" \
> +    daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"

Hmm. Since it is a script now, it would be more readable this way, I
think:

sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
linux32 --32bit i386 sh -c '
	: update packages first &&
	apt update >/dev/null &&
	apt install -y build-essential libcurl4-openssl-dev libssl-dev \
		libexpat-dev gettext python >/dev/null &&

	: now build and test &&
	cd /usr/src/git &&
	DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
	GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
	GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
	GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
	make -j2 test
'

This is completely untested (pun intended ;-))...

Ciao,
Dscho

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Lars Schneider]

> On 02 Mar 2017, at 12:24, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Lars,
> 
> On Thu, 2 Mar 2017, Lars Schneider wrote:
> 
>> The patch looks good to me in general but I want to propose the following
>> changes:
> 
> I know you are using your script to generate this mail, but I would have
> liked to see v2 in the subject ;-)

Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in 
my email client. Now I am wondering... is the next version v2 or v3 :D


>> (1) Move all the docker magic into a dedicated file
>> "ci/run-linux-32-build.sh" This way people should be able to run this
>> build on their local machines without TravisCI. However, I haven't
>> tested this.
> 
> I considered this, but there is serious overlap: the `docker pull` call
> and the `docker run` call *have* to refer to the same image. It's very
> easy for them to get out of sync if you have that information in two
> files. Maybe make that an option of the script, defaulting to
> daald/ubuntu32:xenial?

Right. I missed that. How about something like that?

      before_install:
        - ci/run-linux32-build.sh --pull-container
      before_script:
      script: ci/run-linux32-build.sh


> BTW speaking of Docker: it would be nicer if there was a Docker image that
> already had the build-essentials installed, to save on startup time. But I
> did not find any that was reasonably up-to-date.

True. But installing everything just takes a minute and we don't need to
maintain anything...


>> +set -e
> 
> Is this really necessary? I really like to avoid `set -e`, in particular
> when we do pretty much everything in && chains anyway.

Agreed, not really necessary here as we just invoke one command.
Out of curiosity: Why do you try to avoid it? I set it by default in all 
my scripts.


>> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
>> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
>> +
>> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
>> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
>> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
>> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
>> +
>> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
>> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
>> +
>> +sudo docker run \
>> +    --interactive --volume "${PWD}:/usr/src/git" \
>> +    daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
> 
> Hmm. Since it is a script now, it would be more readable this way, I
> think:
> 
> sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
> linux32 --32bit i386 sh -c '
> 	: update packages first &&
> 	apt update >/dev/null &&
> 	apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> 		libexpat-dev gettext python >/dev/null &&
> 
> 	: now build and test &&
> 	cd /usr/src/git &&
> 	DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> 	GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> 	GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> 	GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> 	make -j2 test
> '

That looks better! I'll try it!

- Lars

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Johannes Schindelin]

Hi Lars,


On Thu, 2 Mar 2017, Lars Schneider wrote:

> > On 02 Mar 2017, at 12:24, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 
> > On Thu, 2 Mar 2017, Lars Schneider wrote:
> > 
> >> The patch looks good to me in general but I want to propose the
> >> following changes:
> > 
> > I know you are using your script to generate this mail, but I would
> > have liked to see v2 in the subject ;-)
> 
> Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in my
> email client. Now I am wondering... is the next version v2 or v3 :D

Since there was no v2, the next one should *definitely* be v2... ;-)

> >> (1) Move all the docker magic into a dedicated file
> >> "ci/run-linux-32-build.sh" This way people should be able to run this
> >> build on their local machines without TravisCI. However, I haven't
> >> tested this.
> > 
> > I considered this, but there is serious overlap: the `docker pull`
> > call and the `docker run` call *have* to refer to the same image. It's
> > very easy for them to get out of sync if you have that information in
> > two files. Maybe make that an option of the script, defaulting to
> > daald/ubuntu32:xenial?
> 
> Right. I missed that. How about something like that?
> 
>       before_install:
>         - ci/run-linux32-build.sh --pull-container
>       before_script:
>       script: ci/run-linux32-build.sh

I'd prefer

	before_install:
	  - docker pull daald/ubuntu32:xenial
	before_script:
	script: ci/run-linux32-build.sh daald/ubuntu32:xenial

> > BTW speaking of Docker: it would be nicer if there was a Docker image
> > that already had the build-essentials installed, to save on startup
> > time. But I did not find any that was reasonably up-to-date.
> 
> True. But installing everything just takes a minute and we don't need to
> maintain anything...

And when there are network problems (like there were on Tuesday, right
when I developed the first v1 of this patch) then we have another set of
problems that make Travis fail. Even if the code in the PR or branch is
actually good. I'd like to avoid false positives, if possible.

> >> +set -e
> > 
> > Is this really necessary? I really like to avoid `set -e`, in
> > particular when we do pretty much everything in && chains anyway.
> 
> Agreed, not really necessary here as we just invoke one command.  Out of
> curiosity: Why do you try to avoid it? I set it by default in all my
> scripts.

I try to avoid it because it encourages a style that omits helpful error
messages.

> >> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> >> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> >> +
> >> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> >> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> >> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> >> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> >> +
> >> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> >> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> >> +
> >> +sudo docker run \
> >> +    --interactive --volume "${PWD}:/usr/src/git" \
> >> +    daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
> > 
> > Hmm. Since it is a script now, it would be more readable this way, I
> > think:
> > 
> > sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
> > linux32 --32bit i386 sh -c '
> > 	: update packages first &&
> > 	apt update >/dev/null &&
> > 	apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> > 		libexpat-dev gettext python >/dev/null &&
> > 
> > 	: now build and test &&
> > 	cd /usr/src/git &&
> > 	DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> > 	GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> > 	GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> > 	GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> > 	make -j2 test
> > '
> 
> That looks better! I'll try it!

Thanks!
Dscho

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Ramsay Jones]

On 02/03/17 11:24, Johannes Schindelin wrote:
> Hi Lars,
> 
> On Thu, 2 Mar 2017, Lars Schneider wrote:
> 
[snip]
>> One thing that still bugs me: In the Linux32 environment prove adds the
>> CPU times to every test run: ( 0.02 usr  0.00 sys +  0.00 cusr  0.00
>> csys ...  Has anyone an idea why that happens and how we can disable it?
> 
> I have no idea.

I have no idea either, but it is not unique to this 32bit Linux, but
rather the version of prove. For example, I am seeing this on Linux
Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
this. (They used different Ubuntu LTS releases).

[Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]

ATB,
Ramsay Jones

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Christian Couder]

On Thu, Mar 2, 2017 at 3:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
>> >> +set -e
>> >
>> > Is this really necessary? I really like to avoid `set -e`, in
>> > particular when we do pretty much everything in && chains anyway.
>>
>> Agreed, not really necessary here as we just invoke one command.  Out of
>> curiosity: Why do you try to avoid it? I set it by default in all my
>> scripts.
>
> I try to avoid it because it encourages a style that omits helpful error
> messages.

Yeah, we prefer to define and use a die() function like this:

die () {
    printf >&2 '%s\n' "$*"
    exit 1
}

do_something || die "meaningful error message"

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 02 Mar 2017, at 12:24, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> 
>> Hi Lars,
>> 
>> On Thu, 2 Mar 2017, Lars Schneider wrote:
>> 
>>> The patch looks good to me in general but I want to propose the following
>>> changes:
>> 
>> I know you are using your script to generate this mail, but I would have
>> liked to see v2 in the subject ;-)
>
> Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in 
> my email client. Now I am wondering... is the next version v2 or v3 :D

Another question is which v3 people mean in the discussion, when you
and Dscho work on improvements at the same time and each post the
"next" version marked as "v3", and they comment on one of them?

Between "v2" and "v3", it would not make too much difference to the
readership, when it is clear that two people are working to produce
competing improvements without much coordination (i.e. lack of "ok,
I'll send a reroll marked as vX in a minite"--"ok, I'll wait and
comment on it" exchange).  People watching from the sideline know
"ah this is v3 from Lars which is the highest numbered one from him
on this topic".  As long as you do not mark your next one "v1",
you'd be OK ;-).

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Mar 2017, Junio C Hamano wrote:

> Another question is which v3 people mean in the discussion, when you
> and Dscho work on improvements at the same time and each post the
> "next" version marked as "v3", and they comment on one of them?

But then, Lars & I communicate in a more direct way than the mailing list
when discussing teeny tiny details such as: "could you have a look at my
mail" or "how would you change .travis.yml to do XYZ".

With that level of communication, there is almost no danger of two v3s.

Ciao,
Johannes

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 2 Mar 2017, Junio C Hamano wrote:
>
>> Another question is which v3 people mean in the discussion, when you
>> and Dscho work on improvements at the same time and each post the
>> "next" version marked as "v3", and they comment on one of them?
>
> But then, Lars & I communicate in a more direct way than the mailing list
> when discussing teeny tiny details such as: "could you have a look at my
> mail" or "how would you change .travis.yml to do XYZ".
>
> With that level of communication, there is almost no danger of two v3s.

Sure, that is true between two (or more) people who communicate
privately.  The issue you raised on Lars's "v1" and your original
unversioned one can be seen either (1) as similar to having two v1's
or (2) having (an implicit) v0 and v1 that won't cause confusion.

As I said already, because the v0 and v1 (or v1 and v1) came from
two different people, it's not such a big deal if Lars named his
first one v1 or v2, just like it won't cause problem if his reroll
were done as v2 or v3.  

I see he did v2 which you Acked in a different thread.  Will replace
what's been on 'pu' and running with Travis the past few days with
it.  Let's wait for one or more Travis cycles and then merge it to
'next'.

Thanks.

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I see he did v2 which you Acked in a different thread.  Will replace
> what's been on 'pu' and running with Travis the past few days with
> it.  Let's wait for one or more Travis cycles and then merge it to
> 'next'.

https://travis-ci.org/git/git/jobs/207517043 is an output from 'pu'
with the v2 patch; it seems to have fell into a funny state.  The
output ends like so:

    No output has been received in the last 10m0s, this potentially
    indicates a stalled build or something wrong with the build itself.

I only recently started looking at Travis logs, so I cannot tell if
it is just a usual flake (e.g. some builds a few days ago seems to
have failed due to not being able to check out the tree being
tested, which I do not think is our fault) that we shouldn't worry
about, or if it is a sign of a real problem.

Unrelated to linux-32, the same build has hard failure with Apple
clang in t0021 with the rot13-filter.pl thing, by the way.

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I only recently started looking at Travis logs, so I cannot tell if
> it is just a usual flake (e.g. some builds a few days ago seems to
> have failed due to not being able to check out the tree being
> tested, which I do not think is our fault) that we shouldn't worry
> about, or if it is a sign of a real problem.

Tonight's pushout also seems to stall the same way. Dscho's
unversioned one didn't exhibit the problem?
https://travis-ci.org/git/git/jobs/206811396

> Unrelated to linux-32, the same build has hard failure with Apple
> clang in t0021 with the rot13-filter.pl thing, by the way.

This one may be a Heisenbug which may indicate some raciness in the
clean/smudge filter protocol.
The latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to
have passed OK.

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Lars Schneider]

> On 04 Mar 2017, at 05:11, Junio C Hamano <gitster@pobox.com> wrote:
> 
> On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> I only recently started looking at Travis logs, so I cannot tell if
>> it is just a usual flake (e.g. some builds a few days ago seems to
>> have failed due to not being able to check out the tree being
>> tested, which I do not think is our fault) that we shouldn't worry
>> about, or if it is a sign of a real problem.
> 
> Tonight's pushout also seems to stall the same way. Dscho's
> unversioned one didn't exhibit the problem?
> https://travis-ci.org/git/git/jobs/206811396

Did Travis find our first 32bit bug? I briefly looked into it
and the following new topic on pu seems to cause the issue:

http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/
https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd

The "git log" call in the new 4211 test fails with:
*** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 ***

More output here:
https://travis-ci.org/larsxschneider/git/builds/207715343



>> Unrelated to linux-32, the same build has hard failure with Apple
>> clang in t0021 with the rot13-filter.pl thing, by the way.
> 
> This one may be a Heisenbug which may indicate some raciness in the
> clean/smudge filter protocol.
> The latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to
> have passed OK.

I agree, there seems to be some kind of race condition in
"t0021.15 - required process filter should filter data". I try to
look into it.


- Lars

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Vegard Nossum]

On 04/03/2017 18:23, Lars Schneider wrote:
> Did Travis find our first 32bit bug? I briefly looked into it
> and the following new topic on pu seems to cause the issue:
>
> http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/
> https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd
>
> The "git log" call in the new 4211 test fails with:
> *** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 ***
>
> More output here:
> https://travis-ci.org/larsxschneider/git/builds/207715343

It looks like it's hitting the bug the patch is supposed to fix.

Are you quite sure it's running the "git" binary that was just built (as
opposed to e.g. the system binary installed inside the container)?


Vegard

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Vegard Nossum]

On 04/03/2017 19:08, Vegard Nossum wrote:
> On 04/03/2017 18:23, Lars Schneider wrote:
>> Did Travis find our first 32bit bug? I briefly looked into it
>> and the following new topic on pu seems to cause the issue:
>>
>> http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/
>>
>> https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd
>>
>>
>> The "git log" call in the new 4211 test fails with:
>> *** Error in `/usr/src/git/git': malloc: top chunk is corrupt:
>> 0x09ff4a78 ***
>>
>> More output here:
>> https://travis-ci.org/larsxschneider/git/builds/207715343
>
> It looks like it's hitting the bug the patch is supposed to fix.
>
> Are you quite sure it's running the "git" binary that was just built (as
> opposed to e.g. the system binary installed inside the container)?

Nevermind, I can reproduce it locally.

==10205== Invalid write of size 4
==10205==    at 0x4031ED2: memcpy (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==10205==    by 0x811C4B0: memcpy (string3.h:53)
==10205==    by 0x811C4B0: range_set_copy.isra.7 (line-log.c:46)
==10205==    by 0x811C51B: line_log_data_copy_one (line-log.c:628)
==10205==    by 0x811C55D: line_log_data_copy (line-log.c:642)
==10205==    by 0x811C5E3: add_line_range (line-log.c:708)
==10205==    by 0x811D767: line_log_init (line-log.c:745)
==10205==    by 0x808B1CD: cmd_log_init_finish (log.c:204)
==10205==    by 0x808C9C8: cmd_log_init (log.c:213)
==10205==    by 0x808C9C8: cmd_log (log.c:681)
==10205==    by 0x804CF14: run_builtin (git.c:373)
==10205==    by 0x804CF14: handle_builtin (git.c:574)
==10205==    by 0x804D264: run_argv (git.c:626)
==10205==    by 0x804D264: cmd_main (git.c:703)
==10205==    by 0x804C448: main (common-main.c:43)
==10205==  Address 0x4cde9c8 is 0 bytes after a block of size 1,600 alloc'd
==10205==    at 0x402D17C: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==10205==    by 0x402F370: realloc (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==10205==    by 0x819CC0F: xrealloc (wrapper.c:137)
==10205==    by 0x811C1C8: range_set_grow (line-log.c:21)
==10205==    by 0x811C499: range_set_init (line-log.c:32)
==10205==    by 0x811C499: range_set_copy.isra.7 (line-log.c:45)
==10205==    by 0x811C51B: line_log_data_copy_one (line-log.c:628)
==10205==    by 0x811C55D: line_log_data_copy (line-log.c:642)
==10205==    by 0x811C5E3: add_line_range (line-log.c:708)
==10205==    by 0x811D767: line_log_init (line-log.c:745)
==10205==    by 0x808B1CD: cmd_log_init_finish (log.c:204)
==10205==    by 0x808C9C8: cmd_log_init (log.c:213)
==10205==    by 0x808C9C8: cmd_log (log.c:681)
==10205==    by 0x804CF14: run_builtin (git.c:373)
==10205==    by 0x804CF14: handle_builtin (git.c:574)

At a glance, looks like range_set_copy() is using
sizeof(struct range_set) == 12, but
range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using
sizeof(rs->range) == 8.


Vegard

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

On 04/03/2017 20:49, Vegard Nossum wrote:
> On 04/03/2017 19:08, Vegard Nossum wrote:
>> On 04/03/2017 18:23, Lars Schneider wrote:
>>> Did Travis find our first 32bit bug? I briefly looked into it
>>> and the following new topic on pu seems to cause the issue:
>>>
>>> http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/
>>>
>>>
>>> https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd
>>>
>>>
>>>
>>> The "git log" call in the new 4211 test fails with:
>>> *** Error in `/usr/src/git/git': malloc: top chunk is corrupt:
>>> 0x09ff4a78 ***
>>>
>>> More output here:
>>> https://travis-ci.org/larsxschneider/git/builds/207715343
[...]
> At a glance, looks like range_set_copy() is using
> sizeof(struct range_set) == 12, but
> range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using
> sizeof(rs->range) == 8.

Attached patch seems to fix it -- basically, range_set_copy() is trying
to copy more than it should. It was uncovered with the test case from
Allan's commit because it's creating enough ranges to overflow the
initial allocation on 32-bit.


Vegard

[-- Attachment #2: range_set_copy.patch --]
[-- Type: text/x-patch, Size: 511 bytes --]

diff --git a/line-log.c b/line-log.c
index 951029665..cb0dc1110 100644
--- a/line-log.c
+++ b/line-log.c
@@ -43,7 +43,7 @@ void range_set_release(struct range_set *rs)
 static void range_set_copy(struct range_set *dst, struct range_set *src)
 {
 	range_set_init(dst, src->nr);
-	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
+	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));
 	dst->nr = src->nr;
 }
 static void range_set_move(struct range_set *dst, struct range_set *src)

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King]

On Sat, Mar 04, 2017 at 09:08:40PM +0100, Vegard Nossum wrote:

> > At a glance, looks like range_set_copy() is using
> > sizeof(struct range_set) == 12, but
> > range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using
> > sizeof(rs->range) == 8.
> 
> Attached patch seems to fix it -- basically, range_set_copy() is trying
> to copy more than it should. It was uncovered with the test case from
> Allan's commit because it's creating enough ranges to overflow the
> initial allocation on 32-bit.

Ugh, yeah, that is definitely a bug.

> diff --git a/line-log.c b/line-log.c
> index 951029665..cb0dc1110 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -43,7 +43,7 @@ void range_set_release(struct range_set *rs)
>  static void range_set_copy(struct range_set *dst, struct range_set *src)
>  {
>  	range_set_init(dst, src->nr);
> -	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
> +	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));

I think "sizeof(*dst->ranges)" is probably an even better fix, as it
infers the type of "dst". But these days we have COPY_ARRAY() to make it
even harder to get this kind of thing wrong.

I grepped for 'memcpy.*sizeof' and found one other case that's not a
bug, but is questionable.

Of the "good" cases, I think most of them could be converted into
something more obviously-correct, which would make auditing easier. The
three main cases I saw were:

  1. Ones which can probably be converted to COPY_ARRAY().

  2. Ones which just copy a single object, like:

       memcpy(&dst, &src, sizeof(dst));

     Perhaps we should be using struct assignment like:

       dst = src;

     here. It's safer and it should give the compiler more room to
     optimize. The only downside is that if you have pointers, it is
     easy to write "dst = src" when you meant "*dst = *src".

  3. There were a number of alloc-and-copy instances. The copy part is
     the same as (2) above, but you have to repeat the size, which is
     potentially error-prone. I wonder if we would want something like:

       #define ALLOC_COPY(dst, src) do { \
         (dst) = xmalloc(sizeof(*(dst))); \
	 COPY_ARRAY(dst, src, 1); \
       while(0)

     That avoids having to specify the size at all, and triggers a
     compile-time error if "src" and "dst" point to objects of different
     sizes.

     I suspect our friendly neighborhood coccinelle wizards could cook
     up a conversion.

-Peff

[PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy

[Jeff King]

On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:

> >  	range_set_init(dst, src->nr);
> > -	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
> > +	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));
> 
> I think "sizeof(*dst->ranges)" is probably an even better fix, as it
> infers the type of "dst". But these days we have COPY_ARRAY() to make it
> even harder to get this kind of thing wrong.

So here's your fix wrapped up with a commit message, mostly for Junio's
convenience. I listed you as the author, since you did the hard part. If
you're OK with it, please indicate that it's OK to add your
signed-off-by. If you prefer to do it differently, feel free to post
your own patch.

-- >8 --
From: Vegard Nossum <vegard.nossum@oracle.com>
Subject: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy

This memcpy meant to get the sizeof a "struct range", not a
"range_set", as the former is what our array holds. Rather
than swap out the types, let's convert this site to
COPY_ARRAY, which avoids the problem entirely (and confirms
that the src and dst types match).

Note for curiosity's sake that this bug doesn't trigger on
I32LP64 systems, but does on ILP32 systems. The mistaken
"struct range_set" has two ints and a pointer. That's 16
bytes on LP64, or 12 on ILP32. The correct "struct range"
type has two longs, which is also 16 on LP64, but only 8 on
ILP32.

Likewise an IL32P64 system would experience the bug.

Signed-off-by: Jeff King <peff@peff.net>
---
 line-log.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/line-log.c b/line-log.c
index 65f3558b3..a2477f601 100644
--- a/line-log.c
+++ b/line-log.c
@@ -43,9 +43,10 @@ void range_set_release(struct range_set *rs)
 static void range_set_copy(struct range_set *dst, struct range_set *src)
 {
 	range_set_init(dst, src->nr);
-	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
+	COPY_ARRAY(dst->ranges, src->ranges, src->nr);
 	dst->nr = src->nr;
 }
+
 static void range_set_move(struct range_set *dst, struct range_set *src)
 {
 	range_set_release(dst);
-- 
2.12.0.426.g9d5d0eeae

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King]

On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:

> I grepped for 'memcpy.*sizeof' and found one other case that's not a
> bug, but is questionable.

And here's the fix for that case. It can be applied separately from the
other patch if need be.

-- >8 --
Subject: [PATCH] ewah: fix eword_t/uint64_t confusion

The ewah subsystem typedefs eword_t to be uint64_t, but some
code uses a bare uint64_t. This isn't a bug now, but it's a
potential maintenance problem if the definition of eword_t
ever changes. Let's use the correct type.

Note that we can't use COPY_ARRAY() here because the source
and destination point to objects of different sizes. For
that reason we'll also skip the usual "sizeof(*dst)" and use
the real type, which should make it more clear that there's
something tricky going on.

Signed-off-by: Jeff King <peff@peff.net>
---
 ewah/ewah_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index 61f6a4357..f73210973 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -142,8 +142,8 @@ int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len)
 	 * the endianness conversion in a separate pass to ensure
 	 * we're loading 8-byte aligned words.
 	 */
-	memcpy(self->buffer, ptr, self->buffer_size * sizeof(uint64_t));
-	ptr += self->buffer_size * sizeof(uint64_t);
+	memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));
+	ptr += self->buffer_size * sizeof(eword_t);
 
 	for (i = 0; i < self->buffer_size; ++i)
 		self->buffer[i] = ntohll(self->buffer[i]);
-- 
2.12.0.426.g9d5d0eeae

Re: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy

[Vegard Nossum]

On 05/03/2017 12:44, Jeff King wrote:
> On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:
>
>>>  	range_set_init(dst, src->nr);
>>> -	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
>>> +	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));
>>
>> I think "sizeof(*dst->ranges)" is probably an even better fix, as it
>> infers the type of "dst". But these days we have COPY_ARRAY() to make it
>> even harder to get this kind of thing wrong.
>
> So here's your fix wrapped up with a commit message, mostly for Junio's
> convenience. I listed you as the author, since you did the hard part. If
> you're OK with it, please indicate that it's OK to add your
> signed-off-by. If you prefer to do it differently, feel free to post
> your own patch.

Thanks.

>
> -- >8 --
> From: Vegard Nossum <vegard.nossum@oracle.com>
> Subject: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy
>
> This memcpy meant to get the sizeof a "struct range", not a
> "range_set", as the former is what our array holds. Rather
> than swap out the types, let's convert this site to
> COPY_ARRAY, which avoids the problem entirely (and confirms
> that the src and dst types match).
>
> Note for curiosity's sake that this bug doesn't trigger on
> I32LP64 systems, but does on ILP32 systems. The mistaken
> "struct range_set" has two ints and a pointer. That's 16
> bytes on LP64, or 12 on ILP32. The correct "struct range"
> type has two longs, which is also 16 on LP64, but only 8 on
> ILP32.
>
> Likewise an IL32P64 system would experience the bug.
>
> Signed-off-by: Jeff King <peff@peff.net>

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

> ---
>  line-log.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/line-log.c b/line-log.c
> index 65f3558b3..a2477f601 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -43,9 +43,10 @@ void range_set_release(struct range_set *rs)
>  static void range_set_copy(struct range_set *dst, struct range_set *src)
>  {
>  	range_set_init(dst, src->nr);
> -	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
> +	COPY_ARRAY(dst->ranges, src->ranges, src->nr);
>  	dst->nr = src->nr;
>  }
> +
>  static void range_set_move(struct range_set *dst, struct range_set *src)
>  {
>  	range_set_release(dst);
>


Vegard

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Lars Schneider]

> On 02 Mar 2017, at 16:17, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> 
> On 02/03/17 11:24, Johannes Schindelin wrote:
>> Hi Lars,
>> 
>> On Thu, 2 Mar 2017, Lars Schneider wrote:
>> 
> [snip]
>>> One thing that still bugs me: In the Linux32 environment prove adds the
>>> CPU times to every test run: ( 0.02 usr  0.00 sys +  0.00 cusr  0.00
>>> csys ...  Has anyone an idea why that happens and how we can disable it?
>> 
>> I have no idea.
> 
> I have no idea either, but it is not unique to this 32bit Linux, but
> rather the version of prove. For example, I am seeing this on Linux
> Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
> this. (They used different Ubuntu LTS releases).
> 
> [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]

I think I found it. It was introduced in TAP::Harness v3.34:
https://github.com/Perl-Toolchain-Gang/Test-Harness/commit/66cbf6355928b4828db517a99f1099b7fed35e90

... and it is enabled with the "--timer" switch.

- Lars

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Ramsay Jones]

On 05/03/17 17:38, Lars Schneider wrote:
>> On 02 Mar 2017, at 16:17, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> On 02/03/17 11:24, Johannes Schindelin wrote:
>>> On Thu, 2 Mar 2017, Lars Schneider wrote:
>>>
>> [snip]
>>>> One thing that still bugs me: In the Linux32 environment prove adds the
>>>> CPU times to every test run: ( 0.02 usr  0.00 sys +  0.00 cusr  0.00
>>>> csys ...  Has anyone an idea why that happens and how we can disable it?
>>>
>>> I have no idea.
>>
>> I have no idea either, but it is not unique to this 32bit Linux, but
>> rather the version of prove. For example, I am seeing this on Linux
>> Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
>> this. (They used different Ubuntu LTS releases).
>>
>> [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]
> 
> I think I found it. It was introduced in TAP::Harness v3.34:
> https://github.com/Perl-Toolchain-Gang/Test-Harness/commit/66cbf6355928b4828db517a99f1099b7fed35e90
> 
> ... and it is enabled with the "--timer" switch.

Yep, that looks like it.

When I updated to Mint 18, this broke a perl script of mine, so I had
a quick look to see what I could do to suppress it. The man page seemed
to imply that you could replace the output formatter, but that didn't
take me too far (search CPAN for TAP::Parser::Formatter: ;-) ). I suppose
you could replace Tap::Formatter::Base, or some such, but I didn't need
to go that far - I simply changed a couple of regex-es to ignore the
excess output! :-P

Do you really need to suppress that timing information or, like me, can
you simply ignore it?

ATB,
Ramsay Jones

Re: [PATCH v1] Travis: also test on 32-bit Linux

[René Scharfe]

Am 05.03.2017 um 12:36 schrieb Jeff King:
> I grepped for 'memcpy.*sizeof' and found one other case that's not a
> bug, but is questionable.
> 
> Of the "good" cases, I think most of them could be converted into
> something more obviously-correct, which would make auditing easier. The
> three main cases I saw were:

>   2. Ones which just copy a single object, like:
> 
>        memcpy(&dst, &src, sizeof(dst));
> 
>      Perhaps we should be using struct assignment like:
> 
>        dst = src;
> 
>      here. It's safer and it should give the compiler more room to
>      optimize. The only downside is that if you have pointers, it is
>      easy to write "dst = src" when you meant "*dst = *src".

Compilers can usually inline memcpy(3) calls, but assignments are
shorter and more pleasing to the eye, and we get a type check for
free.  How about this?

-- >8 --
Subject: [PATCH] cocci: use assignment operator to copy structs

Add a semantic patch for converting memcpy(3) calls targeting
addresses of variables (i.e., variables preceded by &) -- which are
basically always structs -- to simple assignments, and apply it to
the current tree.  The resulting code is shorter, simpler and its
type safety is checked by the compiler.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/log.c                 |  2 +-
 contrib/coccinelle/copy.cocci | 31 +++++++++++++++++++++++++++++++
 convert.c                     |  2 +-
 credential-cache--daemon.c    |  2 +-
 daemon.c                      |  2 +-
 line-log.c                    |  2 +-
 revision.c                    |  2 +-
 7 files changed, 37 insertions(+), 6 deletions(-)
 create mode 100644 contrib/coccinelle/copy.cocci

diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc2d8..23bb9a9e76 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1030,7 +1030,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (!origin)
 		return;
 
-	memcpy(&opts, &rev->diffopt, sizeof(opts));
+	opts = rev->diffopt;
 	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 
 	diff_setup_done(&opts);
diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
new file mode 100644
index 0000000000..f0d883932a
--- /dev/null
+++ b/contrib/coccinelle/copy.cocci
@@ -0,0 +1,31 @@
+@@
+type T;
+T dst;
+T src;
+@@
+(
+- memcpy(&dst, &src, sizeof(dst));
++ dst = src;
+|
+- memcpy(&dst, &src, sizeof(src));
++ dst = src;
+|
+- memcpy(&dst, &src, sizeof(T));
++ dst = src;
+)
+
+@@
+type T;
+T dst;
+T *src;
+@@
+(
+- memcpy(&dst, src, sizeof(dst));
++ dst = *src;
+|
+- memcpy(&dst, src, sizeof(*src));
++ dst = *src;
+|
+- memcpy(&dst, src, sizeof(T));
++ dst = *src;
+)
diff --git a/convert.c b/convert.c
index 8d652bf27c..4bae12be6b 100644
--- a/convert.c
+++ b/convert.c
@@ -290,7 +290,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 	if ((checksafe == SAFE_CRLF_WARN ||
 	    (checksafe == SAFE_CRLF_FAIL)) && len) {
 		struct text_stat new_stats;
-		memcpy(&new_stats, &stats, sizeof(new_stats));
+		new_stats = stats;
 		/* simulate "git add" */
 		if (convert_crlf_into_lf) {
 			new_stats.lonelf += new_stats.crlf;
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 46c5937526..798cf33c3a 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -22,7 +22,7 @@ static void cache_credential(struct credential *c, int timeout)
 	e = &entries[entries_nr++];
 
 	/* take ownership of pointers */
-	memcpy(&e->item, c, sizeof(*c));
+	e->item = *c;
 	memset(c, 0, sizeof(*c));
 	e->expiration = time(NULL) + timeout;
 }
diff --git a/daemon.c b/daemon.c
index 473e6b6b63..f891398aad 100644
--- a/daemon.c
+++ b/daemon.c
@@ -785,7 +785,7 @@ static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_
 
 	newborn = xcalloc(1, sizeof(*newborn));
 	live_children++;
-	memcpy(&newborn->cld, cld, sizeof(*cld));
+	newborn->cld = *cld;
 	memcpy(&newborn->address, addr, addrlen);
 	for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
 		if (!addrcmp(&(*cradle)->address, &newborn->address))
diff --git a/line-log.c b/line-log.c
index 65f3558b3b..64f141e200 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1093,7 +1093,7 @@ static int process_all_files(struct line_log_data **range_out,
 				rg = rg->next;
 			assert(rg);
 			rg->pair = diff_filepair_dup(queue->queue[i]);
-			memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));
+			rg->diff = *pairdiff;
 		}
 		free(pairdiff);
 	}
diff --git a/revision.c b/revision.c
index b37dbec378..289977c796 100644
--- a/revision.c
+++ b/revision.c
@@ -2738,7 +2738,7 @@ int prepare_revision_walk(struct rev_info *revs)
 	struct object_array old_pending;
 	struct commit_list **next = &revs->commits;
 
-	memcpy(&old_pending, &revs->pending, sizeof(old_pending));
+	old_pending = revs->pending;
 	revs->pending.nr = 0;
 	revs->pending.alloc = 0;
 	revs->pending.objects = NULL;
-- 
2.12.0

Re: [PATCH v1] Travis: also test on 32-bit Linux

[René Scharfe]

Am 05.03.2017 um 12:36 schrieb Jeff King:
> I grepped for 'memcpy.*sizeof' and found one other case that's not a
> bug, but is questionable.
>
> Of the "good" cases, I think most of them could be converted into
> something more obviously-correct, which would make auditing easier. The
> three main cases I saw were:

>   3. There were a number of alloc-and-copy instances. The copy part is
>      the same as (2) above, but you have to repeat the size, which is
>      potentially error-prone. I wonder if we would want something like:
>
>        #define ALLOC_COPY(dst, src) do { \
>          (dst) = xmalloc(sizeof(*(dst))); \
> 	 COPY_ARRAY(dst, src, 1); \
>        while(0)
>
>      That avoids having to specify the size at all, and triggers a
>      compile-time error if "src" and "dst" point to objects of different
>      sizes.

Or you could call it DUP or similar.  And you could use ALLOC_ARRAY in
its definition and let it infer the size implicitly (don't worry too
much about the multiplication with one):

	#define DUPLICATE_ARRAY(dst, src, n) do {	\
		ALLOC_ARRAY((dst), (n));		\
		COPY_ARRAY((dst), (src), (n));		\
	} while (0)
	#define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)

But do we even want such a thing?  Duplicating objects should be rare, 
and keeping allocation and assignment/copying separate makes for more 
flexible building blocks.  Adding ALLOC (and CALLOC) for single objects 
could be more widely useful, I think.

René

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King]

On Fri, Mar 10, 2017 at 01:14:45AM +0100, René Scharfe wrote:

> >   3. There were a number of alloc-and-copy instances. The copy part is
> >      the same as (2) above, but you have to repeat the size, which is
> >      potentially error-prone. I wonder if we would want something like:
> > 
> >        #define ALLOC_COPY(dst, src) do { \
> >          (dst) = xmalloc(sizeof(*(dst))); \
> > 	 COPY_ARRAY(dst, src, 1); \
> >        while(0)
> > 
> >      That avoids having to specify the size at all, and triggers a
> >      compile-time error if "src" and "dst" point to objects of different
> >      sizes.
> 
> Or you could call it DUP or similar.  And you could use ALLOC_ARRAY in
> its definition and let it infer the size implicitly (don't worry too
> much about the multiplication with one):
> 
> 	#define DUPLICATE_ARRAY(dst, src, n) do {	\
> 		ALLOC_ARRAY((dst), (n));		\
> 		COPY_ARRAY((dst), (src), (n));		\
> 	} while (0)
> 	#define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)
> 
> But do we even want such a thing?  Duplicating objects should be rare, and
> keeping allocation and assignment/copying separate makes for more flexible
> building blocks.  Adding ALLOC (and CALLOC) for single objects could be more
> widely useful, I think.

There's no reason you can't have both the building blocks and the thing
that is built for them. I think there are 5 spots that would use
DUPLICATE(), but I agree that there are more which could use ALLOC().

I'd be more worried that we're slowly drifting away from idiomatic C.
If it's safer, that's good. But if it makes it hard for people new to
the project to figure out what the hell is going on, that may be not so
good.

Here's the list of DUPLICATE spots, for reference.

---
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..f0ac1c511 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -661,8 +661,8 @@ static struct origin *find_rename(struct scoreboard *sb,
 static void add_blame_entry(struct blame_entry ***queue,
 			    const struct blame_entry *src)
 {
-	struct blame_entry *e = xmalloc(sizeof(*e));
-	memcpy(e, src, sizeof(*e));
+	struct blame_entry *e;
+	DUPLICATE(e, src);
 	origin_incref(e->suspect);
 
 	e->next = **queue;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844..d6cb893cf 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -206,8 +206,8 @@ static void llist_sorted_difference_inplace(struct llist *A,
 static inline struct pack_list * pack_list_insert(struct pack_list **pl,
 					   struct pack_list *entry)
 {
-	struct pack_list *p = xmalloc(sizeof(struct pack_list));
-	memcpy(p, entry, sizeof(struct pack_list));
+	struct pack_list *p;
+	DUPLICATE(p, entry);
 	p->next = *pl;
 	*pl = p;
 	return p;
@@ -238,8 +238,7 @@ static struct pack_list * pack_list_difference(const struct pack_list *A,
 			return pack_list_difference(A->next, B);
 		pl = pl->next;
 	}
-	ret = xmalloc(sizeof(struct pack_list));
-	memcpy(ret, A, sizeof(struct pack_list));
+	DUPLICATE(ret, A);
 	ret->next = pack_list_difference(A->next, B);
 	return ret;
 }
@@ -450,8 +449,7 @@ static void minimize(struct pack_list **min)
 		perm_all = perm = get_permutations(non_unique, n);
 		while (perm) {
 			if (is_superset(perm->pl, missing)) {
-				new_perm = xmalloc(sizeof(struct pll));
-				memcpy(new_perm, perm, sizeof(struct pll));
+				DUPLICATE(new_perm, perm);
 				new_perm->next = perm_ok;
 				perm_ok = new_perm;
 			}
diff --git a/diff.c b/diff.c
index 051761be4..dfe02f403 100644
--- a/diff.c
+++ b/diff.c
@@ -1168,8 +1168,8 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
 				 struct diff_filespec *two)
 {
 	int i;
-	struct diff_options *o = xmalloc(sizeof(struct diff_options));
-	memcpy(o, orig_opts, sizeof(struct diff_options));
+	struct diff_options *o;
+	DUPLICATE(o, orig_opts);
 
 	ecbdata->diff_words =
 		xcalloc(1, sizeof(struct diff_words_data));

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King]

On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote:

> >   2. Ones which just copy a single object, like:
> > 
> >        memcpy(&dst, &src, sizeof(dst));
> > 
> >      Perhaps we should be using struct assignment like:
> > 
> >        dst = src;
> > 
> >      here. It's safer and it should give the compiler more room to
> >      optimize. The only downside is that if you have pointers, it is
> >      easy to write "dst = src" when you meant "*dst = *src".
> 
> Compilers can usually inline memcpy(3) calls, but assignments are
> shorter and more pleasing to the eye, and we get a type check for
> free.  How about this?

Yeah, I mostly wasn't sure how people felt about "shorter and more
pleasing". It _is_ shorter and there's less to get wrong. But the
memcpy() screams "hey, I am making a copy" and is idiomatic to at least
a certain generation of C programmers.

I guess something like COPY(dst, src) removes the part that you can get
wrong, while still screaming copy. It's not idiomatic either, but at
least it stands out. I dunno.

> diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
> new file mode 100644
> index 0000000000..f0d883932a
> --- /dev/null
> +++ b/contrib/coccinelle/copy.cocci
> @@ -0,0 +1,31 @@
> +@@
> +type T;
> +T dst;
> +T src;
> [...]
> +type T;
> +T dst;
> +T *src;

I think this misses the other two cases: (*dst, src) and (*dst, *src).

Perhaps squash this in?

---
 builtin/blame.c               |  2 +-
 builtin/pack-redundant.c      |  4 ++--
 contrib/coccinelle/copy.cocci | 32 ++++++++++++++++++++++++++++++++
 diff.c                        |  4 ++--
 dir.c                         |  2 +-
 fast-import.c                 |  6 +++---
 line-log.c                    |  2 +-
 7 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..d0d917b37 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -680,7 +680,7 @@ static void dup_entry(struct blame_entry ***queue,
 {
 	origin_incref(src->suspect);
 	origin_decref(dst->suspect);
-	memcpy(dst, src, sizeof(*src));
+	*dst = *src;
 	dst->next = **queue;
 	**queue = dst;
 	*queue = &dst->next;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844..ac1d8111e 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -207,7 +207,7 @@ static inline struct pack_list * pack_list_insert(struct pack_list **pl,
 					   struct pack_list *entry)
 {
 	struct pack_list *p = xmalloc(sizeof(struct pack_list));
-	memcpy(p, entry, sizeof(struct pack_list));
+	*p = *entry;
 	p->next = *pl;
 	*pl = p;
 	return p;
@@ -451,7 +451,7 @@ static void minimize(struct pack_list **min)
 		while (perm) {
 			if (is_superset(perm->pl, missing)) {
 				new_perm = xmalloc(sizeof(struct pll));
-				memcpy(new_perm, perm, sizeof(struct pll));
+				*new_perm = *perm;
 				new_perm->next = perm_ok;
 				perm_ok = new_perm;
 			}
diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
index f0d883932..da9969c84 100644
--- a/contrib/coccinelle/copy.cocci
+++ b/contrib/coccinelle/copy.cocci
@@ -29,3 +29,35 @@ T *src;
 - memcpy(&dst, src, sizeof(T));
 + dst = *src;
 )
+
+@@
+type T;
+T *dst;
+T src;
+@@
+(
+- memcpy(dst, &src, sizeof(*dst));
++ *dst = src;
+|
+- memcpy(dst, &src, sizeof(src));
++ *dst = src;
+|
+- memcpy(dst, &src, sizeof(T));
++ *dst = src;
+)
+
+@@
+type T;
+T *dst;
+T *src;
+@@
+(
+- memcpy(dst, src, sizeof(*dst));
++ *dst = *src;
+|
+- memcpy(dst, src, sizeof(*src));
++ *dst = *src;
+|
+- memcpy(dst, src, sizeof(T));
++ *dst = *src;
+)
diff --git a/diff.c b/diff.c
index 051761be4..fbd68ecd0 100644
--- a/diff.c
+++ b/diff.c
@@ -1169,7 +1169,7 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
 {
 	int i;
 	struct diff_options *o = xmalloc(sizeof(struct diff_options));
-	memcpy(o, orig_opts, sizeof(struct diff_options));
+	*o = *orig_opts;
 
 	ecbdata->diff_words =
 		xcalloc(1, sizeof(struct diff_words_data));
@@ -3359,7 +3359,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 
 void diff_setup(struct diff_options *options)
 {
-	memcpy(options, &default_diff_options, sizeof(*options));
+	*options = default_diff_options;
 
 	options->file = stdout;
 
diff --git a/dir.c b/dir.c
index 4541f9e14..d3d0039bf 100644
--- a/dir.c
+++ b/dir.c
@@ -2499,7 +2499,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 	if (next > rd->end)
 		return -1;
 	*untracked_ = untracked = xmalloc(st_add(sizeof(*untracked), len));
-	memcpy(untracked, &ud, sizeof(ud));
+	*untracked = ud;
 	memcpy(untracked->name, data, len + 1);
 	data = next;
 
diff --git a/fast-import.c b/fast-import.c
index 6c13472c4..3e294c2b5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -875,7 +875,7 @@ static struct tree_content *dup_tree_content(struct tree_content *s)
 	for (i = 0; i < s->entry_count; i++) {
 		a = s->entries[i];
 		b = new_tree_entry();
-		memcpy(b, a, sizeof(*a));
+		*b = *a;
 		if (a->tree && is_null_sha1(b->versions[1].sha1))
 			b->tree = dup_tree_content(a->tree);
 		else
@@ -1685,7 +1685,7 @@ static int tree_content_remove(
 
 del_entry:
 	if (backup_leaf)
-		memcpy(backup_leaf, e, sizeof(*backup_leaf));
+		*backup_leaf = *e;
 	else if (e->tree)
 		release_tree_content_recursive(e->tree);
 	e->tree = NULL;
@@ -1735,7 +1735,7 @@ static int tree_content_get(
 	return 0;
 
 found_entry:
-	memcpy(leaf, e, sizeof(*leaf));
+	*leaf = *e;
 	if (e->tree && is_null_sha1(e->versions[1].sha1))
 		leaf->tree = dup_tree_content(e->tree);
 	else
diff --git a/line-log.c b/line-log.c
index 64f141e20..de5bbb5bd 100644
--- a/line-log.c
+++ b/line-log.c
@@ -765,7 +765,7 @@ static void move_diff_queue(struct diff_queue_struct *dst,
 			    struct diff_queue_struct *src)
 {
 	assert(src != dst);
-	memcpy(dst, src, sizeof(struct diff_queue_struct));
+	*dst = *src;
 	DIFF_QUEUE_CLEAR(src);
 }
 

Re: [PATCH v1] Travis: also test on 32-bit Linux

[René Scharfe]

Am 10.03.2017 um 09:18 schrieb Jeff King:
> On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote:
>
>>>   2. Ones which just copy a single object, like:
>>>
>>>        memcpy(&dst, &src, sizeof(dst));
>>>
>>>      Perhaps we should be using struct assignment like:
>>>
>>>        dst = src;
>>>
>>>      here. It's safer and it should give the compiler more room to
>>>      optimize. The only downside is that if you have pointers, it is
>>>      easy to write "dst = src" when you meant "*dst = *src".
>>
>> Compilers can usually inline memcpy(3) calls, but assignments are
>> shorter and more pleasing to the eye, and we get a type check for
>> free.  How about this?
>
> Yeah, I mostly wasn't sure how people felt about "shorter and more
> pleasing". It _is_ shorter and there's less to get wrong. But the
> memcpy() screams "hey, I am making a copy" and is idiomatic to at least
> a certain generation of C programmers.
>
> I guess something like COPY(dst, src) removes the part that you can get
> wrong, while still screaming copy. It's not idiomatic either, but at
> least it stands out. I dunno.

Yes ...

> I think this misses the other two cases: (*dst, src) and (*dst, *src).

... and that's why I left them out.  You can't get dst vs. *dst wrong 
with structs (at least not without the compiler complaining); only safe 
transformations are included in this round.

René

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King]

On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote:

> > I think this misses the other two cases: (*dst, src) and (*dst, *src).
> 
> ... and that's why I left them out.  You can't get dst vs. *dst wrong with
> structs (at least not without the compiler complaining); only safe
> transformations are included in this round.

I don't think the transformation could be wrong without the original
code being wrong.

I'm also not sure why mine would be unsafe and yours would be safe. It
seems like the other way around, because mine will do:

  *dst = ...

which would fail unless "dst" is a pointer. Whereas in yours, we end up
with:

  dst = ...

and somebody mistaking pointer assignment for a struct copy would not
notice.

But either way, I don't think we can have a rule like "you can use
struct assignment only if you don't have a pointer for the destination".
That's too arcane to expect developers and reviewers to follow. We
should either allow struct assignment or not.

Or have I totally misunderstood your point?

-Peff

Re: [PATCH v1] Travis: also test on 32-bit Linux

[René Scharfe]

Am 10.03.2017 um 18:57 schrieb Jeff King:
> On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote:
>
>>> I think this misses the other two cases: (*dst, src) and (*dst, *src).
>>
>> ... and that's why I left them out.  You can't get dst vs. *dst wrong with
>> structs (at least not without the compiler complaining); only safe
>> transformations are included in this round.
>
> I don't think the transformation could be wrong without the original
> code being wrong.

Avoiding to introduce bugs with automatic transformations is essential, 
indeed -- if we're not careful here we'd be better off keeping the 
original code.

> I'm also not sure why mine would be unsafe and yours would be safe. It
> seems like the other way around, because mine will do:
>
>   *dst = ...
>
> which would fail unless "dst" is a pointer. Whereas in yours, we end up
> with:
>
>   dst = ...
>
> and somebody mistaking pointer assignment for a struct copy would not
> notice.

If dst is a struct then having *dst on the left-hand side results in a 
compiler error -- you can't get that one wrong.  If it's a pointer then 
both dst and *dst can be valid (pointer assignment vs. content copy), so 
there is the possibility of making a mistake without the compiler noticing.

> But either way, I don't think we can have a rule like "you can use
> struct assignment only if you don't have a pointer for the destination".
> That's too arcane to expect developers and reviewers to follow. We
> should either allow struct assignment or not.

With an automatic transformation in place it's more like "you can't use 
memcpy() in this case any more as it gets turned into an assignment with 
the next cocci patch".  I think we shouldn't be that restrictive for 
pointers as destinations (yet?).

René

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> I think this misses the other two cases: (*dst, src) and (*dst, *src).
>
> ... and that's why I left them out.  You can't get dst vs. *dst wrong
> with structs (at least not without the compiler complaining); only
> safe transformations are included in this round.

I haven't followed this discussion to the end, but the omission of 2
out of obvious 4 did pique my curiosity when I saw it, too, and made
me wonder if the omission was deliberate.  If so, it would be nice
to state why in the log message (or in copy.cocci file itself as a
comment).

It also made me wonder if we would be helped with a further
combinatorial explosion from "T **dstp, **srcp" and somesuch (in
other words, I am wondering why a rule for 'T *src' that uses '*src'
need to be spelled out separately when there already is a good rule
for 'T src' that uses 'src'---is that an inherent restriction of the
tool?).

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King]

On Fri, Mar 10, 2017 at 12:13:11PM -0800, Junio C Hamano wrote:

> René Scharfe <l.s.r@web.de> writes:
> 
> >> I think this misses the other two cases: (*dst, src) and (*dst, *src).
> >
> > ... and that's why I left them out.  You can't get dst vs. *dst wrong
> > with structs (at least not without the compiler complaining); only
> > safe transformations are included in this round.
> 
> I haven't followed this discussion to the end, but the omission of 2
> out of obvious 4 did pique my curiosity when I saw it, too, and made
> me wonder if the omission was deliberate.  If so, it would be nice
> to state why in the log message (or in copy.cocci file itself as a
> comment).

Yeah, it definitely would be worth mentioning. I'm still undecided on
whether we want to be endorsing struct assignment more fully.

> It also made me wonder if we would be helped with a further
> combinatorial explosion from "T **dstp, **srcp" and somesuch (in
> other words, I am wondering why a rule for 'T *src' that uses '*src'
> need to be spelled out separately when there already is a good rule
> for 'T src' that uses 'src'---is that an inherent restriction of the
> tool?).

I had that thought, too, but I think the 4-way rules are necessary,
because the transformations aren't the same in each case. E.g., for the
four cases, the resulting assignments are:

    (dst, src): dst = src;
   (dst, *src): dst = *src;
   (*dst, src): *dst = src;
  (*dst, *src): *dst = *src;

For pointer-to-pointer, I assumed the tool would handle that
automatically by matching "T" as "T*". Though if that is the case, I
think "(dst, src)" and "(*dst, *src)" would be equivalent (though of
course our rule matches are different, as you do not memcpy the raw
structs).

-Peff

Re: [PATCH v1] Travis: also test on 32-bit Linux

[René Scharfe]

Am 10.03.2017 um 21:13 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>> I think this misses the other two cases: (*dst, src) and (*dst, *src).
>>
>> ... and that's why I left them out.  You can't get dst vs. *dst wrong
>> with structs (at least not without the compiler complaining); only
>> safe transformations are included in this round.
>
> I haven't followed this discussion to the end, but the omission of 2
> out of obvious 4 did pique my curiosity when I saw it, too, and made
> me wonder if the omission was deliberate.  If so, it would be nice
> to state why in the log message (or in copy.cocci file itself as a
> comment).
>
> It also made me wonder if we would be helped with a further
> combinatorial explosion from "T **dstp, **srcp" and somesuch (in
> other words, I am wondering why a rule for 'T *src' that uses '*src'
> need to be spelled out separately when there already is a good rule
> for 'T src' that uses 'src'---is that an inherent restriction of the
> tool?).

There are redundancies. This semantic patch here:

	@@
	type T;
	T dst;
	T *src;
	@@
	- memcpy(&dst, src, sizeof(dst));
	+ dst = *src;

would match e.g. this (from convert.c):

	memcpy(&new_stats, &stats, sizeof(new_stats));

and transform it to:

	new_stats = *&stats;

We'd need just one more rule to remove the "*&" part and could then get 
rid of two of the "T src" variants, to arrive at something like this:

	@@
	type T;
	T dst, src;
	@@
	- memcpy(&dst, &src, sizeof(src));
	+ dst = src;

	@ r @
	type T;
	T dst;
	T *src;
	@@
	(
	- memcpy(&dst, src, sizeof(dst));
	+ dst = *src;
	|
	- memcpy(&dst, src, sizeof(*src));
	+ dst = *src;
	|
	- memcpy(&dst, src, sizeof(T));
	+ dst = *src;
	)

	@ depends on r @
	expression E;
	@@
	- *&
	  E

René

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> 	@ depends on r @
> 	expression E;
> 	@@
> 	- *&
> 	  E

I guess my source of the confusion is that the tool that understands
the semantics of the C language still needs to be told about that.

I was hoping that something that understands C only needs to be told
only a single rule:

	type T
	T src, dst

	-memcpy(&dst, &src, sizeof(dst));
	+dst = src;

and then can apply that rule to this code in four ways:

	struct foo A, *Bp;

	memcpy(Bp, &A, sizeof(*Bp));
	memcpy(Bp, &A, sizeof(A));
	memcpy(&src, dstp, sizeof(A));
	memcpy(&src, dstp, sizeof(*Bp));

to obtain its rewrite:

	struct foo A, *Bp;

	*Bp = A;
	*Bp = A;
	A = *Bp;
	A = *Bp;

by knowing that (*Bp) is of type "struct foo" (even though Bp is of
type "struct foo *") and sizeof(dst) and sizeof(src) are the same
thing in the rule because src and dst are both of type T.

Re: [PATCH v1] Travis: also test on 32-bit Linux

[René Scharfe]

Am 11.03.2017 um 00:33 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> 	@ depends on r @
>> 	expression E;
>> 	@@
>> 	- *&
>> 	  E
> 
> I guess my source of the confusion is that the tool that understands
> the semantics of the C language still needs to be told about that.

Understanding that E and *&E have the same type does not imply (for a
machine) that E alone is preferable.

> I was hoping that something that understands C only needs to be told
> only a single rule:
> 
> 	type T
> 	T src, dst
> 
> 	-memcpy(&dst, &src, sizeof(dst));
> 	+dst = src;
> 
> and then can apply that rule to this code in four ways:
> 
> 	struct foo A, *Bp;
> 
> 	memcpy(Bp, &A, sizeof(*Bp));
> 	memcpy(Bp, &A, sizeof(A));
> 	memcpy(&src, dstp, sizeof(A));
> 	memcpy(&src, dstp, sizeof(*Bp));

I guess src is A and dstp is Bp?

Coccinelle does not know that the sizeof expressions are equivalent,
but you could normalize them with an additional rule and then use a
single memcpy transformation like this:

	@ normalize_sizeof @
	type T;
	T var;
	@@
	- sizeof(var)
	+ sizeof(T)

	@ r @
	type T;
	T *dst;
	T *src;
	@@
	- memcpy(dst, src, sizeof(T));
	+ *dst = *src;

	@ depends on r @
	expression E;
	@@
	- *&
	  E

That would give you what you expected here:

> to obtain its rewrite:
> 
> 	struct foo A, *Bp;
> 
> 	*Bp = A;
> 	*Bp = A;
> 	A = *Bp;
> 	A = *Bp;

But that normalization rule would be applied everywhere because it's so
broad, and that's probably not what we want.  You could replace it with
an isomorphism like this one:

	Expression
	@@
	type T;
	T E;
	@@

	sizeof(T) => sizeof E

In your example it allows you to specify sizeof(struct foo) (or a
generic sizeof(T) as in rule r above) in the semantic patch and
Coccinelle would let that match sizeof(A) and sizeof(*Bp) in the C
file as well.

Isomorphisms are kept in a separate file, the default one is
/usr/lib/coccinelle/standard.iso on my machine and you can chose a
different one with --iso-file.  Perhaps we want to have our own for
git, but we'd need to synchronize it with upstream from time to time.

There is already a very similar isomorphism rule in the default file,
but I don't think I understand it:

	Expression
	@ sizeof_type_expr @
	pure type T; // pure because we drop a metavar
	T E;
	@@

	sizeof(T) => sizeof E

In particular I'm a bit worried about the lack of documentation for
"pure", and I don't understand the comment.  There's another comment
at the top of another rule in the same file:

// pure means that either the whole field reference expression is dropped,
// or E is context code and has no attached + code
// not really... pure means matches a unitary unplussed metavariable
// but this rule doesn't work anyway

Hmm.

Here's an isomorphism that allows you to use "sizeof *src" (the third
part for T is not strictly needed for your example):

	Expression
	@ sizeof_equiv @
	type T;
	T E1, E2;
	@@

	sizeof E1 <=> sizeof E2 <=> sizeof(T)

That's the kind of bidirectional equivalence you expected to be part
of Coccinelle's standard set of rules, right?  With that rule in place
the following semantic patch matches your four cases:

	@ r @
	type T;
	T *dst;
	T *src;
	@@
	- memcpy(dst, src, sizeof *dst);
	+ *dst = *src;

	@ depends on r @
	expression E;
	@@
	- *&
	  E

But I'd be careful with that as long as "pure" is present in that
standard rule and not fully understood.  The isomorphism sizeof_equiv
doesn't seem to cause any trouble with our existing semantic patches
and the code in master, though.

René