{"thread":{"id":"48382","subject":"[PATCH] coccinelle: avoid wrong transformation suggestions from commit.cocci","startedAt":"2018-04-30T09:32:25Z","lastAt":"2018-04-30T11:38:40Z","messageCount":2,"participants":["SZEDER Gábor","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"346129","messageId":"20180430093153.13040-1-szeder.dev@gmail.com","threadId":"48382","inReplyTo":null,"subject":"[PATCH] coccinelle: avoid wrong transformation suggestions from commit.cocci","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-30T09:31:53Z","receivedAt":"2018-04-30T09:32:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The semantic patch 'contrib/coccinelle/commit.cocci' added in\n2e27bd7731 (treewide: replace maybe_tree with accessor methods,\n2018-04-06) is supposed to \"ensure that all references to the\n'maybe_tree' member of struct commit are either mutations or accesses\nthrough get_commit_tree()\".  So get_commit_tree() clearly must be able\nto directly access the 'maybe_tree' member, and 'commit.cocci' has a\nbit of a roundabout workaround to ensure that get_commit_tree()'s\ndirect access in its return statement is not transformed: after all\nreferences to 'maybe_tree' have been transformed to a call to\nget_commit_tree(), including the reference in get_commit_tree()\nitself, the last rule transforms back a 'return get_commit_tree()'\nstatement, back then found only in get_commit_tree() itself, to a\ndirect access.\n\nUnfortunately, already the very next commit shows that this workaround\nis insufficient: 7b8a21dba1 (commit-graph: lazy-load trees for\ncommits, 2018-04-06) extends get_commit_tree() with a condition\ndirectly accessing the 'maybe_tree' member, and Coccinelle with\n'commit.cocci' promptly detects it and suggests a transformation to\navoid it.  This transformation is clearly wrong, because calling\nget_commit_tree() to access 'maybe_tree' _in_ get_commit_tree() would\nobviously lead to recursion.  Furthermore, the same commit added\nanother, more specialized getter function get_commit_tree_in_graph(),\nwhose legitimate direct access to 'maybe_tree' triggers a similar\nwrong transformation suggestion.\n\nExclude both of these getter functions from the general rule in\n'commit.cocci' that matches their direct accesses to 'maybe_tree'.\nAlso exclude load_tree_for_commit(), which, as static helper funcion\nof get_commit_tree_in_graph(), has legitimate direct access to\n'maybe_tree' as well.\n\nThe last rule transforming back 'return get_commit_tree()' statements\nto direct accesses thus became unnecessary, remove it.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n contrib/coccinelle/commit.cocci | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci\nindex ac38525941..a7e9215ffc 100644\n--- a/contrib/coccinelle/commit.cocci\n+++ b/contrib/coccinelle/commit.cocci\n@@ -10,11 +10,15 @@ expression c;\n - c->maybe_tree->object.oid.hash\n + get_commit_tree_oid(c)->hash\n \n+// These excluded functions must access c->maybe_tree direcly.\n @@\n+identifier f !~ \"^(get_commit_tree|get_commit_tree_in_graph|load_tree_for_commit)$\";\n expression c;\n @@\n+  f(...) {...\n - c->maybe_tree\n + get_commit_tree(c)\n+  ...}\n \n @@\n expression c;\n@@ -22,9 +26,3 @@ expression s;\n @@\n - get_commit_tree(c) = s\n + c->maybe_tree = s\n-\n-@@\n-expression c;\n-@@\n-- return get_commit_tree(c);\n-+ return c->maybe_tree;\n-- \n2.17.0.551.g86756ed296\n\n"},{"id":"346140","messageId":"82a385c3-62fe-e471-73ae-0c0448640f3b@gmail.com","threadId":"48382","inReplyTo":"20180430093153.13040-1-szeder.dev@gmail.com","subject":"Re: [PATCH] coccinelle: avoid wrong transformation suggestions from commit.cocci","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-04-30T11:38:34Z","receivedAt":"2018-04-30T11:38:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/30/2018 5:31 AM, SZEDER Gábor wrote:\n> The semantic patch 'contrib/coccinelle/commit.cocci' added in\n> 2e27bd7731 (treewide: replace maybe_tree with accessor methods,\n> 2018-04-06) is supposed to \"ensure that all references to the\n> 'maybe_tree' member of struct commit are either mutations or accesses\n> through get_commit_tree()\".  So get_commit_tree() clearly must be able\n> to directly access the 'maybe_tree' member, and 'commit.cocci' has a\n> bit of a roundabout workaround to ensure that get_commit_tree()'s\n> direct access in its return statement is not transformed: after all\n> references to 'maybe_tree' have been transformed to a call to\n> get_commit_tree(), including the reference in get_commit_tree()\n> itself, the last rule transforms back a 'return get_commit_tree()'\n> statement, back then found only in get_commit_tree() itself, to a\n> direct access.\n>\n> Unfortunately, already the very next commit shows that this workaround\n> is insufficient: 7b8a21dba1 (commit-graph: lazy-load trees for\n> commits, 2018-04-06) extends get_commit_tree() with a condition\n> directly accessing the 'maybe_tree' member, and Coccinelle with\n> 'commit.cocci' promptly detects it and suggests a transformation to\n> avoid it.  This transformation is clearly wrong, because calling\n> get_commit_tree() to access 'maybe_tree' _in_ get_commit_tree() would\n> obviously lead to recursion.  Furthermore, the same commit added\n> another, more specialized getter function get_commit_tree_in_graph(),\n> whose legitimate direct access to 'maybe_tree' triggers a similar\n> wrong transformation suggestion.\n\nThanks for catching this, Szeder. Sorry for the noise.\n\n> Exclude both of these getter functions from the general rule in\n> 'commit.cocci' that matches their direct accesses to 'maybe_tree'.\n> Also exclude load_tree_for_commit(), which, as static helper funcion\n> of get_commit_tree_in_graph(), has legitimate direct access to\n> 'maybe_tree' as well.\n\nThis is an interesting feature of Coccinelle. Happy to learn it.\n\n> The last rule transforming back 'return get_commit_tree()' statements\n> to direct accesses thus became unnecessary, remove it.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n\nI applied this locally on 'next' and ran the check. I succeeded with no \nchanges.\n\nThanks!\n\nReviewed-by: Derrick Stolee <dstolee@microsoft.com>\n\n\n> ---\n>   contrib/coccinelle/commit.cocci | 10 ++++------\n>   1 file changed, 4 insertions(+), 6 deletions(-)\n>\n> diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci\n> index ac38525941..a7e9215ffc 100644\n> --- a/contrib/coccinelle/commit.cocci\n> +++ b/contrib/coccinelle/commit.cocci\n> @@ -10,11 +10,15 @@ expression c;\n>   - c->maybe_tree->object.oid.hash\n>   + get_commit_tree_oid(c)->hash\n>   \n> +// These excluded functions must access c->maybe_tree direcly.\n>   @@\n> +identifier f !~ \"^(get_commit_tree|get_commit_tree_in_graph|load_tree_for_commit)$\";\n>   expression c;\n>   @@\n> +  f(...) {...\n>   - c->maybe_tree\n>   + get_commit_tree(c)\n> +  ...}\n>   \n>   @@\n>   expression c;\n> @@ -22,9 +26,3 @@ expression s;\n>   @@\n>   - get_commit_tree(c) = s\n>   + c->maybe_tree = s\n> -\n> -@@\n> -expression c;\n> -@@\n> -- return get_commit_tree(c);\n> -+ return c->maybe_tree;\n\n"}]}