Skip to content
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.

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

CONFIG_PICOLIBC + sparse = zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h' #63003

Closed
marc-hb opened this issue Sep 22, 2023 · 18 comments
Assignees
Labels
area: Build System area: picolibc Picolibc C Standard Library area: Toolchains Toolchains bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Stale

Comments

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 22, 2023

Describe the bug

CONFIG_PICOLIBC is currently incompatible with sparse due to some .h files complexities.

EDIT October 2nd with newer, simpler, SOF-free reproduction

# Compiles and provides all sparse warnings
west build -b intel_adsp_cavs25  samples/hello_world -p -- \
 -DCONFIG_MINIMAL_LIBC=y -DZEPHYR_SCA_VARIANT=sparse

# Does not compile and can't provide some sparse warnings
west build -b intel_adsp_cavs25  samples/hello_world -p -- \
 -DCONFIG_PICOLIBC=y -DZEPHYR_SCA_VARIANT=sparse 



Slower, -j1 version that does not bury the relevant error:

west build -b intel_adsp_cavs25  samples/hello_world -p -- \
 -DCONFIG_PICOLIBC=y  -DZEPHYR_SCA_VARIANT=sparse

ninja -j1 -C build

Building C object zephyr/lib/libc/picolibc/CMakeFiles/lib__libc__picolibc.dir/libc-hooks.c.obj
zephyr/lib/libc/picolibc/libc-hooks.c: note: in included file:
zephyr/include/zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h'

OLDER, initial and more complex reproduction that requires SOF. Kept here for completeness, you can probably skip it.

Commit f0daf90 switched CONFIG_PICOLIBC on by default. This broke the "sparse" analyzer used in the SOF project:

https://github.com/thesofproject/sof/actions/runs/6247162197/job/16959223474?pr=8235

/zep_workspace/zephyr/lib/os/printk.c:123:33: error: undefined identifier 'FDEV_SETUP_STREAM'
/zep_workspace/zephyr/lib/os/printk.c:144:32: error: undefined identifier 'FDEV_SETUP_STREAM'
/zep_workspace/zephyr/lib/os/printk.c:144:49: error: invalid initializer
/zep_workspace/zephyr/include/zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h'

Note this is not just a sparse warning or false positive: compilation with sparse actually FAILS, it cannot even complete.

To Reproduce

Detailed steps at


Expected behavior

Sparse can still sparse.

Impact

Broken SOF CI

Logs and console output

Environment (please complete the following information):

Github runner Ubuntu 22.04 LTS

Zephyr CI image with Zephyr SDK + marc-hb/sparse@3848a76ba49f336

zephyr commit 3415905 and SOF commit 75337b41eb51)

Additional context

These two workarounds were tested successfully:

  • git revert f0daf90

  • CONFIG_MINIMAL_LIBC=y as in ~/SOF/sof/scripts/xtensa-build-zephyr.py -p mtl --cmake-args=-DZEPHYR_SCA_VARIANT=sparse --cmake-args=-DCONFIG_LOG_USE_VLA=n --cmake-args=-DCONFIG_MINIMAL_LIBC=y

@marc-hb marc-hb added the bug The issue is a bug, or the PR is fixing a bug label Sep 22, 2023
@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 22, 2023

@keith-packard can you help?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 22, 2023

@keith-packard

Is the analyzer not using the picolibc version of stdio.h for some reason? That's where the macro is defined.

Oh this is fun! We don't have picolibc in our west manifest... Yet we have this CONFIG_ by default and compilation "almost works".

# CONFIG_MINIMAL_LIBC is not set
CONFIG_PICOLIBC=y

More precisely:

  1. Ignoring sparse for a moment: how come we can successfully compile with this in our zephyr/.config file? In other words, why is sparse only failing?!?

  2. Similar question when using sparse: why is the build system complaining so late?

The case where CONFIG_PICOLIBC=y is true and picolibc is missing needs much better error handling IMHO.

@keith-packard
Copy link
Collaborator

The case where CONFIG_PICOLIBC=y is true and picolibc is missing needs much better error handling IMHO.

This was briefly touched on during the picolibc-by-default discussions in the Zephyr TSC where the general opinion was that having modules that were required to be in the west manifest was deemed "ok". Error handling when it's missing could definitely be improved though...

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 23, 2023

Sorry I just read some picolibc documentation for the first time and I realized there's a picolibc in the Zephyr SDK 0.16.1. No need for the module, my bad.

I finally debugged this issue a little bit using the most effective build debug tool: breaking the build in various ways.

So the regular (non-sparse) build gets FDEV_SETUP_STREAM from
zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/picolibc/include/stdio.h indeed.

I confirmed this breaking the FDEV_SETUP_STREAM definition in that file.

(this identical and duplicate (!) stdio.h file is not used:
zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/picolibc/xtensa-intel_ace15_mtpm_zephyr-elf/sys-include/stdio.h)

