sof icon indicating copy to clipboard operation
sof copied to clipboard

add sparse build support

Open keqiaozhang opened this issue 3 years ago • 7 comments

keqiaozhang avatar Jul 05 '22 07:07 keqiaozhang

We should try to make this wrapper script smaller, not bigger; moving features to Zephyr's CMake and the rest of the Zephyr build.

This was confirmed in a meeting this morning: a lot of the overlays logic in this script should be moved ASAP to Zephyr "applications" (whatever that means, I believe @aborisovich understands this well). Wrapper scripts add layers of obfuscations and the less often people have to use this script and the more often they can invoke west directly, the better. west is already a wrapper! On top of git and CMake, which is itself on top of ninja...

EDIT1: it's started happening now:

  • https://github.com/thesofproject/sof/pull/6004

EDIT2: adding link to:

  • https://github.com/zephyrproject-rtos/docker-image/pull/109

marc-hb avatar Jul 07 '22 00:07 marc-hb

What is the goal/need of building sparse? Is this a requirement for all Xtensa architecture platforms? Is this sparse build should occur always or be a switchable option? If so this should be done on much lower level in CMake files not as part of python script. There is already a -C flag in ./scripts/xtensa-build-zephyr.py: image

So, support for sparse build should be done by just calling a command: ./sof/scripts/xtensa-build-zephyr.py <platform> -C '-DBUILD_SPARSE=ON'

Then, in ./sof/src/arch/xtensa/CMakeLists.txt you should use: if(BUILD_SPARSE): CMake FetchContent (to clone sparse at configure time) and then FetchContent_MakeAvailable . Have a look at how GTest framework does it Googletest quickstart - set up a project . Alternatively you could try have a go with CMake ExternalProject_Add. It looks easier to implement but less convenient to use because this just creates a new target during Cmake configure and actual clone, build and all operations are happening at Cmake build time (what can be troublesome if you wish to alter Cmake configure steps of external projects).

aborisovich avatar Aug 18 '22 15:08 aborisovich

I think @keqiaozhang has already been working on a very different alternative

marc-hb avatar Aug 18 '22 20:08 marc-hb

What is the goal/need of building sparse? Is this a requirement for all Xtensa architecture platforms? Is this sparse build should occur always or be a switchable option?

@aborisovich my proposal would be to build it for each PR but we could start with just one configuration, e.g. TGL. Ideally we need an "all-yes-config" for this - to build as much of the source base as possible. Then we should add builds for all other architectures - those, that support Zephyr building. I understand, that we don't get a 100% code base coverage unless we actually build all platforms, so that should be something we have to consider too. And yes, I haven't worked with github CI hooks, but I'd expect it to be a separate GH CI "pipeline" (sorry for a gitlab-ism). And - most importantly - we need to parse an evaluate the resulting log - not just check the return code. But that can also be achieved with simple grep commands. We do already have a "GitHub Actions / gcc-build-only" test, maybe we could extend that with this, or even replace plane gcc building with Zephyr+sparse?

lyakh avatar Aug 19 '22 07:08 lyakh

@keqiaozhang @lyakh @aborisovich can we have something simple and very similar to the Linux sparse invocation which everyone understands and uses make (cmake in our case). This would also need to align with Zephyr. https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html#using-sparse

lgirdwood avatar Aug 19 '22 09:08 lgirdwood

@keqiaozhang @lyakh @aborisovich can we have something simple and very similar to the Linux sparse invocation which everyone understands and uses make (cmake in our case). This would also need to align with Zephyr. https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html#using-sparse

