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

sparse: picolibc/include/complex.h: error: too many errors #680

Open
marc-hb opened this issue Jan 23, 2024 · 10 comments
Open

sparse: picolibc/include/complex.h: error: too many errors #680

marc-hb opened this issue Jan 23, 2024 · 10 comments

Comments

@marc-hb
Copy link

marc-hb commented Jan 23, 2024

[This was found in zephyrproject-rtos/zephyr/issues/63417 (which is fixed now) but it's not related to it. It was also filed earlier in zephyrproject-rtos/zephyr/issues/67035 but I don't think it's related to Zephyr. Also: I'm less confused now]

When trying to use the "sparse" on Zephyr with picolibc, complex.h is alone responsible for 7,300 lines of warnings out of 10,000 lines total.

In fact complex.h triggers so many warnings that sparse aborts when compiling some (most?) files:

zephyr/build/modules/picolibc/picolibc/include/complex.h:103:15: error: too many errors

Of course one of the reason is very likely that complex.h is included in many C files (which is not going to change).

If not all warnings, it should be possible to reduce the number of these warnings to a volume that is easier to filter and that does not cause sparse to abort entirely.

And hey who knows: maybe some of these warnings are finding actual problems?

Reproduction with sparse version 365b95203727. Compile with make, add to PATH.

Initialize Zephyr
cd zephyr
git checkout 6df6935b674c
west update
 west build  samples/hello_world  -b rpi_4b --  -DZEPHYR_SCA_VARIANT=sparse   -DCONFIG_PICOLIBC_USE_MODULE=y -DCONFIG_PICOLIBC_USE_MODULE=y 

Warning: -DCONFIG_PICOLIBC_USE_MODULE=y is required to avoid issue #63003

cc: @keith-packard

@keith-packard
Copy link
Collaborator

As far as I can tell, this is just a bug in sparse. It doesn't appear to support _Complex? If so, why is something including <complex.h>?

I also cannot reproduce this with picolibc 1.8.6 via PR zephyrproject-rtos/zephyr#67891 -- I wonder if something has changed to make it magically work?

@marc-hb
Copy link
Author

marc-hb commented Jan 23, 2024