Another important build debug trick: using ninja -j1 to remove a ton of multithreaded noise and focus on the FIRST error. Which gives:

[1/253] Building C object zephyr/lib/libc/picolibc/CMakeFiles/lib__libc__picolibc.dir/libc-hooks.c.obj
SOF/zephyr/lib/libc/picolibc/libc-hooks.c: note: in included file:
SOF/zephyr/include/zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h'

Again, I confirmed that
zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/picolibc/include/sys/_timespec.h is used by the regular (non-sparse) build.

Examining the command lines I noticed the suspicious --specs=picolibc.specs and I think this is where the problem lies. I strongly suspect picolibc.specs is somehow incompatible with sparse @tejlmand any clue?

I made the small change below and it makes the regular build fail just like the sparse build:

--- picolibc.specs	2023/09/23 08:39:05	1.1
+++ picolibc.specs	2023/09/23 08:39:13
@@ -4,7 +4,7 @@
 %rename cc1plus	picolibc_cc1plus
 
 *cpp:
--isystem %{-picolibc-prefix=*:%*/picolibc/include/; -picolibc-buildtype=*:%:getenv(GCC_EXEC_PREFIX ../../picolibc/include/%*); :%:getenv(GCC_EXEC_PREFIX ../../picolibc/include)} %(picolibc_cpp)
+-isystem %{-picolibc-prefix=*:%*/picolibc/include/; -picolibc-buildtype=*:%:getenv(GCC_EXEC_PREFIX ../../picolibc/include/%*); :%:getenv(GCC_EXEC_PREFIX ../../picolibc/includeBREAKIT)} %(picolibc_cpp)
 
 *cc1:
  %(picolibc_cc1) 

Does all this make sense?

marc-hb added a commit to marc-hb/sof that referenced this issue Sep 23, 2023
Starting from Zephyr commit f0daf904bb02, CONFIG_PICOLIBC is on by
default.

PICOLIBC does not seem compatible with sparse yet:
zephyrproject-rtos/zephyr#63003

Even if it were compatible with sparse, it seems like a pretty big
change that we should not immediately and blindly accept.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb added area: Build System area: Toolchains Toolchains area: picolibc Picolibc C Standard Library labels Sep 23, 2023
@keith-packard
Copy link
Collaborator

Yup, if sparse isn't parsing the .specs file to pull in the picolibc header path, it's not going to work very well. If you know you're using the Zephyr SDK, you can probably hard-code the picolibc header path for sparse to use. Otherwise, figuring out where the headers are will require digging through whatever toolchain is in use.

kv2019i pushed a commit to thesofproject/sof that referenced this issue Sep 25, 2023
Starting from Zephyr commit f0daf904bb02, CONFIG_PICOLIBC is on by
default.

PICOLIBC does not seem compatible with sparse yet:
zephyrproject-rtos/zephyr#63003

Even if it were compatible with sparse, it seems like a pretty big
change that we should not immediately and blindly accept.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 26, 2023

Sorry @jhedberg I have neither the time nor the knowledge to fix this.

@lyakh or @tejlmand maybe?

If no one can help then SOF will have to choose between sparse and PICOLIBC and we'll close this as won't fix.

@marc-hb marc-hb removed their assignment Sep 26, 2023
@jhedberg jhedberg added the priority: low Low impact/importance bug label Sep 26, 2023
@keith-packard
Copy link
Collaborator

If we switched the SDK to use picolibc by default with newlib as an option requiring additional configuration, then this would work as expected. The alternative is to figure out how to extract include path information from the toolchain and pass that to sparse?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 26, 2023

If no one can help then SOF will have to choose between sparse and PICOLIBC

Correction: we can use CONFIG_MINIMAL_LIBC=y for the sparse build ONLY. Will submit that to SOF CI now.

EDIT: successfully added to SOF CI in thesofproject/sof@0061953a595

marc-hb added a commit to marc-hb/sof that referenced this issue Sep 26, 2023
Add a sparse-specific workaround for the incompatibility with
picolibc (the new Zephyr default)
zephyrproject-rtos/zephyr#63003

