Repository navigation
[BUG] Cmake runs initially without CONFIG vars set #3617
Description
Describe the bug
When we build SOF for some reason the initial call to cmake does not include any configs. You can see the travis log below that make *defconfig is not called till after the first call to cmake. This is causing problems with refactoring as some of the variables are resolving to null and causing bugs.
Assuming there is a reason for this initial no config call, then we are calling into areas without configs like arch/xtensa which need those configs to run properly, which means we should be returning. Maybe we are just missing a check at the top of arch/xtensa to return if a xtensa config is not set?
To Reproduce
run xtensa-build-all.sh with a message to long your platform config. notice empty line in first run.
Reproduction Rate
How often does the issue happen ? i.e. 1/10 (once in ten attempts), 1/1000 or all the time.
100%
Expected behavior
Cmake should not run without the config
Impact
Blocks refactoring for fuzzer
Environment
- Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
- Kernel: N/A
- SOF: e7a339b
- Name of the topology file
- Topology: N/A
- Name of the platform(s) on which the bug is observed.
- Platform: N/A
Screenshots or console output
$ ./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -r $PLATFORM
Build in build_tgl_gcc
+ cmake -DTOOLCHAIN=xtensa-cnl-elf -DROOT_DIR=/home/sof/work/sof.git/../xtensa-root/xtensa-cnl-elf -DMEU_OPENSSL= '' -DRIMAGE_PRIVATE_KEY=/home/sof/work/sof.git/keys/otc_private_key_3k.pem ..
-- Preparing Xtensa toolchain
-- The C compiler identification is GNU 9.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /home/sof/work/xtensa-cnl-elf/bin/xtensa-cnl-elf-gcc
-- Linking git pre-commit hook
-- Linking git post-commit hook
-- Found Git: /usr/bin/git (found version "2.17.1")
-- GIT_TAG / GIT_LOG_HASH : v1.5-rc1-898-ge7a339bd4cc7 / e7a339bd4
-- Source content hash: f770dbfc
-- Generating /home/sof/work/sof.git/build_tgl_gcc/generated/include/version.h
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.6.9", minimum required is "3.0")
-- Configuring done
-- Generating done
-- Build files have been written to: /home/sof/work/sof.git/build_tgl_gcc
+ make tgplp_defconfig
Scanning dependencies of target tgplp_defconfig
Applying olddefconfig with tgplp_defconfig
Using existing configuration '.config' as base
Configuration written to '.config'
Built target tgplp_defconfig
-- Preparing Xtensa toolchain
-- GIT_TAG / GIT_LOG_HASH : v1.5-rc1-898-ge7a339bd4cc7 / e7a339bd4
-- Source content hash: f770dbfc
-- Up-to-date /home/sof/work/sof.git/build_tgl_gcc/generated/include/version.h
-- Configuring done
-- Generating done
-- Build files have been written to: /home/sof/work/sof.git/build_tgl_gcc
Scanning dependencies of target overrideconfig
Using existing configuration '.config' as base
Configuration written to '.config'
Built target overrideconfig
-- Preparing Xtensa toolchain
-- GIT_TAG / GIT_LOG_HASH : v1.5-rc1-898-ge7a339bd4cc7 / e7a339bd4
-- Source content hash: f770dbfc
-- Up-to-date /home/sof/work/sof.git/build_tgl_gcc/generated/include/version.h
-- Configuring done
-- Generating done
-- Build files have been written to: /home/sof/work/sof.git/build_tgl_gcc
Scanning dependencies of target genconfig
Activity
lgirdwood commentedon Nov 18, 2020
@jajanusz @marc-hb any inputs here ?
marc-hb commentedon Nov 18, 2020
I often invoke
cmakeandmake/ninjadirectly (not the least when... fixing them) and from a (lack of) user friendliness perspective I always found the intermediate and extradefconfigstep super annoying. Meta-build systems that build entire system images/distributions are very likely incompatible with such an intermediate step.Just for the record this extra defconfig step is official and documented:
https://thesofproject.github.io/latest/getting_started/build-guide/build-from-scratch.html#incremental-builds
What's even more annoying is I found no way to
cleanthe build directory without losing my.config! Must be saved and restored every time, really painful.So before even looking at the details of the specific issues listed by @cujomalainey I would absolutely love if the defconfig step were integrated in the CMake step. This is what Zephyr does BTW: https://docs.zephyrproject.org/2.3.0/guides/kconfig/setting.html#setting-symbols-in-configuration-files
Note this would of course require passing config information at
cmaketime, either withcmake -Dsomething=aplconfigor by switching to a source directory with some desired.conffiles. Zephyr supports the combination of both.@jajanusz was the current design a conscious choice or just the easiest way for you to implement it at the time?
cc: kconfig master @plbossart , @xiulipan
cujomalainey commentedon Nov 18, 2020
Ditto to marc. I think that would prevent my issues I am seeing. Right now all I am trying to do is add a linker script to the host build and I tried extracting sof_add_ld_script (name might not be exact, going off memory) from xtensa so I could share it with the host and it broke because a bunch of vars were evaluating to nothing causing arg count failures due to missing configs.
xiulipan commentedon Nov 19, 2020
@marc-hb I think this issue is not related with kconfig, it is totally a cmake config issue. I think we can include the
make tgplp_defconfigin cmake stage.marc-hb commentedon Nov 19, 2020
The
.configfile is a kconfig file so it's not clear what you meant here.I just checked and as expected
make foo_defconfig VERBOSE=1invokesscripts/kconfig/olddefconfig.pycujomalainey commentedon Nov 20, 2020
just an fyi, tried adding a return else case to the platform select in arch/xtensa/cmakelist and it doesnt work.
cujomalainey commentedon Dec 28, 2020
@slawblauciak can we bump this to a P1/P2? This is blocking oss-fuzz from covering a large part of the firmware and also I think making integration of future testbench tools harder for @ShriramShastry
lgirdwood commentedon Jan 12, 2021
@mwasko can we assign someone to fix for v1.7 final. I don't think this will block the first RC.
mwasko commentedon Jan 25, 2021
@bkokoszx please take a look
6 remaining items
bkokoszx commentedon Jan 27, 2021
@marc-hb Thanks, feel free to ask if you need any help.
@cujomalainey
Thanks, but I have one additional question. :) I understand that in the "first" round of CMake the .config file is not created yet. We do not have defined any
CONFIG_*defines at that stage yet. Aftermake *_defconfig (e.g. make tgplp_defconfig)there is "second" round of cmake and at that stage we haveCONFIG_*defines defined.I do not fully understand why you need those defines necessarily at the first stage. Do you want to make it just more readable or e.g. add some function which take specific
CONFIG_*(and it case when it is empty string it causes the error)?cujomalainey commentedon Jan 27, 2021
Not quite, I would argue the first stage should not even run.
What I am trying to do is refactor the build system so it is more friendly to x86 test builds, currently I need to be able to link in ld scripts. If I modify the ld script function right now to take in paths based on config values (which its currently accessing globally) it causes breakages in the first step even though function isn't even relevant for kconfig generation.
If we remove this double step process I think we can clean up a lot of code.
ktrzcinx commentedon Jan 27, 2021
@cujomalainey
if(DEFINED CONFIG_VAR_NAME)orif(CONFIG_VAR_NAME)wouldn't solve your problem ?lgirdwood commentedon Jan 27, 2021
Thank you @marc-hb - I guess you are the cmake "Maintainer" now.
cujomalainey commentedon Jan 27, 2021
@ktrzcinx that sounds more like treating the symptom than the problem. Likely I could add that but it would likely litter the repo with them
cmake: generate the complete .config at configuration time.
marc-hb commentedon Apr 30, 2021
@cujomalainey is there anything left to do here, if not can this be closed?
cujomalainey commentedon Apr 30, 2021
yes i think we are done here