search

thesofproject / sof

Sound Open Firmware
Other
533 stars 307 forks source link

[FEATURE] Add sparse build to CI #6317

Closed lyakh closed 1 year ago

lyakh commented 2 years ago

Is your feature request related to a problem? Please describe. We added sparse support to Zephyr and we are in the process of adding it to SOF, but without running automated sparse builds on PRs we are guaranteed to regularly break sparse-guarded features like cached vs. uncached memory disambiguation.

Describe the solution you'd like A Zephyr+sparse build has to be executed for each PR along the lines of

west build -d tgl-sparse -b intel_adsp_cavs25 zephyr/samples/subsys/audio/sof -- -DSPARSE=y

Output has to be verified for regressions. It must be checked, that no sparse errors are added. Additionally we'll have to gradually verify and fix all currently reported sparse warnings.

Additional context Note, that Xtensa support has only recently been added to upstream sparse, so we need to use the git version from https://git.kernel.org/pub/scm/devel/sparse/sparse.git/ instead of any distribution-provided packages.

marc-hb commented 2 years ago

Thanks @lyakh for filing this! Some questions:

  1. Is using sparse from git + the west build command you gave enough to run sparse on Zephyr now? No other dependency, command or magic required?
  2. Is the current sparse run mostly clean for Zephyr? If not, what is the order of magnitude of the number of errors left to fix?
  3. You wrote "... and we are in the process of adding it to SOF" but your west build command already points at zephyr/sample/subsys/audio/sof which includes SOF code. Does "adding it to SOF" require a different command/process?

Before anything gets automated, it must be easy and well defined to run manually first.

lyakh commented 2 years ago

Thanks @lyakh for filing this! Some questions:

  1. Is using sparse from git + the west build command you gave enough to run sparse on Zephyr now? No other dependency, command or magic required?

@marc-hb I think so, yes, haven't tried to set up a new testing environment from scratch in a while though

  1. Is the current sparse run mostly clean for Zephyr? If not, what is the order of magnitude of the number of errors left to fix?

At least when I added sparse to Zephyr I fixed all errors that popped up in an SOF build. And since Zephyr also uses other static code analysers their code looks mostly fine for those, yes.

  1. You wrote "... and we are in the process of adding it to SOF" but your west build command already points at zephyr/sample/subsys/audio/sof which includes SOF code. Does "adding it to SOF" require a different command/process?

No, no different command or process. You can build SOF with Zephyr with sparse using that command now. By "in the process" I meant that we're adding uses of sparse constructs, namely of sparse address spaces. But even without those you can already build SOF with Zephyr with sparse.

Before anything gets automated, it must be easy and well defined to run manually first.

fredoh9 commented 2 years ago

Enhancement and P1 is not usual case. If this is P1, we need a assignee. @greg-intel will you take this?

marc-hb commented 2 years ago

Enhancement and P1 is not usual case

I'm not a sparse expert but I heard it can spot elusive caching issues that tend to require weeks-long task forces otherwise. At least that's how I understand the sales pitch :-)

greg-intel commented 2 years ago

@fredoh9 It was set to P1 per the weekly sync, as an important issue and one planned to be completed for this quarter (I believe, the other two requests are specifically mentioned as P2 Q4).

I thought people more familiar with this had already volunteered, but thanks for the quick explanation, I think I understand at a high level, but a variety of the smaller details are missing.

marc-hb commented 2 years ago

Is using sparse from git + the west build command you gave enough to run sparse on Zephyr now? No other dependency, command or magic required?

@marc-hb I think so, yes, haven't tried to set up a new testing environment from scratch in a while though

There is a golden, "from scratch", reference environment always available: the official zephyr-build container maintained by the Zephyr project and used by multiple CIs including SOF CI. I just tried running west build -d tgl-sparse -b intel_adsp_cavs25 zephyr/samples/subsys/audio/sof -- -DSPARSE=y in it and it failed in an unexpected way. west build -d tgl-sparse -b intel_adsp_cavs25 zephyr/samples/subsys/audio/sof works fine in the exact same environement (after deleting the stale tgl-sparse/ build directory). @lyakh can you please find what's wrong?

docker pull zephyrprojectrtos/zephyr-build
# in a different window while the above is running
west init zeptest
cd zeptest
git log --oneline
d1d5acd2cd (HEAD -> main, origin/main, origin/HEAD) coding guidelines: comply with MISRA C:2012 Rule 8.2
05ec016157 MAINTAINERS: Add keith-packard as C library collaborator
d326683b56 MAINTAINERS: Add picolibc entry

