scripts: coccinelle #3991

AndreaTagliavini · 2020-07-22T09:56:46Z

scripts: add Coccinelle support

Having to deal with a large project, it is useful to have a tool that allows developers to analyze their code.
Coccinelle is a software used to match code patterns and to transform them programmatically, using ".cocci"scripts, which are also called: Semantic Patches.
This pull request, implements "coccicheck", a script that allows to run semantic patches on the OP-TEE code in three possible ways:

Run a local semantic patch
Run a whole set of local semantic patches
Run a set of external semantic patches
The script is callable as a makefile target, so in order to launch it run:
make coccicheck

The pull request also adds three semantic patches:

bad_null: produces patches which simplify expressions that compare to NULL
initialize_variables: matches non-initialized variables and produces patches that do so
warning_overflow: prints a warning on each occurrence of a specific pattern that could cause an overflow
All the patches produced by this tool are printed to stdout.

Coccinelle is a utility tool used to match specific code patterns in order to modify or statically analyze them. ".cocci"scripts (Semantic Patches) are used to define such patterns and also to specify what actions have to be performed. Add script coccicheck, which allows to execute Coccinelle semantic patches on top of op-tee source code. It is called using the "coccicheck" target in the op-tee Makefile. The script invokes semantic patches in 3 fashions: - use a single semantic patch: specify and execute a single semantic patch from the local set. - use multiple semantic patches: execute all semantic patches from the local set. - use a subset of external patches: specify a single or multiple semantic patches, defined outside of the local set. Reviewed-by: Paolo Valente <paolo.valente@linaro.org> Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

Add a semantic patch that matches various boolean expressions that compare to NULL, and produces patches that replace them with an equivalent reduced form (e.g. "foo == NULL" becomes "!foo"). Reviewed-by: Paolo Valente <paolo.valente@linaro.org> Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

Add a semantic patch that matches non-initialized variables and produces patches that set values depending on the variable type. - Numeric variables are set to 0 - Size_t arrays are set to {0} - Other arrays are set to {NULL} - Every other variable is set to NULL Reviewed-by: Paolo Valente <paolo.valente@linaro.org> Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

Add a semantic patch that matches the following construct: if(A + B > C) For each occurrence of the construct, a warning is printed, in order to highlight possible overflows. Reviewed-by: Paolo Valente <paolo.valente@linaro.org> Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

jenswi-linaro · 2020-07-22T11:28:39Z

Nice work! Can you add SPDX-License-Identifier for all the files too please?

jenswi-linaro · 2020-07-22T11:39:44Z

Is it possible to use this on individual diffs/patches too or do you need to run it on the entire tree?
It would be nice to be able to test only the diff of a pull request. A bit like what we can do with checkpatch.

jenswi-linaro · 2020-07-22T11:45:53Z

It would also be nice to ignore certain directories and files, like what we're ignoring for checkpatch:

                core/include/gen-asm-defines.h \
                core/lib/lib{fdt,tomcrypt} core/lib/zlib \
                lib/libutils lib/libmbedtls \
                core/arch/arm/include/arm{32,64}.h \
                core/arch/arm/plat-ti/api_monitor_index_a{9,15}.h \
                core/arch/arm/dts

jforissier · 2020-07-22T11:46:29Z

Regarding variable initialization: we initalize structs and arrays with = { };.

Add SPDX-License-Identifier. Add option USE_SUBTREE, which let one run semantic patches on a specific directory instead of the whole tree. Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

Add SPDX-License-Identifier to semantic patches. Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

AndreaTagliavini · 2020-07-22T14:09:14Z

Nice work! Can you add SPDX-License-Identifier for all the files too please?

I added just the identifier, if you want i could also add the copyright

AndreaTagliavini · 2020-07-22T14:10:25Z

Is it possible to use this on individual diffs/patches too or do you need to run it on the entire tree?
It would be nice to be able to test only the diff of a pull request. A bit like what we can do with checkpatch.

I just added an option that allows one to specify a sub-tree, so semantic patches are executed just from that location (but still recursively).

jenswi-linaro · 2020-07-22T14:38:15Z

Nice work! Can you add SPDX-License-Identifier for all the files too please?

I added just the identifier, if you want i could also add the copyright

Yes please.

jenswi-linaro · 2020-07-22T15:43:22Z

Is it possible to use this on individual diffs/patches too or do you need to run it on the entire tree?
It would be nice to be able to test only the diff of a pull request. A bit like what we can do with checkpatch.

I just added an option that allows one to specify a sub-tree, so semantic patches are executed just from that location (but still recursively).

If we could have another option to skip a number of sub-trees also it would be perfect. The problem is that we have certain 3rd-party libraries like LTC and mbedTLS that we'd rather not get warnings from unless we choose to.

