add sparse build support by keqiaozhang · Pull Request #5984 · thesofproject/sof

keqiaozhang · 2022-07-05T07:59:45Z

No description provided.

add a function to get overlays of each platform. Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>

add a new option -s/--sparse to support sparse build for Zephyr. Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>

lgirdwood

Not a python expert, but looks fine from a quick look. @marc-hb any comments ?

marc-hb

50 lines of code is a lot to just build with -DSPARSE=y.

I don't think this belongs to this script, I see almost nothing in common with the rest of the script.

Sparse is never going to work on Windows, we don't need the overhead and verbosity of python to run a few west commands, a shell script is enough.

The iteration over all platforms can be done only in CI, it does not need to be in this repo (it can be in .github/)

We don't need full overlay flexibility to build sparse; we will always stick to a couple of configurations so they can be hardcoded. If someone wants to try something locally they can edit the hardcoding locally.

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

marc-hb · 2022-07-07T00:31:03Z

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:

Move the SOF app from zephyr to where it belongs #6004

EDIT2: adding link to:

Dockerfile: Install sparse package for code analysis zephyrproject-rtos/docker-image#109

miRoox · 2022-07-11T07:00:47Z

+ sparse_build_cmd.append('--')
+ if args.cmake_args:
+ build_cmd += args.cmake_args
+ sparse_build_cmd += ["-DSPARSE=y"]


Is it possible to just pass -DSPARSE=y to cmake_args in the command line when we want Sparse build.

@lyakh I assume Zephyr has a build option to enable sparse at the top level we should reuse here ? Can you comment.

west (= Zephyr's "top-level") has the ability to passthrough anything to CMake, which is what @keqiaozhang did here: https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#one-time-cmake-arguments
This is infinitely scalable and saves west the need to add a new west --sparse option and then a new west --something option and then west --something-else etc.
I think west build ... -- too_long_list_of_overalds -DSPARSE=y is correct, I also think it would be much less code in a semi-hardcoded CI script. No need to add it to this python script (that the Zephyr team does not like)
In the future too_long_list_of_overlays should be gone and usage of this script reduced.

@marc-hb ok, thanks - topic for Wednesday call.

@lyakh I assume Zephyr has a build option to enable sparse at the top level we should reuse here ? Can you comment.
@lgirdwood sorry, I thought I replied to this: that is the correct way to build with sparse support - by adding the -DSPARSE=y key to cmake

aborisovich · 2022-08-18T15:28:21Z

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:

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

Please move Sparse build to CMake

marc-hb · 2022-08-18T20:14:26Z

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

lyakh · 2022-08-19T07:39:20Z

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?

lgirdwood · 2022-08-19T09:01:00Z

@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

aborisovich · 2022-08-19T22:46:20Z

@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.

lgirdwood · 2022-08-21T08:52:06Z

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.

lyakh · 2022-08-26T08:58:12Z

@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!

lgirdwood · 2022-08-31T11:02:33Z

@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.

marc-hb · 2022-08-31T22:25:40Z

I'll submit something tomorrow at the latest

EDIT: first step in

New sof/zephyr/docker-run.sh script (to help with sparse and other --cmake-args) #6232

marc-hb · 2022-08-31T23:07:42Z

@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)

lyakh · 2022-09-01T06:11:57Z

@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...

marc-hb · 2022-09-01T17:54:43Z

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.

lyakh · 2022-09-02T05:50:11Z

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 · 2022-09-02T05:57:15Z

@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 · 2022-09-02T06:15:45Z

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

marc-hb · 2022-09-02T17:59:00Z

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.

lgirdwood · 2022-09-07T21:03:18Z

@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 ?

marc-hb · 2022-09-07T21:41:30Z

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.

keqiaozhang · 2022-09-13T07:25:21Z

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

keqiaozhang · 2022-09-13T08:12:10Z

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

install the latest zephyr-sdk-0.15.0
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
update zephyr project: ./script/xtensa-build-zephyr.py -u
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.

lyakh · 2022-09-13T10:48:24Z

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?

aborisovich · 2022-09-15T18:16:46Z

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

marc-hb · 2022-09-15T20:50:17Z

I will have a look today.

marc-hb · 2022-09-16T23:20:01Z

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 · 2022-09-17T01:54:44Z

DONE !!!!

[SKIP CI] Add sparse build to Github Actions #6311

lyakh · 2022-09-19T06:17:42Z

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.

marc-hb · 2022-09-27T09:51:04Z

@keqiaozhang close?

keqiaozhang · 2022-09-27T10:16:04Z

@keqiaozhang close?

Yes, closing this PR.

xtensa-build-zephyr.py: move overlays code to a function
518433e
add a function to get overlays of each platform. Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>

keqiaozhang requested review from aborisovich and marc-hb as code owners July 5, 2022 07:59

xtensa-build-zephyr.py: add sparse build support
81cb27d
add a new option -s/--sparse to support sparse build for Zephyr. Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>

keqiaozhang force-pushed the sparse branch from 9b96904 to 81cb27dCompare July 5, 2022 08:26

lgirdwood reviewed Jul 5, 2022
View reviewed changes

marc-hb requested changes Jul 5, 2022
View reviewed changes

miRoox reviewed Jul 11, 2022
View reviewed changes

aborisovich suggested changes Aug 18, 2022
View reviewed changes

marc-hb self-assigned this Aug 31, 2022

marc-hb marked this pull request as draft September 9, 2022 18:43

aborisovich self-requested a review September 15, 2022 18:01

keqiaozhang closed this Sep 27, 2022

marc-hb mentioned this pull request Oct 28, 2022
[FEATURE] Add sparse build to CI #6317
Closed

Uh oh!

Conversation

keqiaozhang commented Jul 5, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Jul 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miRooxJul 11, 2022

Choose a reason for hiding this comment

Uh oh!

lgirdwoodJul 11, 2022

Choose a reason for hiding this comment

Uh oh!

marc-hbJul 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwoodJul 12, 2022

Choose a reason for hiding this comment

Uh oh!

lyakhAug 19, 2022

Choose a reason for hiding this comment

Uh oh!

aborisovich commented Aug 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aborisovich left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Aug 18, 2022

Uh oh!

lyakh commented Aug 19, 2022

Uh oh!

lgirdwood commented Aug 19, 2022

Uh oh!

aborisovich commented Aug 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Aug 21, 2022

Uh oh!

lyakh commented Aug 26, 2022

Uh oh!

lgirdwood commented Aug 31, 2022

Uh oh!

marc-hb commented Aug 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Aug 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Sep 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Sep 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Sep 2, 2022

Uh oh!

lyakh commented Sep 2, 2022

Uh oh!

lyakh commented Sep 2, 2022

Uh oh!

marc-hb commented Sep 2, 2022

Uh oh!

lgirdwood commented Sep 7, 2022

Uh oh!

marc-hb commented Sep 7, 2022

Uh oh!

keqiaozhang commented Sep 13, 2022

Uh oh!

keqiaozhang commented Sep 13, 2022

Uh oh!

lyakh commented Sep 13, 2022

marc-hb left a comment •
edited
Loading

marc-hb commented Jul 7, 2022•
edited
Loading

marc-hbJul 11, 2022•
edited
Loading

aborisovich commented Aug 18, 2022•
edited
Loading

aborisovich commented Aug 19, 2022•
edited
Loading

marc-hb commented Aug 31, 2022•
edited
Loading

marc-hb commented Aug 31, 2022•
edited
Loading

lyakh commented Sep 1, 2022•
edited
Loading

marc-hb commented Sep 1, 2022•
edited
Loading

marc-hb commented Sep 16, 2022•
edited
Loading