Skip to content

build system: enable support for static analysis #21411

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 22, 2025
Merged

build system: enable support for static analysis #21411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0069a6b
build system: enable support for static analysis
maribu Apr 14, 2025
e8edd95
dist/tools/compile_commands: add awareness for -fanalyzer
maribu Apr 14, 2025
eb6127e
dist/tools/zsh-completion: add awareness for STATIC_ANALYSIS
maribu Apr 14, 2025
0af3fa3
build system: Enable static analysis in the CI
maribu Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Makefile.base
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ ifneq (,$(SRCXX))
endif
endif

# If static analysis is enabled *and* the module claims to compile fine with it,
# add the corresponding flags
ifeq (1,$(STATIC_ANALYSIS))
ifeq (1,$(MODULE_SUPPORTS_STATIC_ANALYSIS))
CFLAGS := $(CFLAGS_STATIC_ANALYSIS) $(CFLAGS)
endif
endif

compile-commands: | $(DIRS:%=COMPILE-COMMANDS--%)
$(file >$(BINDIR)/$(MODULE)/compile_cmds.txt,SRC: $(sort $(SRC) $(SRC_NO_LTO)))
$(file >>$(BINDIR)/$(MODULE)/compile_cmds.txt,SRC_NO_LTO: $(sort $(SRC_NO_LTO)))
Expand Down
16 changes: 16 additions & 0 deletions Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,22 @@ ifeq ($(RIOT_CI_BUILD),1)
endif
# be more quiet when building for CI
QUIETER=1

# Enable static analysis only for some boards to not spent to much CPU time.
# This list is intentionally a subset of the `QUICKBUILD_BOARDS` from
# `.murdock`
Comment on lines +524 to +526
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to check it against QUICKBUILD_BOARDS? (As in: check that BOARDS_CI_STATIC_ANALYSIS is actually a subset and print a warning if it's not?) Maybe at some point that variable gets changed in .murdock and this list here will be forgotten.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this would not necessarily be an issue if not. It would only mean that static analysis may fail only in the full Murdock run prior merge.

It would probably be more convenient to keep them aligned, so that response time is faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly do you mean with "more convenient to keep them aligned"?

We could do something like this:

ifneq ($(BOARDS_CI_STATIC_ANALYSIS),$(filter $(BOARDS_CI_STATIC_ANALYSIS),$(QUICKBUILD_BOARDS)))
  $(warning BOARDS_CI_STATIC_ANALYSIS contains boards not covered by the CI quick build!)
endif

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the following:

diff --git a/Makefile.include b/Makefile.include
index 761186ca49..a30329baa6 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -536,6 +536,11 @@ ifeq ($(RIOT_CI_BUILD),1)
     else
       export STATIC_ANALYSIS := 0
     endif
+    BOARDS_CI_STATIC_ANALYSIS_NOT_IN_QUICKBUILD_BOARDS := \
+      $(filter-out $(QUICKBUILD_BOARDS),$(BOARDS_CI_STATIC_ANALYSIS))
+    ifneq (,$(BOARDS_CI_STATIC_ANALYSIS_NOT_IN_QUICKBUILD_BOARDS))
+      $(warning $(BOARDS_CI_STATIC_ANALYSIS_NOT_IN_QUICKBUILD_BOARDS) from BOARDS_CI_STATIC_ANALYSIS are not part of the quick build boards!)
+    endif
 endif
 
 ifeq ($(QUIETER),1)

which would result in the following warning:

make RIOT_CI_BUILD=1 -C examples/basic/hello-world
make: Entering directory '/home/maribu/Repos/software/RIOT/master/examples/basic/hello-world'
/home/maribu/Repos/software/RIOT/master/examples/basic/hello-world/../../../Makefile.include:542: esp32-wroom-32 native64 nrf52840dk samr21-xpro stm32f429i-disc1 from BOARDS_CI_STATIC_ANALYSIS are not part of the quick build boards!
Building application "hello-world" for "nucleo-f767zi" with CPU "stm32".

   text	  data	   bss	   dec	   hex	filename
   9544	   108	  2632	 12284	  2ffc	/home/maribu/Repos/software/RIOT/master/examples/basic/hello-world/bin/nucleo-f767zi/hello-world.elf
make: Leaving directory '/home/maribu/Repos/software/RIOT/master/examples/basic/hello-world'

The problem is that one variable is from the shell script .murdock, the other from Makefile.include.

What exactly do you mean with "more convenient to keep them aligned"?

If there would be a board for in BOARDS_CI_STATIC_ANALYSIS that is not a quick build board, a contributor would only notice the failure on the final Murdock run, after a PR has been ACKed and send to the merge queue.

But having PRs being rejected last minuted is by design, as otherwise the quick build Murdock run wouldn't be quick anymore. (And the quick build board are picked in a way to be as representative as possible, so that the chance of a build failure in the long Murdock run while it succeeded the short one is pretty low, except for out of memory issues.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving forward: I think adding a check to ensure that BOARDS_CI_STATIC_ANALYSIS is a subset of QUICKBUILD_BOARDS will probably produce more maintainability overhead than benefit. If we get annoyed with static analysis failing last minute too often, we can update either the QUICKBUILD_BOARDS or the BOARDS_CI_STATIC_ANALYSIS to overlap again and better represent the boards we have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thougt about this some more and considering we know who to poke when it causes issues, we can go forward with this solution 😋

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fair :-)

