* Bug in unused.cocci?
@ 2023-05-11 9:16 Phillip Wood
2023-05-11 14:55 ` Taylor Blau
0 siblings, 1 reply; 3+ messages in thread
From: Phillip Wood @ 2023-05-11 9:16 UTC (permalink / raw)
To: Git Mailing List; +Cc: Ævar Arnfjörð Bjarmason
The CI static analysis job for my unit test framework patch[1] fails[2] with
Coccinelle suggests the following changes in
'contrib/coccinelle/ALL.cocci.patch':
diff -u -p a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
--- a/t/unit-tests/t-strbuf.c
+++ b/t/unit-tests/t-strbuf.c
@@ -27,13 +27,9 @@ static void t_static_init(void)
static void t_dynamic_init(void)
{
- struct strbuf buf;
-
- strbuf_init(&buf, 1024);
check_uint(buf.len, ==, 0);
check_uint(buf.alloc, >=, 1024);
check_char(buf.buf[0], ==, '\0');
- strbuf_release(&buf);
}
static void t_addch(struct strbuf *buf, void *data)
error: Coccinelle suggested some changes
I think this is due to a bug in unused.cocci. I'm not sure what is going
wrong and admittedly we're unlikely to see code where an strbuf is
initialized and then used it without calling any of the strbuf_*
functions within our main codebase but it would be nice if the rule
could handle this.
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/
[2]
https://github.com/phillipwood/git/actions/runs/4938207776/jobs/8827751328
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Bug in unused.cocci?
2023-05-11 9:16 Bug in unused.cocci? Phillip Wood
@ 2023-05-11 14:55 ` Taylor Blau
2023-05-12 15:07 ` Phillip Wood
0 siblings, 1 reply; 3+ messages in thread
From: Taylor Blau @ 2023-05-11 14:55 UTC (permalink / raw)
To: phillip.wood; +Cc: Git Mailing List, Ævar Arnfjörð Bjarmason
On Thu, May 11, 2023 at 10:16:21AM +0100, Phillip Wood wrote:
> I think this is due to a bug in unused.cocci. I'm not sure what is going
> wrong and admittedly we're unlikely to see code where an strbuf is
> initialized and then used it without calling any of the strbuf_* functions
> within our main codebase but it would be nice if the rule could handle this.
I don't think that this is a bug in unused.cocci, but rather a bug in
spatch not being able to read t/unit-tests/test-lib.h.
$ spatch --verbose-parsing --debug --all-includes \
--sp-file contrib/coccinelle/unused.old.cocci \
t/unit-tests/t-strbuf.c | grep '^bad'
init_defs_builtins: /usr/lib/coccinelle/standard.h
-----------------------------------------------------------------------
processing semantic patch file: contrib/coccinelle/unused.old.cocci
with isos from: /usr/lib/coccinelle/standard.iso
-----------------------------------------------------------------------
HANDLING: t/unit-tests/t-strbuf.c
parse error
= error in t/unit-tests/test-lib.h; set verbose_parsing for more info
badcount: 3
bad: int test_done(void);
bad:
bad: /* Skip the current test. */
BAD:!!!!! __attribute__((format (printf, 1, 2)))
From my understanding, spatch happily ignores macros that it doesn't
understand (like check_uint() and check_char()), so to it this code
looks like:
struct strbuf buf;
strbuf_init(&buf, 1024);
strbuf_release(&buf);
which it marks as unused and applies the patch. Strangely, if you force
it to pre-process with the appropriate macro file by passing it
explicitly, it works as expected:
$ spatch --macro-file t/unit-tests/test-lib.h \
--sp-file contrib/coccinelle/unused.old.cocci \
t/unit-tests/t-strbuf.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
init_defs: t/unit-tests/test-lib.h
HANDLING: t/unit-tests/t-strbuf.c
I am puzzled by spatch's behavior here.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Bug in unused.cocci?
2023-05-11 14:55 ` Taylor Blau
@ 2023-05-12 15:07 ` Phillip Wood
0 siblings, 0 replies; 3+ messages in thread
From: Phillip Wood @ 2023-05-12 15:07 UTC (permalink / raw)
To: Taylor Blau, phillip.wood
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason
Hi Taylor
On 11/05/2023 15:55, Taylor Blau wrote:
> On Thu, May 11, 2023 at 10:16:21AM +0100, Phillip Wood wrote:
>> I think this is due to a bug in unused.cocci. I'm not sure what is going
>> wrong and admittedly we're unlikely to see code where an strbuf is
>> initialized and then used it without calling any of the strbuf_* functions
>> within our main codebase but it would be nice if the rule could handle this.
>
> I don't think that this is a bug in unused.cocci, but rather a bug in
> spatch not being able to read t/unit-tests/test-lib.h.
>
> $ spatch --verbose-parsing --debug --all-includes \
> --sp-file contrib/coccinelle/unused.old.cocci \
> t/unit-tests/t-strbuf.c | grep '^bad'
> init_defs_builtins: /usr/lib/coccinelle/standard.h
> -----------------------------------------------------------------------
> processing semantic patch file: contrib/coccinelle/unused.old.cocci
> with isos from: /usr/lib/coccinelle/standard.iso
> -----------------------------------------------------------------------
>
> HANDLING: t/unit-tests/t-strbuf.c
> parse error
> = error in t/unit-tests/test-lib.h; set verbose_parsing for more info
> badcount: 3
> bad: int test_done(void);
> bad:
> bad: /* Skip the current test. */
> BAD:!!!!! __attribute__((format (printf, 1, 2)))
>
> From my understanding, spatch happily ignores macros that it doesn't
> understand (like check_uint() and check_char()), so to it this code
> looks like:
>
> struct strbuf buf;
>
> strbuf_init(&buf, 1024);
> strbuf_release(&buf);
>
> which it marks as unused and applies the patch. Strangely, if you force
> it to pre-process with the appropriate macro file by passing it
> explicitly, it works as expected:
>
> $ spatch --macro-file t/unit-tests/test-lib.h \
> --sp-file contrib/coccinelle/unused.old.cocci \
> t/unit-tests/t-strbuf.c
> init_defs_builtins: /usr/lib/coccinelle/standard.h
> init_defs: t/unit-tests/test-lib.h
> HANDLING: t/unit-tests/t-strbuf.c
>
> I am puzzled by spatch's behavior here.
Thanks for looking at this. It's good that unused.cocci is not buggy but
I agree spatch's behavior is confusing. There is a similar test for
STRBUF_INIT which looks like
static void t_static_init(void)
{
struct strbuf buf = STRBUF_INIT;
check_uint(buf.len, ==, 0);
check_uint(buf.alloc, ==, 0);
if (check(buf.buf == strbuf_slopbuf))
return; /* avoid de-referencing buf.buf */
check_char(buf.buf[0], ==, '\0');
}
Which does not trigger this issue. Presumably the "if" statement is
stopping it from ignoring the check() macro even though it does not
understand it. If I change t_dynamic_init() to do
if (!check(buf.buf != NULL))
check_char(buf.buf[0], ==, '\0');
Then the static analysis job passes but I don't think that is really
fixing the problem.
Thanks
Phillip
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-12 15:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-11 9:16 Bug in unused.cocci? Phillip Wood
2023-05-11 14:55 ` Taylor Blau
2023-05-12 15:07 ` Phillip Wood
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).