Skip to content

Commit 7f8d098

Browse files
gitstergitster
committed
Merge branch 'ab/cocci-unused'
Add Coccinelle rules to detect the pattern of initializing and then finalizing a structure without using it in between at all, which happens after code restructuring and the compilers fail to recognize as an unused variable. * ab/cocci-unused: cocci: generalize "unused" rule to cover more than "strbuf" cocci: add and apply a rule to find "unused" strbufs cocci: have "coccicheck{,-pending}" depend on "coccicheck-test" cocci: add a "coccicheck-test" target and test *.cocci rules Makefile & .gitignore: ignore & clean "git.res", not "*.res" Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
2 parents 6d00385 + 06f5f89 commit 7f8d098

File tree

13 files changed

+219
-16
lines changed

13 files changed

+219
-16
lines changed

‎.gitignore

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@
185185
/git-worktree
186186
/git-write-tree
187187
/git-core-*/?*
188+
/git.res
188189
/gitweb/GITWEB-BUILD-OPTIONS
189190
/gitweb/gitweb.cgi
190191
/gitweb/static/gitweb.js
@@ -225,7 +226,6 @@
225226
*.hcc
226227
*.obj
227228
*.lib
228-
*.res
229229
*.sln
230230
*.sp
231231
*.suo

‎Makefile

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,7 +1292,7 @@ SANITIZE_ADDRESS =
12921292
# For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
12931293
# usually result in less CPU usage at the cost of higher peak memory.
12941294
# Setting it to 0 will feed all files in a single spatch invocation.
1295-
SPATCH_FLAGS = --all-includes --patch .
1295+
SPATCH_FLAGS = --all-includes
12961296
SPATCH_BATCH_SIZE = 1
12971297

12981298
include config.mak.uname
@@ -3126,6 +3126,8 @@ check: $(GENERATED_H)
31263126
exit 1; \
31273127
fi
31283128

3129+
COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
3130+
31293131
%.cocci.patch: %.cocci $(COCCI_SOURCES)
31303132
$(QUIET_SPATCH) \
31313133
if test $(SPATCH_BATCH_SIZE) = 0; then \
@@ -3134,7 +3136,8 @@ check: $(GENERATED_H)
31343136
limit='-n $(SPATCH_BATCH_SIZE)'; \
31353137
fi; \
31363138
if ! echo $(COCCI_SOURCES) | xargs $$limit \
3137-
$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \
3139+
$(SPATCH) $(SPATCH_FLAGS) \
3140+
--sp-file $< --patch . \
31383141
>$@+ 2>$@.log; \
31393142
then \
31403143
cat $@.log; \
@@ -3145,9 +3148,27 @@ check: $(GENERATED_H)
31453148
then \
31463149
echo ' ' SPATCH result: $@; \
31473150
fi
3151+
3152+
COCCI_TEST_RES_GEN = $(addprefix .build/,$(COCCI_TEST_RES))
3153+
$(COCCI_TEST_RES_GEN): .build/%.res : %.c
3154+
$(COCCI_TEST_RES_GEN): .build/%.res : %.res
3155+
$(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinelle/%.cocci
3156+
$(call mkdir_p_parent_template)
3157+
$(QUIET_SPATCH_T)$(SPATCH) $(SPATCH_FLAGS) \
3158+
--very-quiet --no-show-diff \
3159+
--sp-file $< -o $@ \
3160+
$(@:.build/%.res=%.c) && \
3161+
cmp $(@:.build/%=%) $@ || \
3162+
git -P diff --no-index $(@:.build/%=%) $@ 2>/dev/null; \
3163+
3164+
.PHONY: coccicheck-test
3165+
coccicheck-test: $(COCCI_TEST_RES_GEN)
3166+
3167+
coccicheck: coccicheck-test
31483168
coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
31493169

31503170
# See contrib/coccinelle/README
3171+
coccicheck-pending: coccicheck-test
31513172
coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
31523173

31533174
.PHONY: coccicheck coccicheck-pending
@@ -3415,12 +3436,13 @@ profile-clean:
34153436
$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
34163437

34173438
cocciclean:
3439+
$(RM) -r .build/contrib/coccinelle
34183440
$(RM) contrib/coccinelle/*.cocci.patch*
34193441

34203442
clean: profile-clean coverage-clean cocciclean
34213443
$(RM) -r .build
34223444
$(RM) po/git.pot po/git-core.pot
3423-
$(RM) *.res
3445+
$(RM) git.res
34243446
$(RM) $(OBJECTS)
34253447
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
34263448
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X

‎builtin/fetch.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,7 +1109,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
11091109
struct fetch_head *fetch_head)
11101110
{
11111111
int url_len, i, rc = 0;
1112-
struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
1112+
struct strbuf note = STRBUF_INIT;
11131113
const char *what, *kind;
11141114
struct ref *rm;
11151115
char *url;
@@ -1276,7 +1276,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
12761276

12771277
abort:
12781278
strbuf_release(&note);
1279-
strbuf_release(&err);
12801279
free(url);
12811280
return rc;
12821281
}

‎builtin/merge.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)
375375
static void restore_state(const struct object_id *head,
376376
const struct object_id *stash)
377377
{
378-
struct strbuf sb = STRBUF_INIT;
379378
const char *args[] = { "stash", "apply", NULL, NULL };
380379

381380
if (is_null_oid(stash))
@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,
391390
*/
392391
run_command_v_opt(args, RUN_GIT_CMD);
393392

394-
strbuf_release(&sb);
395393
refresh_cache(REFRESH_QUIET);
396394
}
397395