BOARDS_CI_STATIC_ANALYSIS := \
esp32-wroom-32 \
native64 \
nrf52840dk \
samr21-xpro \
stm32f429i-disc1 \
#
ifneq (,$(filter $(BOARD),$(BOARDS_CI_STATIC_ANALYSIS)))
export STATIC_ANALYSIS := 1
else
export STATIC_ANALYSIS := 0
endif
endif

ifeq ($(QUIETER),1)
Expand Down
3 changes: 3 additions & 0 deletions core/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,7 @@ SRC := $(filter-out mbox.c msg.c msg_bus.c thread.c thread_flags.c,$(wildcard *.
# enable submodules
SUBMODULES := 1

# this module is expected to pass static analysis
MODULE_SUPPORTS_STATIC_ANALYSIS := 1

include $(RIOTBASE)/Makefile.base
2 changes: 2 additions & 0 deletions dist/tools/compile_commands/compile_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,8 @@ def generate_compile_commands(args):
'-mlongcalls',
# GCC specific diagnostics: Tell GCC address space starts at 0
'--param=min-pagesize=0',
# -fanalyzer is GCC only
'-fanalyzer',
]
_args.filter_out.extend(flags)
generate_compile_commands(_args)
3 changes: 2 additions & 1 deletion dist/tools/zsh-completion/zsh-riot.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
function _boards {

Check warning on line 1 in dist/tools/zsh-completion/zsh-riot.sh

View workflow job for this annotation

GitHub Actions / static-tests 

The mentioned syntax error was in this brace group. [SC1009]
local -a _boards_available
local -a _board_dirs
_board_dirs=( ${(s[ ])EXTERNAL_BOARD_DIRS} )
Expand All @@ -7,7 +7,7 @@
local _repo_root="$(git rev-parse --show-toplevel)"
_board_dirs+=("$_repo_root/boards")
fi
for dir ("$_board_dirs[@]") \

Check failure on line 10 in dist/tools/zsh-completion/zsh-riot.sh

View workflow job for this annotation

GitHub Actions / static-tests 

Expected 'do'. [SC1058]

Check failure on line 10 in dist/tools/zsh-completion/zsh-riot.sh

View workflow job for this annotation

GitHub Actions / static-tests 

Expected 'do'. Fix any mentioned problems and try again. [SC1072]

Check failure on line 10 in dist/tools/zsh-completion/zsh-riot.sh

View workflow job for this annotation

GitHub Actions / static-tests 

Couldn't parse this for loop. Fix to allow more checks. [SC1073]
_boards_available+=($(ls $dir | grep -v common))

_describe 'board' _boards_available
Expand Down Expand Up @@ -95,7 +95,7 @@
"info-buildsize:print the size of the firmware for the given board"
"info-buildsizes-diff:compare the size of two firmware builds for two given directories"
"info-cpu:print the CPU family for the given board"
"info-features-missing:list features missing by the given baord in regard to the given app"
"info-features-missing:list features missing by the given board in regard to the given app"
"info-features-provided:list features provided by the given board"
"info-features-required:list features required by the given app"
"info-features-used:list features of the given board used by the given app"
Expand Down Expand Up @@ -131,6 +131,7 @@
'PROGRAMMER[Select the programmer software to flash (debug) with]:programmer:_programmers'
'OPENOCD_DEBUG_ADAPTER[Select the programmer hardware to use with OpenOCD]:hw_programmer:_hw_programmers'
'OPENOCD_RESET_USE_CONNECT_ASSERT_SRST[Let OpenOCD attach while reset signal is asserted]:bool:_bools'
'STATIC_ANALYSIS[Enable static analysis for modules that claim support]:bool:_bools'
)

_values -w 'variables' $vars
Expand Down
3 changes: 3 additions & 0 deletions makefiles/toolchain/gnu.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ OPTIONAL_CFLAGS_BLACKLIST += -Wdocumentation -Wno-error=documentation -Wno-docum
# We use GDB for debugging
include $(RIOTMAKE)/tools/gdb.inc.mk

# GCC's static analysis tools can be enabled using -fanalyzer
CFLAGS_STATIC_ANALYSIS := -fanalyzer

# Data address spaces starts at zero for all supported architectures. This fixes
# compilation at least on MSP430 and AVR.
# See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105523
Expand Down
1 change: 1 addition & 0 deletions makefiles/vars.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export CXX # The CXX compiler to use.
export CCAS # The C compiler to use for assembler files, typically the same as CC.
export CFLAGS # The compiler flags. Must only ever be used with `+=`.
export CFLAGS_CPU # CPU architecture specific compiler flags
export CFLAGS_STATIC_ANALYSIS# Additional CFLAGS to use when static analysis should be enabled
export CXXUWFLAGS # (Patterns of) flags in CFLAGS that should not be passed to CXX.
export CXXEXFLAGS # Additional flags that should be passed to CXX.
export CCASUWFLAGS # (Patterns of) flags in CFLAGS that should not be passed to CCAS.
Expand Down
Loading