west update sof hal_extensa
cd ..
docker run -u 1000 -ti -v "$(pwd)":/workdir docker.io/zephyrprojectrtos/zephyr-build:latest /bin/bash
chown -R 1000 zeptest
cd zeptest

export ZEPHYR_BASE=/workdir/zeptest/zephyr

# just works, obviously because it's routinely tested in CI
west build -d tgl-sparse -b intel_adsp_cavs25 zephyr/samples/subsys/audio/sof

# fails
rm -rf tgl-sparse
west build -d tgl-sparse -b intel_adsp_cavs25 zephyr/samples/subsys/audio/sof -- -DSPARSE=y

-- Generated device_extern.h: /workdir/zeptest/tgl-sparse/zephyr/include/generated/device_extern.h
-- Including generated dts.cmake file: /workdir/zeptest/tgl-sparse/zephyr/dts.cmake
Parsing /workdir/zeptest/zephyr/Kconfig
Loaded configuration '/workdir/zeptest/zephyr/boards/xtensa/intel_adsp_cavs25/intel_adsp_cavs25_defconfig'
Merged configuration '/workdir/zeptest/zephyr/samples/subsys/audio/sof/prj.conf'
Merged configuration '/workdir/zeptest/zephyr/samples/subsys/audio/sof/boards/intel_adsp_cavs25.conf'
Configuration saved to '/workdir/zeptest/tgl-sparse/zephyr/.config'
Kconfig header saved to '/workdir/zeptest/tgl-sparse/zephyr/include/generated/autoconf.h'

warning: DAI_SSP_HAS_POWER_CONTROL (defined at drivers/dai/intel/ssp/Kconfig.ssp:16) was assigned
the value 'y' but got the value 'n'. Check these unsatisfied dependencies: DAI_INTEL_SSP (=n), DAI
(=n). See http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_DAI_SSP_HAS_POWER_CONTROL and/or
look up DAI_SSP_HAS_POWER_CONTROL in the menuconfig/guiconfig interface. The Application Development
Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual
might be helpful too.

CMake Error at /workdir/zeptest/zephyr/cmake/compiler/gcc/target.cmake:17 (message):
  C compiler
  /opt/toolchains/zephyr-sdk-0.14.2/xtensa-intel_s1000_zephyr-elf/bin/xtensa-intel_s1000_zephyr-elf-gcc
  not found - Please check your toolchain installation
Call Stack (most recent call first):
  /workdir/zeptest/zephyr/cmake/modules/target_toolchain.cmake:63 (include)
  /workdir/zeptest/zephyr/cmake/modules/zephyr_default.cmake:121 (include)
  /workdir/zeptest/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /workdir/zeptest/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:91 (include_boilerplate)
  CMakeLists.txt:5 (find_package)

-- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: /usr/local/bin/cmake -DWEST_PYTHON=/usr/bin/python3 -B/workdir/zeptest/tgl-sparse -S/workdir/zeptest/zephyr/samples/subsys/audio/sof -GNinja -DBOARD=intel_adsp_cavs25 -DSPARSE=y

/opt/toolchains/zephyr-sdk-0.14.2/xtensa-intel_s1000_zephyr-elf/bin/xtensa-intel_s1000_zephyr-elf-gcc does exist in the container.

lyakh commented 2 years ago

@marc-hb right, the error message comes out misleading. sparse isn't installed. As explained, you have to install it manually in your docker image from git. For an explanation (and eventual fox for the error message) see this snippet https://github.com/zephyrproject-rtos/zephyr/blob/ee22ae79a4aaa2efb90580888737257724cb5e7d/cmake/compiler/gcc/target.cmake#L8-L18

marc-hb commented 2 years ago

Wow, this error message is one of the most irrelevant-looking I've ever seen. Could you file a new Zephyr bug about it? The Zephyr project has incredibly good CMake experts; we can ask them for ideas on how to fix this error message.

~Also, can you please add sparse to this list?~ It's a one-line change. https://github.com/zephyrproject-rtos/docker-image/blob/master/Dockerfile

After a manual apt install sparse in the container the build now fails like this below, it looks like it's not using an appropriate toolchain?

Before adding sparse to CI it needs to be reasonably easy to use outside CI. Failures in CI needs to be reproducible outside CI, otherwise CI is just wasting energy producing reports that can't be acted upon.

