{"thread":{"id":"45254","subject":"[PATCH] Travis: also test on 32-bit Linux","startedAt":"2017-02-28T19:37:46Z","lastAt":"2017-03-11T14:21:14Z","messageCount":36,"participants":["Johannes Schindelin","Junio C Hamano","Lars Schneider","Ramsay Jones","Christian Couder","Vegard Nossum","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312873","messageId":"c76a133a57514a332828099d342c9763fd946bfa.1488309430.git.johannes.schindelin@gmx.de","threadId":"45254","inReplyTo":null,"subject":"[PATCH] Travis: also test on 32-bit Linux","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-28T19:17:28Z","receivedAt":"2017-02-28T19:37:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When Git v2.9.1 was released, it had a bug that showed only on Windows\nand on 32-bit systems: our assumption that `unsigned long` can hold\n64-bit values turned out to be wrong.\n\nThis could have been caught earlier if we had a Continuous Testing\nset up that includes a build and test run on 32-bit Linux.\n\nLet's do this (and take care of the Windows build later). This patch\nasks Travis CI to install a Docker image with 32-bit libraries and then\ngoes on to build and test Git using this 32-bit setup.\n\nA big thank you to Lars Schneider without whose help this patch would\nnot have happened.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/travis-32-bit-v1\nFetch-It-Via: git fetch https://github.com/dscho/git travis-32-bit-v1\n\n .travis.yml | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 9c63c8c3f68..87d9e9051a6 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -39,6 +39,17 @@ env:\n \n matrix:\n include:\n+ - env: Linux32\n+ os: linux\n+ compiler: clang\n+ sudo: required\n+ services:\n+ - docker\n+ before_install:\n+ - docker pull daald/ubuntu32:xenial\n+ before_script:\n+ script:\n+ - \"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'\\\"\"\n - env: Documentation\n os: linux\n compiler: clang\n\nbase-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858\n-- \n2.12.0.windows.1.3.g8a117c48243\n"},{"id":"312891","messageId":"alpine.DEB.2.20.1702282019460.3767@virtualbox","threadId":"45254","inReplyTo":"c76a133a57514a332828099d342c9763fd946bfa.1488309430.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Travis: also test on 32-bit Linux","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-28T20:34:07Z","receivedAt":"2017-02-28T20:36:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Feb 2017, Johannes Schindelin wrote:\n\n> When Git v2.9.1 was released, it had a bug that showed only on Windows\n> and on 32-bit systems: our assumption that `unsigned long` can hold\n> 64-bit values turned out to be wrong.\n> \n> This could have been caught earlier if we had a Continuous Testing set\n> up that includes a build and test run on 32-bit Linux.\n> \n> Let's do this (and take care of the Windows build later). This patch\n> asks Travis CI to install a Docker image with 32-bit libraries and then\n> goes on to build and test Git using this 32-bit setup.\n\nFor the record, I first tested this with the LONG_IS_32BIT flag forced to\n`true` so that the date tests must fail. They did fail as expected (search\nfor \"not ok\" in this output):\n\n\thttps://travis-ci.org/git/git/jobs/206199002\n\n(I actually cannot see the log right now, Travis seems to be under heavy\nload...)\n\nA much cleaned up version that does not force that LONG_IS_32BIT produced\nthis log:\n\n\thttps://travis-ci.org/git/git/jobs/206228708\n\n(Same here, my browser fails to load the log, probably because Travis\nexperiences quite high a load...)\n\nPlease note that this approach is not without problems. It would appear\nthat Travis currently has serious problems to even *reach* the Docker Hub,\nand therefore cannot download the Docker image:\n\n\thttps://travis-ci.org/git/git/jobs/206292947#L6\n\nCiao,\nJohannes\n"},{"id":"313036","messageId":"CAPc5daXQ72dmRv4zdqaOXs3eScgDgJ=P4PiQ00L7pnEa1d8=UA@mail.gmail.com","threadId":"45254","inReplyTo":"c76a133a57514a332828099d342c9763fd946bfa.1488309430.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-02T05:06:26Z","receivedAt":"2017-03-02T05:06:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Feb 28, 2017 at 11:17 AM, Johannes Schindelin\n wrote:\n> .... This patch\n> asks Travis CI to install a Docker image with 32-bit libraries and then\n> goes on to build and test Git using this 32-bit setup.\n>\n> A big thank you to Lars Schneider without whose help this patch would\n> not have happened.\n\nThis has been in 'pu' for a few days, and\nhttps://travis-ci.org/git/git/builds shows that we have\na new build job running successfully.\n\nGood job ;-)\n\nThanks.\n"},{"id":"313058","messageId":"20170302105157.59791-1-larsxschneider@gmail.com","threadId":"45254","inReplyTo":"c76a133a57514a332828099d342c9763fd946bfa.1488309430.git.johannes.schindelin@gmx.de","subject":"[PATCH v1] Travis: also test on 32-bit Linux","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-02T10:51:57Z","receivedAt":"2017-03-02T10:59:52Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Johannes Schindelin \n\nWhen Git v2.9.1 was released, it had a bug that showed only on Windows\nand on 32-bit systems: our assumption that `unsigned long` can hold\n64-bit values turned out to be wrong.\n\nThis could have been caught earlier if we had a Continuous Testing\nset up that includes a build and test run on 32-bit Linux.\n\nLet's do this (and take care of the Windows build later). This patch\nasks Travis CI to install a Docker image with 32-bit libraries and then\ngoes on to build and test Git using this 32-bit setup.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Lars Schneider \n---\n\nThanks for the patch Dscho!\n\nThe patch looks good to me in general but I want to propose the following\nchanges:\n\n(1) Move all the docker magic into a dedicated file \"ci/run-linux-32-build.sh\"\n This way people should be able to run this build on their local machines\n without TravisCI. However, I haven't tested this.\n\n(2) The docker build command inherits the Git test environment variables.\n This way we use the same environment variables as in all other TravisCI\n builds (plus it would use *your* variables if you run it locally).\n\n(3) Silence the apt update/git output as is it clutters the log.\n I did not silence stderr output!\n\n(4) Remove (to my knowledge) superfluous \"compiler: clang\" in the Linux32 job.\n\nI added my sign-off. I hope this is the right thing to do in this \"I took your\npatch and changed it to suggest an improvement\" situation.\n\nYou can see a successful run here:\nhttps://travis-ci.org/larsxschneider/git/jobs/206945950\n\n\nOne thing that still bugs me: In the Linux32 environment prove adds the\nCPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 csys ...\nHas anyone an idea why that happens and how we can disable it?\n\n\nCheers,\nLars\n\n\nNotes:\n Base Ref:\n Web-Diff: https://github.com/larsxschneider/git/commit/82995ed59c\n Checkout: git fetch https://github.com/larsxschneider/git travisci/linux32-v1 && git checkout 82995ed59c\n\n .travis.yml | 9 +++++++++\n ci/run-linux32-build.sh | 21 +++++++++++++++++++++\n 2 files changed, 30 insertions(+)\n create mode 100755 ci/run-linux32-build.sh\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 9c63c8c3f6..c8c789c437 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -39,6 +39,15 @@ env:\n\n matrix:\n include:\n+ - env: Linux32\n+ os: linux\n+ sudo: required\n+ services:\n+ - docker\n+ before_install:\n+ - docker pull daald/ubuntu32:xenial\n+ before_script:\n+ script: ci/run-linux32-build.sh\n - env: Documentation\n os: linux\n compiler: clang\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nnew file mode 100755\nindex 0000000000..b892fbdc9e\n--- /dev/null\n+++ b/ci/run-linux32-build.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+#\n+# Build and test Git in a docker container running a 32-bit Ubuntu Linux\n+#\n+\n+set -e\n+\n+APT_INSTALL=\"apt update >/dev/null && apt install -y build-essential \"\\\n+\"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null\"\n+\n+TEST_GIT_ENV=\"DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET \"\\\n+\"GIT_PROVE_OPTS=\\\"$GIT_PROVE_OPTS\\\" \"\\\n+\"GIT_TEST_OPTS=\\\"$GIT_TEST_OPTS\\\" \"\\\n+\"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB\"\n+\n+TEST_GIT_CMD=\"linux32 --32bit i386 sh -c \"\\\n+\"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'\"\n+\n+sudo docker run \\\n+ --interactive --volume \"${PWD}:/usr/src/git\" \\\n+ daald/ubuntu32:xenial /bin/bash -c \"$TEST_GIT_CMD\"\n\nbase-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858\n--\n2.11.1\n\n"},{"id":"313061","messageId":"alpine.DEB.2.20.1703021210170.3767@virtualbox","threadId":"45254","inReplyTo":"20170302105157.59791-1-larsxschneider@gmail.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-02T11:24:30Z","receivedAt":"2017-03-02T12:19:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Thu, 2 Mar 2017, Lars Schneider wrote:\n\n> The patch looks good to me in general but I want to propose the following\n> changes:\n\nI know you are using your script to generate this mail, but I would have\nliked to see v2 in the subject ;-)\n\n> (1) Move all the docker magic into a dedicated file\n> \"ci/run-linux-32-build.sh\" This way people should be able to run this\n> build on their local machines without TravisCI. However, I haven't\n> tested this.\n\nI considered this, but there is serious overlap: the `docker pull` call\nand the `docker run` call *have* to refer to the same image. It's very\neasy for them to get out of sync if you have that information in two\nfiles. Maybe make that an option of the script, defaulting to\ndaald/ubuntu32:xenial?\n\nBTW speaking of Docker: it would be nicer if there was a Docker image that\nalready had the build-essentials installed, to save on startup time. But I\ndid not find any that was reasonably up-to-date.\n\n> (2) The docker build command inherits the Git test environment\n> variables. This way we use the same environment variables as in all\n> other TravisCI builds (plus it would use *your* variables if you run it\n> locally).\n\nGood!\n\n> (3) Silence the apt update/git output as is it clutters the log. I did\n> not silence stderr output!\n\nAlso good!\n\n> (4) Remove (to my knowledge) superfluous \"compiler: clang\" in the\n> Linux32 job.\n\nI copied that from one of your experimental .travis.yml patches ;-)\n\n> I added my sign-off. I hope this is the right thing to do in this \"I\n> took your patch and changed it to suggest an improvement\" situation.\n\nAbsolutely. Thank you for taking it from here.\n\n> One thing that still bugs me: In the Linux32 environment prove adds the\n> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00\n> csys ... Has anyone an idea why that happens and how we can disable it?\n\nI have no idea.\n\n> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\n> new file mode 100755\n> index 0000000000..b892fbdc9e\n> --- /dev/null\n> +++ b/ci/run-linux32-build.sh\n> @@ -0,0 +1,21 @@\n> +#!/bin/sh\n> +#\n> +# Build and test Git in a docker container running a 32-bit Ubuntu Linux\n> +#\n> +\n> +set -e\n\nIs this really necessary? I really like to avoid `set -e`, in particular\nwhen we do pretty much everything in && chains anyway.\n\n> +APT_INSTALL=\"apt update >/dev/null && apt install -y build-essential \"\\\n> +\"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null\"\n> +\n> +TEST_GIT_ENV=\"DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET \"\\\n> +\"GIT_PROVE_OPTS=\\\"$GIT_PROVE_OPTS\\\" \"\\\n> +\"GIT_TEST_OPTS=\\\"$GIT_TEST_OPTS\\\" \"\\\n> +\"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB\"\n> +\n> +TEST_GIT_CMD=\"linux32 --32bit i386 sh -c '\"\\\n> +\"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'\"\n> +\n> +sudo docker run \\\n> + --interactive --volume \"${PWD}:/usr/src/git\" \\\n> + daald/ubuntu32:xenial /bin/bash -c \"$TEST_GIT_CMD\"\n\nHmm. Since it is a script now, it would be more readable this way, I\nthink:\n\nsudo docker run --volume \"${PWD}:/usr/src/git\" \"${1:-daald/ubuntu32:xenial}\" \\\nlinux32 --32bit i386 sh -c '\n\t: update packages first &&\n\tapt update >/dev/null &&\n\tapt install -y build-essential libcurl4-openssl-dev libssl-dev \\\n\t\tlibexpat-dev gettext python >/dev/null &&\n\n\t: now build and test &&\n\tcd /usr/src/git &&\n\tDEFAULT_TEST_TARGET='\"$DEFAULT_TEST_TARGET\"' \\\n\tGIT_PROVE_OPTS='\"$GIT_PROVE_OPTS\"' \\\n\tGIT_TEST_OPTS='\"$GIT_TEST_OPTS\"' \\\n\tGIT_TEST_CLONE_2GB='\"$GIT_TEST_CLONE_2GB\"' \\\n\tmake -j2 test\n'\n\nThis is completely untested (pun intended ;-))...\n\nCiao,\nDscho\n"},{"id":"313070","messageId":"CFA1C4B4-0FDA-424D-87A4-EEE1F9BB3712@gmail.com","threadId":"45254","inReplyTo":"alpine.DEB.2.20.1703021210170.3767@virtualbox","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-02T11:38:47Z","receivedAt":"2017-03-02T14:52:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 02 Mar 2017, at 12:24, Johannes Schindelin wrote:\n> \n> Hi Lars,\n> \n> On Thu, 2 Mar 2017, Lars Schneider wrote:\n> \n>> The patch looks good to me in general but I want to propose the following\n>> changes:\n> \n> I know you are using your script to generate this mail, but I would have\n> liked to see v2 in the subject ;-)\n\nYeah, sorry. I already had a \"D'oh\" moment *after* I saw the email in \nmy email client. Now I am wondering... is the next version v2 or v3 :D\n\n\n>> (1) Move all the docker magic into a dedicated file\n>> \"ci/run-linux-32-build.sh\" This way people should be able to run this\n>> build on their local machines without TravisCI. However, I haven't\n>> tested this.\n> \n> I considered this, but there is serious overlap: the `docker pull` call\n> and the `docker run` call *have* to refer to the same image. It's very\n> easy for them to get out of sync if you have that information in two\n> files. Maybe make that an option of the script, defaulting to\n> daald/ubuntu32:xenial?\n\nRight. I missed that. How about something like that?\n\n before_install:\n - ci/run-linux32-build.sh --pull-container\n before_script:\n script: ci/run-linux32-build.sh\n\n\n> BTW speaking of Docker: it would be nicer if there was a Docker image that\n> already had the build-essentials installed, to save on startup time. But I\n> did not find any that was reasonably up-to-date.\n\nTrue. But installing everything just takes a minute and we don't need to\nmaintain anything...\n\n\n>> +set -e\n> \n> Is this really necessary? I really like to avoid `set -e`, in particular\n> when we do pretty much everything in && chains anyway.\n\nAgreed, not really necessary here as we just invoke one command.\nOut of curiosity: Why do you try to avoid it? I set it by default in all \nmy scripts.\n\n\n>> +APT_INSTALL=\"apt update >/dev/null && apt install -y build-essential \"\\\n>> +\"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null\"\n>> +\n>> +TEST_GIT_ENV=\"DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET \"\\\n>> +\"GIT_PROVE_OPTS=\\\"$GIT_PROVE_OPTS\\\" \"\\\n>> +\"GIT_TEST_OPTS=\\\"$GIT_TEST_OPTS\\\" \"\\\n>> +\"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB\"\n>> +\n>> +TEST_GIT_CMD=\"linux32 --32bit i386 sh -c '\"\\\n>> +\"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'\"\n>> +\n>> +sudo docker run \\\n>> + --interactive --volume \"${PWD}:/usr/src/git\" \\\n>> + daald/ubuntu32:xenial /bin/bash -c \"$TEST_GIT_CMD\"\n> \n> Hmm. Since it is a script now, it would be more readable this way, I\n> think:\n> \n> sudo docker run --volume \"${PWD}:/usr/src/git\" \"${1:-daald/ubuntu32:xenial}\" \\\n> linux32 --32bit i386 sh -c '\n> \t: update packages first &&\n> \tapt update >/dev/null &&\n> \tapt install -y build-essential libcurl4-openssl-dev libssl-dev \\\n> \t\tlibexpat-dev gettext python >/dev/null &&\n> \n> \t: now build and test &&\n> \tcd /usr/src/git &&\n> \tDEFAULT_TEST_TARGET='\"$DEFAULT_TEST_TARGET\"' \\\n> \tGIT_PROVE_OPTS='\"$GIT_PROVE_OPTS\"' \\\n> \tGIT_TEST_OPTS='\"$GIT_TEST_OPTS\"' \\\n> \tGIT_TEST_CLONE_2GB='\"$GIT_TEST_CLONE_2GB\"' \\\n> \tmake -j2 test\n> '\n\nThat looks better! I'll try it!\n\n- Lars"},{"id":"313072","messageId":"alpine.DEB.2.20.1703021519330.3767@virtualbox","threadId":"45254","inReplyTo":"CFA1C4B4-0FDA-424D-87A4-EEE1F9BB3712@gmail.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-02T14:22:42Z","receivedAt":"2017-03-02T14:56:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\n\nOn Thu, 2 Mar 2017, Lars Schneider wrote:\n\n> > On 02 Mar 2017, at 12:24, Johannes Schindelin wrote:\n> > \n> > On Thu, 2 Mar 2017, Lars Schneider wrote:\n> > \n> >> The patch looks good to me in general but I want to propose the\n> >> following changes:\n> > \n> > I know you are using your script to generate this mail, but I would\n> > have liked to see v2 in the subject ;-)\n> \n> Yeah, sorry. I already had a \"D'oh\" moment *after* I saw the email in my\n> email client. Now I am wondering... is the next version v2 or v3 :D\n\nSince there was no v2, the next one should *definitely* be v2... ;-)\n\n> >> (1) Move all the docker magic into a dedicated file\n> >> \"ci/run-linux-32-build.sh\" This way people should be able to run this\n> >> build on their local machines without TravisCI. However, I haven't\n> >> tested this.\n> > \n> > I considered this, but there is serious overlap: the `docker pull`\n> > call and the `docker run` call *have* to refer to the same image. It's\n> > very easy for them to get out of sync if you have that information in\n> > two files. Maybe make that an option of the script, defaulting to\n> > daald/ubuntu32:xenial?\n> \n> Right. I missed that. How about something like that?\n> \n> before_install:\n> - ci/run-linux32-build.sh --pull-container\n> before_script:\n> script: ci/run-linux32-build.sh\n\nI'd prefer\n\n\tbefore_install:\n\t - docker pull daald/ubuntu32:xenial\n\tbefore_script:\n\tscript: ci/run-linux32-build.sh daald/ubuntu32:xenial\n\n> > BTW speaking of Docker: it would be nicer if there was a Docker image\n> > that already had the build-essentials installed, to save on startup\n> > time. But I did not find any that was reasonably up-to-date.\n> \n> True. But installing everything just takes a minute and we don't need to\n> maintain anything...\n\nAnd when there are network problems (like there were on Tuesday, right\nwhen I developed the first v1 of this patch) then we have another set of\nproblems that make Travis fail. Even if the code in the PR or branch is\nactually good. I'd like to avoid false positives, if possible.\n\n> >> +set -e\n> > \n> > Is this really necessary? I really like to avoid `set -e`, in\n> > particular when we do pretty much everything in && chains anyway.\n> \n> Agreed, not really necessary here as we just invoke one command. Out of\n> curiosity: Why do you try to avoid it? I set it by default in all my\n> scripts.\n\nI try to avoid it because it encourages a style that omits helpful error\nmessages.\n\n> >> +APT_INSTALL=\"apt update >/dev/null && apt install -y build-essential \"\\\n> >> +\"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null\"\n> >> +\n> >> +TEST_GIT_ENV=\"DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET \"\\\n> >> +\"GIT_PROVE_OPTS=\\\"$GIT_PROVE_OPTS\\\" \"\\\n> >> +\"GIT_TEST_OPTS=\\\"$GIT_TEST_OPTS\\\" \"\\\n> >> +\"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB\"\n> >> +\n> >> +TEST_GIT_CMD=\"linux32 --32bit i386 sh -c '\"\\\n> >> +\"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'\"\n> >> +\n> >> +sudo docker run \\\n> >> + --interactive --volume \"${PWD}:/usr/src/git\" \\\n> >> + daald/ubuntu32:xenial /bin/bash -c \"$TEST_GIT_CMD\"\n> > \n> > Hmm. Since it is a script now, it would be more readable this way, I\n> > think:\n> > \n> > sudo docker run --volume \"${PWD}:/usr/src/git\" \"${1:-daald/ubuntu32:xenial}\" \\\n> > linux32 --32bit i386 sh -c '\n> > \t: update packages first &&\n> > \tapt update >/dev/null &&\n> > \tapt install -y build-essential libcurl4-openssl-dev libssl-dev \\\n> > \t\tlibexpat-dev gettext python >/dev/null &&\n> > \n> > \t: now build and test &&\n> > \tcd /usr/src/git &&\n> > \tDEFAULT_TEST_TARGET='\"$DEFAULT_TEST_TARGET\"' \\\n> > \tGIT_PROVE_OPTS='\"$GIT_PROVE_OPTS\"' \\\n> > \tGIT_TEST_OPTS='\"$GIT_TEST_OPTS\"' \\\n> > \tGIT_TEST_CLONE_2GB='\"$GIT_TEST_CLONE_2GB\"' \\\n> > \tmake -j2 test\n> > '\n> \n> That looks better! I'll try it!\n\nThanks!\nDscho\n"},{"id":"313074","messageId":"4a27f62d-5576-df9d-257f-60ff327547e6@ramsayjones.plus.com","threadId":"45254","inReplyTo":"alpine.DEB.2.20.1703021210170.3767@virtualbox","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-02T15:17:41Z","receivedAt":"2017-03-02T15:33:20Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 02/03/17 11:24, Johannes Schindelin wrote:\n> Hi Lars,\n> \n> On Thu, 2 Mar 2017, Lars Schneider wrote:\n> \n[snip]\n>> One thing that still bugs me: In the Linux32 environment prove adds the\n>> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00\n>> csys ... Has anyone an idea why that happens and how we can disable it?\n> \n> I have no idea.\n\nI have no idea either, but it is not unique to this 32bit Linux, but\nrather the version of prove. For example, I am seeing this on Linux\nMint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do\nthis. (They used different Ubuntu LTS releases).\n\n[Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]\n\nATB,\nRamsay Jones\n\n"},{"id":"313078","messageId":"CAP8UFD0MDVMVOG=bbcEeRbKX+aTNKfLd9yZn5zW7T9KxeRsebQ@mail.gmail.com","threadId":"45254","inReplyTo":"alpine.DEB.2.20.1703021519330.3767@virtualbox","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-03-02T15:53:28Z","receivedAt":"2017-03-02T16:03:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Mar 2, 2017 at 3:22 PM, Johannes Schindelin\n wrote:\n>\n>> >> +set -e\n>> >\n>> > Is this really necessary? I really like to avoid `set -e`, in\n>> > particular when we do pretty much everything in && chains anyway.\n>>\n>> Agreed, not really necessary here as we just invoke one command. Out of\n>> curiosity: Why do you try to avoid it? I set it by default in all my\n>> scripts.\n>\n> I try to avoid it because it encourages a style that omits helpful error\n> messages.\n\nYeah, we prefer to define and use a die() function like this:\n\ndie () {\n printf >&2 '%s\\n' \"$*\"\n exit 1\n}\n\ndo_something || die \"meaningful error message\"\n"},{"id":"313085","messageId":"xmqqinnrd098.fsf@gitster.mtv.corp.google.com","threadId":"45254","inReplyTo":"CFA1C4B4-0FDA-424D-87A4-EEE1F9BB3712@gmail.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-02T18:03:31Z","receivedAt":"2017-03-02T18:58:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 02 Mar 2017, at 12:24, Johannes Schindelin wrote:\n>> \n>> Hi Lars,\n>> \n>> On Thu, 2 Mar 2017, Lars Schneider wrote:\n>> \n>>> The patch looks good to me in general but I want to propose the following\n>>> changes:\n>> \n>> I know you are using your script to generate this mail, but I would have\n>> liked to see v2 in the subject ;-)\n>\n> Yeah, sorry. I already had a \"D'oh\" moment *after* I saw the email in \n> my email client. Now I am wondering... is the next version v2 or v3 :D\n\nAnother question is which v3 people mean in the discussion, when you\nand Dscho work on improvements at the same time and each post the\n\"next\" version marked as \"v3\", and they comment on one of them?\n\nBetween \"v2\" and \"v3\", it would not make too much difference to the\nreadership, when it is clear that two people are working to produce\ncompeting improvements without much coordination (i.e. lack of \"ok,\nI'll send a reroll marked as vX in a minite\"--\"ok, I'll wait and\ncomment on it\" exchange). People watching from the sideline know\n\"ah this is v3 from Lars which is the highest numbered one from him\non this topic\". As long as you do not mark your next one \"v1\",\nyou'd be OK ;-).\n"},{"id":"313124","messageId":"alpine.DEB.2.20.1703030315580.3767@virtualbox","threadId":"45254","inReplyTo":"xmqqinnrd098.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-03T02:17:30Z","receivedAt":"2017-03-03T02:25:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 2 Mar 2017, Junio C Hamano wrote:\n\n> Another question is which v3 people mean in the discussion, when you\n> and Dscho work on improvements at the same time and each post the\n> \"next\" version marked as \"v3\", and they comment on one of them?\n\nBut then, Lars & I communicate in a more direct way than the mailing list\nwhen discussing teeny tiny details such as: \"could you have a look at my\nmail\" or \"how would you change .travis.yml to do XYZ\".\n\nWith that level of communication, there is almost no danger of two v3s.\n\nCiao,\nJohannes\n"},{"id":"313218","messageId":"xmqqh93a9p5r.fsf@gitster.mtv.corp.google.com","threadId":"45254","inReplyTo":"alpine.DEB.2.20.1703030315580.3767@virtualbox","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T18:43:44Z","receivedAt":"2017-03-03T18:59:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 2 Mar 2017, Junio C Hamano wrote:\n>\n>> Another question is which v3 people mean in the discussion, when you\n>> and Dscho work on improvements at the same time and each post the\n>> \"next\" version marked as \"v3\", and they comment on one of them?\n>\n> But then, Lars & I communicate in a more direct way than the mailing list\n> when discussing teeny tiny details such as: \"could you have a look at my\n> mail\" or \"how would you change .travis.yml to do XYZ\".\n>\n> With that level of communication, there is almost no danger of two v3s.\n\nSure, that is true between two (or more) people who communicate\nprivately. The issue you raised on Lars's \"v1\" and your original\nunversioned one can be seen either (1) as similar to having two v1's\nor (2) having (an implicit) v0 and v1 that won't cause confusion.\n\nAs I said already, because the v0 and v1 (or v1 and v1) came from\ntwo different people, it's not such a big deal if Lars named his\nfirst one v1 or v2, just like it won't cause problem if his reroll\nwere done as v2 or v3. \n\nI see he did v2 which you Acked in a different thread. Will replace\nwhat's been on 'pu' and running with Travis the past few days with\nit. Let's wait for one or more Travis cycles and then merge it to\n'next'.\n\nThanks.\n"},{"id":"313250","messageId":"xmqq8tol7vs1.fsf@gitster.mtv.corp.google.com","threadId":"45254","inReplyTo":"xmqqh93a9p5r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-04T00:03:42Z","receivedAt":"2017-03-04T00:18:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I see he did v2 which you Acked in a different thread. Will replace\n> what's been on 'pu' and running with Travis the past few days with\n> it. Let's wait for one or more Travis cycles and then merge it to\n> 'next'.\n\nhttps://travis-ci.org/git/git/jobs/207517043 is an output from 'pu'\nwith the v2 patch; it seems to have fell into a funny state. The\noutput ends like so:\n\n No output has been received in the last 10m0s, this potentially\n indicates a stalled build or something wrong with the build itself.\n\nI only recently started looking at Travis logs, so I cannot tell if\nit is just a usual flake (e.g. some builds a few days ago seems to\nhave failed due to not being able to check out the tree being\ntested, which I do not think is our fault) that we shouldn't worry\nabout, or if it is a sign of a real problem.\n\nUnrelated to linux-32, the same build has hard failure with Apple\nclang in t0021 with the rot13-filter.pl thing, by the way.\n\n"},{"id":"313259","messageId":"CAPc5daW=gtN18JZTQMqUje5fxL4oNdTucB0dXFbybPRJggPBUw@mail.gmail.com","threadId":"45254","inReplyTo":"xmqq8tol7vs1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-04T04:11:12Z","receivedAt":"2017-03-04T04:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano wrote:\n>\n> I only recently started looking at Travis logs, so I cannot tell if\n> it is just a usual flake (e.g. some builds a few days ago seems to\n> have failed due to not being able to check out the tree being\n> tested, which I do not think is our fault) that we shouldn't worry\n> about, or if it is a sign of a real problem.\n\nTonight's pushout also seems to stall the same way. Dscho's\nunversioned one didn't exhibit the problem?\nhttps://travis-ci.org/git/git/jobs/206811396\n\n> Unrelated to linux-32, the same build has hard failure with Apple\n> clang in t0021 with the rot13-filter.pl thing, by the way.\n\nThis one may be a Heisenbug which may indicate some raciness in the\nclean/smudge filter protocol.\nThe latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to\nhave passed OK.\n"},{"id":"313273","messageId":"2205F1A7-A694-4F40-B994-D68C3947F2BB@gmail.com","threadId":"45254","inReplyTo":"CAPc5daW=gtN18JZTQMqUje5fxL4oNdTucB0dXFbybPRJggPBUw@mail.gmail.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-04T17:23:29Z","receivedAt":"2017-03-04T17:24:32Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 04 Mar 2017, at 05:11, Junio C Hamano wrote:\n> \n> On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano wrote:\n>> \n>> I only recently started looking at Travis logs, so I cannot tell if\n>> it is just a usual flake (e.g. some builds a few days ago seems to\n>> have failed due to not being able to check out the tree being\n>> tested, which I do not think is our fault) that we shouldn't worry\n>> about, or if it is a sign of a real problem.\n> \n> Tonight's pushout also seems to stall the same way. Dscho's\n> unversioned one didn't exhibit the problem?\n> https://travis-ci.org/git/git/jobs/206811396\n\nDid Travis find our first 32bit bug? I briefly looked into it\nand the following new topic on pu seems to cause the issue:\n\nhttp://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/\nhttps://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd\n\nThe \"git log\" call in the new 4211 test fails with:\n*** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 ***\n\nMore output here:\nhttps://travis-ci.org/larsxschneider/git/builds/207715343\n\n\n\n>> Unrelated to linux-32, the same build has hard failure with Apple\n>> clang in t0021 with the rot13-filter.pl thing, by the way.\n> \n> This one may be a Heisenbug which may indicate some raciness in the\n> clean/smudge filter protocol.\n> The latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to\n> have passed OK.\n\nI agree, there seems to be some kind of race condition in\n\"t0021.15 - required process filter should filter data\". I try to\nlook into it.\n\n\n- Lars"},{"id":"313276","messageId":"f5f5886a-aaec-7426-ea33-f5d65516348b@oracle.com","threadId":"45254","inReplyTo":"2205F1A7-A694-4F40-B994-D68C3947F2BB@gmail.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-04T18:08:15Z","receivedAt":"2017-03-04T18:08:39Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 04/03/2017 18:23, Lars Schneider wrote:\n> Did Travis find our first 32bit bug? I briefly looked into it\n> and the following new topic on pu seems to cause the issue:\n>\n> http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/\n> https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd\n>\n> The \"git log\" call in the new 4211 test fails with:\n> *** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 ***\n>\n> More output here:\n> https://travis-ci.org/larsxschneider/git/builds/207715343\n\nIt looks like it's hitting the bug the patch is supposed to fix.\n\nAre you quite sure it's running the \"git\" binary that was just built (as\nopposed to e.g. the system binary installed inside the container)?\n\n\nVegard\n"},{"id":"313282","messageId":"af31ef46-bd0c-c3f2-5a1e-7d97da6ec9a0@oracle.com","threadId":"45254","inReplyTo":"f5f5886a-aaec-7426-ea33-f5d65516348b@oracle.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-04T19:49:23Z","receivedAt":"2017-03-04T19:49:48Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 04/03/2017 19:08, Vegard Nossum wrote:\n> On 04/03/2017 18:23, Lars Schneider wrote:\n>> Did Travis find our first 32bit bug? I briefly looked into it\n>> and the following new topic on pu seems to cause the issue:\n>>\n>> http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/\n>>\n>> https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd\n>>\n>>\n>> The \"git log\" call in the new 4211 test fails with:\n>> *** Error in `/usr/src/git/git': malloc: top chunk is corrupt:\n>> 0x09ff4a78 ***\n>>\n>> More output here:\n>> https://travis-ci.org/larsxschneider/git/builds/207715343\n>\n> It looks like it's hitting the bug the patch is supposed to fix.\n>\n> Are you quite sure it's running the \"git\" binary that was just built (as\n> opposed to e.g. the system binary installed inside the container)?\n\nNevermind, I can reproduce it locally.\n\n==10205== Invalid write of size 4\n==10205== at 0x4031ED2: memcpy (in \n/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)\n==10205== by 0x811C4B0: memcpy (string3.h:53)\n==10205== by 0x811C4B0: range_set_copy.isra.7 (line-log.c:46)\n==10205== by 0x811C51B: line_log_data_copy_one (line-log.c:628)\n==10205== by 0x811C55D: line_log_data_copy (line-log.c:642)\n==10205== by 0x811C5E3: add_line_range (line-log.c:708)\n==10205== by 0x811D767: line_log_init (line-log.c:745)\n==10205== by 0x808B1CD: cmd_log_init_finish (log.c:204)\n==10205== by 0x808C9C8: cmd_log_init (log.c:213)\n==10205== by 0x808C9C8: cmd_log (log.c:681)\n==10205== by 0x804CF14: run_builtin (git.c:373)\n==10205== by 0x804CF14: handle_builtin (git.c:574)\n==10205== by 0x804D264: run_argv (git.c:626)\n==10205== by 0x804D264: cmd_main (git.c:703)\n==10205== by 0x804C448: main (common-main.c:43)\n==10205== Address 0x4cde9c8 is 0 bytes after a block of size 1,600 alloc'd\n==10205== at 0x402D17C: malloc (in \n/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)\n==10205== by 0x402F370: realloc (in \n/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)\n==10205== by 0x819CC0F: xrealloc (wrapper.c:137)\n==10205== by 0x811C1C8: range_set_grow (line-log.c:21)\n==10205== by 0x811C499: range_set_init (line-log.c:32)\n==10205== by 0x811C499: range_set_copy.isra.7 (line-log.c:45)\n==10205== by 0x811C51B: line_log_data_copy_one (line-log.c:628)\n==10205== by 0x811C55D: line_log_data_copy (line-log.c:642)\n==10205== by 0x811C5E3: add_line_range (line-log.c:708)\n==10205== by 0x811D767: line_log_init (line-log.c:745)\n==10205== by 0x808B1CD: cmd_log_init_finish (log.c:204)\n==10205== by 0x808C9C8: cmd_log_init (log.c:213)\n==10205== by 0x808C9C8: cmd_log (log.c:681)\n==10205== by 0x804CF14: run_builtin (git.c:373)\n==10205== by 0x804CF14: handle_builtin (git.c:574)\n\nAt a glance, looks like range_set_copy() is using\nsizeof(struct range_set) == 12, but\nrange_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using\nsizeof(rs->range) == 8.\n\n\nVegard\n"},{"id":"313284","messageId":"282895e1-d9eb-2368-a8e7-8085ad9b17ed@oracle.com","threadId":"45254","inReplyTo":"af31ef46-bd0c-c3f2-5a1e-7d97da6ec9a0@oracle.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-04T20:08:40Z","receivedAt":"2017-03-04T20:09:48Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 04/03/2017 20:49, Vegard Nossum wrote:\n> On 04/03/2017 19:08, Vegard Nossum wrote:\n>> On 04/03/2017 18:23, Lars Schneider wrote:\n>>> Did Travis find our first 32bit bug? I briefly looked into it\n>>> and the following new topic on pu seems to cause the issue:\n>>>\n>>> http://public-inbox.org/git/20170302172902.16850-1-allan.x.xavier@oracle.com/\n>>>\n>>>\n>>> https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd\n>>>\n>>>\n>>>\n>>> The \"git log\" call in the new 4211 test fails with:\n>>> *** Error in `/usr/src/git/git': malloc: top chunk is corrupt:\n>>> 0x09ff4a78 ***\n>>>\n>>> More output here:\n>>> https://travis-ci.org/larsxschneider/git/builds/207715343\n[...]\n> At a glance, looks like range_set_copy() is using\n> sizeof(struct range_set) == 12, but\n> range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using\n> sizeof(rs->range) == 8.\n\nAttached patch seems to fix it -- basically, range_set_copy() is trying\nto copy more than it should. It was uncovered with the test case from\nAllan's commit because it's creating enough ranges to overflow the\ninitial allocation on 32-bit.\n\n\nVegard\n\n\ndiff --git a/line-log.c b/line-log.c\nindex 951029665..cb0dc1110 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -43,7 +43,7 @@ void range_set_release(struct range_set *rs)\n static void range_set_copy(struct range_set *dst, struct range_set *src)\n {\n \trange_set_init(dst, src->nr);\n-\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));\n+\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));\n \tdst->nr = src->nr;\n }\n static void range_set_move(struct range_set *dst, struct range_set *src)\n"},{"id":"313290","messageId":"20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net","threadId":"45254","inReplyTo":"282895e1-d9eb-2368-a8e7-8085ad9b17ed@oracle.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-05T11:36:19Z","receivedAt":"2017-03-05T11:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2017 at 09:08:40PM +0100, Vegard Nossum wrote:\n\n> > At a glance, looks like range_set_copy() is using\n> > sizeof(struct range_set) == 12, but\n> > range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using\n> > sizeof(rs->range) == 8.\n> \n> Attached patch seems to fix it -- basically, range_set_copy() is trying\n> to copy more than it should. It was uncovered with the test case from\n> Allan's commit because it's creating enough ranges to overflow the\n> initial allocation on 32-bit.\n\nUgh, yeah, that is definitely a bug.\n\n> diff --git a/line-log.c b/line-log.c\n> index 951029665..cb0dc1110 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -43,7 +43,7 @@ void range_set_release(struct range_set *rs)\n> static void range_set_copy(struct range_set *dst, struct range_set *src)\n> {\n> \trange_set_init(dst, src->nr);\n> -\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));\n> +\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));\n\nI think \"sizeof(*dst->ranges)\" is probably an even better fix, as it\ninfers the type of \"dst\". But these days we have COPY_ARRAY() to make it\neven harder to get this kind of thing wrong.\n\nI grepped for 'memcpy.*sizeof' and found one other case that's not a\nbug, but is questionable.\n\nOf the \"good\" cases, I think most of them could be converted into\nsomething more obviously-correct, which would make auditing easier. The\nthree main cases I saw were:\n\n 1. Ones which can probably be converted to COPY_ARRAY().\n\n 2. Ones which just copy a single object, like:\n\n memcpy(&dst, &src, sizeof(dst));\n\n Perhaps we should be using struct assignment like:\n\n dst = src;\n\n here. It's safer and it should give the compiler more room to\n optimize. The only downside is that if you have pointers, it is\n easy to write \"dst = src\" when you meant \"*dst = *src\".\n\n 3. There were a number of alloc-and-copy instances. The copy part is\n the same as (2) above, but you have to repeat the size, which is\n potentially error-prone. I wonder if we would want something like:\n\n #define ALLOC_COPY(dst, src) do { \\\n (dst) = xmalloc(sizeof(*(dst))); \\\n\t COPY_ARRAY(dst, src, 1); \\\n while(0)\n\n That avoids having to specify the size at all, and triggers a\n compile-time error if \"src\" and \"dst\" point to objects of different\n sizes.\n\n I suspect our friendly neighborhood coccinelle wizards could cook\n up a conversion.\n\n-Peff\n"},{"id":"313291","messageId":"20170305114446.wqm33o52yizlffju@sigill.intra.peff.net","threadId":"45254","inReplyTo":"20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net","subject":"[PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-05T11:44:46Z","receivedAt":"2017-03-05T11:44:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:\n\n> > \trange_set_init(dst, src->nr);\n> > -\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));\n> > +\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));\n> \n> I think \"sizeof(*dst->ranges)\" is probably an even better fix, as it\n> infers the type of \"dst\". But these days we have COPY_ARRAY() to make it\n> even harder to get this kind of thing wrong.\n\nSo here's your fix wrapped up with a commit message, mostly for Junio's\nconvenience. I listed you as the author, since you did the hard part. If\nyou're OK with it, please indicate that it's OK to add your\nsigned-off-by. If you prefer to do it differently, feel free to post\nyour own patch.\n\n-- >8 --\nFrom: Vegard Nossum \nSubject: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy\n\nThis memcpy meant to get the sizeof a \"struct range\", not a\n\"range_set\", as the former is what our array holds. Rather\nthan swap out the types, let's convert this site to\nCOPY_ARRAY, which avoids the problem entirely (and confirms\nthat the src and dst types match).\n\nNote for curiosity's sake that this bug doesn't trigger on\nI32LP64 systems, but does on ILP32 systems. The mistaken\n\"struct range_set\" has two ints and a pointer. That's 16\nbytes on LP64, or 12 on ILP32. The correct \"struct range\"\ntype has two longs, which is also 16 on LP64, but only 8 on\nILP32.\n\nLikewise an IL32P64 system would experience the bug.\n\nSigned-off-by: Jeff King \n---\n line-log.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 65f3558b3..a2477f601 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -43,9 +43,10 @@ void range_set_release(struct range_set *rs)\n static void range_set_copy(struct range_set *dst, struct range_set *src)\n {\n \trange_set_init(dst, src->nr);\n-\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));\n+\tCOPY_ARRAY(dst->ranges, src->ranges, src->nr);\n \tdst->nr = src->nr;\n }\n+\n static void range_set_move(struct range_set *dst, struct range_set *src)\n {\n \trange_set_release(dst);\n-- \n2.12.0.426.g9d5d0eeae\n\n\n"},{"id":"313292","messageId":"20170305114638.x7g767js2m3othga@sigill.intra.peff.net","threadId":"45254","inReplyTo":"20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-05T11:46:38Z","receivedAt":"2017-03-05T11:46:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:\n\n> I grepped for 'memcpy.*sizeof' and found one other case that's not a\n> bug, but is questionable.\n\nAnd here's the fix for that case. It can be applied separately from the\nother patch if need be.\n\n-- >8 --\nSubject: [PATCH] ewah: fix eword_t/uint64_t confusion\n\nThe ewah subsystem typedefs eword_t to be uint64_t, but some\ncode uses a bare uint64_t. This isn't a bug now, but it's a\npotential maintenance problem if the definition of eword_t\never changes. Let's use the correct type.\n\nNote that we can't use COPY_ARRAY() here because the source\nand destination point to objects of different sizes. For\nthat reason we'll also skip the usual \"sizeof(*dst)\" and use\nthe real type, which should make it more clear that there's\nsomething tricky going on.\n\nSigned-off-by: Jeff King \n---\n ewah/ewah_io.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/ewah/ewah_io.c b/ewah/ewah_io.c\nindex 61f6a4357..f73210973 100644\n--- a/ewah/ewah_io.c\n+++ b/ewah/ewah_io.c\n@@ -142,8 +142,8 @@ int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len)\n \t * the endianness conversion in a separate pass to ensure\n \t * we're loading 8-byte aligned words.\n \t */\n-\tmemcpy(self->buffer, ptr, self->buffer_size * sizeof(uint64_t));\n-\tptr += self->buffer_size * sizeof(uint64_t);\n+\tmemcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));\n+\tptr += self->buffer_size * sizeof(eword_t);\n \n \tfor (i = 0; i < self->buffer_size; ++i)\n \t\tself->buffer[i] = ntohll(self->buffer[i]);\n-- \n2.12.0.426.g9d5d0eeae\n\n"},{"id":"313293","messageId":"8bd81e58-6d9d-b7f3-b1b0-bb2a352c92c9@oracle.com","threadId":"45254","inReplyTo":"20170305114446.wqm33o52yizlffju@sigill.intra.peff.net","subject":"Re: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-05T12:20:00Z","receivedAt":"2017-03-05T12:20:24Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 05/03/2017 12:44, Jeff King wrote:\n> On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:\n>\n>>> \trange_set_init(dst, src->nr);\n>>> -\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));\n>>> +\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));\n>>\n>> I think \"sizeof(*dst->ranges)\" is probably an even better fix, as it\n>> infers the type of \"dst\". But these days we have COPY_ARRAY() to make it\n>> even harder to get this kind of thing wrong.\n>\n> So here's your fix wrapped up with a commit message, mostly for Junio's\n> convenience. I listed you as the author, since you did the hard part. If\n> you're OK with it, please indicate that it's OK to add your\n> signed-off-by. If you prefer to do it differently, feel free to post\n> your own patch.\n\nThanks.\n\n>\n> -- >8 --\n> From: Vegard Nossum \n> Subject: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy\n>\n> This memcpy meant to get the sizeof a \"struct range\", not a\n> \"range_set\", as the former is what our array holds. Rather\n> than swap out the types, let's convert this site to\n> COPY_ARRAY, which avoids the problem entirely (and confirms\n> that the src and dst types match).\n>\n> Note for curiosity's sake that this bug doesn't trigger on\n> I32LP64 systems, but does on ILP32 systems. The mistaken\n> \"struct range_set\" has two ints and a pointer. That's 16\n> bytes on LP64, or 12 on ILP32. The correct \"struct range\"\n> type has two longs, which is also 16 on LP64, but only 8 on\n> ILP32.\n>\n> Likewise an IL32P64 system would experience the bug.\n>\n> Signed-off-by: Jeff King \n\nSigned-off-by: Vegard Nossum \n\n> ---\n> line-log.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/line-log.c b/line-log.c\n> index 65f3558b3..a2477f601 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -43,9 +43,10 @@ void range_set_release(struct range_set *rs)\n> static void range_set_copy(struct range_set *dst, struct range_set *src)\n> {\n> \trange_set_init(dst, src->nr);\n> -\tmemcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));\n> +\tCOPY_ARRAY(dst->ranges, src->ranges, src->nr);\n> \tdst->nr = src->nr;\n> }\n> +\n> static void range_set_move(struct range_set *dst, struct range_set *src)\n> {\n> \trange_set_release(dst);\n>\n\n\nVegard\n"},{"id":"313298","messageId":"125BAC7C-0FCF-4E0E-9757-2BFACD1910C5@gmail.com","threadId":"45254","inReplyTo":"4a27f62d-5576-df9d-257f-60ff327547e6@ramsayjones.plus.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-05T17:38:37Z","receivedAt":"2017-03-05T17:45:20Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 02 Mar 2017, at 16:17, Ramsay Jones wrote:\n> \n> \n> \n> On 02/03/17 11:24, Johannes Schindelin wrote:\n>> Hi Lars,\n>> \n>> On Thu, 2 Mar 2017, Lars Schneider wrote:\n>> \n> [snip]\n>>> One thing that still bugs me: In the Linux32 environment prove adds the\n>>> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00\n>>> csys ... Has anyone an idea why that happens and how we can disable it?\n>> \n>> I have no idea.\n> \n> I have no idea either, but it is not unique to this 32bit Linux, but\n> rather the version of prove. For example, I am seeing this on Linux\n> Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do\n> this. (They used different Ubuntu LTS releases).\n> \n> [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]\n\nI think I found it. It was introduced in TAP::Harness v3.34:\nhttps://github.com/Perl-Toolchain-Gang/Test-Harness/commit/66cbf6355928b4828db517a99f1099b7fed35e90\n\n... and it is enabled with the \"--timer\" switch.\n\n- Lars"},{"id":"313301","messageId":"1caa38bc-8423-dded-f01d-95ff3e75b92a@ramsayjones.plus.com","threadId":"45254","inReplyTo":"125BAC7C-0FCF-4E0E-9757-2BFACD1910C5@gmail.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-05T22:16:28Z","receivedAt":"2017-03-05T22:16:37Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/03/17 17:38, Lars Schneider wrote:\n>> On 02 Mar 2017, at 16:17, Ramsay Jones wrote:\n>> On 02/03/17 11:24, Johannes Schindelin wrote:\n>>> On Thu, 2 Mar 2017, Lars Schneider wrote:\n>>>\n>> [snip]\n>>>> One thing that still bugs me: In the Linux32 environment prove adds the\n>>>> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00\n>>>> csys ... Has anyone an idea why that happens and how we can disable it?\n>>>\n>>> I have no idea.\n>>\n>> I have no idea either, but it is not unique to this 32bit Linux, but\n>> rather the version of prove. For example, I am seeing this on Linux\n>> Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do\n>> this. (They used different Ubuntu LTS releases).\n>>\n>> [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]\n> \n> I think I found it. It was introduced in TAP::Harness v3.34:\n> https://github.com/Perl-Toolchain-Gang/Test-Harness/commit/66cbf6355928b4828db517a99f1099b7fed35e90\n> \n> ... and it is enabled with the \"--timer\" switch.\n\nYep, that looks like it.\n\nWhen I updated to Mint 18, this broke a perl script of mine, so I had\na quick look to see what I could do to suppress it. The man page seemed\nto imply that you could replace the output formatter, but that didn't\ntake me too far (search CPAN for TAP::Parser::Formatter: ;-) ). I suppose\nyou could replace Tap::Formatter::Base, or some such, but I didn't need\nto go that far - I simply changed a couple of regex-es to ignore the\nexcess output! :-P\n\nDo you really need to suppress that timing information or, like me, can\nyou simply ignore it?\n\nATB,\nRamsay Jones\n\n\n"},{"id":"313726","messageId":"63a6ec61-653d-6307-4739-2ebaa8dbde35@web.de","threadId":"45254","inReplyTo":"20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-10T00:14:45Z","receivedAt":"2017-03-10T00:15:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.03.2017 um 12:36 schrieb Jeff King:\n> I grepped for 'memcpy.*sizeof' and found one other case that's not a\n> bug, but is questionable.\n>\n> Of the \"good\" cases, I think most of them could be converted into\n> something more obviously-correct, which would make auditing easier. The\n> three main cases I saw were:\n\n> 3. There were a number of alloc-and-copy instances. The copy part is\n> the same as (2) above, but you have to repeat the size, which is\n> potentially error-prone. I wonder if we would want something like:\n>\n> #define ALLOC_COPY(dst, src) do { \\\n> (dst) = xmalloc(sizeof(*(dst))); \\\n> \t COPY_ARRAY(dst, src, 1); \\\n> while(0)\n>\n> That avoids having to specify the size at all, and triggers a\n> compile-time error if \"src\" and \"dst\" point to objects of different\n> sizes.\n\nOr you could call it DUP or similar. And you could use ALLOC_ARRAY in\nits definition and let it infer the size implicitly (don't worry too\nmuch about the multiplication with one):\n\n\t#define DUPLICATE_ARRAY(dst, src, n) do {\t\\\n\t\tALLOC_ARRAY((dst), (n));\t\t\\\n\t\tCOPY_ARRAY((dst), (src), (n));\t\t\\\n\t} while (0)\n\t#define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)\n\nBut do we even want such a thing? Duplicating objects should be rare, \nand keeping allocation and assignment/copying separate makes for more \nflexible building blocks. Adding ALLOC (and CALLOC) for single objects \ncould be more widely useful, I think.\n\nRené\n"},{"id":"313727","messageId":"c553da50-e5ca-d064-e75c-46e5a5042935@web.de","threadId":"45254","inReplyTo":"20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-10T00:14:16Z","receivedAt":"2017-03-10T00:15:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.03.2017 um 12:36 schrieb Jeff King:\n> I grepped for 'memcpy.*sizeof' and found one other case that's not a\n> bug, but is questionable.\n> \n> Of the \"good\" cases, I think most of them could be converted into\n> something more obviously-correct, which would make auditing easier. The\n> three main cases I saw were:\n\n> 2. Ones which just copy a single object, like:\n> \n> memcpy(&dst, &src, sizeof(dst));\n> \n> Perhaps we should be using struct assignment like:\n> \n> dst = src;\n> \n> here. It's safer and it should give the compiler more room to\n> optimize. The only downside is that if you have pointers, it is\n> easy to write \"dst = src\" when you meant \"*dst = *src\".\n\nCompilers can usually inline memcpy(3) calls, but assignments are\nshorter and more pleasing to the eye, and we get a type check for\nfree. How about this?\n\n-- >8 --\nSubject: [PATCH] cocci: use assignment operator to copy structs\n\nAdd a semantic patch for converting memcpy(3) calls targeting\naddresses of variables (i.e., variables preceded by &) -- which are\nbasically always structs -- to simple assignments, and apply it to\nthe current tree. The resulting code is shorter, simpler and its\ntype safety is checked by the compiler.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/log.c | 2 +-\n contrib/coccinelle/copy.cocci | 31 +++++++++++++++++++++++++++++++\n convert.c | 2 +-\n credential-cache--daemon.c | 2 +-\n daemon.c | 2 +-\n line-log.c | 2 +-\n revision.c | 2 +-\n 7 files changed, 37 insertions(+), 6 deletions(-)\n create mode 100644 contrib/coccinelle/copy.cocci\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 55d20cc2d8..23bb9a9e76 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1030,7 +1030,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (!origin)\n \t\treturn;\n \n-\tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n+\topts = rev->diffopt;\n \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \n \tdiff_setup_done(&opts);\ndiff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci\nnew file mode 100644\nindex 0000000000..f0d883932a\n--- /dev/null\n+++ b/contrib/coccinelle/copy.cocci\n@@ -0,0 +1,31 @@\n+@@\n+type T;\n+T dst;\n+T src;\n+@@\n+(\n+- memcpy(&dst, &src, sizeof(dst));\n++ dst = src;\n+|\n+- memcpy(&dst, &src, sizeof(src));\n++ dst = src;\n+|\n+- memcpy(&dst, &src, sizeof(T));\n++ dst = src;\n+)\n+\n+@@\n+type T;\n+T dst;\n+T *src;\n+@@\n+(\n+- memcpy(&dst, src, sizeof(dst));\n++ dst = *src;\n+|\n+- memcpy(&dst, src, sizeof(*src));\n++ dst = *src;\n+|\n+- memcpy(&dst, src, sizeof(T));\n++ dst = *src;\n+)\ndiff --git a/convert.c b/convert.c\nindex 8d652bf27c..4bae12be6b 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -290,7 +290,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \tif ((checksafe == SAFE_CRLF_WARN ||\n \t (checksafe == SAFE_CRLF_FAIL)) && len) {\n \t\tstruct text_stat new_stats;\n-\t\tmemcpy(&new_stats, &stats, sizeof(new_stats));\n+\t\tnew_stats = stats;\n \t\t/* simulate \"git add\" */\n \t\tif (convert_crlf_into_lf) {\n \t\t\tnew_stats.lonelf += new_stats.crlf;\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 46c5937526..798cf33c3a 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -22,7 +22,7 @@ static void cache_credential(struct credential *c, int timeout)\n \te = &entries[entries_nr++];\n \n \t/* take ownership of pointers */\n-\tmemcpy(&e->item, c, sizeof(*c));\n+\te->item = *c;\n \tmemset(c, 0, sizeof(*c));\n \te->expiration = time(NULL) + timeout;\n }\ndiff --git a/daemon.c b/daemon.c\nindex 473e6b6b63..f891398aad 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -785,7 +785,7 @@ static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_\n \n \tnewborn = xcalloc(1, sizeof(*newborn));\n \tlive_children++;\n-\tmemcpy(&newborn->cld, cld, sizeof(*cld));\n+\tnewborn->cld = *cld;\n \tmemcpy(&newborn->address, addr, addrlen);\n \tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n \t\tif (!addrcmp(&(*cradle)->address, &newborn->address))\ndiff --git a/line-log.c b/line-log.c\nindex 65f3558b3b..64f141e200 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1093,7 +1093,7 @@ static int process_all_files(struct line_log_data **range_out,\n \t\t\t\trg = rg->next;\n \t\t\tassert(rg);\n \t\t\trg->pair = diff_filepair_dup(queue->queue[i]);\n-\t\t\tmemcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));\n+\t\t\trg->diff = *pairdiff;\n \t\t}\n \t\tfree(pairdiff);\n \t}\ndiff --git a/revision.c b/revision.c\nindex b37dbec378..289977c796 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2738,7 +2738,7 @@ int prepare_revision_walk(struct rev_info *revs)\n \tstruct object_array old_pending;\n \tstruct commit_list **next = &revs->commits;\n \n-\tmemcpy(&old_pending, &revs->pending, sizeof(old_pending));\n+\told_pending = revs->pending;\n \trevs->pending.nr = 0;\n \trevs->pending.alloc = 0;\n \trevs->pending.objects = NULL;\n-- \n2.12.0\n"},{"id":"313736","messageId":"20170310074558.ifse3omthmtih73l@sigill.intra.peff.net","threadId":"45254","inReplyTo":"63a6ec61-653d-6307-4739-2ebaa8dbde35@web.de","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T07:45:58Z","receivedAt":"2017-03-10T07:46:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 01:14:45AM +0100, René Scharfe wrote:\n\n> > 3. There were a number of alloc-and-copy instances. The copy part is\n> > the same as (2) above, but you have to repeat the size, which is\n> > potentially error-prone. I wonder if we would want something like:\n> > \n> > #define ALLOC_COPY(dst, src) do { \\\n> > (dst) = xmalloc(sizeof(*(dst))); \\\n> > \t COPY_ARRAY(dst, src, 1); \\\n> > while(0)\n> > \n> > That avoids having to specify the size at all, and triggers a\n> > compile-time error if \"src\" and \"dst\" point to objects of different\n> > sizes.\n> \n> Or you could call it DUP or similar. And you could use ALLOC_ARRAY in\n> its definition and let it infer the size implicitly (don't worry too\n> much about the multiplication with one):\n> \n> \t#define DUPLICATE_ARRAY(dst, src, n) do {\t\\\n> \t\tALLOC_ARRAY((dst), (n));\t\t\\\n> \t\tCOPY_ARRAY((dst), (src), (n));\t\t\\\n> \t} while (0)\n> \t#define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)\n> \n> But do we even want such a thing? Duplicating objects should be rare, and\n> keeping allocation and assignment/copying separate makes for more flexible\n> building blocks. Adding ALLOC (and CALLOC) for single objects could be more\n> widely useful, I think.\n\nThere's no reason you can't have both the building blocks and the thing\nthat is built for them. I think there are 5 spots that would use\nDUPLICATE(), but I agree that there are more which could use ALLOC().\n\nI'd be more worried that we're slowly drifting away from idiomatic C.\nIf it's safer, that's good. But if it makes it hard for people new to\nthe project to figure out what the hell is going on, that may be not so\ngood.\n\nHere's the list of DUPLICATE spots, for reference.\n\n---\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex f7aa95f4b..f0ac1c511 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -661,8 +661,8 @@ static struct origin *find_rename(struct scoreboard *sb,\n static void add_blame_entry(struct blame_entry ***queue,\n \t\t\t const struct blame_entry *src)\n {\n-\tstruct blame_entry *e = xmalloc(sizeof(*e));\n-\tmemcpy(e, src, sizeof(*e));\n+\tstruct blame_entry *e;\n+\tDUPLICATE(e, src);\n \torigin_incref(e->suspect);\n \n \te->next = **queue;\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 72c815844..d6cb893cf 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -206,8 +206,8 @@ static void llist_sorted_difference_inplace(struct llist *A,\n static inline struct pack_list * pack_list_insert(struct pack_list **pl,\n \t\t\t\t\t struct pack_list *entry)\n {\n-\tstruct pack_list *p = xmalloc(sizeof(struct pack_list));\n-\tmemcpy(p, entry, sizeof(struct pack_list));\n+\tstruct pack_list *p;\n+\tDUPLICATE(p, entry);\n \tp->next = *pl;\n \t*pl = p;\n \treturn p;\n@@ -238,8 +238,7 @@ static struct pack_list * pack_list_difference(const struct pack_list *A,\n \t\t\treturn pack_list_difference(A->next, B);\n \t\tpl = pl->next;\n \t}\n-\tret = xmalloc(sizeof(struct pack_list));\n-\tmemcpy(ret, A, sizeof(struct pack_list));\n+\tDUPLICATE(ret, A);\n \tret->next = pack_list_difference(A->next, B);\n \treturn ret;\n }\n@@ -450,8 +449,7 @@ static void minimize(struct pack_list **min)\n \t\tperm_all = perm = get_permutations(non_unique, n);\n \t\twhile (perm) {\n \t\t\tif (is_superset(perm->pl, missing)) {\n-\t\t\t\tnew_perm = xmalloc(sizeof(struct pll));\n-\t\t\t\tmemcpy(new_perm, perm, sizeof(struct pll));\n+\t\t\t\tDUPLICATE(new_perm, perm);\n \t\t\t\tnew_perm->next = perm_ok;\n \t\t\t\tperm_ok = new_perm;\n \t\t\t}\ndiff --git a/diff.c b/diff.c\nindex 051761be4..dfe02f403 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1168,8 +1168,8 @@ static void init_diff_words_data(struct emit_callback *ecbdata,\n \t\t\t\t struct diff_filespec *two)\n {\n \tint i;\n-\tstruct diff_options *o = xmalloc(sizeof(struct diff_options));\n-\tmemcpy(o, orig_opts, sizeof(struct diff_options));\n+\tstruct diff_options *o;\n+\tDUPLICATE(o, orig_opts);\n \n \tecbdata->diff_words =\n \t\txcalloc(1, sizeof(struct diff_words_data));\n"},{"id":"313737","messageId":"20170310081759.yka476hnw4w3mghs@sigill.intra.peff.net","threadId":"45254","inReplyTo":"c553da50-e5ca-d064-e75c-46e5a5042935@web.de","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T08:18:00Z","receivedAt":"2017-03-10T08:18:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote:\n\n> > 2. Ones which just copy a single object, like:\n> > \n> > memcpy(&dst, &src, sizeof(dst));\n> > \n> > Perhaps we should be using struct assignment like:\n> > \n> > dst = src;\n> > \n> > here. It's safer and it should give the compiler more room to\n> > optimize. The only downside is that if you have pointers, it is\n> > easy to write \"dst = src\" when you meant \"*dst = *src\".\n> \n> Compilers can usually inline memcpy(3) calls, but assignments are\n> shorter and more pleasing to the eye, and we get a type check for\n> free. How about this?\n\nYeah, I mostly wasn't sure how people felt about \"shorter and more\npleasing\". It _is_ shorter and there's less to get wrong. But the\nmemcpy() screams \"hey, I am making a copy\" and is idiomatic to at least\na certain generation of C programmers.\n\nI guess something like COPY(dst, src) removes the part that you can get\nwrong, while still screaming copy. It's not idiomatic either, but at\nleast it stands out. I dunno.\n\n> diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci\n> new file mode 100644\n> index 0000000000..f0d883932a\n> --- /dev/null\n> +++ b/contrib/coccinelle/copy.cocci\n> @@ -0,0 +1,31 @@\n> +@@\n> +type T;\n> +T dst;\n> +T src;\n> [...]\n> +type T;\n> +T dst;\n> +T *src;\n\nI think this misses the other two cases: (*dst, src) and (*dst, *src).\n\nPerhaps squash this in?\n\n---\n builtin/blame.c | 2 +-\n builtin/pack-redundant.c | 4 ++--\n contrib/coccinelle/copy.cocci | 32 ++++++++++++++++++++++++++++++++\n diff.c | 4 ++--\n dir.c | 2 +-\n fast-import.c | 6 +++---\n line-log.c | 2 +-\n 7 files changed, 42 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex f7aa95f4b..d0d917b37 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -680,7 +680,7 @@ static void dup_entry(struct blame_entry ***queue,\n {\n \torigin_incref(src->suspect);\n \torigin_decref(dst->suspect);\n-\tmemcpy(dst, src, sizeof(*src));\n+\t*dst = *src;\n \tdst->next = **queue;\n \t**queue = dst;\n \t*queue = &dst->next;\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 72c815844..ac1d8111e 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -207,7 +207,7 @@ static inline struct pack_list * pack_list_insert(struct pack_list **pl,\n \t\t\t\t\t struct pack_list *entry)\n {\n \tstruct pack_list *p = xmalloc(sizeof(struct pack_list));\n-\tmemcpy(p, entry, sizeof(struct pack_list));\n+\t*p = *entry;\n \tp->next = *pl;\n \t*pl = p;\n \treturn p;\n@@ -451,7 +451,7 @@ static void minimize(struct pack_list **min)\n \t\twhile (perm) {\n \t\t\tif (is_superset(perm->pl, missing)) {\n \t\t\t\tnew_perm = xmalloc(sizeof(struct pll));\n-\t\t\t\tmemcpy(new_perm, perm, sizeof(struct pll));\n+\t\t\t\t*new_perm = *perm;\n \t\t\t\tnew_perm->next = perm_ok;\n \t\t\t\tperm_ok = new_perm;\n \t\t\t}\ndiff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci\nindex f0d883932..da9969c84 100644\n--- a/contrib/coccinelle/copy.cocci\n+++ b/contrib/coccinelle/copy.cocci\n@@ -29,3 +29,35 @@ T *src;\n - memcpy(&dst, src, sizeof(T));\n + dst = *src;\n )\n+\n+@@\n+type T;\n+T *dst;\n+T src;\n+@@\n+(\n+- memcpy(dst, &src, sizeof(*dst));\n++ *dst = src;\n+|\n+- memcpy(dst, &src, sizeof(src));\n++ *dst = src;\n+|\n+- memcpy(dst, &src, sizeof(T));\n++ *dst = src;\n+)\n+\n+@@\n+type T;\n+T *dst;\n+T *src;\n+@@\n+(\n+- memcpy(dst, src, sizeof(*dst));\n++ *dst = *src;\n+|\n+- memcpy(dst, src, sizeof(*src));\n++ *dst = *src;\n+|\n+- memcpy(dst, src, sizeof(T));\n++ *dst = *src;\n+)\ndiff --git a/diff.c b/diff.c\nindex 051761be4..fbd68ecd0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1169,7 +1169,7 @@ static void init_diff_words_data(struct emit_callback *ecbdata,\n {\n \tint i;\n \tstruct diff_options *o = xmalloc(sizeof(struct diff_options));\n-\tmemcpy(o, orig_opts, sizeof(struct diff_options));\n+\t*o = *orig_opts;\n \n \tecbdata->diff_words =\n \t\txcalloc(1, sizeof(struct diff_words_data));\n@@ -3359,7 +3359,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n \n void diff_setup(struct diff_options *options)\n {\n-\tmemcpy(options, &default_diff_options, sizeof(*options));\n+\t*options = default_diff_options;\n \n \toptions->file = stdout;\n \ndiff --git a/dir.c b/dir.c\nindex 4541f9e14..d3d0039bf 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2499,7 +2499,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,\n \tif (next > rd->end)\n \t\treturn -1;\n \t*untracked_ = untracked = xmalloc(st_add(sizeof(*untracked), len));\n-\tmemcpy(untracked, &ud, sizeof(ud));\n+\t*untracked = ud;\n \tmemcpy(untracked->name, data, len + 1);\n \tdata = next;\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 6c13472c4..3e294c2b5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -875,7 +875,7 @@ static struct tree_content *dup_tree_content(struct tree_content *s)\n \tfor (i = 0; i < s->entry_count; i++) {\n \t\ta = s->entries[i];\n \t\tb = new_tree_entry();\n-\t\tmemcpy(b, a, sizeof(*a));\n+\t\t*b = *a;\n \t\tif (a->tree && is_null_sha1(b->versions[1].sha1))\n \t\t\tb->tree = dup_tree_content(a->tree);\n \t\telse\n@@ -1685,7 +1685,7 @@ static int tree_content_remove(\n \n del_entry:\n \tif (backup_leaf)\n-\t\tmemcpy(backup_leaf, e, sizeof(*backup_leaf));\n+\t\t*backup_leaf = *e;\n \telse if (e->tree)\n \t\trelease_tree_content_recursive(e->tree);\n \te->tree = NULL;\n@@ -1735,7 +1735,7 @@ static int tree_content_get(\n \treturn 0;\n \n found_entry:\n-\tmemcpy(leaf, e, sizeof(*leaf));\n+\t*leaf = *e;\n \tif (e->tree && is_null_sha1(e->versions[1].sha1))\n \t\tleaf->tree = dup_tree_content(e->tree);\n \telse\ndiff --git a/line-log.c b/line-log.c\nindex 64f141e20..de5bbb5bd 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -765,7 +765,7 @@ static void move_diff_queue(struct diff_queue_struct *dst,\n \t\t\t struct diff_queue_struct *src)\n {\n \tassert(src != dst);\n-\tmemcpy(dst, src, sizeof(struct diff_queue_struct));\n+\t*dst = *src;\n \tDIFF_QUEUE_CLEAR(src);\n }\n \n"},{"id":"313753","messageId":"04f4849c-e1e0-f0ac-5b1e-10a343391db4@web.de","threadId":"45254","inReplyTo":"20170310081759.yka476hnw4w3mghs@sigill.intra.peff.net","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-10T16:20:13Z","receivedAt":"2017-03-10T16:20:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.03.2017 um 09:18 schrieb Jeff King:\n> On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote:\n>\n>>> 2. Ones which just copy a single object, like:\n>>>\n>>> memcpy(&dst, &src, sizeof(dst));\n>>>\n>>> Perhaps we should be using struct assignment like:\n>>>\n>>> dst = src;\n>>>\n>>> here. It's safer and it should give the compiler more room to\n>>> optimize. The only downside is that if you have pointers, it is\n>>> easy to write \"dst = src\" when you meant \"*dst = *src\".\n>>\n>> Compilers can usually inline memcpy(3) calls, but assignments are\n>> shorter and more pleasing to the eye, and we get a type check for\n>> free. How about this?\n>\n> Yeah, I mostly wasn't sure how people felt about \"shorter and more\n> pleasing\". It _is_ shorter and there's less to get wrong. But the\n> memcpy() screams \"hey, I am making a copy\" and is idiomatic to at least\n> a certain generation of C programmers.\n>\n> I guess something like COPY(dst, src) removes the part that you can get\n> wrong, while still screaming copy. It's not idiomatic either, but at\n> least it stands out. I dunno.\n\nYes ...\n\n> I think this misses the other two cases: (*dst, src) and (*dst, *src).\n\n... and that's why I left them out. You can't get dst vs. *dst wrong \nwith structs (at least not without the compiler complaining); only safe \ntransformations are included in this round.\n\nRené\n"},{"id":"313760","messageId":"20170310175719.cxol7d262n2rlcb2@sigill.intra.peff.net","threadId":"45254","inReplyTo":"04f4849c-e1e0-f0ac-5b1e-10a343391db4@web.de","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T17:57:19Z","receivedAt":"2017-03-10T17:57:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote:\n\n> > I think this misses the other two cases: (*dst, src) and (*dst, *src).\n> \n> ... and that's why I left them out. You can't get dst vs. *dst wrong with\n> structs (at least not without the compiler complaining); only safe\n> transformations are included in this round.\n\nI don't think the transformation could be wrong without the original\ncode being wrong.\n\nI'm also not sure why mine would be unsafe and yours would be safe. It\nseems like the other way around, because mine will do:\n\n *dst = ...\n\nwhich would fail unless \"dst\" is a pointer. Whereas in yours, we end up\nwith:\n\n dst = ...\n\nand somebody mistaking pointer assignment for a struct copy would not\nnotice.\n\nBut either way, I don't think we can have a rule like \"you can use\nstruct assignment only if you don't have a pointer for the destination\".\nThat's too arcane to expect developers and reviewers to follow. We\nshould either allow struct assignment or not.\n\nOr have I totally misunderstood your point?\n\n-Peff\n"},{"id":"313766","messageId":"3a1cd5cf-cf13-7ef1-7b47-d4b083b64d11@web.de","threadId":"45254","inReplyTo":"20170310175719.cxol7d262n2rlcb2@sigill.intra.peff.net","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-10T18:31:22Z","receivedAt":"2017-03-10T18:31:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.03.2017 um 18:57 schrieb Jeff King:\n> On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote:\n>\n>>> I think this misses the other two cases: (*dst, src) and (*dst, *src).\n>>\n>> ... and that's why I left them out. You can't get dst vs. *dst wrong with\n>> structs (at least not without the compiler complaining); only safe\n>> transformations are included in this round.\n>\n> I don't think the transformation could be wrong without the original\n> code being wrong.\n\nAvoiding to introduce bugs with automatic transformations is essential, \nindeed -- if we're not careful here we'd be better off keeping the \noriginal code.\n\n> I'm also not sure why mine would be unsafe and yours would be safe. It\n> seems like the other way around, because mine will do:\n>\n> *dst = ...\n>\n> which would fail unless \"dst\" is a pointer. Whereas in yours, we end up\n> with:\n>\n> dst = ...\n>\n> and somebody mistaking pointer assignment for a struct copy would not\n> notice.\n\nIf dst is a struct then having *dst on the left-hand side results in a \ncompiler error -- you can't get that one wrong. If it's a pointer then \nboth dst and *dst can be valid (pointer assignment vs. content copy), so \nthere is the possibility of making a mistake without the compiler noticing.\n\n> But either way, I don't think we can have a rule like \"you can use\n> struct assignment only if you don't have a pointer for the destination\".\n> That's too arcane to expect developers and reviewers to follow. We\n> should either allow struct assignment or not.\n\nWith an automatic transformation in place it's more like \"you can't use \nmemcpy() in this case any more as it gets turned into an assignment with \nthe next cocci patch\". I think we shouldn't be that restrictive for \npointers as destinations (yet?).\n\nRené\n"},{"id":"313790","messageId":"xmqqfuikyjoo.fsf@gitster.mtv.corp.google.com","threadId":"45254","inReplyTo":"04f4849c-e1e0-f0ac-5b1e-10a343391db4@web.de","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-10T20:13:11Z","receivedAt":"2017-03-10T20:13:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> I think this misses the other two cases: (*dst, src) and (*dst, *src).\n>\n> ... and that's why I left them out. You can't get dst vs. *dst wrong\n> with structs (at least not without the compiler complaining); only\n> safe transformations are included in this round.\n\nI haven't followed this discussion to the end, but the omission of 2\nout of obvious 4 did pique my curiosity when I saw it, too, and made\nme wonder if the omission was deliberate. If so, it would be nice\nto state why in the log message (or in copy.cocci file itself as a\ncomment).\n\nIt also made me wonder if we would be helped with a further\ncombinatorial explosion from \"T **dstp, **srcp\" and somesuch (in\nother words, I am wondering why a rule for 'T *src' that uses '*src'\nneed to be spelled out separately when there already is a good rule\nfor 'T src' that uses 'src'---is that an inherent restriction of the\ntool?).\n\n\n\n\n"},{"id":"313792","messageId":"20170310201840.fubshjp2bjg4bjxq@sigill.intra.peff.net","threadId":"45254","inReplyTo":"xmqqfuikyjoo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T20:18:40Z","receivedAt":"2017-03-10T20:18:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 12:13:11PM -0800, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> >> I think this misses the other two cases: (*dst, src) and (*dst, *src).\n> >\n> > ... and that's why I left them out. You can't get dst vs. *dst wrong\n> > with structs (at least not without the compiler complaining); only\n> > safe transformations are included in this round.\n> \n> I haven't followed this discussion to the end, but the omission of 2\n> out of obvious 4 did pique my curiosity when I saw it, too, and made\n> me wonder if the omission was deliberate. If so, it would be nice\n> to state why in the log message (or in copy.cocci file itself as a\n> comment).\n\nYeah, it definitely would be worth mentioning. I'm still undecided on\nwhether we want to be endorsing struct assignment more fully.\n\n> It also made me wonder if we would be helped with a further\n> combinatorial explosion from \"T **dstp, **srcp\" and somesuch (in\n> other words, I am wondering why a rule for 'T *src' that uses '*src'\n> need to be spelled out separately when there already is a good rule\n> for 'T src' that uses 'src'---is that an inherent restriction of the\n> tool?).\n\nI had that thought, too, but I think the 4-way rules are necessary,\nbecause the transformations aren't the same in each case. E.g., for the\nfour cases, the resulting assignments are:\n\n (dst, src): dst = src;\n (dst, *src): dst = *src;\n (*dst, src): *dst = src;\n (*dst, *src): *dst = *src;\n\nFor pointer-to-pointer, I assumed the tool would handle that\nautomatically by matching \"T\" as \"T*\". Though if that is the case, I\nthink \"(dst, src)\" and \"(*dst, *src)\" would be equivalent (though of\ncourse our rule matches are different, as you do not memcpy the raw\nstructs).\n\n-Peff\n"},{"id":"313804","messageId":"c93a61a4-05d9-8e49-9734-12c81289517b@web.de","threadId":"45254","inReplyTo":"xmqqfuikyjoo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-10T22:04:32Z","receivedAt":"2017-03-10T22:05:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.03.2017 um 21:13 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>> I think this misses the other two cases: (*dst, src) and (*dst, *src).\n>>\n>> ... and that's why I left them out. You can't get dst vs. *dst wrong\n>> with structs (at least not without the compiler complaining); only\n>> safe transformations are included in this round.\n>\n> I haven't followed this discussion to the end, but the omission of 2\n> out of obvious 4 did pique my curiosity when I saw it, too, and made\n> me wonder if the omission was deliberate. If so, it would be nice\n> to state why in the log message (or in copy.cocci file itself as a\n> comment).\n>\n> It also made me wonder if we would be helped with a further\n> combinatorial explosion from \"T **dstp, **srcp\" and somesuch (in\n> other words, I am wondering why a rule for 'T *src' that uses '*src'\n> need to be spelled out separately when there already is a good rule\n> for 'T src' that uses 'src'---is that an inherent restriction of the\n> tool?).\n\nThere are redundancies. This semantic patch here:\n\n\t@@\n\ttype T;\n\tT dst;\n\tT *src;\n\t@@\n\t- memcpy(&dst, src, sizeof(dst));\n\t+ dst = *src;\n\nwould match e.g. this (from convert.c):\n\n\tmemcpy(&new_stats, &stats, sizeof(new_stats));\n\nand transform it to:\n\n\tnew_stats = *&stats;\n\nWe'd need just one more rule to remove the \"*&\" part and could then get \nrid of two of the \"T src\" variants, to arrive at something like this:\n\n\t@@\n\ttype T;\n\tT dst, src;\n\t@@\n\t- memcpy(&dst, &src, sizeof(src));\n\t+ dst = src;\n\n\t@ r @\n\ttype T;\n\tT dst;\n\tT *src;\n\t@@\n\t(\n\t- memcpy(&dst, src, sizeof(dst));\n\t+ dst = *src;\n\t|\n\t- memcpy(&dst, src, sizeof(*src));\n\t+ dst = *src;\n\t|\n\t- memcpy(&dst, src, sizeof(T));\n\t+ dst = *src;\n\t)\n\n\t@ depends on r @\n\texpression E;\n\t@@\n\t- *&\n\t E\n\nRené\n"},{"id":"313808","messageId":"xmqqr324wvum.fsf@gitster.mtv.corp.google.com","threadId":"45254","inReplyTo":"c93a61a4-05d9-8e49-9734-12c81289517b@web.de","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-10T23:33:21Z","receivedAt":"2017-03-10T23:33:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> \t@ depends on r @\n> \texpression E;\n> \t@@\n> \t- *&\n> \t E\n\nI guess my source of the confusion is that the tool that understands\nthe semantics of the C language still needs to be told about that.\n\nI was hoping that something that understands C only needs to be told\nonly a single rule:\n\n\ttype T\n\tT src, dst\n\n\t-memcpy(&dst, &src, sizeof(dst));\n\t+dst = src;\n\nand then can apply that rule to this code in four ways:\n\n\tstruct foo A, *Bp;\n\n\tmemcpy(Bp, &A, sizeof(*Bp));\n\tmemcpy(Bp, &A, sizeof(A));\n\tmemcpy(&src, dstp, sizeof(A));\n\tmemcpy(&src, dstp, sizeof(*Bp));\n\nto obtain its rewrite:\n\n\tstruct foo A, *Bp;\n\n\t*Bp = A;\n\t*Bp = A;\n\tA = *Bp;\n\tA = *Bp;\n\nby knowing that (*Bp) is of type \"struct foo\" (even though Bp is of\ntype \"struct foo *\") and sizeof(dst) and sizeof(src) are the same\nthing in the rule because src and dst are both of type T.\n"},{"id":"313827","messageId":"b0d41423-7227-3417-b4b1-fcf47a66f64b@web.de","threadId":"45254","inReplyTo":"xmqqr324wvum.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] Travis: also test on 32-bit Linux","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-11T14:17:18Z","receivedAt":"2017-03-11T14:21:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.03.2017 um 00:33 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> \t@ depends on r @\n>> \texpression E;\n>> \t@@\n>> \t- *&\n>> \t E\n> \n> I guess my source of the confusion is that the tool that understands\n> the semantics of the C language still needs to be told about that.\n\nUnderstanding that E and *&E have the same type does not imply (for a\nmachine) that E alone is preferable.\n\n> I was hoping that something that understands C only needs to be told\n> only a single rule:\n> \n> \ttype T\n> \tT src, dst\n> \n> \t-memcpy(&dst, &src, sizeof(dst));\n> \t+dst = src;\n> \n> and then can apply that rule to this code in four ways:\n> \n> \tstruct foo A, *Bp;\n> \n> \tmemcpy(Bp, &A, sizeof(*Bp));\n> \tmemcpy(Bp, &A, sizeof(A));\n> \tmemcpy(&src, dstp, sizeof(A));\n> \tmemcpy(&src, dstp, sizeof(*Bp));\n\nI guess src is A and dstp is Bp?\n\nCoccinelle does not know that the sizeof expressions are equivalent,\nbut you could normalize them with an additional rule and then use a\nsingle memcpy transformation like this:\n\n\t@ normalize_sizeof @\n\ttype T;\n\tT var;\n\t@@\n\t- sizeof(var)\n\t+ sizeof(T)\n\n\t@ r @\n\ttype T;\n\tT *dst;\n\tT *src;\n\t@@\n\t- memcpy(dst, src, sizeof(T));\n\t+ *dst = *src;\n\n\t@ depends on r @\n\texpression E;\n\t@@\n\t- *&\n\t E\n\nThat would give you what you expected here:\n\n> to obtain its rewrite:\n> \n> \tstruct foo A, *Bp;\n> \n> \t*Bp = A;\n> \t*Bp = A;\n> \tA = *Bp;\n> \tA = *Bp;\n\nBut that normalization rule would be applied everywhere because it's so\nbroad, and that's probably not what we want. You could replace it with\nan isomorphism like this one:\n\n\tExpression\n\t@@\n\ttype T;\n\tT E;\n\t@@\n\n\tsizeof(T) => sizeof E\n\nIn your example it allows you to specify sizeof(struct foo) (or a\ngeneric sizeof(T) as in rule r above) in the semantic patch and\nCoccinelle would let that match sizeof(A) and sizeof(*Bp) in the C\nfile as well.\n\nIsomorphisms are kept in a separate file, the default one is\n/usr/lib/coccinelle/standard.iso on my machine and you can chose a\ndifferent one with --iso-file. Perhaps we want to have our own for\ngit, but we'd need to synchronize it with upstream from time to time.\n\nThere is already a very similar isomorphism rule in the default file,\nbut I don't think I understand it:\n\n\tExpression\n\t@ sizeof_type_expr @\n\tpure type T; // pure because we drop a metavar\n\tT E;\n\t@@\n\n\tsizeof(T) => sizeof E\n\nIn particular I'm a bit worried about the lack of documentation for\n\"pure\", and I don't understand the comment. There's another comment\nat the top of another rule in the same file:\n\n// pure means that either the whole field reference expression is dropped,\n// or E is context code and has no attached + code\n// not really... pure means matches a unitary unplussed metavariable\n// but this rule doesn't work anyway\n\nHmm.\n\nHere's an isomorphism that allows you to use \"sizeof *src\" (the third\npart for T is not strictly needed for your example):\n\n\tExpression\n\t@ sizeof_equiv @\n\ttype T;\n\tT E1, E2;\n\t@@\n\n\tsizeof E1 <=> sizeof E2 <=> sizeof(T)\n\nThat's the kind of bidirectional equivalence you expected to be part\nof Coccinelle's standard set of rules, right? With that rule in place\nthe following semantic patch matches your four cases:\n\n\t@ r @\n\ttype T;\n\tT *dst;\n\tT *src;\n\t@@\n\t- memcpy(dst, src, sizeof *dst);\n\t+ *dst = *src;\n\n\t@ depends on r @\n\texpression E;\n\t@@\n\t- *&\n\t E\n\nBut I'd be careful with that as long as \"pure\" is present in that\nstandard rule and not fully understood. The isomorphism sizeof_equiv\ndoesn't seem to cause any trouble with our existing semantic patches\nand the code in master, though.\n\nRené\n"}]}