Also fix comment in commit 2a9473a ("app/prj.conf: disable PICOLIBC
with CONFIG_MINIMAL_LIBC=y"): we don't need to disable PICOLIBC
_everywhere_; we only need to disable it when using sparse.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
lgirdwood pushed a commit to thesofproject/sof that referenced this issue Sep 27, 2023
Add a sparse-specific workaround for the incompatibility with
picolibc (the new Zephyr default)
zephyrproject-rtos/zephyr#63003

Also fix comment in commit 2a9473a ("app/prj.conf: disable PICOLIBC
with CONFIG_MINIMAL_LIBC=y"): we don't need to disable PICOLIBC
_everywhere_; we only need to disable it when using sparse.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@tejlmand
Copy link
Collaborator

tejlmand commented Oct 2, 2023

If no one can help then SOF will have to choose between sparse and PICOLIBC and we'll close this as won't fix.

I don't think this should be closed, as it does seem to be something we should fix.
Though I wonder what makes picolibc so different from newlib nano that sets -specs=nano.specs and still works with SOF.

@keith-packard
Copy link
Collaborator

It works with newlib nano because newlib nano uses almost identical headers, so sparse ends up using the regular newlib headers. With picolibc, we really need the picolibc headers instead. When using sparse and the Zephyr SDK, we could probably compute the required include path:

$ ls $(arm-zephyr-eabi-gcc -print-search-dirs | grep '^install' | sed -e 's/^install: //' -e 's;$;../../../../picolibc/include;')
alloca.h  assert.h    ctype.h	envlock.h   fenv.h     iconv.h	   limits.h  memory.h	 pwd.h	   search.h    ssp	      string.h	  termios.h  utime.h
_ansi.h   bits	      devctl.h	envz.h	    fnmatch.h  ieeefp.h    locale.h  newlib.h	 regdef.h  semihost.h  stdint.h       strings.h   threads.h  utmp.h
argz.h	  byteswap.h  dirent.h	errno.h     getopt.h   inttypes.h  machine   paths.h	 regex.h   setjmp.h    stdio-bufio.h  sys	  time.h     wchar.h
ar.h	  complex.h   elf.h	fastmath.h  glob.h     langinfo.h  malloc.h  picolibc.h  rpc	   signal.h    stdio.h	      _syslist.h  unctrl.h   wctype.h
arpa	  cpio.h      endian.h	fcntl.h     grp.h      libgen.h    math.h    picotls.h	 sched.h   spawn.h     stdlib.h       tar.h	  unistd.h   wordexp.h

@marc-hb
Copy link
Collaborator Author

marc-hb commented Oct 2, 2023

... and still works with SOF.

This incompatibility between CONFIG_PICOLIBC and sparse does actually not require SOF at all. I just edited the description at the top so anyone can reproduce it from a plain Zephyr tree with the most basic build command:

west build -b intel_adsp_cavs25  samples/hello_world -p -- \
 -DCONFIG_PICOLIBC=y  -DZEPHYR_SCA_VARIANT=sparse

ninja -j1 -C build

Building C object zephyr/lib/libc/picolibc/CMakeFiles/lib__libc__picolibc.dir/libc-hooks.c.obj
zephyr/lib/libc/picolibc/libc-hooks.c: note: in included file:
zephyr/include/zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h'

Funny enough west build -b qemu_x86[_64] samples/hello_world -p -- -DZEPHYR_SCA_VARIANT=sparse throws sparse into an infinite loop. Off-topic here but worth knowing!

EDIT: infinite loop reported in #63417 and fixed in sparse.

@keith-packard
Copy link
Collaborator

sparse needs a lot of help finding all of the necessary include directories. I'm unclear how this works for newlib; presumably it's loading a bunch of glibc headers and somehow that works? Mysterious.

In any case, I hacked the xtensa module and the Zephyr cmake file to insert a bunch of additional include paths.

modules/hal/xtensa:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 083b65c..2b281be 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -7,6 +7,8 @@ if(CONFIG_XTENSA_HAL)
   if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_NAME})
     target_include_directories(XTENSA_HAL INTERFACE
                                ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_NAME}/)
+    target_include_directories(XTENSA_HAL INTERFACE
+                               ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_NAME}/xtensa/config/)
   elseif(EXISTS ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_TOOLCHAIN_NAME})
     target_include_directories(XTENSA_HAL INTERFACE
                                ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_TOOLCHAIN_NAME}/)
@@ -21,6 +23,8 @@ if(CONFIG_XTENSA_HAL)
   if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_NAME})
     zephyr_include_directories(XTENSA_HAL INTERFACE
                                ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_NAME}/)
+    zephyr_include_directories(XTENSA_HAL INTERFACE
+                               ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_NAME}/xtensa/config/)
   elseif(EXISTS ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_TOOLCHAIN_NAME})
     zephyr_include_directories(XTENSA_HAL INTERFACE
                                ${CMAKE_CURRENT_LIST_DIR}/zephyr/soc/${SOC_TOOLCHAIN_NAME}/)

zephyr:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6d5c643f3e..f8fdbf4a8c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -298,6 +298,10 @@ if (CONFIG_PICOLIBC AND NOT CONFIG_PICOLIBC_IO_FLOAT)
   zephyr_compile_options($<$<COMPILE_LANGUAGE:C>:$<TARGET_PROPERTY:compiler,no_printf_return_value>>)
 endif()
 