@@ -502,7 +500,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
502500
{
503501
struct commit *remote_head;
504502
struct object_id branch_head;
505-
struct strbuf buf = STRBUF_INIT;
506503
struct strbuf bname = STRBUF_INIT;
507504
struct merge_remote_desc *desc;
508505
const char *ptr;
@@ -590,7 +587,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
590587
oid_to_hex(&remote_head->object.oid), remote);
591588
cleanup:
592589
free(found_ref);
593-
strbuf_release(&buf);
594590
strbuf_release(&bname);
595591
}
596592

‎builtin/repack.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -727,7 +727,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
727727
struct child_process cmd = CHILD_PROCESS_INIT;
728728
struct string_list_item *item;
729729
struct string_list names = STRING_LIST_INIT_DUP;
730-
struct string_list rollback = STRING_LIST_INIT_NODUP;
731730
struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
732731
struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
733732
struct pack_geometry *geometry = NULL;
@@ -1117,7 +1116,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
11171116
}
11181117

11191118
string_list_clear(&names, 0);
1120-
string_list_clear(&rollback, 0);
11211119
string_list_clear(&existing_nonkept_packs, 0);
11221120
string_list_clear(&existing_kept_packs, 0);
11231121
clear_pack_geometry(geometry);

‎contrib/coccinelle/tests/free.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
int use_FREE_AND_NULL(int *v)
2+
{
3+
free(*v);
4+
*v = NULL;
5+
}
6+
7+
int need_no_if(int *v)
8+
{
9+
if (v)
10+
free(v);
11+
}

‎contrib/coccinelle/tests/free.res

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
int use_FREE_AND_NULL(int *v)
2+
{
3+
FREE_AND_NULL(*v);
4+
}
5+
6+
int need_no_if(int *v)
7+
{
8+
free(v);
9+
}