Well I would argue that adding C=1 to make command is intuitive solution... (I'm referring to the link you send). Whatever @keqiaozhang will come up with it will has to be somehow integrated with CMake (unless Sparse is pure Makefile project). I think quite reasonable alternative solution is introducing more west build commands to SOF by adding new west extension commands. Zephyr project gives us "west build" to build a board but we can add out build-sparse or build-tools extension... Just a thought.

aborisovich avatar Aug 19 '22 22:08 aborisovich

I think quite reasonable alternative solution is introducing more west build commands to SOF by adding new west extension commands. Zephyr project gives us "west build" to build a board but we can add out build-sparse or build-tools extension... Just a thought

I'm also good for west support, my ask is that it's a simple command line option.

lgirdwood avatar Aug 21 '22 08:08 lgirdwood

@aborisovich @marc-hb @keqiaozhang any progress with this? We really want sparse support in CI - we get code added regularly that involves buffer manipulation and having an automated check for its correctness would be very helpful!

lyakh avatar Aug 26 '22 08:08 lyakh

@aborisovich @marc-hb @keqiaozhang ping - do we have some alignment yet ? adding @andyross to see how we can all align Zephyr + SOF on SPARSE usage.

lgirdwood avatar Aug 31 '22 11:08 lgirdwood

I'll submit something tomorrow at the latest

EDIT: first step in

  • #6232

marc-hb avatar Aug 31 '22 22:08 marc-hb

@lyakh have you tried -DSPARSE=y with the most recent container and 0.15.0 toolchain?

EDIT: I tried with other platforms and docker image versions and they all fail like this.

In dir: /zep_workspace; running command: west build --build-dir build-tgl --board intel_adsp_cavs25 /zep_workspace/sof/app -p always -- -DSPARSE=y

-- Including generated dts.cmake file: /zep_workspace/build-tgl/zephyr/dts.cmake
Parsing /zep_workspace/zephyr/Kconfig
Loaded configuration '/zep_workspace/zephyr/boards/xtensa/intel_adsp_cavs25/intel_adsp_cavs25_defconfig'
Merged configuration '/zep_workspace/sof/app/prj.conf'
Merged configuration '/zep_workspace/sof/app/boards/intel_adsp_cavs25.conf'
Configuration saved to '/zep_workspace/build-tgl/zephyr/.config'
Kconfig header saved to '/zep_workspace/build-tgl/zephyr/include/generated/autoconf.h'
CMake Error at /zep_workspace/zephyr/cmake/compiler/gcc/target.cmake:17 (message):
  C compiler
  /opt/toolchains/zephyr-sdk-0.15.0/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):
  /zep_workspace/zephyr/cmake/modules/target_toolchain.cmake:63 (include)
  /zep_workspace/zephyr/cmake/modules/zephyr_default.cmake:121 (include)
  /zep_workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /zep_workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:91 (include_boilerplate)
  CMakeLists.txt:5 (find_package)

marc-hb avatar Aug 31 '22 23:08 marc-hb

@lyakh have you tried -DSPARSE=y with the most recent container and 0.15.0 toolchain?

I haven't, but have you also set the env variable? I.e.

export REAL_CC=${HOME}/zephyr-sdk/xtensa/intel_s1000/xtensa-zephyr-elf/bin/xtensa-zephyr-elf-gcc

I actually tried to automate that export in cmake but haven't been able to achieve that. I did add

set(ENV{REAL_CC} ${REAL_CC})

to cmake/compiler/gcc/target.cmake and at least during my testing it was needed too, but it wasn't enough, so both were needed...

lyakh avatar Sep 01 '22 06:09 lyakh

I haven't, but have you also set the env variable?

No I haven't. I'm not a sparse expert. Generally speaking CMake does not play well with environment variables (which are of course evil).

Can you please provide somewhere a complete and detailed list of manual steps to compile with sparse? It must be good and complete enough for any reasonably skilled developer to compile with sparse in less than 1h. Trying to automate something that can't be replicated manually is futile and jumping the gun.

marc-hb avatar Sep 01 '22 17:09 marc-hb

Can you please provide somewhere a complete and detailed list of manual steps to compile with sparse?

@marc-hb that's all, that's literally what I do to build with sparse:

export REAL_CC=$HOME/zephyr-sdk/xtensa/intel_s1000/xtensa-zephyr-elf/bin/xtensa-zephyr-elf-gcc
west build -d tgl-sparse -b intel_adsp_cavs25 modules/audio/sof/app -- -DSPARSE=y

if you still cannot get this to work, I guess I'll have to try to reproduce this in Zephyr's docker image myself

lyakh avatar Sep 02 '22 05:09 lyakh

@marc-hb also, looking at the error log above - is the path /opt/toolchains/zephyr-sdk-0.15.0/xtensa-intel_s1000_zephyr-elf/bin/xtensa-intel_s1000_zephyr-elf-gcc actually there?

lyakh avatar Sep 02 '22 05:09 lyakh

