Opened 3 months ago

Closed 8 weeks ago

#31705 closed task (fixed)

Add sufficient coccinelle tooling to run coccinelle without stress

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 042-should
Cc: Actual Points: 1.5
Parent ID: Points:
Reviewer: teor Sponsor: Sponsor31-can

Description

I think we need two pieces of coccinelle tooling to be able to use it effectively:

1) A script that tells us which files have parsing problems.

2) A script to invoke spatch with the right arguments.

Based on 1 and 2, we can improve our tor-coccinelle.h file to handle more of our codebase, and we can apply coccinelle scripts without trying to remember the name of the "-macro-file-builtins" flag.

Child Tickets

Change History (20)

comment:1 Changed 3 months ago by nickm

See branch ticket31705 with PR at https://github.com/torproject/tor/pull/1321

comment:2 Changed 3 months ago by nickm

Status: assignedneeds_review

CI has passed

comment:3 Changed 3 months ago by asn

Reviewer: teor

comment:4 Changed 3 months ago by nickm

Keywords: 042-can added

comment:5 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

Design review:

The design and overall structure looks good. I am not that familiar with coccinelle, so I will need to run these scripts myself as part of my review.

Should we be running these scripts as part of our code quality checks?
(We could install coccinelle on Travis, so it runs them.)
Is that a separate ticket?

comment:6 Changed 3 months ago by nickm

Is that a separate ticket?

I think/hope that's a separate ticket.

comment:7 Changed 3 months ago by teor

The make check is now #31919

comment:8 Changed 3 months ago by nickm

(Does this still belong in needs_revision?)

comment:9 Changed 3 months ago by teor

Status: needs_revisionneeds_review

comment:10 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

I left some comments on the ticket:

  • document tor-coccinelle.h
  • document test_operator_cleanup
  • make try_parse.sh exit with a non-zero status if it finds any files
  • make (a mode for) try_parse.sh that shows the parsing errors, so they can be fixed
  • it looks like you missed some files:
    $ find src/{app,config,core,feature,lib,test,tools,win32} -name *.c -o -name *.h | xargs ~/dev/tor/scripts/coccinelle/try_parse.sh
    # 143 files
    

Reminder: please fill in actual points on this ticket.

comment:11 Changed 2 months ago by nickm

Keywords: 042-should added; 042-can removed

comment:12 Changed 2 months ago by nickm

Actual Points: 1.5

I have fixed all of the issues above, and gotten the number of files with parsing errors down from 143 to 12. The remaining unhandled files are:

  • Those with funny case statement macros:
    • src/core/mainloop/connection.c
    • src/core/or/reasons.c
    • src/lib/tls/tortls.h
  • Those that use SMARTLIST_FOREACH_JOIN():
    • src/feature/nodelist/networkstatus.c
  • Those that consist entirely or almost entirely of macro definitions that Coccinelle does not understand:
    • src/lib/pubsub/pubsub_macros.h
    • src/lib/testsupport/testsupport.h
    • src/lib/smartlist_core/smartlist_foreach.h
    • src/lib/container/order.c
    • src/lib/container/handles.h
    • src/lib/container/map.c
    • src/lib/container/map.h
    • src/lib/cc/compat_compiler.h

Also, in some cases, I had to disable parsing of certain things with an #ifdef COCCI macro. In the future, if we can figure out how to parse those things, we should use this macro less often.

After these changes, I ran into some merge conflicts with master and its improved annotate_ifdef_directives. I have made a new branch ticket31705_merged to handle those conflicts, which has a PR at https://github.com/torproject/tor/pull/1403 . If CI passes, I'll call it needs_review.

comment:13 Changed 2 months ago by nickm

Status: needs_revisionneeds_review

comment:14 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

Seems fine, but there are some typos and minor commit message/content issues.

How do you want to make progress on the remaining files?
Do you want to set up CI, so we preserve cocci compatibility over time?

comment:15 in reply to:  14 Changed 8 weeks ago by nickm

Status: needs_revisionneeds_review

Replying to teor:

Seems fine, but there are some typos and minor commit message/content issues.

I've added a fixup commits for the typos. To solve the commit message/content issues, I had to squash those and make a new rebased branch, as ticket31705_v2. This still had merge conflicts with master, so it is merged in a ticket31705_v2_merged branch. The PR for that is https://github.com/torproject/tor/pull/1447 .

How do you want to make progress on the remaining files?
Do you want to set up CI, so we preserve cocci compatibility over time?

I don't have a strong opinion here right now: I think it would make more sense to get some experience using coccinelle more in our regular development workflow, so we can decide how to prioritize these items.

comment:16 Changed 8 weeks ago by teor

I would like to check the fixed files using cocci in CI. If we don't, they will degrade over time.
The CI shouldn't take long, I'll see if I can get it done today,

We can exclude the files that are hard to fix, and open a ticket to deal with them later.

comment:17 Changed 8 weeks ago by teor

Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final

comment:18 Changed 8 weeks ago by teor

This is merge_ready.

But there are actually 22 files that don't pass, see:
https://github.com/torproject/tor/pull/1454/files#diff-8eaf87fc1c11a68a2909b7da648da0a9

I'm going to wait until #31919 passes before merging, just in case there are any more surprises.

comment:19 Changed 8 weeks ago by teor

Status: needs_reviewmerge_ready

comment:20 Changed 8 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

CI runs fine in #31919, just tweaking the performance now.

Merged to master.

Note: See TracTickets for help on using tickets.