[1/260] Building C object zephyr/CMakeFiles/offsets.dir/arch/xtensa/core/offsets/offsets.c.obj
FAILED: zephyr/CMakeFiles/offsets.dir/arch/xtensa/core/offsets/offsets.c.obj 
ccache /usr/bin/cgcc -DKERNEL -D_FORTIFY_SOURCE=2 -D__ZEPHYR__=1 -I/workdir/zeptest/zephyr/kernel/include -I/workdir/zeptest/zephyr/arch/xtensa/include -I/workdir/zeptest/zephyr/include/zephyr -I/workdir/zeptest/zephyr/include -Izephyr/include/generated -I/workdir/zeptest/zephyr/soc/xtensa/intel_adsp/cavs_v15 -I/workdir/zeptest/zephyr/soc/xtensa/intel_adsp/common/include -I/workdir/zeptest/modules/hal/xtensa/include -I/workdir/zeptest/modules/hal/xtensa/zephyr/soc/intel_apl_adsp -I/workdir/zeptest/modules/audio/sof/zephyr/include -I/workdir/zeptest/modules/audio/sof/zephyr/../src/platform/intel/cavs/include -I/workdir/zeptest/modules/audio/sof/zephyr/../src/platform/apollolake/include -isystem /workdir/zeptest/zephyr/lib/libc/minimal/include -isystem /opt/toolchains/zephyr-sdk-0.14.1/xtensa-intel_apl_adsp_zephyr-elf/bin/../lib/gcc/xtensa-intel_apl_adsp_zephyr-elf/10.3.0/include -isystem /opt/toolchains/zephyr-sdk-0.14.1/xtensa-intel_apl_adsp_zephyr-elf/bin/../lib/gcc/xtensa-intel_apl_adsp_zephyr-elf/10.3.0/include-fixed -O2 -imacros /workdir/zeptest/tgl-sparse/zephyr/include/generated/autoconf.h -ffreestanding -fno-common -g -gdwarf-4 -fdiagnostics-color=always --sysroot=/opt/toolchains/zephyr-sdk-0.14.1/xtensa-intel_apl_adsp_zephyr-elf/xtensa-intel_apl_adsp_zephyr-elf -D__CHECKER__ -Wno-attributes -imacros /workdir/zeptest/zephyr/include/zephyr/toolchain/zephyr_stdint.h -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wno-main -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-asynchronous-unwind-tables -fno-pie -fno-pic -fstrict-overflow -fno-reorder-functions -fno-defer-pop -fmacro-prefix-map=/workdir/zeptest/zephyr/samples/subsys/audio/sof=CMAKE_SOURCE_DIR -fmacro-prefix-map=/workdir/zeptest/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/workdir/zeptest=WEST_TOPDIR -ffunction-sections -fdata-sections -mlongcalls -std=c99 -nostdinc -MD -MT zephyr/CMakeFiles/offsets.dir/arch/xtensa/core/offsets/offsets.c.obj -MF zephyr/CMakeFiles/offsets.dir/arch/xtensa/core/offsets/offsets.c.obj.d -o zephyr/CMakeFiles/offsets.dir/arch/xtensa/core/offsets/offsets.c.obj -c /workdir/zeptest/zephyr/arch/xtensa/core/offsets/offsets.c
/workdir/zeptest/modules/hal/xtensa/include/xtensa/config/core.h:54:10: error: unable to open 'core-isa.h'
nklayman commented 2 years ago

After a manual apt install sparse in the container the build now fails like this below, it looks like it's not using an appropriate toolchain?

@marc-hb I think you need to build sparse from source in order for it to support the xtensa architecture, as per @lyakh's earlier comment:

Note, that Xtensa support has only recently been added to upstream sparse, so we need to use the git version from https://git.kernel.org/pub/scm/devel/sparse/sparse.git/ instead of any distribution-provided packages.

That would explain why you got an error about using the wrong toolchain.

marc-hb commented 2 years ago

Thanks @nklayman , good catch.

@lyakh, from the top of your head could you share a quick and barely tested set of commands that would build sparse inside the container?

lyakh commented 2 years ago

Thanks @nklayman , good catch.

@lyakh, from the top of your head could you share a quick and barely tested set of commands that would build sparse inside the container?

@marc-hb

git clone https://git.kernel.org/pub/scm/devel/sparse/sparse.git/
cd sparse
make
make install
keqiaozhang commented 2 years ago

@lyakh I have installed the sparse, but run to below errors:

