{"thread":{"id":"52241","subject":"coccinelle: adjustments for array.cocci?","startedAt":"2019-11-12T15:08:59Z","lastAt":"2020-01-25T08:23:55Z","messageCount":41,"participants":["Markus Elfring","René Scharfe","Junio C Hamano","Julia Lawall","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"386017","messageId":"50c77cdc-2b2d-16c8-b413-5eb6a2bae749@web.de","threadId":"52241","inReplyTo":null,"subject":"coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-12T15:08:56Z","receivedAt":"2019-11-12T15:08:59Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"Hello,\n\nI would like to comment implementation details from\nthe commit 177fbab747da4f58cb2a8ce010b3515c86dd67c9 (\"coccinelle: use COPY_ARRAY for copying arrays\").\n\n\nDo you find the following code variant (for the semantic patch language) also useful?\n\n memcpy(\n( ptr, E, n *\n- sizeof(*(ptr))\n+ sizeof(T)\n| arr, E, n *\n- sizeof(*(arr))\n+ sizeof(T)\n| E, ptr, n *\n- sizeof(*(ptr))\n+ sizeof(T)\n| E, arr, n *\n- sizeof(*(arr))\n+ sizeof(T)\n)\n )\n\n\nHow do you think about the following SmPL code variant?\n\n-memcpy\n+COPY_ARRAY\n (\n( dst_ptr\n| dst_arr\n)\n ,\n( src_ptr\n| src_arr\n)\n ,\n- (n) * sizeof(T)\n+ n\n )\n\n\nRegards,\nMarkus\n"},{"id":"386031","messageId":"5189f847-1af1-f050-6c72-576a977f6f12@web.de","threadId":"52241","inReplyTo":"50c77cdc-2b2d-16c8-b413-5eb6a2bae749@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-12T18:37:23Z","receivedAt":"2019-11-12T18:37:44Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.11.19 um 16:08 schrieb Markus Elfring:\n> Hello,\n>\n> I would like to comment implementation details from\n> the commit 177fbab747da4f58cb2a8ce010b3515c86dd67c9 (\"coccinelle: use COPY_ARRAY for copying arrays\").\n>\n>\n> Do you find the following code variant (for the semantic patch language) also useful?\n>\n> memcpy(\n> ( ptr, E, n *\n> - sizeof(*(ptr))\n> + sizeof(T)\n> | arr, E, n *\n> - sizeof(*(arr))\n> + sizeof(T)\n> | E, ptr, n *\n> - sizeof(*(ptr))\n> + sizeof(T)\n> | E, arr, n *\n> - sizeof(*(arr))\n> + sizeof(T)\n> )\n> )\n\nThis reduces duplication in the semantic patch, which is nice. I think\nI tried something like that at the time, but found that it failed to\nproduce some of the cases in 921d49be86 (\"use COPY_ARRAY for copying\narrays\", 2019-06-15) for some reason.\n\n> How do you think about the following SmPL code variant?\n>\n> -memcpy\n> +COPY_ARRAY\n> (\n> ( dst_ptr\n> | dst_arr\n> )\n> ,\n> ( src_ptr\n> | src_arr\n> )\n> ,\n> - (n) * sizeof(T)\n> + n\n> )\n\nThis eliminates duplication in the semantic patch, which is good. It\nmesses up the indentation of n in some of the cases in 921d49be86 (\"use\nCOPY_ARRAY for copying arrays\", 2019-06-15), though. Hmm, but that can\nbe cured by duplicating the comma:\n\n - , (n) * sizeof(T)\n + , n\n\nRené\n"},{"id":"386085","messageId":"xmqqa790cyp1.fsf@gitster-ct.c.googlers.com","threadId":"52241","inReplyTo":"5189f847-1af1-f050-6c72-576a977f6f12@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T02:11:38Z","receivedAt":"2019-11-13T02:11:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> This reduces duplication in the semantic patch, which is nice. I think\n> I tried something like that at the time, but found that it failed to\n> produce some of the cases in 921d49be86 (\"use COPY_ARRAY for copying\n> arrays\", 2019-06-15) for some reason.\n\nThanks for mentioning.\n\nI too recall that seemingly redundant entries were noticed during\nthe review and at least back then removing the seemingly redundant\nones caused failures in rewriting.\n\nThat is why I am hesitant to touch any patch that says \"simplify\ncocci rule\" making it sound as if simplification is a good thing on\nits own. I have no problem with \"we change the rule this way, which\neliminates this false positive / negative, that is demonstrated in\nthe added tests in t/ directory\", though.\n\nThanks.\n\n\n\n"},{"id":"386099","messageId":"fe9b8c08-6fd4-d378-f3ff-8170381b10e0@web.de","threadId":"52241","inReplyTo":"xmqqa790cyp1.fsf@gitster-ct.c.googlers.com","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-13T08:49:46Z","receivedAt":"2019-11-13T08:50:01Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> I too recall that seemingly redundant entries were noticed during\n> the review and at least back then removing the seemingly redundant\n> ones caused failures in rewriting.\n\nI am curious if the redundancy can be reconsidered once more.\n\nDo you refer to open issues around source code reformatting\nand pretty-printing together with the Coccinelle software here?\n\nWould you like to achieve any further improvements also in this area?\n\nRegards,\nMarkus\n"},{"id":"386150","messageId":"xmqqr22b9ptk.fsf@gitster-ct.c.googlers.com","threadId":"52241","inReplyTo":"fe9b8c08-6fd4-d378-f3ff-8170381b10e0@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-14T02:03:51Z","receivedAt":"2019-11-14T02:03:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Elfring writes:\n\n>> I too recall that seemingly redundant entries were noticed during\n>> the review and at least back then removing the seemingly redundant\n>> ones caused failures in rewriting.\n>\n> I am curious if the redundancy can be reconsidered once more.\n>\n> Do you refer to open issues around source code reformatting\n> and pretty-printing together with the Coccinelle software here?\n\nSorry, I do not follow. \n\nIf you are asking if I am interested in following bleeding edge\nCoccinelle development and use this project as a guinea pig to do\nso, then the answer is no. I'd rather see us instead staying on the\ntrailing edge ;-) to make sure that we use common denominator\nfeatures that are known to be available in all widely deployed and\nperhaps a bit dated versions that come with popular distros.\n\nAnd if that means we have to accept inefficient ways to express our\npatterns, we are willing to pay for that cost.\n\nSo, \"the A.cocci file uses a set of inefficient expressions that can\nbe written more concisely like this, using the bleeding edge version\nof the syntax\" is not a useful improvement for the purpose of this\nproject, while \"the A.cocci file uses a set of inefficient\nexpressions that can be written more concisely like this, and all\nversions of cocci that is newer than X would understand the\nnotation. Even distro D that tends to ship with fairly stale\nversions of packages ship version X+n, so this change should be\nsafe\" is very much appreciated.\n\nThanks.\n\n"},{"id":"386183","messageId":"ba5d609a-16ea-d7e9-66e6-19aab94b2acd@web.de","threadId":"52241","inReplyTo":"xmqqr22b9ptk.fsf@gitster-ct.c.googlers.com","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-14T13:15:27Z","receivedAt":"2019-11-14T13:15:47Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">>> I too recall that seemingly redundant entries were noticed during\n>>> the review and at least back then removing the seemingly redundant\n>>> ones caused failures in rewriting.\n>>\n>> I am curious if the redundancy can be reconsidered once more.\n>>\n>> Do you refer to open issues around source code reformatting\n>> and pretty-printing together with the Coccinelle software here?\n>\n> Sorry, I do not follow.\n>\n> If you are asking if I am interested in following bleeding edge\n> Coccinelle development and use this project as a guinea pig to do so,\n\nI did not ask this.\n\nYou mentioned “failures”. - I became curious then if corresponding software\ndevelopment challenges can be clarified a bit more.\n\n\n> then the answer is no.\n\nSuch feedback is reasonable.\n\n\n> I'd rather see us instead staying on the trailing edge ;-)\n> to make sure that we use common denominator features that are known\n> to be available in all widely deployed and perhaps a bit dated versions\n> that come with popular distros.\n\nI find that I am proposing script adjustments within the basic feature set\nfor the semantic patch language here.\nFurther fine-tuning will become possible, won't it?\n\nRegards,\nMarkus\n"},{"id":"386191","messageId":"53346d52-e096-c651-f70a-ce6ca4d82ff9@web.de","threadId":"52241","inReplyTo":"ba5d609a-16ea-d7e9-66e6-19aab94b2acd@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-14T16:41:21Z","receivedAt":"2019-11-14T16:41:28Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.11.19 um 14:15 schrieb Markus Elfring:\n> You mentioned “failures”. - I became curious then if corresponding software\n> development challenges can be clarified a bit more.\n\nLet's try to restore/repeat the pertinent paragraph, with context and\nattribution:\n\nAm 13.11.19 um 03:11 schrieb Junio C Hamano:\n> René Scharfe writes:\n>> Am 12.11.19 um 16:08 schrieb Markus Elfring:\n>>>\n>>> Do you find the following code variant (for the semantic patch language) also useful?\n>>>\n>>> memcpy(\n>>> ( ptr, E, n *\n>>> - sizeof(*(ptr))\n>>> + sizeof(T)\n>>> | arr, E, n *\n>>> - sizeof(*(arr))\n>>> + sizeof(T)\n>>> | E, ptr, n *\n>>> - sizeof(*(ptr))\n>>> + sizeof(T)\n>>> | E, arr, n *\n>>> - sizeof(*(arr))\n>>> + sizeof(T)\n>>> )\n>>> )\n>\n>> This reduces duplication in the semantic patch, which is nice. I think\n>> I tried something like that at the time, but found that it failed to\n>> produce some of the cases in 921d49be86 (\"use COPY_ARRAY for copying\n>> arrays\", 2019-06-15) for some reason.\n> Thanks for mentioning.\n>\n> I too recall that seemingly redundant entries were noticed during\n> the review and at least back then removing the seemingly redundant\n> ones caused failures in rewriting.\n\nYou can see for yourself by:\n\n 1. applying the patch at the bottom to implement your suggested change,\n 2. running \"git show 921d49be86 | patch -p1 -R\" to undo 921d49be86,\n 3. running \"make contrib/coccinelle/array.cocci.patch\",\n 4. running \"patch -p1 8 --\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 46b8d2ee11..e7bcbefcc1 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -12,27 +12,25 @@ T *ptr;\n T[] arr;\n expression E, n;\n @@\n+ memcpy(\n (\n- memcpy(ptr, E,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n- )\n+ ptr, E, n *\n+- sizeof(*(ptr))\n++ sizeof(T)\n |\n- memcpy(arr, E,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n- )\n+ arr, E, n *\n+- sizeof(*(arr))\n++ sizeof(T)\n |\n- memcpy(E, ptr,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n- )\n+ E, ptr, n *\n+- sizeof(*(ptr))\n++ sizeof(T)\n |\n- memcpy(E, arr,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n- )\n+ E, arr, n *\n+- sizeof(*(arr))\n++ sizeof(T)\n )\n+ )\n\n @@\n type T;\n"},{"id":"386193","messageId":"6c4ef61f-5fef-ffc8-82d6-ee42006756b4@web.de","threadId":"52241","inReplyTo":"53346d52-e096-c651-f70a-ce6ca4d82ff9@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-14T17:14:56Z","receivedAt":"2019-11-14T17:15:09Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> If the new version of array.cocci is equivalent to the current one then\n> that last step should show no difference.\n\nI hoped it.\n\n\n> contrib/coccinelle/array.cocci | 30 ++++++++++++++----------------\n> fast-import.c | 2 +-\n> packfile.c | 4 ++--\n> pretty.c | 4 ++--\n> 4 files changed, 19 insertions(+), 21 deletions(-)\n>\n> The changes in array.cocci are expected of course, but the others\n> indicate that the new version missed transformations that the current\n> version generated.\n\nWould we like to submit a bug report for the Coccinelle software?\n\nWhich version did you try out for the comparison of generated patches?\n\nRegards,\nMarkus\n"},{"id":"386196","messageId":"aed296a6-bae0-6fcc-515e-ef96fed24ca6@web.de","threadId":"52241","inReplyTo":"6c4ef61f-5fef-ffc8-82d6-ee42006756b4@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-14T17:46:12Z","receivedAt":"2019-11-14T17:46:21Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.11.19 um 18:14 schrieb Markus Elfring:\n>> If the new version of array.cocci is equivalent to the current one then\n>> that last step should show no difference.\n>\n> I hoped it.\n>\n>\n>> contrib/coccinelle/array.cocci | 30 ++++++++++++++----------------\n>> fast-import.c | 2 +-\n>> packfile.c | 4 ++--\n>> pretty.c | 4 ++--\n>> 4 files changed, 19 insertions(+), 21 deletions(-)\n>>\n>> The changes in array.cocci are expected of course, but the others\n>> indicate that the new version missed transformations that the current\n>> version generated.\n>\n> Would we like to submit a bug report for the Coccinelle software?\n\nNot really, because...\n\n> Which version did you try out for the comparison of generated patches?\n\n... I use the last version of the Debian testing package, 1.0.4.deb-4.\nhttps://tracker.debian.org/pkg/coccinelle says it was removed from\ntesting recently. I was actually waiting for a more recent version\nlike 1.0.8 to be packaged; not sure what's going on there.\n\nAnyway, someone who can reproduce the issue using the latest release\nof Coccinelle would be in a better position to file a bug report.\n\nRené\n"},{"id":"386292","messageId":"6fffd13a-738b-e750-9f5a-f0bfb252855b@web.de","threadId":"52241","inReplyTo":"aed296a6-bae0-6fcc-515e-ef96fed24ca6@web.de","subject":"Re: git-coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-15T11:11:03Z","receivedAt":"2019-11-15T11:11:22Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Anyway, someone who can reproduce the issue using the latest release\n> of Coccinelle would be in a better position to file a bug report.\n\nHello,\n\nI repeated the discussed source code transformation approach together\nwith the software combination “Coccinelle 1.0.8-00004-g842075f7” (OCaml 4.09).\nhttps://github.com/coccinelle/coccinelle/commits/master\n\n1. Yesterday I checked the source files out for the software “Git”\n according to the commit “The first batch post 2.24 cycle”.\n https://github.com/git/git/commit/d9f6f3b6195a0ca35642561e530798ad1469bd41\n\n2. I restored a previous development status by the following command.\n\n git show 921d49be86 | patch -p1 -R\n\n See also:\n https://public-inbox.org/git/53346d52-e096-c651-f70a-ce6ca4d82ff9@web.de/\n\n3. I stored a generated patch based on the currently released SmPL script.\n https://github.com/git/git/blob/177fbab747da4f58cb2a8ce010b3515c86dd67c9/contrib/coccinelle/array.cocci\n\n4. I applied the following patch then.\n\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 46b8d2ee11..89df184bbd 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -12,27 +12,21 @@ T *ptr;\n T[] arr;\n expression E, n;\n @@\n-(\n- memcpy(ptr, E,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n- )\n-|\n- memcpy(arr, E,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n- )\n-|\n- memcpy(E, ptr,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n- )\n-|\n- memcpy(E, arr,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n- )\n+ memcpy(\n+( ptr, E, n *\n+- sizeof(*(ptr))\n++ sizeof(T)\n+| arr, E, n *\n+- sizeof(*(arr))\n++ sizeof(T)\n+| E, ptr, n *\n+- sizeof(*(ptr))\n++ sizeof(T)\n+| E, arr, n *\n+- sizeof(*(arr))\n++ sizeof(T)\n )\n+ )\n\n @@\n type T;\n\n I suggested in this way to move a bit of SmPL code.\n\n5. I stored another generated patch based on the adjusted SmPL script.\n\n6. I performed a corresponding file comparison.\n\n--- array-released.diff\t2019-11-14 21:29:11.020576916 +0100\n+++ array-reduced1.diff\t2019-11-14 21:45:58.931956527 +0100\n@@ -6,24 +6,10 @@\n \tr->entry_count = t->entry_count;\n \tr->delta_depth = t->delta_depth;\n -\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));\n-+\tCOPY_ARRAY(r->entries, t->entries, t->entry_count);\n++\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));\n \trelease_tree_content(t);\n \treturn r;\n }\n-diff -u -p a/pretty.c b/pretty.c\n---- a/pretty.c\n-+++ b/pretty.c\n-@@ -106,8 +106,8 @@ static void setup_commit_formats(void)\n- \tcommit_formats_len = ARRAY_SIZE(builtin_formats);\n- \tbuiltin_formats_len = commit_formats_len;\n- \tALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);\n--\tmemcpy(commit_formats, builtin_formats,\n--\t sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n-+\tCOPY_ARRAY(commit_formats, builtin_formats,\n-+\t\t ARRAY_SIZE(builtin_formats));\n-\n- \tgit_config(git_pretty_formats_config, NULL);\n- }\n diff -u -p a/packfile.c b/packfile.c\n --- a/packfile.c\n +++ b/packfile.c\n@@ -36,17 +22,6 @@\n \t\t} else {\n \t\t\tALLOC_GROW(poi_stack, poi_stack_nr+1, poi_stack_alloc);\n \t\t}\n-@@ -1698,8 +1698,8 @@ void *unpack_entry(struct repository *r,\n- \t\t && delta_stack == small_delta_stack) {\n- \t\t\tdelta_stack_alloc = alloc_nr(delta_stack_nr);\n- \t\t\tALLOC_ARRAY(delta_stack, delta_stack_alloc);\n--\t\t\tmemcpy(delta_stack, small_delta_stack,\n--\t\t\t sizeof(*delta_stack)*delta_stack_nr);\n-+\t\t\tCOPY_ARRAY(delta_stack, small_delta_stack,\n-+\t\t\t\t delta_stack_nr);\n- \t\t} else {\n- \t\t\tALLOC_GROW(delta_stack, delta_stack_nr+1, delta_stack_alloc);\n- \t\t}\n diff -u -p a/compat/regex/regexec.c b/compat/regex/regexec.c\n --- a/compat/regex/regexec.c\n +++ b/compat/regex/regexec.c\n\n\nHow do you think about the differences from this test result?\n\nRegards,\nMarkus\n"},{"id":"386325","messageId":"d2fe2be3-f68f-d5fb-076b-3c740fe5a29a@web.de","threadId":"52241","inReplyTo":"6fffd13a-738b-e750-9f5a-f0bfb252855b@web.de","subject":"Re: git-coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-15T14:20:24Z","receivedAt":"2019-11-15T14:20:37Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> --- array-released.diff\t2019-11-14 21:29:11.020576916 +0100\n> +++ array-reduced1.diff\t2019-11-14 21:45:58.931956527 +0100\n> @@ -6,24 +6,10 @@\n> \tr->entry_count = t->entry_count;\n> \tr->delta_depth = t->delta_depth;\n> -\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));\n> -+\tCOPY_ARRAY(r->entries, t->entries, t->entry_count);\n> ++\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));\n> \trelease_tree_content(t);\n> \treturn r;\n> }\n\nCan another variant for a transformation rule help to clarify unexpected\nsoftware behaviour around data processing with the semantic patch language?\n\n@@\nexpression dst, src, n, E;\ntype T;\nT *ptr;\nT[] arr;\n@@\n memcpy(\n( dst, src, sizeof(\n+ *(\n E\n- [...]\n+ )\n ) * n\n|\n ptr, src, sizeof(\n- *(ptr)\n+ T\n ) * n\n| arr, src, sizeof(\n- *(arr)\n+ T\n ) * n\n| dst, ptr, sizeof(\n- *(ptr)\n+ T\n ) * n\n| dst, arr, sizeof(\n- *(arr)\n+ T\n ) * n\n)\n )\n\n\nelfring@Sonne:~/Projekte/git/lokal> spatch contrib/coccinelle/array-test3.cocci fast-import.c\n…\n\nRegards,\nMarkus\n"},{"id":"386340","messageId":"94301b9c-a397-ae04-c617-92679f4bb018@web.de","threadId":"52241","inReplyTo":"6fffd13a-738b-e750-9f5a-f0bfb252855b@web.de","subject":"Re: git-coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-15T18:50:49Z","receivedAt":"2019-11-15T18:51:00Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> --- array-released.diff\t2019-11-14 21:29:11.020576916 +0100\n> +++ array-reduced1.diff\t2019-11-14 21:45:58.931956527 +0100\n> @@ -6,24 +6,10 @@\n> \tr->entry_count = t->entry_count;\n> \tr->delta_depth = t->delta_depth;\n> -\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));\n> -+\tCOPY_ARRAY(r->entries, t->entries, t->entry_count);\n> ++\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));\n> \trelease_tree_content(t);\n> \treturn r;\n> }\n\nIt took a while to become more aware of software development challenges\nfor the safe data processing with the semantic patch language also\nat such a source code place.\nhttps://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L640\n\nI got the impression that the Coccinelle software is occasionally able\nto determine from the search specification “sizeof(T)” the corresponding\ndata type for code like “*(t->entries)”.\nBut it seems that there are circumstances to consider where the desired\ndata type was not automatically determined.\nThus the data processing can become safer by explicitly expressing\nthe case distinction for the handling of expressions.\n\nAdjusted transformation rule:\n@@\ntype T;\nT* dst_ptr, src_ptr;\nT[] dst_arr, src_arr;\nexpression n, x;\n@@\n-memcpy\n+COPY_ARRAY\n (\n( dst_ptr\n| dst_arr\n)\n ,\n( src_ptr\n| src_arr\n)\n ,\n- (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n+ n\n )\n\n\nRegards,\nMarkus\n"},{"id":"386341","messageId":"75b9417b-14a7-c9c6-25eb-f6e05f340376@web.de","threadId":"52241","inReplyTo":"5189f847-1af1-f050-6c72-576a977f6f12@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-15T20:37:32Z","receivedAt":"2019-11-15T20:37:44Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> This eliminates duplication in the semantic patch, which is good.\n\nThanks that you think in such a direction.\n\n\n> It messes up the indentation of n in some of the cases in 921d49be86 (\"use\n> COPY_ARRAY for copying arrays\", 2019-06-15), though. Hmm, but that can\n> be cured by duplicating the comma:\n\nI have picked up further improvement possibilities for this SmPL script.\nWould you like to integrate any of these changes?\n\n\n@@\nexpression dst, src, n, E;\n@@\n memcpy(dst, src, sizeof(\n+ *(\n E\n- [...]\n+ )\n ) * n\n )\n\n@@\ntype T;\nT *ptr;\nT[] arr;\nexpression E, n;\n@@\n memcpy(\n( ptr, E, sizeof(\n- *(ptr)\n+ T\n ) * n\n| arr, E, sizeof(\n- *(arr)\n+ T\n ) * n\n| E, ptr, sizeof(\n- *(ptr)\n+ T\n ) * n\n| E, arr, sizeof(\n- *(arr)\n+ T\n ) * n\n)\n )\n\n@@\ntype T;\nT* dst_ptr, src_ptr;\nT[] dst_arr, src_arr;\nexpression n, x;\n@@\n-memcpy\n+COPY_ARRAY\n (\n( dst_ptr\n| dst_arr\n)\n ,\n( src_ptr\n| src_arr\n)\n- , (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n+ , n\n )\n\n@@\ntype T;\nT* dst, src, ptr;\nexpression n;\n@@\n(\n-memmove\n+MOVE_ARRAY\n (dst, src\n- , (n) * \\( sizeof(* \\( dst \\| src \\) ) \\| sizeof(T) \\)\n+ , n\n )\n|\n-ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n+ALLOC_ARRAY(ptr, n)\n);\n\n\nNow I observe that the placement of space characters can be a coding style\nconcern at four places for adjusted lines by the generated patch.\nWould you like to clarify remaining issues for pretty-printing\nin such use cases?\n\nRegards,\nMarkus\n"},{"id":"386351","messageId":"alpine.DEB.2.21.1911152000170.8961@hadrien","threadId":"52241","inReplyTo":"94301b9c-a397-ae04-c617-92679f4bb018@web.de","subject":"Re: [Cocci] git-coccinelle: adjustments for array.cocci?","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2019-11-16T01:00:29Z","receivedAt":"2019-11-16T01:01:07Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"\n\nOn Fri, 15 Nov 2019, Markus Elfring wrote:\n\n> > --- array-released.diff\t2019-11-14 21:29:11.020576916 +0100\n> > +++ array-reduced1.diff\t2019-11-14 21:45:58.931956527 +0100\n> > @@ -6,24 +6,10 @@\n> > \tr->entry_count = t->entry_count;\n> > \tr->delta_depth = t->delta_depth;\n> > -\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));\n> > -+\tCOPY_ARRAY(r->entries, t->entries, t->entry_count);\n> > ++\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));\n> > \trelease_tree_content(t);\n> > \treturn r;\n> > }\n>\n> It took a while to become more aware of software development challenges\n> for the safe data processing with the semantic patch language also\n> at such a source code place.\n> https://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L640\n>\n> I got the impression that the Coccinelle software is occasionally able\n> to determine from the search specification “sizeof(T)” the corresponding\n> data type for code like “*(t->entries)”.\n\nIt can determine the type of t->entries if it has access to the definition\nof the type of t. This type may be in a header file. If you want\nCoccinelle to be able to find this information you can use the option\n--all-includes or --recursive-includes. It will be more efficient with\nthe option --include-headers-for-types.\n\njulia\n\n> But it seems that there are circumstances to consider where the desired\n> data type was not automatically determined.\n> Thus the data processing can become safer by explicitly expressing\n> the case distinction for the handling of expressions.\n>\n> Adjusted transformation rule:\n> @@\n> type T;\n> T* dst_ptr, src_ptr;\n> T[] dst_arr, src_arr;\n> expression n, x;\n> @@\n> -memcpy\n> +COPY_ARRAY\n> (\n> ( dst_ptr\n> | dst_arr\n> )\n> ,\n> ( src_ptr\n> | src_arr\n> )\n> ,\n> - (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n> + n\n> )\n>\n>\n> Regards,\n> Markus\n> _______________________________________________\n> Cocci mailing list\n> Cocci@systeme.lip6.fr\n> https://systeme.lip6.fr/mailman/listinfo/cocci\n>"},{"id":"386364","messageId":"4ee4604e-0eb1-d4a6-24bb-52abe0db3f53@web.de","threadId":"52241","inReplyTo":"alpine.DEB.2.21.1911152000170.8961@hadrien","subject":"Re: [Cocci] git-coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-16T06:57:25Z","receivedAt":"2019-11-16T06:57:45Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> https://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L640\n>>\n>> I got the impression that the Coccinelle software is occasionally able\n>> to determine from the search specification “sizeof(T)” the corresponding\n>> data type for code like “*(t->entries)”.\n>\n> It can determine the type of t->entries if it has access to the definition\n> of the type of t.\n\nShould this type determination always work here because the data structure\n“tree_content” for the parameter “t” of the function “grow_tree_content”\nis defined in the same source file?\nhttps://github.com/git/git/blob/3edfcc65fdfc708c1c8f1d314885eecf9beb9b67/fast-import.c#L85\n\n\n> This type may be in a header file. If you want\n> Coccinelle to be able to find this information you can use the option\n> --all-includes or --recursive-includes. It will be more efficient with\n> the option --include-headers-for-types.\n\nSuch information can be more helpful in other situations than the mentioned\ntest case.\n\n\n>> But it seems that there are circumstances to consider where the desired\n>> data type was not automatically determined.\n\nWould you like to take the presented differences from the discussed\nbefore/after comparison better into account?\n\nRegards,\nMarkus\n"},{"id":"386365","messageId":"22a03cac-160b-51be-b015-54ac600d3e92@web.de","threadId":"52241","inReplyTo":"alpine.DEB.2.21.1911152000170.8961@hadrien","subject":"Re: [Cocci] git-coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-16T08:29:54Z","receivedAt":"2019-11-16T08:31:37Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> It can determine the type of t->entries if it has access to the definition\n> of the type of t.\n\nI would like to point another implementation detail out.\n\nAnother known function was also an update candidate.\nhttps://github.com/git/git/blob/9a1180fc304ad9831641e5788e9c8d3dfc10ccdd/pretty.c#L90\n\nelfring@Sonne:~/Projekte/git/lokal> spatch contrib/coccinelle/array.cocci pretty.c\n…\n@@ -106,8 +106,8 @@ static void setup_commit_formats(void)\n commit_formats_len = ARRAY_SIZE(builtin_formats);\n builtin_formats_len = commit_formats_len;\n ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);\n- memcpy(commit_formats, builtin_formats,\n- sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n+ COPY_ARRAY(commit_formats, builtin_formats,\n+ ARRAY_SIZE(builtin_formats));\n\n git_config(git_pretty_formats_config, NULL);\n }\n\n\nThis patch generation can work based on the following SmPL code combination.\n\n“…\nexpression n, x;\n…\n- , (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n…”\n\nThe asterisk should refer to a pointer expression within a sizeof operator.\nI got informed that the semantic patch language would support such a restriction.\n\nThus I have tried out to specify the corresponding metavariables in this way.\n\n“…\nexpression n;\nexpression* x;\n…”\n\nBut the shown diff hunk is not regenerated by this SmPL script variant.\nHow should an array like “builtin_formats” (which is even defined in the same function)\nbe treated by the Coccinelle software in such use cases?\n\nRegards,\nMarkus\n"},{"id":"386372","messageId":"05ab1110-2115-7886-f890-9983caabc52c@web.de","threadId":"52241","inReplyTo":"5189f847-1af1-f050-6c72-576a977f6f12@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-16T16:33:26Z","receivedAt":"2019-11-16T16:33:40Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> This reduces duplication in the semantic patch, which is nice. I think\n> I tried something like that at the time, but found that it failed to\n> produce some of the cases in 921d49be86 (\"use COPY_ARRAY for copying\n> arrays\", 2019-06-15) for some reason.\n\nI propose to integrate an other solution variant.\n\n* How do you think about to delete questionable transformation rules\n together with increasing the usage of nested disjunctions in this script\n for the semantic patch language?\n\n* Can a single transformation rule become sufficient for the discussed\n change pattern?\n\n\n@@\ntype T;\nT* dst_ptr, src_ptr, ptr;\nT[] dst_arr, src_arr;\nexpression n, x;\n@@\n(\n-memcpy\n+COPY_ARRAY\n (\n( dst_ptr\n| dst_arr\n)\n ,\n( src_ptr\n| src_arr\n)\n- , (n) * \\( sizeof(T) \\| sizeof( \\( *(x) \\| x[...] \\) ) \\)\n+ , n\n )\n|\n-memmove\n+MOVE_ARRAY\n (dst_ptr,\n src_ptr\n- , (n) * \\( sizeof(* \\( dst_ptr \\| src_ptr \\) ) \\| sizeof(T) \\)\n+ , n\n )\n|\n-ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n+ALLOC_ARRAY(ptr, n)\n)\n\n\nWould you like to clarify remaining challenges for pretty-printing\nin such use cases?\n\nRegards,\nMarkus\n"},{"id":"386373","messageId":"alpine.DEB.2.21.1911161855400.3558@hadrien","threadId":"52241","inReplyTo":"6fffd13a-738b-e750-9f5a-f0bfb252855b@web.de","subject":"Re: [Cocci] git-coccinelle: adjustments for array.cocci?","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2019-11-16T17:57:38Z","receivedAt":"2019-11-16T17:57:43Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"\n\nOn Fri, 15 Nov 2019, Markus Elfring wrote:\n\n> > Anyway, someone who can reproduce the issue using the latest release\n> > of Coccinelle would be in a better position to file a bug report.\n>\n> Hello,\n>\n> I repeated the discussed source code transformation approach together\n> with the software combination “Coccinelle 1.0.8-00004-g842075f7” (OCaml 4.09).\n> https://github.com/coccinelle/coccinelle/commits/master\n>\n> 1. Yesterday I checked the source files out for the software “Git”\n> according to the commit “The first batch post 2.24 cycle”.\n> https://github.com/git/git/commit/d9f6f3b6195a0ca35642561e530798ad1469bd41\n>\n> 2. I restored a previous development status by the following command.\n>\n> git show 921d49be86 | patch -p1 -R\n>\n> See also:\n> https://public-inbox.org/git/53346d52-e096-c651-f70a-ce6ca4d82ff9@web.de/\n>\n> 3. I stored a generated patch based on the currently released SmPL script.\n> https://github.com/git/git/blob/177fbab747da4f58cb2a8ce010b3515c86dd67c9/contrib/coccinelle/array.cocci\n>\n> 4. I applied the following patch then.\n>\n> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n> index 46b8d2ee11..89df184bbd 100644\n> --- a/contrib/coccinelle/array.cocci\n> +++ b/contrib/coccinelle/array.cocci\n> @@ -12,27 +12,21 @@ T *ptr;\n> T[] arr;\n> expression E, n;\n> @@\n> -(\n> - memcpy(ptr, E,\n> -- n * sizeof(*(ptr))\n> -+ n * sizeof(T)\n> - )\n> -|\n> - memcpy(arr, E,\n> -- n * sizeof(*(arr))\n> -+ n * sizeof(T)\n> - )\n> -|\n> - memcpy(E, ptr,\n> -- n * sizeof(*(ptr))\n> -+ n * sizeof(T)\n> - )\n> -|\n> - memcpy(E, arr,\n> -- n * sizeof(*(arr))\n> -+ n * sizeof(T)\n> - )\n> + memcpy(\n> +( ptr, E, n *\n> +- sizeof(*(ptr))\n> ++ sizeof(T)\n> +| arr, E, n *\n> +- sizeof(*(arr))\n> ++ sizeof(T)\n> +| E, ptr, n *\n> +- sizeof(*(ptr))\n> ++ sizeof(T)\n> +| E, arr, n *\n> +- sizeof(*(arr))\n> ++ sizeof(T)\n> )\n> + )\n\nThis seems quite unreadable, in contrast to the original code.\n\n>\n> @@\n> type T;\n>\n> I suggested in this way to move a bit of SmPL code.\n>\n> 5. I stored another generated patch based on the adjusted SmPL script.\n\nNo idea what it means to store a patch.\n\n> 6. I performed a corresponding file comparison.\n>\n> --- array-released.diff\t2019-11-14 21:29:11.020576916 +0100\n> +++ array-reduced1.diff\t2019-11-14 21:45:58.931956527 +0100\n> @@ -6,24 +6,10 @@\n> \tr->entry_count = t->entry_count;\n> \tr->delta_depth = t->delta_depth;\n> -\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));\n> -+\tCOPY_ARRAY(r->entries, t->entries, t->entry_count);\n> ++\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));\n> \trelease_tree_content(t);\n> \treturn r;\n> }\n\nI have no idea what is being compared here. The COPY_ARRAY thing looks\nnice, but doesn't seem to have anything to do with your semantic patch.\n\njulia\n\n\n\n> -diff -u -p a/pretty.c b/pretty.c\n> ---- a/pretty.c\n> -+++ b/pretty.c\n> -@@ -106,8 +106,8 @@ static void setup_commit_formats(void)\n> - \tcommit_formats_len = ARRAY_SIZE(builtin_formats);\n> - \tbuiltin_formats_len = commit_formats_len;\n> - \tALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);\n> --\tmemcpy(commit_formats, builtin_formats,\n> --\t sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n> -+\tCOPY_ARRAY(commit_formats, builtin_formats,\n> -+\t\t ARRAY_SIZE(builtin_formats));\n> -\n> - \tgit_config(git_pretty_formats_config, NULL);\n> - }\n> diff -u -p a/packfile.c b/packfile.c\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -36,17 +22,6 @@\n> \t\t} else {\n> \t\t\tALLOC_GROW(poi_stack, poi_stack_nr+1, poi_stack_alloc);\n> \t\t}\n> -@@ -1698,8 +1698,8 @@ void *unpack_entry(struct repository *r,\n> - \t\t && delta_stack == small_delta_stack) {\n> - \t\t\tdelta_stack_alloc = alloc_nr(delta_stack_nr);\n> - \t\t\tALLOC_ARRAY(delta_stack, delta_stack_alloc);\n> --\t\t\tmemcpy(delta_stack, small_delta_stack,\n> --\t\t\t sizeof(*delta_stack)*delta_stack_nr);\n> -+\t\t\tCOPY_ARRAY(delta_stack, small_delta_stack,\n> -+\t\t\t\t delta_stack_nr);\n> - \t\t} else {\n> - \t\t\tALLOC_GROW(delta_stack, delta_stack_nr+1, delta_stack_alloc);\n> - \t\t}\n> diff -u -p a/compat/regex/regexec.c b/compat/regex/regexec.c\n> --- a/compat/regex/regexec.c\n> +++ b/compat/regex/regexec.c\n>\n>\n> How do you think about the differences from this test result?\n>\n> Regards,\n> Markus\n> _______________________________________________\n> Cocci mailing list\n> Cocci@systeme.lip6.fr\n> https://systeme.lip6.fr/mailman/listinfo/cocci\n>"},{"id":"386376","messageId":"d232b052-430c-5d44-96d5-b8bff261314d@web.de","threadId":"52241","inReplyTo":"alpine.DEB.2.21.1911161855400.3558@hadrien","subject":"Re: [Cocci] git-coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-16T18:29:29Z","receivedAt":"2019-11-16T18:29:41Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> + memcpy(\n>> +( ptr, E, n *\n>> +- sizeof(*(ptr))\n>> ++ sizeof(T)\n>> +| arr, E, n *\n>> +- sizeof(*(arr))\n>> ++ sizeof(T)\n>> +| E, ptr, n *\n>> +- sizeof(*(ptr))\n>> ++ sizeof(T)\n>> +| E, arr, n *\n>> +- sizeof(*(arr))\n>> ++ sizeof(T)\n>> )\n>> + )\n>\n> This seems quite unreadable, in contrast to the original code.\n\nThe code formatting can vary for improved applications of SmPL disjunctions.\n\nSee also related update suggestions:\n* https://public-inbox.org/git/05ab1110-2115-7886-f890-9983caabc52c@web.de/\n* https://public-inbox.org/git/75b9417b-14a7-c9c6-25eb-f6e05f340376@web.de/\n\n\n>> 5. I stored another generated patch based on the adjusted SmPL script.\n>\n> No idea what it means to store a patch.\n\nI put the output from the program “spatch” into a text file like “array-reduced1.diff”\nin a selected directory.\n\n\n>> 6. I performed a corresponding file comparison.\n>>\n>> --- array-released.diff\t2019-11-14 21:29:11.020576916 +0100\n>> +++ array-reduced1.diff\t2019-11-14 21:45:58.931956527 +0100\n>> @@ -6,24 +6,10 @@\n>> \tr->entry_count = t->entry_count;\n>> \tr->delta_depth = t->delta_depth;\n>> -\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(t->entries[0]));\n>> -+\tCOPY_ARRAY(r->entries, t->entries, t->entry_count);\n>> ++\tmemcpy(r->entries,t->entries,t->entry_count*sizeof(*(t->entries)));\n>> \trelease_tree_content(t);\n>> \treturn r;\n>> }\n>\n> I have no idea what is being compared here.\n\nI suggest to take another look at the described steps then.\n\n\n> The COPY_ARRAY thing looks nice, but doesn't seem to have anything to do\n> with your semantic patch.\n\nI find your interpretation of the presented software situation questionable.\n\n* I got the impression in the meantime that my suggestion for a refactoring\n of a specific SmPL disjunction influenced transformation results for\n a subsequent SmPL rule in unexpected ways.\n\n* Other software adjustments and solution variants can trigger further\n development considerations, can't they?\n\nRegards,\nMarkus\n"},{"id":"386378","messageId":"fc56b970-4ca1-7734-c4bb-f57cae7a273f@web.de","threadId":"52241","inReplyTo":"75b9417b-14a7-c9c6-25eb-f6e05f340376@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-16T21:13:45Z","receivedAt":"2019-11-16T21:13:54Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.11.19 um 21:37 schrieb Markus Elfring:\n>> This eliminates duplication in the semantic patch, which is good.\n>\n> Thanks that you think in such a direction.\n>\n>\n>> It messes up the indentation of n in some of the cases in 921d49be86 (\"use\n>> COPY_ARRAY for copying arrays\", 2019-06-15), though. Hmm, but that can\n>> be cured by duplicating the comma:\n>\n> I have picked up further improvement possibilities for this SmPL script.\n> Would you like to integrate any of these changes?\n\nNot sure, could you please elaborate on the benefits of each proposed\nchange?\n\n> @@\n> expression dst, src, n, E;\n> @@\n> memcpy(dst, src, sizeof(\n> + *(\n> E\n> - [...]\n> + )\n> ) * n\n> )\n\nThat's longer and looks more complicated to me than what we currently have:\n\n @@\n expression dst, src, n, E;\n @@\n memcpy(dst, src, n * sizeof(\n - E[...]\n + *(E)\n ))\n\nAvoiding to duplicate E doesn't seem to be worth it. I can see that\nindenting the sizeof parameter and parentheses could improve readability,\nthough.\n\n> @@\n> type T;\n> T *ptr;\n> T[] arr;\n> expression E, n;\n> @@\n> memcpy(\n> ( ptr, E, sizeof(\n> - *(ptr)\n> + T\n> ) * n\n> | arr, E, sizeof(\n> - *(arr)\n> + T\n> ) * n\n> | E, ptr, sizeof(\n> - *(ptr)\n> + T\n> ) * n\n> | E, arr, sizeof(\n> - *(arr)\n> + T\n> ) * n\n> )\n> )\n\nThis still fails to regenerate two of the changes from 921d49be86 (use\nCOPY_ARRAY for copying arrays, 2019-06-15), at least with for me (and\nCoccinelle 1.0.4).\n\n> @@\n> type T;\n> T* dst_ptr, src_ptr;\n> T[] dst_arr, src_arr;\n> expression n, x;\n> @@\n> -memcpy\n> +COPY_ARRAY\n> (\n> ( dst_ptr\n> | dst_arr\n> )\n> ,\n> ( src_ptr\n> | src_arr\n> )\n> - , (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n> + , n\n> )\n\nThat x could be anything -- it's not tied to the element size of source\nor destination. Such a transformation might change the meaning of the\ncode, as COPY_ARRAY will use the element size of the destination behind\nthe scenes. So that doesn't look safe to me.\n\n> @@\n> type T;\n> T* dst, src, ptr;\n> expression n;\n> @@\n> (\n> -memmove\n> +MOVE_ARRAY\n> (dst, src\n> - , (n) * \\( sizeof(* \\( dst \\| src \\) ) \\| sizeof(T) \\)\n> + , n\n> )\n> |\n> -ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n> +ALLOC_ARRAY(ptr, n)\n> );\n\nmemmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different; why\nwould we want to jam transformations for them into the same rule like\nthis? The only overlap seems to be n. Handling memmove/MOVE_ARRAY and\nmemcpy/COPY_ARRAY together would make more sense, as they take the same\nkinds of parameters.\n\nI didn't know that disjunctions can be specified inline using \\(, \\|\nand \\), though. Rules can be much more compact that way. Mixing\nlanguages like that can also be quite confusing. Syntax highlighting\ncould help; https://github.com/ahf/cocci-syntax at least doesn't\nshow those any different from regular code, though.\n\n> Now I observe that the placement of space characters can be a coding style\n> concern at four places for adjusted lines by the generated patch.\n> Would you like to clarify remaining issues for pretty-printing\n> in such use cases?\n\nIdeally, generated code should adhere to Documentation/CodingGuidelines,\nso that it can be accepted without requiring hand-editing.\n\nRené\n"},{"id":"386379","messageId":"fd15e721-de74-1a4f-be88-7700d583e2f9@web.de","threadId":"52241","inReplyTo":"05ab1110-2115-7886-f890-9983caabc52c@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-16T21:38:14Z","receivedAt":"2019-11-16T21:38:18Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.11.19 um 17:33 schrieb Markus Elfring:\n>> This reduces duplication in the semantic patch, which is nice. I think\n>> I tried something like that at the time, but found that it failed to\n>> produce some of the cases in 921d49be86 (\"use COPY_ARRAY for copying\n>> arrays\", 2019-06-15) for some reason.\n>\n> I propose to integrate an other solution variant.\n>\n> * How do you think about to delete questionable transformation rules\n> together with increasing the usage of nested disjunctions in this script\n> for the semantic patch language?\n\nWhich transformation rules are questionable and why? Removing broken\nor ineffective rules would be very welcome.\n\nSpecifying disjunctions inline can make rules shorter, but harder to\nunderstand due to mixing languages. Perhaps this is a matter of\ngetting used to it, and syntax highlighting might help a bit.\n\n> * Can a single transformation rule become sufficient for the discussed\n> change pattern?\n>\n>\n> @@\n> type T;\n> T* dst_ptr, src_ptr, ptr;\n> T[] dst_arr, src_arr;\n> expression n, x;\n> @@\n> (\n> -memcpy\n> +COPY_ARRAY\n> (\n> ( dst_ptr\n> | dst_arr\n> )\n> ,\n> ( src_ptr\n> | src_arr\n> )\n> - , (n) * \\( sizeof(T) \\| sizeof( \\( *(x) \\| x[...] \\) ) \\)\n> + , n\n> )\n> |\n> -memmove\n> +MOVE_ARRAY\n> (dst_ptr,\n> src_ptr\n> - , (n) * \\( sizeof(* \\( dst_ptr \\| src_ptr \\) ) \\| sizeof(T) \\)\n> + , n\n> )\n> |\n> -ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n> +ALLOC_ARRAY(ptr, n)\n> )\n\nmemmove/MOVE_ARRAY take the same kind of parameters as\nmemcpy/COPY_ARRAY, so handling them in the same rule makes sense.\nThe former could take advantage of the transformations for arrays\nthat the latter has.\n\nMixing in the unrelated xmalloc/ALLOC_ARRAY transformation does\nnot make sense to me, though.\n\nMatching sizeof of anything (with the x) can produce inaccurate\ntransformations, as mentioned in the other reply I just sent.\n\n> Would you like to clarify remaining challenges for pretty-printing\n> in such use cases?\n\nNot sure what you mean here. Did my other reply answer it? If it\ndidn't then please state what's unclear to you.\n\nRené\n"},{"id":"386389","messageId":"57b5d1c9-72c1-6fff-a242-90f5f24f0972@web.de","threadId":"52241","inReplyTo":"fc56b970-4ca1-7734-c4bb-f57cae7a273f@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-17T07:56:10Z","receivedAt":"2019-11-17T07:59:30Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> @@\n>> expression dst, src, n, E;\n>> @@\n>> memcpy(dst, src, sizeof(\n>> + *(\n>> E\n>> - [...]\n>> + )\n>> ) * n\n>> )\n>\n> That's longer and looks more complicated to me\n\nI point another possibility out to express a change specification\nby the means of the semantic patch language.\nHow would you think about such SmPL code if the indentation\nwill be reduced?\n\n\n> than what we currently have:\n> @@\n> expression dst, src, n, E;\n> @@\n> memcpy(dst, src, n * sizeof(\n> - E[...]\n> + *(E)\n> ))\n>\n> Avoiding to duplicate E doesn't seem to be worth it.\n\nI show other development preferences occasionally.\n\n\n> I can see that indenting the sizeof parameter and parentheses could\n> improve readability, though.\n\nThanks that you can follow such coding style aspects.\n\n\n>> @@\n>> type T;\n>> T *ptr;\n>> T[] arr;\n>> expression E, n;\n>> @@\n>> memcpy(\n>> ( ptr, E, sizeof(\n>> - *(ptr)\n>> + T\n>> ) * n\n>> | arr, E, sizeof(\n>> - *(arr)\n>> + T\n>> ) * n\n>> | E, ptr, sizeof(\n>> - *(ptr)\n>> + T\n>> ) * n\n>> | E, arr, sizeof(\n>> - *(arr)\n>> + T\n>> ) * n\n>> )\n>> )\n>\n> This still fails to regenerate two of the changes from 921d49be86\n> (use COPY_ARRAY for copying arrays, 2019-06-15), at least with for me\n> (and Coccinelle 1.0.4).\n\nWould you become keen to find the reasons out for unexpected data processing\nresults (also by the software combination “Coccinelle 1.0.8-00004-g842075f7”)\nat this place?\n\nBut this transformation rule can probably be omitted if the usage\nof SmPL disjunctions will be increased in a subsequent rule, can't it?\n\n\n>> @@\n>> type T;\n>> T* dst_ptr, src_ptr;\n>> T[] dst_arr, src_arr;\n>> expression n, x;\n>> @@\n>> -memcpy\n>> +COPY_ARRAY\n>> (\n>> ( dst_ptr\n>> | dst_arr\n>> )\n>> ,\n>> ( src_ptr\n>> | src_arr\n>> )\n>> - , (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n>> + , n\n>> )\n>\n> That x could be anything -- it's not tied to the element size of source\n> or destination. Such a transformation might change the meaning of the\n> code, as COPY_ARRAY will use the element size of the destination behind\n> the scenes. So that doesn't look safe to me.\n\nWould you like to use the SmPL code “*( \\( src_ptr \\| src_arr \\) )” instead?\n\n\n>> @@\n>> type T;\n>> T* dst, src, ptr;\n>> expression n;\n>> @@\n>> (\n>> -memmove\n>> +MOVE_ARRAY\n>> (dst, src\n>> - , (n) * \\( sizeof(* \\( dst \\| src \\) ) \\| sizeof(T) \\)\n>> + , n\n>> )\n>> |\n>> -ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n>> +ALLOC_ARRAY(ptr, n)\n>> );\n>\n> memmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different;\n\nThese functions provide another programming interface.\n\n\n> why would we want to jam transformations for them into the same rule\n> like this?\n\nPossible nicer run time characteristics by the Coccinelle software.\n\n\n> The only overlap seems to be n.\n\nThese case distinctions can share also the metavariable “T” for the\ndesired source code deletion.\n\n\n> Handling memmove/MOVE_ARRAY and memcpy/COPY_ARRAY together would make\n> more sense, as they take the same kinds of parameters.\n\nWould you like to adjust the SmPL code in such a design direction?\n\n\n> I didn't know that disjunctions can be specified inline using \\(, \\|\n> and \\), though. Rules can be much more compact that way.\n\nI hope that more corresponding software improvements can be achieved.\n\n\n> Mixing languages like that can also be quite confusing.\n\nI agree to this development concern.\n\n\n>> Now I observe that the placement of space characters can be a coding style\n>> concern at four places for adjusted lines by the generated patch.\n>> Would you like to clarify remaining issues for pretty-printing\n>> in such use cases?\n>\n> Ideally, generated code should adhere to Documentation/CodingGuidelines,\n> so that it can be accepted without requiring hand-editing.\n\nBut how does the software situation look like if the original source code\nwould contain coding style issues?\n\nIt seems to be possible to specify SmPL code in a way so that even questionable\ncode layout would be preserved by an automatic transformation.\n\nRegards,\nMarkus\n"},{"id":"386390","messageId":"50b265f0-bcab-d0ec-a714-07e94ceaa508@web.de","threadId":"52241","inReplyTo":"fd15e721-de74-1a4f-be88-7700d583e2f9@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-17T08:19:34Z","receivedAt":"2019-11-17T08:19:39Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Which transformation rules are questionable and why?\n\nIt was chosen to transform source code fragments (pointer expressions)\nby two SmPL rules so that the search pattern “sizeof(T)” would work\nin the third rule.\n\n\n> Removing broken or ineffective rules would be very welcome.\n\nI suggest to reconsider programming opportunities also by the means of\nthe semantic patch language.\n\n\n> Specifying disjunctions inline can make rules shorter, but harder to\n> understand due to mixing languages. Perhaps this is a matter of\n> getting used to it, and syntax highlighting might help a bit.\n\nI agree to this view.\n\n\n> Mixing in the unrelated xmalloc/ALLOC_ARRAY transformation does\n> not make sense to me, though.\n\nI propose to increase the sharing (or reuse) of involved metavariables.\n\n\n> Matching sizeof of anything (with the x) can produce inaccurate\n> transformations, as mentioned in the other reply I just sent.\n\nWould you like to apply any further SmPL code fine-tuning?\n\nRegards,\nMarkus\n"},{"id":"386394","messageId":"37c84512-ba83-51ce-4253-ea0f7bd41de0@web.de","threadId":"52241","inReplyTo":"57b5d1c9-72c1-6fff-a242-90f5f24f0972@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-17T13:40:03Z","receivedAt":"2019-11-17T13:40:10Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.11.19 um 08:56 schrieb Markus Elfring:\n>>> @@\n>>> expression dst, src, n, E;\n>>> @@\n>>> memcpy(dst, src, sizeof(\n>>> + *(\n>>> E\n>>> - [...]\n>>> + )\n>>> ) * n\n>>> )\n>>\n>> That's longer and looks more complicated to me\n>\n> I point another possibility out to express a change specification\n> by the means of the semantic patch language.\n> How would you think about such SmPL code if the indentation\n> will be reduced?\n\nWhitespace is not what makes the above example more complicated than the\nequivalent rule below; separating the pieces of simple expressions does.\n\n>> than what we currently have:\n>> @@\n>> expression dst, src, n, E;\n>> @@\n>> memcpy(dst, src, n * sizeof(\n>> - E[...]\n>> + *(E)\n>> ))\n\n>>> @@\n>>> type T;\n>>> T *ptr;\n>>> T[] arr;\n>>> expression E, n;\n>>> @@\n>>> memcpy(\n>>> ( ptr, E, sizeof(\n>>> - *(ptr)\n>>> + T\n>>> ) * n\n>>> | arr, E, sizeof(\n>>> - *(arr)\n>>> + T\n>>> ) * n\n>>> | E, ptr, sizeof(\n>>> - *(ptr)\n>>> + T\n>>> ) * n\n>>> | E, arr, sizeof(\n>>> - *(arr)\n>>> + T\n>>> ) * n\n>>> )\n>>> )\n>>\n>> This still fails to regenerate two of the changes from 921d49be86\n>> (use COPY_ARRAY for copying arrays, 2019-06-15), at least with for me\n>> (and Coccinelle 1.0.4).\n>\n> Would you become keen to find the reasons out for unexpected data processing\n> results (also by the software combination “Coccinelle 1.0.8-00004-g842075f7”)\n> at this place?\n\nIt looks like a bug in Coccinelle to me and I'd like to see it fixed if\nthat's confirmed, of course. And I'd like to see Debian pick up a newer\nversion, preferably containing that fix. But at least until then our\nsemantic patches need to work around it.\n\n> But this transformation rule can probably be omitted if the usage\n> of SmPL disjunctions will be increased in a subsequent rule, can't it?\n\nPerhaps, but I don't see how. Do you?\n\n>>> @@\n>>> type T;\n>>> T* dst_ptr, src_ptr;\n>>> T[] dst_arr, src_arr;\n>>> expression n, x;\n>>> @@\n>>> -memcpy\n>>> +COPY_ARRAY\n>>> (\n>>> ( dst_ptr\n>>> | dst_arr\n>>> )\n>>> ,\n>>> ( src_ptr\n>>> | src_arr\n>>> )\n>>> - , (n) * \\( sizeof(T) \\| sizeof(*(x)) \\)\n>>> + , n\n>>> )\n>>\n>> That x could be anything -- it's not tied to the element size of source\n>> or destination. Such a transformation might change the meaning of the\n>> code, as COPY_ARRAY will use the element size of the destination behind\n>> the scenes. So that doesn't look safe to me.\n>\n> Would you like to use the SmPL code “*( \\( src_ptr \\| src_arr \\) )” instead?\n\nThat leaves out dst_ptr and dst_arr.\n\nAnd what would it mean to match e.g. this ?\n\n\tmemcpy(dst_ptr, src_ptr, n * sizeof(*src_arr))\n\nAt least the element size would be the same, but I'd rather shy away from\ntransforming weird cases like this automatically.\n\n>>> @@\n>>> type T;\n>>> T* dst, src, ptr;\n>>> expression n;\n>>> @@\n>>> (\n>>> -memmove\n>>> +MOVE_ARRAY\n>>> (dst, src\n>>> - , (n) * \\( sizeof(* \\( dst \\| src \\) ) \\| sizeof(T) \\)\n>>> + , n\n>>> )\n>>> |\n>>> -ptr = xmalloc((n) * \\( sizeof(*ptr) \\| sizeof(T) \\))\n>>> +ALLOC_ARRAY(ptr, n)\n>>> );\n>>\n>> memmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different;\n>\n> These functions provide another programming interface.\n\nHuh, which one specifically? Here are the signatures of the functions\nand macros, for reference:\n\n void *memmove(void *dest, const void *src, size_t n);\n void *memcpy(void *dest, const void *src, size_t n);\n\n COPY_ARRAY(dst, src, n)\n MOVE_ARRAY(dst, src, n)\n\n>> why would we want to jam transformations for them into the same rule\n>> like this?\n>\n> Possible nicer run time characteristics by the Coccinelle software.\n\nHow much faster is it exactly?\n\nSpeedups are good, but I think readability of rules is more important\nthan coccicheck duration.\n\n>> Handling memmove/MOVE_ARRAY and memcpy/COPY_ARRAY together would make\n>> more sense, as they take the same kinds of parameters.\n>\n> Would you like to adjust the SmPL code in such a design direction?\n\nI can't find any examples in our code base that would be transformed by\na generalized rule. That reduces my own motivation to tinker with the\nexisting rules to close to zero.\n\n>>> Now I observe that the placement of space characters can be a coding style\n>>> concern at four places for adjusted lines by the generated patch.\n>>> Would you like to clarify remaining issues for pretty-printing\n>>> in such use cases?\n>>\n>> Ideally, generated code should adhere to Documentation/CodingGuidelines,\n>> so that it can be accepted without requiring hand-editing.\n>\n> But how does the software situation look like if the original source code\n> would contain coding style issues?\n\nThe same: Generated code should not add coding style issues. We can\nstill use results that need to be polished, but that's a manual step\nwhich reduces the benefits of automation.\n\n> It seems to be possible to specify SmPL code in a way so that even questionable\n> code layout would be preserved by an automatic transformation.\n\nThat may be acceptable.\n\nRené\n"},{"id":"386395","messageId":"f28f5fb8-2814-9df5-faf2-7146ed1a1f4d@web.de","threadId":"52241","inReplyTo":"50b265f0-bcab-d0ec-a714-07e94ceaa508@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-17T13:40:46Z","receivedAt":"2019-11-17T13:40:48Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.11.19 um 09:19 schrieb Markus Elfring:\n>> Which transformation rules are questionable and why?\n>\n> It was chosen to transform source code fragments (pointer expressions)\n> by two SmPL rules so that the search pattern “sizeof(T)” would work\n> in the third rule.\n\nAh, right, it would be nice to get rid of those normalization rules,\nespecially the second one. I don't see how, though, without either\ncausing a combinatorial explosion or loosening up the matching too much.\n\n>> Matching sizeof of anything (with the x) can produce inaccurate\n>> transformations, as mentioned in the other reply I just sent.\n>\n> Would you like to apply any further SmPL code fine-tuning?\n\nI guess that's a question for Junio, and his reply in\nhttps://public-inbox.org/git/xmqqa790cyp1.fsf@gitster-ct.c.googlers.com/\nseems relevant.\n\nRené\n"},{"id":"386396","messageId":"eff19da9-3f9f-0cf0-1e88-64d2acdbabcd@web.de","threadId":"52241","inReplyTo":"37c84512-ba83-51ce-4253-ea0f7bd41de0@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-17T18:19:40Z","receivedAt":"2019-11-17T18:19:48Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Whitespace is not what makes the above example more complicated than the\n> equivalent rule below;\n\nA different code layout might help in a better understanding for such\nchange specifications.\n\n\n> separating the pieces of simple expressions does.\n\nWill there occasionally be a need to change only the required source code parts?\n\n\n>>> than what we currently have:\n>>> @@\n>>> expression dst, src, n, E;\n>>> @@\n>>> memcpy(dst, src, n * sizeof(\n>>> - E[...]\n>>> + *(E)\n>>> ))\n\nAre any circumstances to consider where only the essential implementation details\nshould be touched by an automatic software transformation?\n\n\n>>>> @@\n>>>> type T;\n>>>> T *ptr;\n>>>> T[] arr;\n>>>> expression E, n;\n>>>> @@\n>>>> memcpy(\n>>>> ( ptr, E, sizeof(\n>>>> - *(ptr)\n>>>> + T\n>>>> ) * n\n>>>> | arr, E, sizeof(\n>>>> - *(arr)\n>>>> + T\n>>>> ) * n\n>>>> | E, ptr, sizeof(\n>>>> - *(ptr)\n>>>> + T\n>>>> ) * n\n>>>> | E, arr, sizeof(\n>>>> - *(arr)\n>>>> + T\n>>>> ) * n\n>>>> )\n>>>> )\n>>>\n>>> This still fails to regenerate two of the changes from 921d49be86\n>>> (use COPY_ARRAY for copying arrays, 2019-06-15), at least with for me\n>>> (and Coccinelle 1.0.4).\n>>\n>> Would you become keen to find the reasons out for unexpected data processing\n>> results (also by the software combination “Coccinelle 1.0.8-00004-g842075f7”)\n>> at this place?\n>\n> It looks like a bug in Coccinelle to me\n\nWe might stumble also on just another (temporary) software limitation.\n\n\n> and I'd like to see it fixed\n\nWould you like to support corresponding development anyhow?\n\n\n> if that's confirmed, of course.\n\nI am curious if further feedback will evolve for affected software areas.\n\n\n> And I'd like to see Debian pick up a newer version, preferably containing that fix.\n\nI assume that you can wait a long time for progress in the software\ndistribution direction.\n\n\n> But at least until then our semantic patches need to work around it.\n\nWould another concrete fix for the currently discussed SmPL script\nbe better than a “workaround”?\n\n\n>> But this transformation rule can probably be omitted if the usage\n>> of SmPL disjunctions will be increased in a subsequent rule, can't it?\n>\n> Perhaps, but I don't see how. Do you?\n\nObviously, yes (in principle according to my proposal from yesterday).\nhttps://public-inbox.org/git/05ab1110-2115-7886-f890-9983caabc52c@web.de/\n\n\n>> Would you like to use the SmPL code “*( \\( src_ptr \\| src_arr \\) )” instead?\n>\n> That leaves out dst_ptr and dst_arr.\n\nHow many items should finally be filtered in the discussed SmPL disjunction?\n\n\n> And what would it mean to match e.g. this ?\n>\n> \tmemcpy(dst_ptr, src_ptr, n * sizeof(*src_arr))\n\nThe Coccinelle software takes care for commutativity by isomorphisms.\nhttps://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L241\n\n\n> At least the element size would be the same, but I'd rather shy away from\n> transforming weird cases like this automatically.\n\nDo you mean to specify additional restrictions by SmPL code?\n\n\n> void *memmove(void *dest, const void *src, size_t n);\n> void *memcpy(void *dest, const void *src, size_t n);\n>\n> COPY_ARRAY(dst, src, n)\n> MOVE_ARRAY(dst, src, n)\n\nCan the replacement of these functions by macro calls be combined further\nby improved SmPL code?\n\n\n>> Possible nicer run time characteristics by the Coccinelle software.\n>\n> How much faster is it exactly?\n\nThe answer will depend on efforts which you would like to invest\nin corresponding (representative) measurements.\n\n\n> Speedups are good, but I think readability of rules is more important\n> than coccicheck duration.\n\nI hope that a more pleasing balance can be found for the involved\nusability factors.\n\n\n>> But how does the software situation look like if the original source code\n>> would contain coding style issues?\n>\n> The same: Generated code should not add coding style issues.\n\nSuch an expectation is generally nice. - But target conflicts can occur there.\n\n\n> We can still use results that need to be polished, but that's a manual step\n> which reduces the benefits of automation.\n\nI am curious how the software development practice will evolve further.\n\nRegards,\nMarkus\n"},{"id":"386397","messageId":"ac67f805-fbff-68e9-214e-3f353a1c038f@web.de","threadId":"52241","inReplyTo":"f28f5fb8-2814-9df5-faf2-7146ed1a1f4d@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-17T18:36:28Z","receivedAt":"2019-11-17T18:36:32Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> It was chosen to transform source code fragments (pointer expressions)\n>> by two SmPL rules so that the search pattern “sizeof(T)” would work\n>> in the third rule.\n>\n> Ah, right, it would be nice to get rid of those normalization rules,\n> especially the second one.\n\nThanks for such positive feedback.\n\n\n> I don't see how,\n\nWhere is your view too limited at the moment?\n\n\n> though, without either causing a combinatorial explosion\n\nGrowing combinations can become more interesting, can't they?\n\n\n> or loosening up the matching too much.\n\nI hope that we can achieve another reasonably safe transformation\napproach together.\n\nRegards,\nMarkus\n"},{"id":"386456","messageId":"0d9cf772-268d-bd00-1cbb-0bbbec9dfc9a@web.de","threadId":"52241","inReplyTo":"f28f5fb8-2814-9df5-faf2-7146ed1a1f4d@web.de","subject":"[PATCH] coccinelle: improve array.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-18T16:10:33Z","receivedAt":"2019-11-18T16:10:38Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"From: Markus Elfring \nDate: Mon, 18 Nov 2019 17:00:37 +0100\n\nThis script contained transformation rules for the semantic patch language\nwhich used similar code.\n\n1. Delete two SmPL rules which were used to transform source code fragments\n (pointer expressions) so that the search pattern “sizeof(T)” would work\n in the third rule.\n See also the topic “coccinelle: adjustments for array.cocci?”:\n https://public-inbox.org/git/f28f5fb8-2814-9df5-faf2-7146ed1a1f4d@web.de/\n\n2. Combine the remaining rules by using six SmPL disjunctions.\n\n3. Adjust case distinctions and corresponding metavariables so that\n the desired search for update candidates can be more complete.\n\n4. Increase the precision for the specification of required changes.\n\nSigned-off-by: Markus Elfring \n---\n contrib/coccinelle/array.cocci | 100 ++++++---------------------------\n 1 file changed, 18 insertions(+), 82 deletions(-)\n\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex 46b8d2ee11..bcd6ff4793 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -1,90 +1,26 @@\n-@@\n-expression dst, src, n, E;\n-@@\n- memcpy(dst, src, n * sizeof(\n-- E[...]\n-+ *(E)\n- ))\n-\n @@\n type T;\n-T *ptr;\n-T[] arr;\n-expression E, n;\n-@@\n-(\n- memcpy(ptr, E,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n- )\n-|\n- memcpy(arr, E,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n- )\n-|\n- memcpy(E, ptr,\n-- n * sizeof(*(ptr))\n-+ n * sizeof(T)\n- )\n-|\n- memcpy(E, arr,\n-- n * sizeof(*(arr))\n-+ n * sizeof(T)\n- )\n-)\n-\n-@@\n-type T;\n-T *dst_ptr;\n-T *src_ptr;\n-T[] dst_arr;\n T[] src_arr;\n-expression n;\n+expression n, dst_e, src_e;\n+expression* dst_p_e, src_p_e;\n @@\n (\n-- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_ptr, src_ptr, n)\n-|\n-- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_ptr, src_arr, n)\n-|\n-- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_arr, src_ptr, n)\n-|\n-- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n-+ COPY_ARRAY(dst_arr, src_arr, n)\n-)\n-\n-@@\n-type T;\n-T *dst;\n-T *src;\n-expression n;\n-@@\n (\n-- memmove(dst, src, (n) * sizeof(*dst));\n-+ MOVE_ARRAY(dst, src, n);\n-|\n-- memmove(dst, src, (n) * sizeof(*src));\n-+ MOVE_ARRAY(dst, src, n);\n+-memcpy\n++COPY_ARRAY\n |\n-- memmove(dst, src, (n) * sizeof(T));\n-+ MOVE_ARRAY(dst, src, n);\n+-memmove\n++MOVE_ARRAY\n+)\n+ (\n+ dst_e,\n+ src_e\n+- , (n) * \\( sizeof(T) \\| sizeof( \\( *(src_p_e) \\| src_e[...] \\| src_arr \\) ) \\)\n++ , n\n+ )\n+|\n++ALLOC_ARRAY(\n+ dst_p_e\n+- = xmalloc((n) * \\( sizeof( \\( *(src_p_e) \\| src_e[...] \\| src_arr \\) ) \\| sizeof(T) \\))\n++ , n)\n )\n-\n-@@\n-type T;\n-T *ptr;\n-expression n;\n-@@\n-- ptr = xmalloc((n) * sizeof(*ptr));\n-+ ALLOC_ARRAY(ptr, n);\n-\n-@@\n-type T;\n-T *ptr;\n-expression n;\n-@@\n-- ptr = xmalloc((n) * sizeof(T));\n-+ ALLOC_ARRAY(ptr, n);\n--\n2.24.0\n\n"},{"id":"386546","messageId":"321802c9-e5ea-452f-a3fd-7e01ab84b1f9@web.de","threadId":"52241","inReplyTo":"eff19da9-3f9f-0cf0-1e88-64d2acdbabcd@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-19T19:14:54Z","receivedAt":"2019-11-19T19:15:02Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.11.19 um 19:19 schrieb Markus Elfring:\n>> Whitespace is not what makes the above example more complicated than the\n>> equivalent rule below;\n>\n> A different code layout might help in a better understanding for such\n> change specifications.\n>\n>\n>> separating the pieces of simple expressions does.\n>\n> Will there occasionally be a need to change only the required source code parts?\n\nChanging parts that don't need to be changed does not make sense to me.\nWhy do you ask and how does it relate to the example at hand?\n\n>>>> than what we currently have:\n>>>> @@\n>>>> expression dst, src, n, E;\n>>>> @@\n>>>> memcpy(dst, src, n * sizeof(\n>>>> - E[...]\n>>>> + *(E)\n>>>> ))\n>\n> Are any circumstances to consider where only the essential implementation details\n> should be touched by an automatic software transformation?\n\nI don't understand this question.\n\n>> It looks like a bug in Coccinelle to me\n>\n> We might stumble also on just another (temporary) software limitation.\n>\n>\n>> and I'd like to see it fixed\n>\n> Would you like to support corresponding development anyhow?\n\nI don't see me learning OCaml in the near future. Or are you looking\nfor donations? :)\n\n>> But at least until then our semantic patches need to work around it.\n>\n> Would another concrete fix for the currently discussed SmPL script\n> be better than a “workaround”?\n\nThese are different things. Fixes (repairs) are always welcome. But\nthey should not rely on SmPL constructs that only work properly using\nunreleased versions of Coccinelle.\n\n>>> Would you like to use the SmPL code “*( \\( src_ptr \\| src_arr \\) )” instead?\n>>\n>> That leaves out dst_ptr and dst_arr.\n>\n> How many items should finally be filtered in the discussed SmPL disjunction?\n\nLet's see: dst and src can be pointers or array references, which makes\nfour combinations. sizeof could either operate the shared type or on an\nelement of dst or an element of src. An element can be accessed either\nusing dereference (*) or subscript ([]). That makes five possible\nvariations for the sizeof, right? So twenty combinations in total.\n\n>> And what would it mean to match e.g. this ?\n>>\n>> \tmemcpy(dst_ptr, src_ptr, n * sizeof(*src_arr))\n>\n> The Coccinelle software takes care for commutativity by isomorphisms.\n> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L241\n\nOK, but I had a different concern (more below).\n\n>> At least the element size would be the same, but I'd rather shy away from\n>> transforming weird cases like this automatically.\n>\n> Do you mean to specify additional restrictions by SmPL code?\n\nLet's take this silly C fragment as an example:\n\n\tchar *src = strdup(\"foo\");\n\tsize_t src_len = strlen(src);\n\tchar *dst = malloc(src_len);\n\tchar unrelated[17];\n\tmemcpy(dst, src, src_len * sizeof(*unrelated));\n\nMy point is that taking the size of something that is neither source nor\ndestination is weird enough that it should be left alone by semantic\npatches. Matching should be precise enough to avoid false\ntransformations.\n\n>> void *memmove(void *dest, const void *src, size_t n);\n>> void *memcpy(void *dest, const void *src, size_t n);\n>>\n>> COPY_ARRAY(dst, src, n)\n>> MOVE_ARRAY(dst, src, n)\n>\n> Can the replacement of these functions by macro calls be combined further\n> by improved SmPL code?\n\nVery likely.\n\n>>> Possible nicer run time characteristics by the Coccinelle software.\n>>\n>> How much faster is it exactly?\n>\n> The answer will depend on efforts which you would like to invest\n> in corresponding (representative) measurements.\n\nIs that some kind of quantum effect? ;-)\n\nWhen I try to convince people to apply a patch that is intended to\nspeed up something, I often use https://github.com/sharkdp/hyperfine\nthese days.\n\nRené\n"},{"id":"386547","messageId":"9146ab06-944e-307b-b4af-9b0ffbb323f3@web.de","threadId":"52241","inReplyTo":"ac67f805-fbff-68e9-214e-3f353a1c038f@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-19T19:15:01Z","receivedAt":"2019-11-19T19:15:07Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.11.19 um 19:36 schrieb Markus Elfring:\n>>> It was chosen to transform source code fragments (pointer expressions)\n>>> by two SmPL rules so that the search pattern “sizeof(T)” would work\n>>> in the third rule.\n>>\n>> Ah, right, it would be nice to get rid of those normalization rules,\n>> especially the second one.\n>\n> Thanks for such positive feedback.\n>\n>\n>> I don't see how,\n>\n> Where is your view too limited at the moment?\n\nI don't know. Or perhaps it's rather too wide and I worry about\nirrelevant details?\n\n>> though, without either causing a combinatorial explosion\n>\n> Growing combinations can become more interesting, can't they?\n\nPerhaps, but not if they blow up the size of the semantic patch or\nthe runtime of Coccinelle.\n\nRené\n"},{"id":"386548","messageId":"d291ec11-c0f3-2918-193d-49fcbd65a18e@web.de","threadId":"52241","inReplyTo":"0d9cf772-268d-bd00-1cbb-0bbbec9dfc9a@web.de","subject":"Re: [PATCH] coccinelle: improve array.cocci","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-19T19:15:05Z","receivedAt":"2019-11-19T19:15:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\nAm 18.11.19 um 17:10 schrieb Markus Elfring:\n> From: Markus Elfring \n> Date: Mon, 18 Nov 2019 17:00:37 +0100\n>\n> This script contained transformation rules for the semantic patch language\n> which used similar code.\n>\n> 1. Delete two SmPL rules which were used to transform source code fragments\n> (pointer expressions) so that the search pattern “sizeof(T)” would work\n> in the third rule.\n> See also the topic “coccinelle: adjustments for array.cocci?”:\n> https://public-inbox.org/git/f28f5fb8-2814-9df5-faf2-7146ed1a1f4d@web.de/\n>\n> 2. Combine the remaining rules by using six SmPL disjunctions.\n>\n> 3. Adjust case distinctions and corresponding metavariables so that\n> the desired search for update candidates can be more complete.\n>\n> 4. Increase the precision for the specification of required changes.\n>\n> Signed-off-by: Markus Elfring \n> ---\n> contrib/coccinelle/array.cocci | 100 ++++++---------------------------\n> 1 file changed, 18 insertions(+), 82 deletions(-)\n\nThe diff is hard to read, so here's the resulting semantic patch:\n\n-- start --\n@@\ntype T;\nT[] src_arr;\nexpression n, dst_e, src_e;\nexpression* dst_p_e, src_p_e;\n@@\n(\n(\n-memcpy\n+COPY_ARRAY\n|\n-memmove\n+MOVE_ARRAY\n)\n (\n dst_e,\n src_e\n- , (n) * \\( sizeof(T) \\| sizeof( \\( *(src_p_e) \\| src_e[...] \\| src_arr \\) ) \\)\n+ , n\n )\n|\n+ALLOC_ARRAY(\n dst_p_e\n- = xmalloc((n) * \\( sizeof( \\( *(src_p_e) \\| src_e[...] \\| src_arr \\) ) \\| sizeof(T) \\))\n+ , n)\n)\n-- end --\n\nI like that COPY_ARRAY and MOVE_ARRAY are handled in the same rule,\nas they share the same parameters and do the same -- except that\nthe latter handles overlaps, while the former may be a bit faster.\n\nAnd I like that it's short.\n\nI don't like that ALLOC_ARRAY is handled in the same rule, as it is\nquite different from the other two macros.\n\nCoccinelle needs significantly longer to apply the new version.\nHere are times for master:\n\nBenchmark #1: make contrib/coccinelle/array.cocci.patch\n Time (mean ± σ): 19.314 s ± 0.200 s [User: 19.065 s, System: 0.224 s]\n Range (min … max): 19.009 s … 19.718 s 10 runs\n\n... and here with the patch applied:\n\nBenchmark #1: make contrib/coccinelle/array.cocci.patch\n Time (mean ± σ): 43.420 s ± 0.490 s [User: 43.087 s, System: 0.273 s]\n Range (min … max): 42.636 s … 44.359 s 10 runs\n\nThe current version checks if source and destination are of the same type,\nand whether the sizeof operand is either said type or an element of source\nor destination. The new one does not. So I don't see claim 4 (\"Increase\nthe precision\") fulfilled, quite the opposite rather. It can produce e.g.\na transformation like this:\n\n void f(int *dst, char *src, size_t n)\n {\n-\tmemcpy(dst, src, n * sizeof(short));\n+\tCOPY_ARRAY(dst, src, n);\n }\n\nThe COPY_ARRAY there effectively expands to:\n\n\tmemcpy(dst, src, n * sizeof(*dst));\n\n... which is quite different -- if short is 2 bytes wide and int 4 bytes\nthen we copy twice as many bytes as before.\n\nI think an automatic transformation should only be generated if it is\nsafe. It's hard to spot a weird case in a generated patch amid ten\nwell-behaving ones.\n\n>\n> diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\n> index 46b8d2ee11..bcd6ff4793 100644\n> --- a/contrib/coccinelle/array.cocci\n> +++ b/contrib/coccinelle/array.cocci\n> @@ -1,90 +1,26 @@\n> -@@\n> -expression dst, src, n, E;\n> -@@\n> - memcpy(dst, src, n * sizeof(\n> -- E[...]\n> -+ *(E)\n> - ))\n> -\n> @@\n> type T;\n> -T *ptr;\n> -T[] arr;\n> -expression E, n;\n> -@@\n> -(\n> - memcpy(ptr, E,\n> -- n * sizeof(*(ptr))\n> -+ n * sizeof(T)\n> - )\n> -|\n> - memcpy(arr, E,\n> -- n * sizeof(*(arr))\n> -+ n * sizeof(T)\n> - )\n> -|\n> - memcpy(E, ptr,\n> -- n * sizeof(*(ptr))\n> -+ n * sizeof(T)\n> - )\n> -|\n> - memcpy(E, arr,\n> -- n * sizeof(*(arr))\n> -+ n * sizeof(T)\n> - )\n> -)\n> -\n> -@@\n> -type T;\n> -T *dst_ptr;\n> -T *src_ptr;\n> -T[] dst_arr;\n> T[] src_arr;\n> -expression n;\n> +expression n, dst_e, src_e;\n> +expression* dst_p_e, src_p_e;\n> @@\n> (\n> -- memcpy(dst_ptr, src_ptr, (n) * sizeof(T))\n> -+ COPY_ARRAY(dst_ptr, src_ptr, n)\n> -|\n> -- memcpy(dst_ptr, src_arr, (n) * sizeof(T))\n> -+ COPY_ARRAY(dst_ptr, src_arr, n)\n> -|\n> -- memcpy(dst_arr, src_ptr, (n) * sizeof(T))\n> -+ COPY_ARRAY(dst_arr, src_ptr, n)\n> -|\n> -- memcpy(dst_arr, src_arr, (n) * sizeof(T))\n> -+ COPY_ARRAY(dst_arr, src_arr, n)\n> -)\n> -\n> -@@\n> -type T;\n> -T *dst;\n> -T *src;\n> -expression n;\n> -@@\n> (\n> -- memmove(dst, src, (n) * sizeof(*dst));\n> -+ MOVE_ARRAY(dst, src, n);\n> -|\n> -- memmove(dst, src, (n) * sizeof(*src));\n> -+ MOVE_ARRAY(dst, src, n);\n> +-memcpy\n> ++COPY_ARRAY\n> |\n> -- memmove(dst, src, (n) * sizeof(T));\n> -+ MOVE_ARRAY(dst, src, n);\n> +-memmove\n> ++MOVE_ARRAY\n> +)\n> + (\n> + dst_e,\n> + src_e\n> +- , (n) * \\( sizeof(T) \\| sizeof( \\( *(src_p_e) \\| src_e[...] \\| src_arr \\) ) \\)\n> ++ , n\n> + )\n> +|\n> ++ALLOC_ARRAY(\n> + dst_p_e\n> +- = xmalloc((n) * \\( sizeof( \\( *(src_p_e) \\| src_e[...] \\| src_arr \\) ) \\| sizeof(T) \\))\n> ++ , n)\n> )\n> -\n> -@@\n> -type T;\n> -T *ptr;\n> -expression n;\n> -@@\n> -- ptr = xmalloc((n) * sizeof(*ptr));\n> -+ ALLOC_ARRAY(ptr, n);\n> -\n> -@@\n> -type T;\n> -T *ptr;\n> -expression n;\n> -@@\n> -- ptr = xmalloc((n) * sizeof(T));\n> -+ ALLOC_ARRAY(ptr, n);\n> --\n> 2.24.0\n>\n"},{"id":"386549","messageId":"a4a882eb-5e0d-dbcf-fd01-9d5831c4a8e6@web.de","threadId":"52241","inReplyTo":"321802c9-e5ea-452f-a3fd-7e01ab84b1f9@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-19T20:21:40Z","receivedAt":"2019-11-19T20:21:53Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> Will there occasionally be a need to change only the required source code parts?\n>\n> Changing parts that don't need to be changed does not make sense to me.\n> Why do you ask and how does it relate to the example at hand?\n\nHow does such feedback fit to the discussed SmPL change specification?\n\n- E[...]\n+ *(E)\n\n\n>> Would you like to support corresponding development anyhow?\n>\n> I don't see me learning OCaml in the near future.\n\nThis can be fine.\n\n\n> Or are you looking for donations?\n\nOther (software) projects can benefit also from additional resources,\ncan't they?\n\nRegards,\nMarkus\n"},{"id":"386622","messageId":"d053612d-107b-fdb2-b722-6455ef068239@web.de","threadId":"52241","inReplyTo":"d291ec11-c0f3-2918-193d-49fcbd65a18e@web.de","subject":"Re: coccinelle: improve array.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-20T09:01:04Z","receivedAt":"2019-11-20T09:01:16Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> I like that COPY_ARRAY and MOVE_ARRAY are handled in the same rule,\n> as they share the same parameters and do the same -- except that\n> the latter handles overlaps, while the former may be a bit faster.\n>\n> And I like that it's short.\n\nThanks for such positive feedback after our growing discussion.\n\n\n> I don't like that ALLOC_ARRAY is handled in the same rule, as it is\n> quite different from the other two macros.\n\nThis case distinction can share a few metavariables with the other\ntransformation approach, can't it?\n\n\n> Coccinelle needs significantly longer to apply the new version.\n\nThis can happen because of a more complete source code search pattern,\ncan't it?\n\nThe data processing can benefit from parallelisation (if desired.)\nhttps://github.com/coccinelle/coccinelle/blob/66a1118e04a6aaf1acdae89623313c8e05158a8d/docs/manual/spatch_options.tex#L745\n\n\n> Here are times for master:\n\nThe SmPL script execution times can be analysed also directly with\nthe help of the Coccinelle software by profiling functionality.\nhttps://github.com/coccinelle/coccinelle/blob/66a1118e04a6aaf1acdae89623313c8e05158a8d/docs/manual/spatch_options.tex#L736\n\n\n> ... and here with the patch applied:\n>\n> Benchmark #1: make contrib/coccinelle/array.cocci.patch\n> Time (mean ± σ): 43.420 s ± 0.490 s [User: 43.087 s, System: 0.273 s]\n\nI got an other distribution of run times on my test system.\n\n\n> The current version checks if source and destination are of the same type,\n> and whether the sizeof operand is either said type or an element of source\n> or destination.\n\nThe specification of metavariables for pointer types has got some consequences.\n\n\n> The new one does not.\n\nI suggest to use a search for (pointer) expressions instead.\nThis approach can trigger other consequences then.\n\n\n> So I don't see claim 4 (\"Increase the precision\") fulfilled,\n\nI tried to express an adjustment on the change granularity by the plus\nand minus characters at the beginning of the lines in the semantic patch.\n\nThe SmPL disjunctions provide also more common functionality now.\n\n\n> quite the opposite rather.\n\nThe search for compatible pointers can become even more challenging.\n\n\n> I think an automatic transformation should only be generated if it is safe.\n\nDifferent expectations can occur around safety and change convenience.\n\nWould you eventually work with SmPL script variants in parallel according\nto different confidence settings?\n\n\n> It's hard to spot a weird case in a generated patch amid ten\n> well-behaving ones.\n\nI can follow also this development concern to some degree.\n\nRegards,\nMarkus\n"},{"id":"386739","messageId":"d4a5d22a-7ea3-641c-c502-fc99ce194f2a@web.de","threadId":"52241","inReplyTo":"a4a882eb-5e0d-dbcf-fd01-9d5831c4a8e6@web.de","subject":"Re: coccinelle: adjustments for array.cocci?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-21T19:01:54Z","receivedAt":"2019-11-21T19:02:03Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.11.19 um 21:21 schrieb Markus Elfring:\n>>> Will there occasionally be a need to change only the required source code parts?\n>>\n>> Changing parts that don't need to be changed does not make sense to me.\n>> Why do you ask and how does it relate to the example at hand?\n>\n> How does such feedback fit to the discussed SmPL change specification?\n>\n> - E[...]\n> + *(E)\n\nThe cited fragment is from a rule that normalizes references to array\nelements which are fed to sizeof. It reduces the number of combinations\nto consider in the rules following it, but it's not in itself a change\nwe'd want to apply.\n\nThe next helper rule turns sizeof operating on array elements to sizeof\non specific types. That one is much uglier, as it removes the\ninformation from whence the inferred type came.\n\nIn practice none of these helpers transformed any code that wasn't\nmatched by the final rule for using COPY_ARRAY. It would be nice to get\nrid of them nevertheless, to rule out such side-effects. I just don't\nsee a practical way to make do without them, though.\n\nRené\n"},{"id":"386740","messageId":"4f55b06b-35f3-da06-ae86-8a4068f78027@web.de","threadId":"52241","inReplyTo":"d053612d-107b-fdb2-b722-6455ef068239@web.de","subject":"Re: coccinelle: improve array.cocci","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-21T19:02:05Z","receivedAt":"2019-11-21T19:02:08Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.11.19 um 10:01 schrieb Markus Elfring:\n>> I don't like that ALLOC_ARRAY is handled in the same rule, as it is\n>> quite different from the other two macros.\n>\n> This case distinction can share a few metavariables with the other\n> transformation approach, can't it?\n\nCan it can, but should it? In my opinion it should not; separate\nconcerns should get their own rules. That's easier to manage for\ndevelopers. I suspect it's also easier for Coccinelle to evaluate,\nbut didn't check.\n\n>> Coccinelle needs significantly longer to apply the new version.\n>\n> This can happen because of a more complete source code search pattern,\n> can't it?\n\nPerhaps.\n\n> The data processing can benefit from parallelisation (if desired.)\n> https://github.com/coccinelle/coccinelle/blob/66a1118e04a6aaf1acdae89623313c8e05158a8d/docs/manual/spatch_options.tex#L745\n\nRight. I use MAKEFLAGS += -j6, which runs six spatch instances in\nparallel for the coccicheck make target of Git instead.\n\n>> Here are times for master:\n>\n> The SmPL script execution times can be analysed also directly with\n> the help of the Coccinelle software by profiling functionality.\n> https://github.com/coccinelle/coccinelle/blob/66a1118e04a6aaf1acdae89623313c8e05158a8d/docs/manual/spatch_options.tex#L736\n\nOK, so --profile allows to analyze in which of its parts Coccinelle\nspends the extra time.\n\n>> The current version checks if source and destination are of the same type,\n>> and whether the sizeof operand is either said type or an element of source\n>> or destination.\n>\n> The specification of metavariables for pointer types has got some consequences.\n>\n>\n>> The new one does not.\n>\n> I suggest to use a search for (pointer) expressions instead.\n> This approach can trigger other consequences then.\n\nWhy don't we need to check the type?\n\n>> So I don't see claim 4 (\"Increase the precision\") fulfilled,\n>\n> I tried to express an adjustment on the change granularity by the plus\n> and minus characters at the beginning of the lines in the semantic patch.\n\nHmm, to me \"precision\" means to transform exactly those cases that are\nintended to be transformed, i.e. to avoid false positives and negatives.\nWhat you seem to mean here I'd rather describe as \"reduce duplication\".\n\n> The SmPL disjunctions provide also more common functionality now.\n>\n>\n>> quite the opposite rather.\n>\n> The search for compatible pointers can become even more challenging.\n\nIt's what we currently have, in an a clunky way.\n\n>> I think an automatic transformation should only be generated if it is safe.\n>\n> Different expectations can occur around safety and change convenience.\n>\n> Would you eventually work with SmPL script variants in parallel according\n> to different confidence settings?\n\nMe? No. If I can't trust automatic transformations then I don't want\nthem. I can already generate bugs fast enough manually, thank you\nvery much. :)\n\nRené\n"},{"id":"386741","messageId":"06ff24b6-f154-9ec6-7b22-05b0ea664a36@web.de","threadId":"52241","inReplyTo":"4f55b06b-35f3-da06-ae86-8a4068f78027@web.de","subject":"Re: coccinelle: improve array.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-21T19:44:12Z","receivedAt":"2019-11-21T19:44:22Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> This case distinction can share a few metavariables with the other\n>> transformation approach, can't it?\n>\n> Can it can, but should it? In my opinion it should not;\n\nI presented a software design in an other direction.\nSome data processing approaches can benefit from sharing common information.\n\n\n> separate concerns should get their own rules.\n\nStrict separation triggers corresponding consequences.\n\n\n> That's easier to manage for developers.\n\nThis view can be reasonable.\n\n\n> I suspect it's also easier for Coccinelle to evaluate, but didn't check.\n\nI find such an assumption questionable.\n\n\n> I use MAKEFLAGS += -j6, which runs six spatch instances in\n> parallel for the coccicheck make target of Git instead.\n\nThe program “spatch” supports parallelisation also directly by the parameter “--jobs”.\nDid you try it out occasionally?\n\n\n> OK, so --profile allows to analyze in which of its parts Coccinelle\n> spends the extra time.\n\nSome information about time distribution will be displayed.\n\n\n>> I suggest to use a search for (pointer) expressions instead.\n>> This approach can trigger other consequences then.\n>\n> Why don't we need to check the type?\n\nI got the impression that we stumble on a general challenge for generic\nsource code searches.\nHow many efforts would we like to invest in solving type safety issues?\n\n\n>> Would you eventually work with SmPL script variants in parallel according\n>> to different confidence settings?\n>\n> Me? No.\n\nSuch a view can be fine.\n\nBut I am also still trying to improve various implementation details\ndespite of known software limitations.\n\n\n> If I can't trust automatic transformations then I don't want them.\n\nI need to live with compromises together also with current development tools.\n\n\n> I can already generate bugs fast enough manually, thank you very much. :)\n\nThis is usual.\n\nI hope that specific tools can make our lives occasionally a bit easier.\n\nRegards,\nMarkus\n"},{"id":"386798","messageId":"xmqqsgmg5uck.fsf@gitster-ct.c.googlers.com","threadId":"52241","inReplyTo":"d291ec11-c0f3-2918-193d-49fcbd65a18e@web.de","subject":"Re: [PATCH] coccinelle: improve array.cocci","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T05:54:35Z","receivedAt":"2019-11-22T05:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The current version checks if source and destination are of the same type,\n> and whether the sizeof operand is either said type or an element of source\n> or destination. The new one does not. So I don't see claim 4 (\"Increase\n> the precision\") fulfilled, quite the opposite rather. It can produce e.g.\n> a transformation like this:\n>\n> void f(int *dst, char *src, size_t n)\n> {\n> -\tmemcpy(dst, src, n * sizeof(short));\n> +\tCOPY_ARRAY(dst, src, n);\n> }\n>\n> The COPY_ARRAY there effectively expands to:\n>\n> \tmemcpy(dst, src, n * sizeof(*dst));\n>\n> ... which is quite different -- if short is 2 bytes wide and int 4 bytes\n> then we copy twice as many bytes as before.\n>\n> I think an automatic transformation should only be generated if it is\n> safe. It's hard to spot a weird case in a generated patch amid ten\n> well-behaving ones.\n\nNicely said; I agree 100% with you that the priority of this project\nis to use these *.cocci transformations in such a way that they are\nabsolutely safe---so that humans do not have to spend time sifting\nthe result through to find accidental bad transformations.\n\nAnd thanks for taking time to very clearly explain why the proposed\nrewrite is not something we want to take.\n"},{"id":"386807","messageId":"ac5a0968-734b-6395-c1d3-7267370d2286@web.de","threadId":"52241","inReplyTo":"xmqqsgmg5uck.fsf@gitster-ct.c.googlers.com","subject":"Re: coccinelle: improve array.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-22T07:34:20Z","receivedAt":"2019-11-22T07:34:36Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Nicely said; I agree 100% with you that the priority of this project\n> is to use these *.cocci transformations in such a way that they are\n> absolutely safe\n\nSuch a goal can be generally desirable.\n\nBut I got the impression that there are target conflicts to consider\nfor the currently discussed SmPL script.\nThe available transformation approaches show different open issues,\ndon't they?\n\nYour desire is easier to fulfil for other change patterns.\n\n\n> ---so that humans do not have to spend time sifting the result through\n> to find accidental bad transformations.\n\nAutomatic source code analysis contains the usual risk for false positives.\nHow many efforts would we like to invest in improving corresponding\nsoftware solutions?\n\n\n> And thanks for taking time to very clearly explain why the proposed\n> rewrite is not something we want to take.\n\nWould you like to check once more if additional update candidates\nwill be found in the source files with the presented SmPL script variant?\n\nRegards,\nMarkus\n"},{"id":"386831","messageId":"20191122152950.GZ23183@szeder.dev","threadId":"52241","inReplyTo":"06ff24b6-f154-9ec6-7b22-05b0ea664a36@web.de","subject":"Re: coccinelle: improve array.cocci","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-22T15:29:50Z","receivedAt":"2019-11-22T15:29:58Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Nov 21, 2019 at 08:44:12PM +0100, Markus Elfring wrote:\n> The program “spatch” supports parallelisation also directly by the parameter “--jobs”.\n> Did you try it out occasionally?\n\nI did try --jobs on a couple of occasions, and the results always\nvaried between broken, not working, or downright making things even\nslower.\n\n\n $ spatch --version\n spatch version 1.0.4 with Python support and with PCRE support\n $ spatch --sp-file contrib/coccinelle/array.cocci --all-includes --patch . --jobs 2 alias.c alloc.c\n init_defs_builtins: /usr/lib/coccinelle/standard.h\n HANDLING: alias.c alloc.c\n Fatal error: exception Sys_error(\"array: No such file or directory\")\n\nThis issue seems to be fixed in later versions, but this is the\nversion what many distros still ship and what is used in our CI\nbuilds, so we do care about 1.0.4.\n\n\n $ spatch --version\n spatch version 1.0.8 compiled with OCaml version 4.05.0\n Flags passed to the configure script: [none]\n OCaml scripting support: yes\n Python scripting support: yes\n Syntax of regular expressions: PCRE\n $ /usr/bin/time --format='%e | %M' make contrib/coccinelle/array.cocci.patch\n SPATCH contrib/coccinelle/array.cocci\n 102.06 | 129084\n\nOur Makefile recipes run Coccinelle in a sequential loop, one 'spatch'\ninvocation for each source file by default. Therefore, merely passing\nin '--jobs ' doesn't bring any runtime benefits:\n\n $ /usr/bin/time --format='%e | %M' make SPATCH_FLAGS='--all-includes --patch . --jobs 8' contrib/coccinelle/array.cocci.patch\n SPATCH contrib/coccinelle/array.cocci\n 105.31 | 118512\n\nSome time ago we found that invoking 'spatch' with multiple files at\nonce does bring notable speedup (with 1.0.4), although at the cost of\ndrastically increased memory footprint, see commit 960154b9c1\n(coccicheck: optionally batch spatch invocations, 2019-05-06). Alas,\ntrying to use that in the hope that 'spatch' can do more in parallel\nif it has more files to process at once doesn't bring any runtime\nbenefits, either:\n\n $ /usr/bin/time --format='%e | %M' make SPATCH_FLAGS='--all-includes --patch . --jobs 8' SPATCH_BATCH_SIZE=8 contrib/coccinelle/array.cocci.patch\n SPATCH contrib/coccinelle/array.cocci\n 116.27 | 349964\n\nAnd by further increasing the batch size it just gets notably slower;\nalso note the order of magnitude higher max memory usage:\n\n $ /usr/bin/time --format='%e | %M' make SPATCH_FLAGS='--all-includes --patch . --jobs 8' SPATCH_BATCH_SIZE=32 contrib/coccinelle/array.cocci.patch\n SPATCH contrib/coccinelle/array.cocci\n 197.70 | 1205784\n\nIt appears that batching 'spatch' invocations with 1.0.8 does not\nbring the same benefits as with 1.0.4, but brings slowdowns instead...\n\nAnyway, looking at 'ps u -L' output it appears that 'spatch' doesn't\nreally do any parallel work, and there are only two 'spatch' processes\nand no threads despite '--jobs 8':\n\n szeder 2561 0.4 0.5 36944 21520 pts/0 S+ 15:31 0:00 spatch\n szeder 2567 97.1 30.5 1228372 1205332 pts/0 R+ 15:31 0:29 spatch\n\n\nNote that 1.0.8 above was run in a Docker container, while 1.0.4 on\nthe host. This may or may not have influenced the runtimes reported\nabove. FWIW, 'make -j4 coccicheck' parallelizes just fine even in the\ncontainer and with 1.0.8.\n\n\nA different approach relying on 'make -j' to parallelize 'spatch'\ninvocations was discussed here:\n\n https://public-inbox.org/git/20180802115522.16107-1-szeder.dev@gmail.com/T/#u\n\n"},{"id":"386832","messageId":"c9cc76dc-615f-ff89-6305-64d897734a4a@web.de","threadId":"52241","inReplyTo":"20191122152950.GZ23183@szeder.dev","subject":"Re: coccinelle: improve array.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-22T16:17:56Z","receivedAt":"2019-11-22T16:18:08Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> I did try --jobs on a couple of occasions,\n\nThanks for your feedback.\n\n\n> and the results always varied between broken, not working,\n\nThe parallelisation support by the Coccinelle software was questionable\nfor a while.\n\n\n> or downright making things even slower.\n\nI wonder about this information.\n\n\n…\n> spatch version 1.0.4 with Python support and with PCRE support\n…\n> Fatal error: exception Sys_error(\"array: No such file or directory\")\n\nAnother bit of background information:\n* https://lore.kernel.org/cocci/99082e9d-8047-eee3-68dd-9849868d4a96@users.sourceforge.net/\n https://systeme.lip6.fr/pipermail/cocci/2016-August/003546.html\n\n* https://lore.kernel.org/cocci/alpine.DEB.2.10.1506171707190.2578@hadrien/\n https://systeme.lip6.fr/pipermail/cocci/2015-June/002141.html\n\n\n…\n> Therefore, merely passing in '--jobs ' doesn't bring any runtime benefits:\n\nThis program parameter should be used for other command variants.\n\n…\n> It appears that batching 'spatch' invocations with 1.0.8 does not\n> bring the same benefits as with 1.0.4, but brings slowdowns instead...\n\nWould you like to share your experiences also on the Coccinelle mailing list?\n\nRegards,\nMarkus\n"},{"id":"390453","messageId":"df8abb18-ed59-aed2-82a2-e410a181233b@web.de","threadId":"52241","inReplyTo":"0d9cf772-268d-bd00-1cbb-0bbbec9dfc9a@web.de","subject":"Re: coccinelle: improve array.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2020-01-25T08:23:36Z","receivedAt":"2020-01-25T08:23:55Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> This script contained transformation rules for the semantic patch language\n> which used similar code.\n\nHow do you think about to adjust any implementation details according to\nthe shown proposal?\n\nWould you like to continue corresponding code review?\n\nRegards,\nMarkus\n"}]}