Some device tree integers may be signed or unsigned depending on their value

[marc-hb]

Describe the bug

Among other integer insanities, the sign of literals in C depends on their value and on the C version used, the compiler used and probably the phase of the moon too.

As luck has it, some addresses like the one added in commit d76419973a081 fall between 2^31 and 2^32 and can end up being signed in some conditions.

https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.4-rc1 has u32 and u64 and no signed type. https://elinux.org/Device_Tree_Mysteries#Signed_Property_Values and notably http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159682.html explain how to indirectly define signed values using integer expressions, e.g.: (0 - 5).

The demo in #53467 has a hack that appends u and silences the sparse warnings below. An earlier and naive attempted to add a u suffix unconditionally but that breaks linker scripts.

https://github.com/thesofproject/sof/actions/runs/3831594561/jobs/6520904053

warning: decimal constant 2684485632 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here
warning: decimal constant 2684485632 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here
...

https://faultlore.com/blah/rust-layouts-and-abis/#the-c-integer-hierarchy https://faultlore.com/blah/c-isnt-a-language/

Impact

It does not seem to cause any real issue so far but the static analyzer "sparse" is catching this and complaining about it. Because this register is used several times in one .h file (a pretty common thing), sparse logs end up being spammed 1200 times with that same warning.

Apr 2023 EDIT: as later found by @lyakh , this causes the sparse tool to STOP analysis with a "too many warnings" error. So the impact is actually fairly high.

Also, integer complexities and "cast abuse" have been known to cause hard to debug issues and vulnerabilities so maybe better safe than sorry.

Expected behavior

Device tree integers are always unsigned.

To Reproduce

To reproduce the 1200 sparse warnings:

git clone https://github.com/thesofproject/sof/commits/f32b6ac0fea3 in a new west workspace, initialize it and update it and then run sparse like this:

./scripts/xtensa-build-zephyr.py  --pristine --cmake-args=-DSPARSE=y mtl

You must also install sparse from https://github.com/marc-hb/sparse/commits/main and uninstall ccache. More details in https://github.com/thesofproject/sof/actions/runs/3831594561/jobs/6520904053

Uninstall ccache and --pristine are required because sparse currently breaks the incremental build in various obscure ways, hopefully fixed by

• 52671.

Logs and console output

https://github.com/thesofproject/sof/actions/runs/3831594561/jobs/6520904053 Note there is a lot of other, unrelated warnings in that log on top of the 1200 relevant ones.

Environment (please complete the following information):

sparse static analyzer on Linux.

[mbolivar-nordic]

It does not seem to cause any real issue so far but the static analyzer "sparse" is catching this and complaining about it.

Gonna mark low, then. I don't think the juice is worth the squeeze fixing this right now given people's workloads.

[marc-hb]

You just reminded me this checkpatch warning: "A patch subject line should describe the change not the tool that found it"

[github-actions[bot]]

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.

[marc-hb]

Still spamming sparse output massively: https://github.com/thesofproject/sof/actions/runs/4389646297/jobs/7687421819

[lyakh]

Having too many sparse warnings has a real negative effect on our ability to use sparse output effectively: sparse stops producing warnings after reaching a certain umber of them for each individual file. It prints a message "warning: too many warnings" and stops printing any further one for that file. Currently we have large numbers of "dubious x & !y" and "Variable length array is used." sparse warnings in SOF builds, both generated by logging calls like comp_info() and those often exceed that threshold. That leads to warnings that we are actually interested in like "different address spaces" being lost.

[marc-hb]

Raising priority based on @lyakh's last comment ("too many warnings" real-world examples in: https://github.com/thesofproject/sof/pull/7488)

@galak is this something you or someone else could take a look at? (@mbolivar-nordic has no time for this right now)

[github-actions[bot]]

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.

[marc-hb]

Still happening https://github.com/thesofproject/sof/actions/runs/5324802896/jobs/9644627642?pr=7838

/zep_workspace/zephyr/soc/xtensa/intel_adsp/common/include/cpu_init.h:100:15: warning: decimal constant 3187671040 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here
/zep_workspace/zephyr/soc/xtensa/intel_adsp/cavs/sram.c:114:24: warning: decimal constant 3187671040 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here
/zep_workspace/zephyr/soc/xtensa/intel_adsp/cavs/sram.c:146:24: warning: decimal constant 3196059648 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here

[github-actions[bot]]

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.

[marc-hb]

Still happening https://github.com/thesofproject/sof/actions/runs/6134027541/job/16646452352

[carlescufi]

Architecture WG:

• @gmarull suggest an approach like here: https://github.com/zephyrproject-rtos/zephyr/pull/56684
• @henrikbrixandersen remarks that the DT spec only supports u32 and u64 natively
• @gmarull points out that Linux has accessors for signed values: https://elixir.bootlin.com/linux/latest/source/include/linux/of.h#L1388
• @henrikbrixandersen points out that our current macros are "wrong". Eg. DT_REG_ADDR does not add a U. One option would be to add _U and _S macros or to change the existing ones to append U
• @galak and @mbolivar-ampere what's your take?

[galak]

I would suggest something along the lines of what @gmarull suggested. Introduce some macro variants, I'm assuming this is really only needed for DT_REG type macros.

[github-actions[bot]]

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.

[marc-hb]

Still happening, still the most verbose "sparse" warning by far: https://github.com/thesofproject/sof/actions/runs/7325312100/job/19949765369

[stephanosio]

We have encountered this issue in the main CI while compiling for C++98 (i.e. C90), now in the GCC itself:

In file included from /__w/zephyr/zephyr/include/zephyr/arch/arm/arch.h:50,
                 from /__w/zephyr/zephyr/include/zephyr/arch/cpu.h:19,
                 from /__w/zephyr/zephyr/include/zephyr/kernel_includes.h:37,
                 from /__w/zephyr/zephyr/include/zephyr/kernel.h:17,
                 from /__w/zephyr/zephyr/tests/lib/cpp/cxx/src/main.cpp:18:
/__w/zephyr/zephyr/include/zephyr/arch/arm/cortex_a_r/timer.h:40:1: error: this decimal constant is unsigned only in ISO C90 [-Werror]
   40 | DEVICE_MMIO_TOPLEVEL_STATIC(timer_regs, ARM_TIMER_NODE

Given that C++98/C90 support is not too absurd, we will need to resolve this one way or another.

[github-actions[bot]]

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.

[marc-hb]

Still responsible for the most spam in sparse analysis, by far: https://github.com/thesofproject/sof/actions/runs/8776716191/job/24080631356

EDIT: No change as of May 29th 2024: https://github.com/thesofproject/sof/actions/runs/9253499236/job/25453294833?pr=9168

[github-actions[bot]]

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.

[marc-hb]

This issue has been marked as stale because it has been open (more than) 60 days with no activity.

Still failing: https://github.com/thesofproject/sof/actions/runs/10240624093/job/28327745444

This is not one of these problems that accidentally "fixes itself" when someone fixes something related.

It's not some intermittent issue with a reproduction rate that might get lower either.

As long as no one does any work specifically to fix it, the "stale" bot will be wrong.