etienne-lms · 2020-08-05T09:52:32Z

Makefile

 checkpatch: checkpatch-staging checkpatch-working

 checkpatch-working:
 	${q}./scripts/checkpatch.sh

 checkpatch-staging:
 	${q}./scripts/checkpatch.sh --cached
+
+coccicheck:
+	${q} ./scripts/coccicheck


minor: remove space, for consistency with the makefile.

etienne-lms · 2020-08-05T09:54:15Z

scripts/coccicheck

@@ -0,0 +1,132 @@
+#!/bin/bash
+
+DIR="`pwd`"


DIR not used. can remove.

etienne-lms · 2020-08-05T09:55:51Z

scripts/coccicheck

+
+SPATCH="`which ${SPATCH:=spatch}`"
+if [ ! -x "$SPATCH" ]; then
+    echo 'spatch is required to run this script, exiting.'


suggest "Error: " as prefix trace message.

etienne-lms · 2020-08-05T09:57:35Z

scripts/coccicheck

+###########################################################################################
+# Help section
+
+if [[ $H -ge 1 ]]; then


what is $H?

etienne-lms · 2020-08-05T10:01:35Z

scripts/coccicheck

+# Help section
+
+if [[ $H -ge 1 ]]; then
+    echo "coccicheck help section"


Maybe: "Help section for optee_os makefile coccicheck target"
Looks strange that this shell script shows debug info related to makefile usage.

etienne-lms · 2020-08-05T12:15:27Z

scripts/coccinelle/initialize_variables.cocci

+
+/************************************************************************************
+* This semantic patch matches all occurrencies of variable declaration without 
+* initialization and then initializes them accordingly to their type.


s/accordingly/according/

etienne-lms · 2020-08-05T12:18:31Z

scripts/coccinelle/initialize_variables.cocci

+|
+//  size_t array
+size_t v[]
+ = {0}


prefer { 0 } over {0}.

etienne-lms · 2020-08-05T12:19:44Z

scripts/coccinelle/initialize_variables.cocci

+|
+//  every other type
+t v
+ = NULL


Is this really applicable? NULL likely not applies to structure, enums, ...

I was just thinking of a placeholder value to assign to every other type, but you're right. Maybe for now i can simply remove these lines, and let others specify a type when necessary.

Prefer = { } for anonymous type as per Optee OS coding rules.

etienne-lms · 2020-08-05T12:21:15Z

scripts/coccinelle/initialize_variables.cocci

+|
+//  generic array
+t v[]
+ = {NULL}


prefer { } for generic array, as per https://optee.readthedocs.io/en/latest/general/coding_standards.html?highlight=conding%20style#coding-standards.

etienne-lms · 2020-08-05T12:24:12Z

scripts/coccicheck

+
+coccinelle () {
+    COCCI="$1"
+    run_cmd $SPATCH $VIRTUAL $FLAGS --cocci-file $COCCI $OPTIONS || exit 1 


FLAGS used here is not documented. It is related to coccinelle legacy env variables?

github-actions · 2020-09-21T01:01:56Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

AndreaTagliavini added 4 commits June 27, 2020 10:16

AndreaTagliavini changed the title ~~Coccinelle dev~~ scripts: coccinelle Jul 22, 2020

AndreaTagliavini added 2 commits July 22, 2020 16:04

scripts: coccicheck

f16582e

Add SPDX-License-Identifier. Add option USE_SUBTREE, which let one run semantic patches on a specific directory instead of the whole tree. Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

scripts: coccinelle

2c8faba

Add SPDX-License-Identifier to semantic patches. Signed-off-by: Andrea Tagliavini <tagliavini.andrea.1995@gmail.com>

etienne-lms reviewed Aug 5, 2020

View reviewed changes

github-actions bot added the Stale label Sep 21, 2020

jbech-linaro added enhancement and removed Stale labels Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: coccinelle #3991

scripts: coccinelle #3991

AndreaTagliavini commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020 •

edited

jforissier commented Jul 22, 2020

AndreaTagliavini commented Jul 22, 2020

AndreaTagliavini commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

AndreaTagliavini Aug 7, 2020

etienne-lms Aug 7, 2020

etienne-lms Aug 5, 2020

etienne-lms Aug 5, 2020

github-actions bot commented Sep 21, 2020

		@@ -0,0 +1,132 @@
		#!/bin/bash

		DIR="`pwd`"

scripts: coccinelle #3991

Are you sure you want to change the base?

scripts: coccinelle #3991

Conversation

AndreaTagliavini commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020 • edited

jforissier commented Jul 22, 2020

AndreaTagliavini commented Jul 22, 2020

AndreaTagliavini commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

jenswi-linaro commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2020

jenswi-linaro commented Jul 22, 2020 •

edited