@marc-hb do I understand it correctly, that all your "sparse" questions are now resolved with #6232 and #6238 ?

lyakh avatar Sep 02 '22 06:09 lyakh

export REAL_CC=$HOME/zephyr-sdk/xtensa/intel_s1000/xtensa-zephyr-elf/bin/xtensa-zephyr-elf-gcc

There's no xtensa/intel_s1000/ directory anymore, that was changed months ago. Can you please switch to a recent Zephyr SDK and recent Zephyr main branch?

Don't bother with the container, I can handle the container but we need to all use reasonably recent parts.

is the path /opt/toolchains/zephyr-sdk-0.15.0/xtensa-intel_s1000_zephyr-elf/bin/xtensa-intel_s1000_zephyr-elf-gcc actually there?

Yes I checked it exists in the container yet CMake fails for some unknown reason. But before we even get into that please upgrade everything to weeks-old versions or even better: days-old versions. Nothing months old.

marc-hb avatar Sep 02 '22 17:09 marc-hb

@marc-hb do I understand it correctly, that all your "sparse" questions are now resolved with #6232 and #6238 ?

Are we good @marc-hb @aborisovich @keqiaozhang now ? Do we need to close this ?

lgirdwood avatar Sep 07 '22 21:09 lgirdwood

Sorry for hijacking this Pull Request and using it as an issue instead. It's been surprisingly convenient though.

I just had a chat with @lyakh about sparse a couple hours ago. Right now he's busy with other more urgent things so really not in a good place to upgrade his Zephyr SDK. Once he can and we are on the same page then we will resume the troubleshooting together.

In the mean time everyone else is more than welcome to give -DSPARSE=y and REAL_CC a try and report their experience here - with or without the container but only with reasonably (and identified) versions of everything please. See exact commands above.

marc-hb avatar Sep 07 '22 21:09 marc-hb

Sorry, I was busy with other tasks recently, I will try sparse build with latest SOF/Zephyr SDK.

keqiaozhang avatar Sep 13 '22 07:09 keqiaozhang

I just tried sparse build with latest sof + zephyr-sdk-0.15.0, build passed without errors. Here are the steps:

  1. install the latest zephyr-sdk-0.15.0
  2. set the REAL_CC: export REAL_CC=$INSTALL_PATH/zephyr-sdk-0.15.0/xtensa-intel_s1000_zephyr-elf/bin/xtensa-intel_s1000_zephyr-elf-gcc
  3. update zephyr project: ./script/xtensa-build-zephyr.py -u
  4. run sparse build: west build -d tgl-sparse -b intel_adsp_cavs25 app -- -DSPARSE=y

The output is attached. sparse.txt

I will try to add sparse build support in .github next.

keqiaozhang avatar Sep 13 '22 08:09 keqiaozhang

I just tried sparse build with latest sof + zephyr-sdk-0.15.0, build passed without errors.

@keqiaozhang thanks for checking, I just was going to write the same - updated the toolchain to the newest one, set $REAL_CC correctly, according to the new directory structure and sparse worked. @marc-hb is it because of the docker?

lyakh avatar Sep 13 '22 10:09 lyakh

What is current status on this? Anyone can sum up? Are we looking for alternatives?

aborisovich avatar Sep 15 '22 18:09 aborisovich

I will have a look today.

marc-hb avatar Sep 15 '22 20:09 marc-hb

Finally got it working in the container. Not sure what sort of hacks have been implemented to compile with sparse but the error handling in the sparse build is really the worst I've ever seen. I'll add an error message decoder ring in the PR

marc-hb avatar Sep 16 '22 23:09 marc-hb

DONE !!!!

  • #6311

marc-hb avatar Sep 17 '22 01:09 marc-hb

DONE !!!!

* [[SKIP CI] Add sparse build to Github Actions #6311](https://github.com/thesofproject/sof/pull/6311)

@marc-hb great, approved, thanks for investigating and fixing it! After that we should add checks for a couple of particularly important for us tests - commented there.

lyakh avatar Sep 19 '22 06:09 lyakh

@keqiaozhang close?

marc-hb avatar Sep 27 '22 09:09 marc-hb

@keqiaozhang close?

Yes, closing this PR.

keqiaozhang avatar Sep 27 '22 10:09 keqiaozhang