Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v1.12] cocci: backport fix about incorrect warnings and resolve warning related to a const qualifier #28287

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

giorio94
Copy link
Member

This PR manually backports a coccicheck fix which can cause the emission of incorrect warning reports, and fixes a const qualifier issue that led to a failure in one of the checks.

cocci: backport fix about incorrect warnings and resolve warning related to a const qualifier

qmonnet and others added 2 commits September 26, 2023 16:15
[ upstream commit 89dd226 ]

Trying to create a new coccinelle check, we realised that some patterns
that we expected to match in the bpf/ were in fact never caught.

Here is a minimal reproducer.

foo.cocci:

    @@
    @@
    (
    - foo = bar;
    + foo = baz;
    )

foo.c (not working):

    int main (void)
    {
    	int foo, bar, baz = 0, coccibreaker __maybe_unused;

    	foo = bar;
    	return foo;
    }

Invocation:

    $ docker run --rm --user 1000 \
        --workdir /workspace -v `pwd`:/workspace -it
        docker.io/cilium/coccicheck:2.4@sha256:24abe3fbb8e829fa41a68a3b76cb4df84fd5a87a7d1d6254c1c1fe5effb5bd1b \
        spatch --include-headers --very-quiet --in-place \
        --sp-file contrib/coccinelle/foo.cocci bpf/foo.c

This does not catch the "foo = bar;" statement in the C file.
However, the pattern is caught in the following file instead:

foo.c (working):

    int main (void)
    {
    	int foo, bar, baz = 0;
    	int coccifine __maybe_unused;

    	foo = bar;
    	return foo;
    }

The __maybe_unused attribute seems to prevent spatch to parse correctly
the function. This happens independently of whether __maybe_unused is
defined in the C code. The behaviour remains the same if we replace
__maybe_unused with some other random word (although replacing it with
"__attribute__((unused))" works as expected).

Running from coccinelle's "master" branch works fine, and when running
spatch locally (no Docker image) we can also observe the following
warnings:

    warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h
    warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso

This error was fixed with commit coccinelle/coccinelle@540888ff426e
("Fix 263: wrong default path for COCCINELLE_HOME") [0], which is not
part of a release yet.

Until it is, we still want to have our code covered by coccinelle. As a
workaround to the issue, we could pass manually the paths to the files
containing the definitions for the isomorphisms and built-in macros for
coccinelle, with spatch options "--macro-file-builtin" and "--iso-file".
Even simpler, we can set $(COCCINELLE_HOME) to the correct path so that
both files are found.

No camels were harmed during this investigation.

[0] https://gitlab.inria.fr/coccinelle/coccinelle/-/commit/540888ff426e

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
make -C bpf coccicheck is currently reporting the following warning:

  EXN: Failure("./lib/proxy.h: 155: try to delete an expanded token: struct") in ./lib/proxy.h

Let's fix it adding the const qualifier to CT_TUPLE_TYPE.

Related: 72fece4 ("bpf: Add some missing const qualifiers")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Sep 26, 2023
@giorio94
Copy link
Member Author

/test-backport-1.12

@giorio94 giorio94 changed the title cocci: backport fix about incorrect warnings and resolve warning related to a const qualifier [v1.12] cocci: backport fix about incorrect warnings and resolve warning related to a const qualifier Sep 26, 2023
@giorio94 giorio94 marked this pull request as ready for review September 26, 2023 14:35
@giorio94 giorio94 requested review from a team as code owners September 26, 2023 14:35
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 27, 2023
@lmb lmb merged commit 51eac11 into v1.12 Sep 27, 2023
91 checks passed
@lmb lmb deleted the pr/giorio94/v1.12/coccicheck-fixes branch September 27, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants