From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
Derrick Stolee <stolee@gmail.com>,
Jacob Keller <jacob.keller@gmail.com>
Subject: [PATCH 1/9] coccinelle: use <...> for function exclusion
Date: Sat, 25 Aug 2018 04:03:46 -0400 [thread overview]
Message-ID: <20180825080346.GA737@sigill.intra.peff.net> (raw)
In-Reply-To: <20180825080031.GA32139@sigill.intra.peff.net>
Sometimes we want to suppress a coccinelle transformation
inside a particular function. For example, in finding
conversions of hashcmp() to oidcmp(), we should not convert
the call in oidcmp() itself, since that would cause infinite
recursion. We write that like this:
@@
identifier f != oidcmp;
expression E1, E2;
@@
f(...) {...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
...}
to match the interior of any function _except_ oidcmp().
Unfortunately, this doesn't catch all cases (e.g., the one
in sequencer.c that this patch fixes). The problem, as
explained by one of the Coccinelle developers in [1], is:
For transformation, A ... B requires that B occur on every
execution path starting with A, unless that execution path
ends up in error handling code. (eg, if (...) { ...
return; }). Here your A is the start of the function. So
you need a call to hashcmp on every path through the
function, which fails when you add ifs.
[...]
Another issue with A ... B is that by default A and B
should not appear in the matched region. So your original
rule matches only the case where every execution path
contains exactly one call to hashcmp, not more than one.
One way to solve this is to put the pattern inside an
angle-bracket pattern like "<... P ...>", which allows zero
or more matches of P. That works (and is what this patch
does), but it has one drawback: it matches more than we care
about, and Coccinelle uses extra CPU. Here are timings for
"make coccicheck" before and after this patch:
[before]
real 1m27.122s
user 7m34.451s
sys 0m37.330s
[after]
real 2m18.040s
user 10m58.310s
sys 0m41.549s
That's not ideal, but it's more important for this to be
correct than to be fast. And coccicheck is already fairly
slow (and people don't run it for every single patch). So
it's an acceptable tradeoff.
There _is_ a better way to do it, which is to record the
position at which we find hashcmp(), and then check it
against the forbidden function list. Like:
@@
position p : script:python() { p[0].current_element != "oidcmp" };
expression E1,E2;
@@
- hashcmp@p(E1->hash, E2->hash)
+ oidcmp(E1, E2)
This is only a little slower than the current code, and does
the right thing in all cases. Unfortunately, not all builds
of Coccinelle include python support (including the ones in
Debian). Requiring it may mean that fewer people can easily
run the tool, which is worse than it simply being a little
slower.
We may want to revisit this decision in the future if:
- builds with python become more common
- we find more uses for python support that tip the
cost-benefit analysis
But for now this patch sticks with the angle-bracket
solution, and converts all existing cocci patches. This
fixes only one missed case in the current code, though it
makes a much better difference for some new rules I'm adding
(converting "!hashcmp()" to "hasheq()" misses over half the
possible conversions using the old form).
[1] https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/
Helped-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Jeff King <peff@peff.net>
---
contrib/coccinelle/commit.cocci | 4 ++--
contrib/coccinelle/object_id.cocci | 20 ++++++++++----------
sequencer.c | 2 +-
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
index aec3345adb..c49aa558f0 100644
--- a/contrib/coccinelle/commit.cocci
+++ b/contrib/coccinelle/commit.cocci
@@ -15,10 +15,10 @@ expression c;
identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$";
expression c;
@@
- f(...) {...
+ f(...) {<...
- c->maybe_tree
+ get_commit_tree(c)
- ...}
+ ...>}
@@
expression c;
diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
index 09afdbf994..5869979be7 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -20,10 +20,10 @@ expression E1;
identifier f != oid_to_hex;
expression E1;
@@
- f(...) {...
+ f(...) {<...
- sha1_to_hex(E1->hash)
+ oid_to_hex(E1)
- ...}
+ ...>}
@@
expression E1, E2;
@@ -35,10 +35,10 @@ expression E1, E2;
identifier f != oid_to_hex_r;
expression E1, E2;
@@
- f(...) {...
+ f(...) {<...
- sha1_to_hex_r(E1, E2->hash)
+ oid_to_hex_r(E1, E2)
- ...}
+ ...>}
@@
expression E1;
@@ -50,10 +50,10 @@ expression E1;
identifier f != oidclr;
expression E1;
@@
- f(...) {...
+ f(...) {<...
- hashclr(E1->hash)
+ oidclr(E1)
- ...}
+ ...>}
@@
expression E1, E2;
@@ -65,10 +65,10 @@ expression E1, E2;
identifier f != oidcmp;
expression E1, E2;
@@
- f(...) {...
+ f(...) {<...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
- ...}
+ ...>}
@@
expression E1, E2;
@@ -92,10 +92,10 @@ expression E1, E2;
identifier f != oidcpy;
expression E1, E2;
@@
- f(...) {...
+ f(...) {<...
- hashcpy(E1->hash, E2->hash)
+ oidcpy(E1, E2)
- ...}
+ ...>}
@@
expression E1, E2;
diff --git a/sequencer.c b/sequencer.c
index 65d371c746..6a11f49651 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4545,7 +4545,7 @@ int skip_unnecessary_picks(void)
if (item->commit->parents->next)
break; /* merge commit */
parent_oid = &item->commit->parents->item->object.oid;
- if (hashcmp(parent_oid->hash, oid->hash))
+ if (oidcmp(parent_oid, oid))
break;
oid = &item->commit->object.oid;
}
--
2.19.0.rc0.412.g7005db4e88
next prev parent reply other threads:[~2018-08-25 8:03 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King
2018-08-25 8:03 ` Jeff King [this message]
2018-08-25 8:05 ` [PATCH 2/9] introduce hasheq() and oideq() Jeff King
2018-08-25 10:58 ` Andrei Rybak
2018-08-25 8:07 ` [PATCH 3/9] convert "oidcmp() == 0" to oideq() Jeff King
2018-08-25 8:36 ` Jeff King
2018-08-27 12:31 ` Derrick Stolee
2018-08-27 12:33 ` Derrick Stolee
2018-08-25 8:08 ` [PATCH 4/9] convert "hashcmp() == 0" to hasheq() Jeff King
2018-08-25 8:09 ` [PATCH 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King
2018-08-25 8:10 ` [PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King
2018-08-25 8:14 ` [PATCH 7/9] convert hashmap comparison functions to oideq() Jeff King
2018-08-25 8:15 ` [PATCH 8/9] read-cache: use oideq() in ce_compare functions Jeff King
2018-08-25 8:17 ` [PATCH 9/9] show_dirstat: simplify same-content check Jeff King
2018-08-25 8:20 ` Eric Sunshine
2018-08-25 8:23 ` Jeff King
2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson
2018-08-27 12:41 ` Derrick Stolee
2018-08-28 21:21 ` Jeff King
2018-08-28 21:22 ` [PATCH v2 1/9] coccinelle: use <...> for function exclusion Jeff King
2018-08-28 21:22 ` [PATCH v2 2/9] introduce hasheq() and oideq() Jeff King
2018-08-28 21:22 ` [PATCH v2 3/9] convert "oidcmp() == 0" to oideq() Jeff King
2018-08-28 21:22 ` [PATCH v2 4/9] convert "hashcmp() == 0" to hasheq() Jeff King
2018-08-28 21:22 ` [PATCH v2 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King
2018-08-28 21:22 ` [PATCH v2 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King
2018-08-28 21:22 ` [PATCH v2 7/9] convert hashmap comparison functions to oideq() Jeff King
2018-08-28 21:22 ` [PATCH v2 8/9] read-cache: use oideq() in ce_compare functions Jeff King
2018-08-28 21:23 ` [PATCH v2 9/9] show_dirstat: simplify same-content check Jeff King
2018-08-28 21:36 ` [PATCH 0/9] introducing oideq() Derrick Stolee
2018-08-29 0:08 ` brian m. carlson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180825080346.GA737@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=jacob.keller@gmail.com \
--cc=sandals@crustytoothpaste.net \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).