-- west build: building application
[1/208] Performing build step for 'smex_ep'
ninja: no work to do.
[4/208] Performing build step for 'sof_logger_ep'
ninja: no work to do.
[10/208] Generating linker_zephyr_pre0.cmd
FAILED: zephyr/linker_zephyr_pre0.cmd /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/linker_zephyr_pre0.cmd
cd /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr && -x assembler-with-cpp -MD -MF linker_zephyr_pre0.cmd.dep -MT linker_zephyr_pre0.cmd -D_LINKER -D_ASMLANGUAGE -imacros /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/include/generated/autoconf.h -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/include/zephyr -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/include -I/home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/include/generated -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/soc/xtensa/intel_adsp/cavs_v25 -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/soc/xtensa/intel_adsp/common/include -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/lib/libc/minimal/include -I/home/keqiao/works/sof-firmware/sof/zephyrproject/modules/hal/atmel/include -I/home/keqiao/works/sof-firmware/sof/zephyr/include -I/home/keqiao/works/sof-firmware/sof/zephyr/../src/platform/intel/cavs/include -I/home/keqiao/works/sof-firmware/sof/zephyr/../src/platform/tigerlake/include -D__GCC_LINKER_CMD__ -DLINKER_DEVICE_HANDLES_PASS1 -E /home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/soc/xtensa/intel_adsp/cavs_v25/linker.ld -P -o linker_zephyr_pre0.cmd && /opt/cmake-3.22.1-linux-x86_64/bin/cmake -E cmake_transform_depfile Ninja gccdepfile /home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/samples/subsys/audio/sof /home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/linker_zephyr_pre0.cmd.dep /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/CMakeFiles/d/c29bd3ec24fe7cb67e788d7df7eda5767658a00aa80f6ad5dd2e3dd8d70c50bc.d
/bin/sh: 1: -x: not found
[11/208] Generating linker_zephyr_pre1.cmd
FAILED: zephyr/linker_zephyr_pre1.cmd /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/linker_zephyr_pre1.cmd
cd /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr && -x assembler-with-cpp -MD -MF linker_zephyr_pre1.cmd.dep -MT linker_zephyr_pre1.cmd -D_LINKER -D_ASMLANGUAGE -imacros /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/include/generated/autoconf.h -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/include/zephyr -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/include -I/home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/include/generated -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/soc/xtensa/intel_adsp/cavs_v25 -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/soc/xtensa/intel_adsp/common/include -I/home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/lib/libc/minimal/include -I/home/keqiao/works/sof-firmware/sof/zephyrproject/modules/hal/atmel/include -I/home/keqiao/works/sof-firmware/sof/zephyr/include -I/home/keqiao/works/sof-firmware/sof/zephyr/../src/platform/intel/cavs/include -I/home/keqiao/works/sof-firmware/sof/zephyr/../src/platform/tigerlake/include -D__GCC_LINKER_CMD__ -DLINKER_ZEPHYR_PREBUILT -E /home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/soc/xtensa/intel_adsp/cavs_v25/linker.ld -P -o linker_zephyr_pre1.cmd && /opt/cmake-3.22.1-linux-x86_64/bin/cmake -E cmake_transform_depfile Ninja gccdepfile /home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr/samples/subsys/audio/sof /home/keqiao/works/sof-firmware/sof/zephyrproject/zephyr /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/zephyr/linker_zephyr_pre1.cmd.dep /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse/CMakeFiles/d/c42cff813fb760f05812ad8d397a5bda6d51365338c80a11420fc4193f6a4808.d
/bin/sh: 1: -x: not found
[19/208] Building C object zephyr/arch/arch/xtensa/core/CMakeFiles/arch__xtensa__core.dir/xtensa-asm2.c.obj
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /usr/local/bin/cmake --build /home/keqiao/works/sof-firmware/sof/zephyrproject/tgl-sparse

I updated the llvm and west to latest, but didn't help. Any ideas?

keqiaozhang commented 2 years ago

@lyakh
I guess you use the default Zephyr SDK gcc xtensa-intel_s1000_zephyr-elf-gcc to build zephyr with Sparse. The default gcc also works for me without errors. The above errors are generated by xcc.

lyakh commented 2 years ago

@lyakh I guess you use the default Zephyr SDK gcc xtensa-intel_s1000_zephyr-elf-gcc to build zephyr with Sparse. The default gcc also works for me without errors. The above errors are generated by xcc.

@keqiaozhang yes, this is compiler-sensitive and since it's a Zephyr feature I think the main target would be the Zephyr toolchain. And I don't think we need to use xcc for this. We use this to get sparse output - warnings and errors, we don't actually care about compilation results.

keqiaozhang commented 2 years ago

@lyakh Thanks for explanation. I will add a GitHub action to check the sparse build in CI next. One tip, we need to set the REAL_CC to Zephyr SDK toolchain, this will let the cgcc use Zephyr SDK toolchain as the compiler to invoke. export REAL_CC=/$YOUR_PATH/zephyr-sdk-0.14.2/xtensa-intel_s1000_zephyr-elf/bin/xtensa-intel_s1000_zephyr-elf-gcc

marc-hb commented 1 year ago

EDIT: first attempt was

Final PR submitted in

EDIT: not final after all, see new requirements scattered in