{"thread":{"id":"59729","subject":"Bug in unused.cocci?","startedAt":"2023-05-11T09:17:01Z","lastAt":"2023-05-12T15:07:17Z","messageCount":3,"participants":["Phillip Wood","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"477029","messageId":"5720a23e-6ac0-9a6d-3d05-4e0f34bc0589@gmail.com","threadId":"59729","inReplyTo":null,"subject":"Bug in unused.cocci?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-11T09:16:21Z","receivedAt":"2023-05-11T09:17:01Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"The CI static analysis job for my unit test framework patch[1] fails[2] with\n\nCoccinelle suggests the following changes in \n'contrib/coccinelle/ALL.cocci.patch':\ndiff -u -p a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c\n--- a/t/unit-tests/t-strbuf.c\n+++ b/t/unit-tests/t-strbuf.c\n@@ -27,13 +27,9 @@ static void t_static_init(void)\n\n  static void t_dynamic_init(void)\n  {\n-\tstruct strbuf buf;\n-\n-\tstrbuf_init(&buf, 1024);\n  \tcheck_uint(buf.len, ==, 0);\n  \tcheck_uint(buf.alloc, >=, 1024);\n  \tcheck_char(buf.buf[0], ==, '\\0');\n-\tstrbuf_release(&buf);\n  }\n\n  static void t_addch(struct strbuf *buf, void *data)\nerror: Coccinelle suggested some changes\n\nI think this is due to a bug in unused.cocci. I'm not sure what is going \nwrong and admittedly we're unlikely to see code where an strbuf is \ninitialized and then used it without calling any of the strbuf_* \nfunctions within our main codebase but it would be nice if the rule \ncould handle this.\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/\n[2] \nhttps://github.com/phillipwood/git/actions/runs/4938207776/jobs/8827751328\n"},{"id":"477035","messageId":"ZF0B2Prhc7jsqa3p@nand.local","threadId":"59729","inReplyTo":"5720a23e-6ac0-9a6d-3d05-4e0f34bc0589@gmail.com","subject":"Re: Bug in unused.cocci?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T14:55:20Z","receivedAt":"2023-05-11T14:57:00Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 11, 2023 at 10:16:21AM +0100, Phillip Wood wrote:\n> I think this is due to a bug in unused.cocci. I'm not sure what is going\n> wrong and admittedly we're unlikely to see code where an strbuf is\n> initialized and then used it without calling any of the strbuf_* functions\n> within our main codebase but it would be nice if the rule could handle this.\n\nI don't think that this is a bug in unused.cocci, but rather a bug in\nspatch not being able to read t/unit-tests/test-lib.h.\n\n    $ spatch --verbose-parsing --debug --all-includes \\\n        --sp-file contrib/coccinelle/unused.old.cocci \\\n        t/unit-tests/t-strbuf.c | grep '^bad'\n    init_defs_builtins: /usr/lib/coccinelle/standard.h\n    -----------------------------------------------------------------------\n    processing semantic patch file: contrib/coccinelle/unused.old.cocci\n    with isos from: /usr/lib/coccinelle/standard.iso\n    -----------------------------------------------------------------------\n\n    HANDLING: t/unit-tests/t-strbuf.c\n    parse error\n     = error in t/unit-tests/test-lib.h; set verbose_parsing for more info\n    badcount: 3\n    bad: int test_done(void);\n    bad:\n    bad: /* Skip the current test. */\n    BAD:!!!!! __attribute__((format (printf, 1, 2)))\n\nFrom my understanding, spatch happily ignores macros that it doesn't\nunderstand (like check_uint() and check_char()), so to it this code\nlooks like:\n\n    struct strbuf buf;\n\n    strbuf_init(&buf, 1024);\n    strbuf_release(&buf);\n\nwhich it marks as unused and applies the patch. Strangely, if you force\nit to pre-process with the appropriate macro file by passing it\nexplicitly, it works as expected:\n\n    $ spatch --macro-file t/unit-tests/test-lib.h \\\n        --sp-file contrib/coccinelle/unused.old.cocci \\\n        t/unit-tests/t-strbuf.c\n    init_defs_builtins: /usr/lib/coccinelle/standard.h\n    init_defs: t/unit-tests/test-lib.h\n    HANDLING: t/unit-tests/t-strbuf.c\n\nI am puzzled by spatch's behavior here.\n\nThanks,\nTaylor\n"},{"id":"477182","messageId":"20d1d7a2-049e-cb53-e276-101995b7c9bb@gmail.com","threadId":"59729","inReplyTo":"ZF0B2Prhc7jsqa3p@nand.local","subject":"Re: Bug in unused.cocci?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-12T15:07:08Z","receivedAt":"2023-05-12T15:07:17Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Taylor\n\nOn 11/05/2023 15:55, Taylor Blau wrote:\n> On Thu, May 11, 2023 at 10:16:21AM +0100, Phillip Wood wrote:\n>> I think this is due to a bug in unused.cocci. I'm not sure what is going\n>> wrong and admittedly we're unlikely to see code where an strbuf is\n>> initialized and then used it without calling any of the strbuf_* functions\n>> within our main codebase but it would be nice if the rule could handle this.\n> \n> I don't think that this is a bug in unused.cocci, but rather a bug in\n> spatch not being able to read t/unit-tests/test-lib.h.\n> \n>      $ spatch --verbose-parsing --debug --all-includes \\\n>          --sp-file contrib/coccinelle/unused.old.cocci \\\n>          t/unit-tests/t-strbuf.c | grep '^bad'\n>      init_defs_builtins: /usr/lib/coccinelle/standard.h\n>      -----------------------------------------------------------------------\n>      processing semantic patch file: contrib/coccinelle/unused.old.cocci\n>      with isos from: /usr/lib/coccinelle/standard.iso\n>      -----------------------------------------------------------------------\n> \n>      HANDLING: t/unit-tests/t-strbuf.c\n>      parse error\n>       = error in t/unit-tests/test-lib.h; set verbose_parsing for more info\n>      badcount: 3\n>      bad: int test_done(void);\n>      bad:\n>      bad: /* Skip the current test. */\n>      BAD:!!!!! __attribute__((format (printf, 1, 2)))\n> \n>  From my understanding, spatch happily ignores macros that it doesn't\n> understand (like check_uint() and check_char()), so to it this code\n> looks like:\n> \n>      struct strbuf buf;\n> \n>      strbuf_init(&buf, 1024);\n>      strbuf_release(&buf);\n> \n> which it marks as unused and applies the patch. Strangely, if you force\n> it to pre-process with the appropriate macro file by passing it\n> explicitly, it works as expected:\n> \n>      $ spatch --macro-file t/unit-tests/test-lib.h \\\n>          --sp-file contrib/coccinelle/unused.old.cocci \\\n>          t/unit-tests/t-strbuf.c\n>      init_defs_builtins: /usr/lib/coccinelle/standard.h\n>      init_defs: t/unit-tests/test-lib.h\n>      HANDLING: t/unit-tests/t-strbuf.c\n> \n> I am puzzled by spatch's behavior here.\n\nThanks for looking at this. It's good that unused.cocci is not buggy but \nI agree spatch's behavior is confusing.  There is a similar test for \nSTRBUF_INIT which looks like\n\n\tstatic void t_static_init(void)\n\t{\n\t\tstruct strbuf buf = STRBUF_INIT;\n\n\t\tcheck_uint(buf.len, ==, 0);\n\t\tcheck_uint(buf.alloc, ==, 0);\n\t\tif (check(buf.buf == strbuf_slopbuf))\n\t\t\treturn; /* avoid de-referencing buf.buf */\n\t\tcheck_char(buf.buf[0], ==, '\\0');\n\t}\n\nWhich does not trigger this issue. Presumably the \"if\" statement is \nstopping it from ignoring the check() macro even though it does not \nunderstand it. If I change t_dynamic_init() to do\n\n\tif (!check(buf.buf != NULL))\n\t\tcheck_char(buf.buf[0], ==, '\\0');\n\nThen the static analysis job passes but I don't think that is really \nfixing the problem.\n\nThanks\n\nPhillip\n"}]}