On Thu, Aug 25, 2022 at 04:36:15PM +0200, Ævar Arnfjörð Bjarmason wrote:
> * Since we create a single "*.cocci.patch+" we don't know where to
> pick up where we left off. Note that (per [1]) we've had a race
> condition since 960154b9c17 (coccicheck: optionally batch spatch
> invocations, 2019-05-06) which might result in us producing corrupt
> patches to to the concurrent appending to "$@+" in the pre-image.
>
> That bug is now fixed.
There is no bug, because there is no concurrent appending to "$@+".
The message you mention seems to be irrelevant, as it talks about
'xargs -P', but the invocation in '%.cocci.patch' targets never used
'-P'.
> Which is why we'll not depend on $(FOUND_H_SOURCES) but the *.o file
> corresponding to the *.c file, if it exists already. This means that
> we can do:
>
> make all
> make coccicheck
> make -W column.h coccicheck
>
> By depending on the *.o we piggy-back on
> COMPUTE_HEADER_DEPENDENCIES. See c234e8a0ecf (Makefile: make the
> "sparse" target non-.PHONY, 2021-09-23) for prior art of doing that
> for the *.sp files. E.g.:
>
> make contrib/coccinelle/free.cocci.patch
> make -W column.h contrib/coccinelle/free.cocci.patch
>
> Will take around 15 seconds for the second command on my 8 core box if
> I didn't run "make" beforehand to create the *.o files. But around 2
> seconds if I did and we have those "*.o" files.
>
> Notes about the approach of piggy-backing on *.o for dependencies:
>
> * It *is* a trade-off since we'll pay the extra cost of running the C
> compiler, but we're probably doing that anyway.
This assumption doesn't hold, and I very much dislike the idea of
depending on *.o files:
- Our static-analysis CI job doesn't build Git, now it will have to.
- I don't have Coccinelle installed, because my distro doesn't ship
it, and though the previous release did ship it, it was outdated.
Instead I use Coccinelle's most recent version from a container
which doesn't contain any build tools apart from 'make' for 'make
coccicheck'.
With this patch series I can't use this containerized Coccinelle
at all, because even though I've already built git on the host,
the dependency on *.o files triggers a BUILD-OPTIONS check during
'make coccicheck', and due to the missing 'curl-config' the build
options do differ, triggering a rebuild, which in the absence of a
compiler fails.
And then the next 'make' on the host will have to rebuild
everything again...
> * We can take better advantage of parallelism, while making sure that
> we don't racily append to the contrib/coccinelle/swap.cocci.patch
> file from multiple workers.
>
> Before this change running "make coccicheck" would by default end
> up pegging just one CPU at the very end for a while, usually as
> we'd finish whichever *.cocci rule was the most expensive.
>
> This could be mitigated by combining "make -jN" with
> SPATCH_BATCH_SIZE, see 960154b9c17 (coccicheck: optionally batch
> spatch invocations, 2019-05-06). But doing so required careful
> juggling, as e.g. setting both to 4 would yield 16 workers.
No, setting both to 4 does yield 4 workers.
SPATCH_BATCH_SIZE has nothing to do with parallelism; it is merely the
number of C source files that we pass to a single 'spatch' invocation,
but for any given semantic patch it's still a sequential loop.