I just switched to picolibc version 764ef4e (the one in zephyrproject-rtos/zephyr#67891) and it made no difference.

If so, why is something including <complex.h>

All the files in newlib/libm/complex/*.c unsurprisingly include complex.h.

@keith-packard
Copy link
Collaborator

All the files in newlib/libm/complex/*.c unsurprisingly include complex.h.

Oh, of course, you're building all of picolibc :-) Sorry for being dense.

I'm using sparse 0.6.4 and not having any issues though. It sure looks like this is just a bug in sparse.

@marc-hb
Copy link
Author

marc-hb commented Jan 23, 2024

I downgraded to sparse 0.6.4 + a cherry-pick of 3ddf02ddcb9 to avoid the infinite loop in tls.c (zephyrproject-rtos/zephyr#63417) and the reproduction is exactly the same. I'm very surprised you don't see these thousands of warnings.

It doesn't appear to support _Complex?

So far I vaguely assumed sparse didn't like the underscore. Now I just found that full symbol _Complex is actually part of a "blocklist", see lucvoo/sparse-dev@16ae3ac5c5d07 ("keyword: explicitly add C99 & C11 keywords"). I'm still not sure what that means: is sparse unsuited to scanning libc implementations? @lucvoo could you explain?

Anyway it's not just _Complex, far from it. I ran again with the quick hack below. The _Complex warning is gone

complex.h:26:8: error: Trying to use reserved word '_Complex' as identifier
complex.h:27:8: error: Trying to use reserved word '_Complex' as identifier
etc.

... but complex.h still hits the "too many errors in complex.h" threshold (each time on each .c file) and it still produces about 7000 lines alone out of a 10,000 total.

EDIT: forgot the "attachment"

--- a/ident-list.h
+++ b/ident-list.h
@@ -31,7 +31,7 @@ IDENT(static);
 /* C99 keywords */
 IDENT(restrict); IDENT(__restrict); IDENT(__restrict__);
 IDENT(_Bool);
-IDENT_RESERVED(_Complex);
+// IDENT_RESERVED(_Complex);
 IDENT_RESERVED(_Imaginary);
 
 /* C11 keywords */
zephyr/build/modules/picolibc/picolibc/include/complex.h:35:15: error: got ccosf
zephyr/build/modules/picolibc/picolibc/include/complex.h:38:16: error: Expected ; at end of declaration
zephyr/build/modules/picolibc/picolibc/include/complex.h:38:16: error: got csin
zephyr/build/modules/picolibc/picolibc/include/complex.h:39:15: error: Expected ; at end of declaration
zephyr/build/modules/picolibc/picolibc/include/complex.h:39:15: error: got csinf
zephyr/build/modules/picolibc/picolibc/include/complex.h:42:16: error: Expected ; at end of declaration
zephyr/build/modules/picolibc/picolibc/include/complex.h:42:16: error: got ctan
zephyr/build/modules/picolibc/picolibc/include/complex.h:43:15: error: Expected ; at end of declaration
zephyr/build/modules/picolibc/picolibc/include/complex.h:43:15: error: got ctanf
zephyr/build/modules/picolibc/picolibc/include/complex.h:47:16: error: Expected ; at end of declaration
zephyr/build/modules/picolibc/picolibc/include/complex.h:47:16: error: got cacosh
zephyr/build/modules/picolibc/picolibc/include/complex.h:48:15: error: Expected ; at end of declaration

@keith-packard
Copy link
Collaborator

So far I vaguely assumed sparse didn't like the underscore. Now I just found that full symbol _Complex is actually part of a "blocklist", see lucvoo/sparse-dev@16ae3ac5c5d07 ("keyword: explicitly add C99 & C11 keywords"). I'm still not sure what that means: is sparse unsuited to scanning libc implementations? @lucvoo could you explain?

That commit makes it pretty clear that sparse doesn't support _Complex at all, so there's no way you're going to get it to deal with any file using those datatypes (double _Complex, float _Complex and long double _Complex).

The cmake configuration should be autodetecting whether the compiler supports this or not; it seems like there's an assumption that sparse supports the same language as the target compiler? Because when I run the command described above, it sets _HAVE_COMPLEX to TRUE, which isn't correct for sparse.

Maybe we just need an explicit parameter to disable _Complex support in picolibc. Alternatively, sparse could be fixed to actually support C11?

@lucvoo
Copy link

lucvoo commented Jan 23, 2024 via email

@keith-packard
Copy link
Collaborator

As far as I understand support for complex types is optional in C11 and can/should be tested via the macro STDC_IEC_559_COMPLEX (which Sparse doesn't define, of course). But sure, ideally, Sparse should support complex types (and the different vectors type and a few other things). It shouldn't be very hard, it's just a question of time and priorities. Best regards, -- Luc

Yes, and the picolibc build system checks to see if the 'toolchain' supports _Complex during the configuration process. It seems that the issue here is that those checks aren't using sparse. Thanks for your explanation.

@marc-hb
Copy link
Author

marc-hb commented Jan 23, 2024

it seems like there's an assumption that sparse supports the same language as the target compiler?

In truth, this is the goal.

Yes, and the picolibc build system checks to see if the 'toolchain' supports _Complex during the configuration process. It seems that the issue here is that those checks aren't using sparse

Pretty much all static C/C++ analyzers I've used work the same: they inject themselves in the middle of the build system to gather compiler invocations: source file names, options, -D macro definitions etc. For zephyr and sparse see this small and insightful commit zephyrproject-rtos/zephyr@91902c5fd4db756. Note the REAL_CC magic there, also explained here: https://github.com/lucvoo/sparse-dev/blob/1cf3d98cf171/cgcc.1

So from a build perspective I'm afraid there is no such thing as "the sparse toolchain" or "the sparse build" that could be configured differently. I don't think anyone would like to analyze sources different from what they actually use.

On the other hand, is there a simple way to disable complex.h from the Zephyr build system/Kconfig? As opposed to auto-detecting it from the toolchain. Not a solution for projects actually using complex numbers but a great solution for everyone else. It would also save some build time; never hurts.

It was also filed earlier in zephyrproject-rtos/zephyr#67035 but I don't think it's related to Zephyr.

Should we close this picolibc issue and move back to a re-opened 67035?

@marc-hb
Copy link
Author

marc-hb commented Jan 23, 2024

So from a build perspective I'm afraid there is no such thing as "the sparse toolchain" or "the sparse build" that could be configured differently. I don't think anyone would like to analyze sources different from what they actually use.

For completeness: some developers may be selfishly interested in warnings only in the subset/component they wrote. From past experience again, filtering this at build time is tricky to achieve: you have to fight the build system. This also hides information from analyzers which have a global view and don't just look at each .c file one by one. See an example and good explanation there: https://www.synopsys.com/blogs/software-security/coverity-and-issues-in-third-party-code.html

So for these "selfish" people it's simpler, safer and generally better to exclude warnings from picolibc or other after analysing the complete project as is. Display filters, no build-time filter.

With sparse we have to filter after analysis anyway due to -Werror complexities: https://marc.info/?l=linux-sparse&m=170571398021000&w=2

So filtering warnings in a per-component basis can be done at the same, post-analysis time; two birds with one stone.

@keith-packard
Copy link
Collaborator

There's currently no way to disable _Complex usage in picolibc via the CMake configuration; autodetection of what the toolchain supports has been sufficient so far. It would be easy enough to add, if someone wanted to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants