{"thread":{"id":"49206","subject":"excluding a function from coccinelle transformation","startedAt":"2018-08-24T06:42:32Z","lastAt":"2018-08-24T21:00:43Z","messageCount":4,"participants":["Jeff King","Julia Lawall"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"356437","messageId":"20180824064228.GA3183@sigill.intra.peff.net","threadId":"49206","inReplyTo":null,"subject":"excluding a function from coccinelle transformation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T06:42:29Z","receivedAt":"2018-08-24T06:42:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In Git's Coccinelle patches, we sometimes want to suppress a\ntransformation inside a particular function. For example, in finding\nconversions of hashcmp() to oidcmp(), we should not convert the call in\noidcmp() itself, since that would cause infinite recursion. We write the\nsemantic patch like this:\n\n  @@\n  identifier f != oidcmp;\n  expression E1, E2;\n  @@\n    f(...) {...\n  - hashcmp(E1->hash, E2->hash)\n  + oidcmp(E1, E2)\n    ...}\n\nThis catches some cases, but not all. For instance, there's one case in\nsequencer.c which it does not convert. Now here's where it gets weird.\nIf I instead use the angle-bracket form of ellipses, like this:\n\n  @@\n  identifier f != oidcmp;\n  expression E1, E2;\n  @@\n    f(...) {<...\n  - hashcmp(E1->hash, E2->hash)\n  + oidcmp(E1, E2)\n    ...>}\n\nthen we do generate the expected diff! Here's a much more cut-down\nsource file that demonstrates the same behavior:\n\n  int foo(void)\n  {\n    if (1)\n      if (!hashcmp(x, y))\n        return 1;\n    return 0;\n  }\n\nIf I remove the initial \"if (1)\" then a diff is generated with either\nsemantic patch (and the particulars of the \"if\" are not important; the\nsame thing happens if it's a while-loop. The key thing seems to be that\nthe code is not in the top-level block of the function).\n\nAnd here's some double-weirdness. I get those results with spatch 1.0.4,\nwhich is what's in Debian unstable. If I then upgrade to 1.0.6 from\nDebian experimental, then _neither_ patch produces any results! Instead\nI get:\n\n  init_defs_builtins: /usr/lib/coccinelle/standard.h\n  (ONCE) Expected tokens oidcmp hashcmp hash\n  Skipping:foo.c\n\n(whereas before, even the failing case said \"HANDLING: foo.c\").\n\nAnd then one final check: I built coccinelle from the current tip of\nhttps://github.com/coccinelle/coccinelle (1.0.7-00504-g670b2243).\nWith my cut-down case, that version generates a diff with either\nsemantic patch. But for the full-blown case in sequencer.c, it still\nonly works with the angle brackets.\n\nSo my questions are:\n\n  - is this a bug in coccinelle? Or I not understand how \"...\" is\n    supposed to work here?\n\n    (It does seem like there was possibly a separate bug introduced in\n    1.0.6 that was later fixed; we can probably ignore that and just\n    focus on the behavior in the current tip of master).\n\n  - is there a better way to represent this kind of \"transform this\n    everywhere _except_ in this function\" semantic patch? (preferably\n    one that does not tickle this bug, if it is indeed a bug ;) ).\n\n-Peff\n"},{"id":"356442","messageId":"alpine.DEB.2.21.1808240652370.2344@hadrien","threadId":"49206","inReplyTo":"20180824064228.GA3183@sigill.intra.peff.net","subject":"Re: [Cocci] excluding a function from coccinelle transformation","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2018-08-24T11:04:27Z","receivedAt":"2018-08-24T11:04:54Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"\n\nOn Fri, 24 Aug 2018, Jeff King wrote:\n\n> In Git's Coccinelle patches, we sometimes want to suppress a\n> transformation inside a particular function. For example, in finding\n> conversions of hashcmp() to oidcmp(), we should not convert the call in\n> oidcmp() itself, since that would cause infinite recursion. We write the\n> semantic patch like this:\n>\n>   @@\n>   identifier f != oidcmp;\n>   expression E1, E2;\n>   @@\n>     f(...) {...\n>   - hashcmp(E1->hash, E2->hash)\n>   + oidcmp(E1, E2)\n>     ...}\n\nThe problem is with how how ... works.  For transformation, A ... B\nrequires that B occur on every execution path starting with A, unless that\nexecution path ends up in error handling code.\n(eg, if (...) { ... return; }).  Here your A is the start if the function.\nSo you need a call to hashcmp on every path through the function, which\nfails when you add ifs.\n\nIf you use * (searching) instead of - and + (transformation) it will only\nrequire that a path exists.  * is mean for bug finding, where you often\nwant to find eg whether there exists a path that is missing a free.\n\nIf you want the exists behavior with a transformation rule, then you can\nput exists at the top of the rule between the initial @@.  I don't suggest\nthis in general, as it can lead to inconsistencies.\n\nWhat you want is what you ended up using, which is <... P ...> which\nallows zero or more occurrences of P.\n\nHowever, this can all be very expensive, because you are matching paths\nthrough the function definition which you don't really care about.  All\nyou care about here is the name.  So another approach is\n\n@@\nposition p : script:python() { p[0].current_element != \"oldcmp\" };\nexpression E1,E2;\n@@\n\n- hashcmp(E1->hash, E2->hash)\n+ oidcmp(E1, E2)\n\n(I assume that \"not equals\" is written != in python)\n\nAnother issue with A ... B is that by default A and B should not appear in\nthe matched region.  So your original rule matches only the case where\nevery execution path contains exactly one call to hashcmp, not more than\none.  So that was another problem with it.\n\njulia\n\n>\n> This catches some cases, but not all. For instance, there's one case in\n> sequencer.c which it does not convert. Now here's where it gets weird.\n> If I instead use the angle-bracket form of ellipses, like this:\n>\n>   @@\n>   identifier f != oidcmp;\n>   expression E1, E2;\n>   @@\n>     f(...) {<...\n>   - hashcmp(E1->hash, E2->hash)\n>   + oidcmp(E1, E2)\n>     ...>}\n>\n> then we do generate the expected diff! Here's a much more cut-down\n> source file that demonstrates the same behavior:\n>\n>   int foo(void)\n>   {\n>     if (1)\n>       if (!hashcmp(x, y))\n>         return 1;\n>     return 0;\n>   }\n>\n> If I remove the initial \"if (1)\" then a diff is generated with either\n> semantic patch (and the particulars of the \"if\" are not important; the\n> same thing happens if it's a while-loop. The key thing seems to be that\n> the code is not in the top-level block of the function).\n>\n> And here's some double-weirdness. I get those results with spatch 1.0.4,\n> which is what's in Debian unstable. If I then upgrade to 1.0.6 from\n> Debian experimental, then _neither_ patch produces any results! Instead\n> I get:\n>\n>   init_defs_builtins: /usr/lib/coccinelle/standard.h\n>   (ONCE) Expected tokens oidcmp hashcmp hash\n>   Skipping:foo.c\n>\n> (whereas before, even the failing case said \"HANDLING: foo.c\").\n>\n> And then one final check: I built coccinelle from the current tip of\n> https://github.com/coccinelle/coccinelle (1.0.7-00504-g670b2243).\n> With my cut-down case, that version generates a diff with either\n> semantic patch. But for the full-blown case in sequencer.c, it still\n> only works with the angle brackets.\n>\n> So my questions are:\n>\n>   - is this a bug in coccinelle? Or I not understand how \"...\" is\n>     supposed to work here?\n>\n>     (It does seem like there was possibly a separate bug introduced in\n>     1.0.6 that was later fixed; we can probably ignore that and just\n>     focus on the behavior in the current tip of master).\n>\n>   - is there a better way to represent this kind of \"transform this\n>     everywhere _except_ in this function\" semantic patch? (preferably\n>     one that does not tickle this bug, if it is indeed a bug ;) ).\n>\n> -Peff\n> _______________________________________________\n> Cocci mailing list\n> Cocci@systeme.lip6.fr\n> https://systeme.lip6.fr/mailman/listinfo/cocci\n>\n"},{"id":"356490","messageId":"20180824205349.GA31853@sigill.intra.peff.net","threadId":"49206","inReplyTo":"alpine.DEB.2.21.1808240652370.2344@hadrien","subject":"Re: [Cocci] excluding a function from coccinelle transformation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T20:53:50Z","receivedAt":"2018-08-24T20:53:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 24, 2018 at 07:04:27AM -0400, Julia Lawall wrote:\n\n> On Fri, 24 Aug 2018, Jeff King wrote:\n> \n> > In Git's Coccinelle patches, we sometimes want to suppress a\n> > transformation inside a particular function. For example, in finding\n> > conversions of hashcmp() to oidcmp(), we should not convert the call in\n> > oidcmp() itself, since that would cause infinite recursion. We write the\n> > semantic patch like this:\n> >\n> >   @@\n> >   identifier f != oidcmp;\n> >   expression E1, E2;\n> >   @@\n> >     f(...) {...\n> >   - hashcmp(E1->hash, E2->hash)\n> >   + oidcmp(E1, E2)\n> >     ...}\n> \n> The problem is with how how ... works.  For transformation, A ... B\n> requires that B occur on every execution path starting with A, unless that\n> execution path ends up in error handling code.\n> (eg, if (...) { ... return; }).  Here your A is the start if the function.\n> So you need a call to hashcmp on every path through the function, which\n> fails when you add ifs.\n\nThank you! This explanation (and the one below about A and B not\nappearing in the matched region) helped my understanding tremendously.\n\n> What you want is what you ended up using, which is <... P ...> which\n> allows zero or more occurrences of P.\n\nAnd now this makes much more sense (I stumbled onto it through brute\nforce, but now I understand _why_ it works).\n\n> However, this can all be very expensive, because you are matching paths\n> through the function definition which you don't really care about.  All\n> you care about here is the name.  So another approach is\n\nYeah, it is. Using the pre-1.0.7 version, the original patch runs in\n~1.3 minutes on my machine. With \"<... P ...>\" it's almost 4 minutes.\nYour python suggestion runs in about 1.5 minutes.\n\nCuriously, 1.0.4 runs the original patch in only 24 seconds, and the\nangle-bracket one takes 52 seconds. I'm not sure if something changed in\ncoccinelle, or if my build is simply less optimized (my 1.0.4 is from\nthe Debian package, and I'm building 1.0.7 from source; I had trouble\nbuilding 1.0.4 from source).\n\n> @@\n> position p : script:python() { p[0].current_element != \"oldcmp\" };\n> expression E1,E2;\n> @@\n> \n> - hashcmp(E1->hash, E2->hash)\n> + oidcmp(E1, E2)\n\nAha, this is exactly the magic I was hoping for. I agree this is the\nbest way to express it. I just had to tweak the patch to include the\nposition:\n\n  - hashcmp@p(E1->hash, E2->hash)\n\nand it worked great. Unfortunately, Debian's spatch is not built with\npython support. :(\n\nI'm not sure if we (the Git project) want to make the jump to requiring\na more specific spatch. OTOH, only a handful of developers actually run\nit, and the python support does seem quite useful. And 1.0.4 is rather\nold at this point.\n\nAgain, thanks very much for your response. I have a much better\nunderstanding of what's going on now, and what our options are for\nmoving forward.\n\n-Peff\n"},{"id":"356491","messageId":"alpine.DEB.2.21.1808241658430.2378@hadrien","threadId":"49206","inReplyTo":"20180824205349.GA31853@sigill.intra.peff.net","subject":"Re: [Cocci] excluding a function from coccinelle transformation","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2018-08-24T21:00:38Z","receivedAt":"2018-08-24T21:00:43Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"\n\nOn Fri, 24 Aug 2018, Jeff King wrote:\n\n> On Fri, Aug 24, 2018 at 07:04:27AM -0400, Julia Lawall wrote:\n>\n> > On Fri, 24 Aug 2018, Jeff King wrote:\n> >\n> > > In Git's Coccinelle patches, we sometimes want to suppress a\n> > > transformation inside a particular function. For example, in finding\n> > > conversions of hashcmp() to oidcmp(), we should not convert the call in\n> > > oidcmp() itself, since that would cause infinite recursion. We write the\n> > > semantic patch like this:\n> > >\n> > >   @@\n> > >   identifier f != oidcmp;\n> > >   expression E1, E2;\n> > >   @@\n> > >     f(...) {...\n> > >   - hashcmp(E1->hash, E2->hash)\n> > >   + oidcmp(E1, E2)\n> > >     ...}\n> >\n> > The problem is with how how ... works.  For transformation, A ... B\n> > requires that B occur on every execution path starting with A, unless that\n> > execution path ends up in error handling code.\n> > (eg, if (...) { ... return; }).  Here your A is the start if the function.\n> > So you need a call to hashcmp on every path through the function, which\n> > fails when you add ifs.\n>\n> Thank you! This explanation (and the one below about A and B not\n> appearing in the matched region) helped my understanding tremendously.\n>\n> > What you want is what you ended up using, which is <... P ...> which\n> > allows zero or more occurrences of P.\n>\n> And now this makes much more sense (I stumbled onto it through brute\n> force, but now I understand _why_ it works).\n>\n> > However, this can all be very expensive, because you are matching paths\n> > through the function definition which you don't really care about.  All\n> > you care about here is the name.  So another approach is\n>\n> Yeah, it is. Using the pre-1.0.7 version, the original patch runs in\n> ~1.3 minutes on my machine. With \"<... P ...>\" it's almost 4 minutes.\n> Your python suggestion runs in about 1.5 minutes.\n>\n> Curiously, 1.0.4 runs the original patch in only 24 seconds, and the\n> angle-bracket one takes 52 seconds. I'm not sure if something changed in\n> coccinelle, or if my build is simply less optimized (my 1.0.4 is from\n> the Debian package, and I'm building 1.0.7 from source; I had trouble\n> building 1.0.4 from source).\n\nI don't remember the exact status of 1.0.4.  It is possible that an\noptimization was found to pose problems and was removed in the meantime.\n\n<... ...> can be useful when you expect it to eg match an if branch.  For\na function with over 1000 lines and many conditionals, it might not be a\ngood idea.  Actually, the main problem is with loops.  If there is a loop\nin the function the performance can be much slower.\n\njulia\n\n>\n> > @@\n> > position p : script:python() { p[0].current_element != \"oldcmp\" };\n> > expression E1,E2;\n> > @@\n> >\n> > - hashcmp(E1->hash, E2->hash)\n> > + oidcmp(E1, E2)\n>\n> Aha, this is exactly the magic I was hoping for. I agree this is the\n> best way to express it. I just had to tweak the patch to include the\n> position:\n>\n>   - hashcmp@p(E1->hash, E2->hash)\n>\n> and it worked great. Unfortunately, Debian's spatch is not built with\n> python support. :(\n>\n> I'm not sure if we (the Git project) want to make the jump to requiring\n> a more specific spatch. OTOH, only a handful of developers actually run\n> it, and the python support does seem quite useful. And 1.0.4 is rather\n> old at this point.\n>\n> Again, thanks very much for your response. I have a much better\n> understanding of what's going on now, and what our options are for\n> moving forward.\n>\n> -Peff\n>\n"}]}