‎contrib/coccinelle/tests/unused.c

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
void test_strbuf(void)
2+
{
3+
struct strbuf sb1 = STRBUF_INIT;
4+
struct strbuf sb2 = STRBUF_INIT;
5+
struct strbuf sb3 = STRBUF_INIT;
6+
struct strbuf sb4 = STRBUF_INIT;
7+
struct strbuf sb5;
8+
struct strbuf sb6 = { 0 };
9+
struct strbuf sb7 = STRBUF_INIT;
10+
struct strbuf sb8 = STRBUF_INIT;
11+
struct strbuf *sp1;
12+
struct strbuf *sp2;
13+
struct strbuf *sp3;
14+
struct strbuf *sp4 = xmalloc(sizeof(struct strbuf));
15+
struct strbuf *sp5 = xmalloc(sizeof(struct strbuf));
16+
struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
17+
struct strbuf *sp7;
18+
19+
strbuf_init(&sb5, 0);
20+
strbuf_init(sp1, 0);
21+
strbuf_init(sp2, 0);
22+
strbuf_init(sp3, 0);
23+
strbuf_init(sp4, 0);
24+
strbuf_init(sp5, 0);
25+
strbuf_init(sp6, 0);
26+
strbuf_init(sp7, 0);
27+
sp7 = xmalloc(sizeof(struct strbuf));
28+
29+
use_before(&sb3);
30+
use_as_str("%s", sb7.buf);
31+
use_as_str("%s", sp1->buf);
32+
use_as_str("%s", sp6->buf);
33+
pass_pp(&sp3);
34+
35+
strbuf_release(&sb1);
36+
strbuf_reset(&sb2);
37+
strbuf_release(&sb3);
38+
strbuf_release(&sb4);
39+
strbuf_release(&sb5);
40+
strbuf_release(&sb6);
41+
strbuf_release(&sb7);
42+
strbuf_release(sp1);
43+
strbuf_release(sp2);
44+
strbuf_release(sp3);
45+
strbuf_release(sp4);
46+
strbuf_release(sp5);
47+
strbuf_release(sp6);
48+
strbuf_release(sp7);
49+
50+
use_after(&sb4);
51+
52+
if (when_strict())
53+
return;
54+
strbuf_release(&sb8);
55+
}
56+
57+
void test_other(void)
58+
{
59+
struct string_list l = STRING_LIST_INIT_DUP;
60+
struct strbuf sb = STRBUF_INIT;
61+
62+
string_list_clear(&l, 0);
63+
string_list_clear(&sb, 0);
64+
}
65+
66+
void test_worktrees(void)
67+
{
68+
struct worktree **w1 = get_worktrees();
69+
struct worktree **w2 = get_worktrees();
70+
struct worktree **w3;
71+
struct worktree **w4;
72+
73+
w3 = get_worktrees();
74+
w4 = get_worktrees();
75+
76+
use_it(w4);
77+
78+
free_worktrees(w1);
79+
free_worktrees(w2);
80+
free_worktrees(w3);
81+
free_worktrees(w4);
82+
}

‎contrib/coccinelle/tests/unused.res

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
void test_strbuf(void)
2+
{
3+
struct strbuf sb3 = STRBUF_INIT;
4+
struct strbuf sb4 = STRBUF_INIT;
5+
struct strbuf sb7 = STRBUF_INIT;
6+
struct strbuf *sp1;
7+
struct strbuf *sp3;
8+
struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
9+
strbuf_init(sp1, 0);
10+
strbuf_init(sp3, 0);
11+
strbuf_init(sp6, 0);
12+
13+
use_before(&sb3);
14+
use_as_str("%s", sb7.buf);
15+
use_as_str("%s", sp1->buf);
16+
use_as_str("%s", sp6->buf);
17+
pass_pp(&sp3);
18+
19+
strbuf_release(&sb3);
20+
strbuf_release(&sb4);
21+
strbuf_release(&sb7);
22+
strbuf_release(sp1);
23+
strbuf_release(sp3);
24+
strbuf_release(sp6);
25+
26+
use_after(&sb4);
27+
28+
if (when_strict())
29+
return;
30+
}
31+
32+
void test_other(void)
33+
{
34+
}
35+
36+
void test_worktrees(void)
37+
{
38+
struct worktree **w4;
39+
40+
w4 = get_worktrees();
41+
42+
use_it(w4);
43+
44+
free_worktrees(w4);
45+
}

‎contrib/coccinelle/unused.cocci

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// This rule finds sequences of "unused" declerations and uses of a
2+
// variable, where "unused" is defined to include only calling the
3+
// equivalent of alloc, init & free functions on the variable.
4+
@@
5+
type T;
6+
identifier I;
7+
// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
8+
constant INIT_MACRO =~ "_INIT";
9+
identifier MALLOC1 =~ "^x?[mc]alloc$";
10+
identifier INIT_ASSIGN1 =~ "^get_worktrees$";
11+
identifier INIT_CALL1 =~ "^[a-z_]*_init$";
12+
identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$";
13+
identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
14+
@@
15+
16+
(
17+
- T I;
18+
|
19+
- T I = { 0 };
20+
|
21+
- T I = INIT_MACRO;
22+
|
23+
- T I = MALLOC1(...);
24+
|
25+
- T I = INIT_ASSIGN1(...);
26+
)
27+
28+
<... when != \( I \| &I \)
29+
(
30+
- \( INIT_CALL1 \)( \( I \| &I \), ...);
31+
|
32+
- I = \( INIT_ASSIGN1 \)(...);
33+
|
34+
- I = MALLOC1(...);
35+
)
36+
...>
37+
38+
(
39+
- \( REL1 \| REL2 \)( \( I \| &I \), ...);
40+
|
41+
- \( REL1 \| REL2 \)( \( &I \| I \) );
42+
)
43+
... when != \( I \| &I \)

0 commit comments

Comments
 (0)