+zephyr_system_include_directories( /opt/zephyr-sdk-0.16.3-2-g8572e5d/xtensa-intel_tgl_adsp_zephyr-elf/picolibc/include
+    /opt/zephyr-sdk-0.16.3-2-g8572e5d/xtensa-intel_tgl_adsp_zephyr-elf/./lib/gcc/xtensa-intel_tgl_adsp_zephyr-elf/12.2.0/include
+    /opt/zephyr-sdk-0.16.3-2-g8572e5d/xtensa-intel_tgl_adsp_zephyr-elf/lib/gcc/xtensa-intel_tgl_adsp_zephyr-elf/12.2.0/include-fixed)
+
 # @Intent: Set compiler specific flag for tentative definitions, no-common
 zephyr_compile_options($<TARGET_PROPERTY:compiler,no_common>)
 

With those paths, sparse at least runs to completion, although it does emit a pile of warnings.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Oct 2, 2023

Thanks @keith-packard for still trying.

BTW I agree with @jhedberg's that this is "low" priority (at least for SOF) as we can very simply turn off PICOLIBC only when using sparse - and I just did.

I hacked the xtensa module and ...

You probably knew this already but this missing #include issue reproduces the same outside Xtensa. After turning off CONFIG_THREAD_LOCAL_STORAGE (workaround for brand new, infinite loop issue #63417) I reproduced this missing #include issue exactly the same on rpi_4b, hifive1, arduino_due and intel_ish_5_4_1.

presumably it's loading a bunch of [glibc] headers and somehow that works? Mysterious.

Isn't that how every C/C++ project in the universe works? I mean even before sparse.

sparse needs a lot of help finding all of the necessary include directories.

I'm afraid every static analyzer is a hack.

A number of analyzers have a "preparation" step where they just observe the regular build first. I'm surprised sparse does not seem to have that step? Maybe it's putting itself at a disadvantage here...

although it does emit a pile of warnings.

That is the feature.

Copy link

github-actions bot commented Dec 2, 2023

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Dec 2, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 17, 2023
@marc-hb marc-hb changed the title CONFIG_PICOLIBC + sparse = zephyr/lib/os/printk.c: error: undefined identifier 'FDEV_SETUP_STREAM' CONFIG_PICOLIBC + sparse = zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h' Dec 28, 2023
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 28, 2023

I don't think this should be closed, as it does seem to be something we should fix.

This bug was auto-closed. It's still valid. @tejlmand do you want to take another look?

It's much easier to debug now that sparse has just been fixed and tls.c does not trigger an infinite loop anymore, no CONFIG_THREAD_LOCAL_STORAGE=n workaround needed anymore. Besides updating sparse, make sure you use sparse.template fix #67036 (now merged)

Interestingly, these two commands fail differently: See update below.

west build -b intel_adsp_cavs25  samples/hello_world --  \
  -DCONFIG_PICOLIBC=y -DZEPHYR_SCA_VARIANT=sparse -DCONFIG_PICOLIBC_USE_MODULE=y

zephyr/build/modules/picolibc/picolibc/include/machine/core-isa.h:2:11: error: unable to open 'xtensa/config/core-isa.h'
west build -b intel_adsp_cavs25  samples/hello_world --  -DCONFIG_PICOLIBC=y -DZEPHYR_SCA_VARIANT=sparse 

zephyr/include/zephyr/posix/sys/stat.h:40:11: error: unable to open 'sys/_timespec.h'

@marc-hb marc-hb reopened this Dec 28, 2023
@github-actions github-actions bot removed the Stale label Dec 29, 2023
@tejlmand
Copy link
Collaborator

. @tejlmand do you want to take another look?

It's much easier now to debug now that sparse has just been fixed and tls.c does not trigger an infinite loop anymore,

Great, then it sounds like there is hope.
Will be on my todo list to be looked at after collab-hwm work.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 23, 2024

I reproduced again with today's commit 6df6935.

Workaround learned in different (and fixed) sparse issue #63417: -DCONFIG_PICOLIBC_USE_MODULE=y! AFAIK this switches away from picolibc in the SDK. It requires picolibc as a (west) module/git repo.

git checkout 6df6935b674c

west build  samples/hello_world  -b rpi_4b --   -DZEPHYR_SCA_VARIANT=sparse -DCONFIG_PICOLIBC=y

=> fails as described

west build  samples/hello_world  -b rpi_4b --   -DZEPHYR_SCA_VARIANT=sparse -DCONFIG_PICOLIBC=y -DCONFIG_PICOLIBC=y -DCONFIG_PICOLIBC=y

 => works!

error: unable to open 'xtensa/config/core-isa.h'

That was when trying to use a different, proprietary toolchain. Ignore, absolutely not needed.

Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Mar 24, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: picolibc Picolibc C Standard Library area: Toolchains Toolchains bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Stale
Projects
None yet
Development

No branches or pull requests

4 participants