module_adapter: Fix compilation error with Xtensa toolchain

[dbaluta]

We get the following error using Xtensa toolchain on i.MX:

src/audio/module_adapter/module_adapter.c:
In function ‘module_adapter_sink_src_prepare’:
src/audio/module_adapter/module_adapter.c:207: error: ‘for’ loop initial declaration used outside C99 mode
src/audio/module_adapter/module_adapter.c:211: error: redefinition of ‘i’
 src/audio/module_adapter/module_adapter.c:207: error: previous definition of ‘i’ was here 
src/audio/module_adapter/module_adapter.c:211: error: ‘for’ loop initial declaration used outside C99 mode

Fix this by declaring i at the top of the function.

Fixes: fa77edf ("pipeline2.0: change module prepare API to use sink/src.c")

[kv2019i]

Thanks. I've been slipping C99'isms through in recent reviews as it seems all our targets we have in our CI accept them (and we seem to have more and more SOF developers customed to C99), but this has not really been discussed anywhere. And we are not enforcing C99 for SOF (we are not building with std=c99), so stricly speaking we should not be using C99 features (or we should enforce C99).

It's still an issue if we don't have any compilers that would complain about this in our PR CI. I wonder if we should just bump the req to C99. I think @dbaluta your Xtensa toolchain would accept this code if you pass std=c99 .

FYI to @aborisovich @marc-hb

[lyakh]

@dbaluta "outside C99 mode" - does it mean that the compiler in principle supports C99 but the flags are wrong? Maybe fix the flags instead?

[marc-hb]

Note the SOF codebase has had some C99 declarations NOT located at the top of a { ... } block since FOREVER, see examples in #3995. These never caused any compilation failures; no one ever complained about them. So the recurring review request to keep declarations at the top of { ...} never made sense (and the request to sometimes keep them at the top of a function made even less sense, even C89 does not require them to be at the top of a function)

EDIT: discussed in https://github.com/orgs/thesofproject/discussions/7395 and elsewhere.

Same thing with C99 comments //, there has been some // since forever (#5046).

Of course this is not a proof that all C99 features never caused any problem to anyone - but it's a pretty good indication.

[kv2019i]

@dbaluta Can you try compilation with "-std=c99" with this toolchain? Given Zephyr does depend on many C99 features explicitly (https://docs.zephyrproject.org/latest/develop/languages/c/index.html ) , it would seem to make sense to bump to align in SOF as well.

[marc-hb]

For the command line Zephyr has -DEXTRA_CFLAGS, see example in 9a23191.

Of course you can also put it in some CMakeLists.txt file.

[dbaluta]

Let me try " "-std=c99" with our toolchain.

[dbaluta]

Note the SOF codebase has had some C99 declarations NOT located at the top of a { ... }

The specific problem here are not the C99 declarations at the top, but actually it looks like C89 isn't smart enough to recognize the scope of the i variable declared inside the for.

Anyhow,

Using:

XTENSA_TOOLS_ROOT=~/work/repos/imx-audio-toolchain/Xtensa_Tool/ EXTRA_CFLAGS="-std=gnu99" ./scripts/xtensa-build-all.sh imx8

fixes the problem. We can fix that in our build env.

Should we turn on c99 by default?

[marc-hb]

The specific problem here are not the C99 declarations at the top, but actually it looks like C89 isn't smart enough to recognize the scope of the i variable declared inside the for.

Yes and pretty sure this is another... C99 feature. I can't imagine any toolchain supporting one of these C99 features but not the other, they're very similar.

Should we turn on c99 by default?

It would make sense considering SOF has been using (some) C99 features since forever. BTW we're in 2023 :-)

BTW even the Linux kernel is finally relaxing (a bit) its "C89 declarations" code style. It was getting in the way of another modern feature: https://lwn.net/Articles/934679/

[dbaluta]

Enabling some more components I get this error:

cc1: warnings being treated as errors
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:46: warning: C99 inline functions are not supported; using GNU89
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:46: warning: to disable this warning use -fgnu89-inline or the gnu_inline function attribute
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:72: warning: C99 inline functions are not supported; using GNU89
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:95: warning: C99 inline functions are not supported; using GNU89
make[2]: *** [CMakeFiles/sof.dir/build.make:773: CMakeFiles/sof.dir/src/audio/drc/drc.c.o] Error 2

Do you see any problems like this with your private Cadence toolchain?

[marc-hb]

I think we generally don't use -Werror with Cadence toolchains but no, I don't remember seeing these warnings or any other warning for that matter.

@dbaluta you can see Cadence compilation logs in every PR, click on "sof-ci/jenkins/pr-fw-build". For instance here: https://sof-ci.01.org/sofpr/PR7841/build9926/build/mtl_zph_ipc4.txt

What gcc version is yours based on?

Our oldest Cadence toolchain still used on the main branch is based on gcc 4: #7114

There may be older ones still in use on stable-v2.2. Just open any recent stable-v2.2 PR and open the same link.

[dbaluta]

Not sure how to figure out which gcc is your xtensa compiler based on.

./Xtensa_Tool/tools/RG-2017.8-linux/XtensaTools/bin/xt-xcc -v
xt-xcc version 12.0.8
Thread model: single

Anyhow, for now I fixed the problem EXTRA_CFLAGS="-std=gnu99 -fgnu89-inline"

I want to understand if you build DRC component which by default is N and caused the troubles mentioned above.

Later edit: Your Cadence compilation logs do not show DRC component being built. As far as I can figure out from the logs. Most likely you will get the same issue if you do CONFIG_DRC=y

[marc-hb]

The Zephyr build system always runs "CC --version". Example

From https://sof-ci.01.org/sofpr/PR7841/build9926/build/tgl_zph.txt

-- Found GnuLd: /srv/home/jenkins/xcc/install/tools/RG-2017.8-linux/XtensaTools/bin/xt-ld (found version "2.23.2") 
-- The C compiler identification is GNU 4.2.0
-- The CXX compiler identification is GNU 4.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /srv/home/jenkins/xcc/install/tools/RG-2017.8-linux/XtensaTools/bin/xt-xcc
-- Found Python3: /usr/bin/python3.8 (found version "3.8.10") found components: Interpreter 

[marc-hb]

I tried --cmake-args=-DCONFIG_COMP_DRC=y and it does not compile at all with IPC4. I mean it's not just a -Werror problem, it's a hard compilation failure.

No need for any Cadence toolchain to reproduce, the compilation failures are the same with the open-source Zephyr SDK. This is enough to fail:

sof/scripts/xtensa-build-zephyr.py  --cmake-args=-DCONFIG_COMP_DRC=y  mtl
# or
sof/scripts/xtensa-build-zephyr.py  --cmake-args=-DCONFIG_COMP_DRC=y  -i IPC4 tgl

cc: @andyross, @cujomalainey

[marc-hb]

On the other hand, --cmake-args=-DCONFIG_COMP_DRC=y works perfectly fine with IPC3, no warning at all:

$HOME/SOF/sof/scripts/xtensa-build-zephyr.py  --cmake-args=-DEXTRA_CFLAGS=-Werror \
   --cmake-args=-DCONFIG_COMP_DRC=y  -p tgl

-- Found GnuLd: /var/home/me/XCC/install/tools/RG-2017.8-linux/XtensaTools/bin/xt-ld (found version "2.23.2") 
-- The C compiler identification is GNU 4.2.0
-- The CXX compiler identification is GNU 4.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /var/home/me/XCC/install/tools/RG-2017.8-linux/XtensaTools/bin/xt-xcc

=> success

[dbaluta]

The Zephyr build system always runs "CC --version". Example

From https://sof-ci.01.org/sofpr/PR7841/build9926/build/tgl_zph.txt

-- Found GnuLd: /srv/home/jenkins/xcc/install/tools/RG-2017.8-linux/XtensaTools/bin/xt-ld (found version "2.23.2") 
-- The C compiler identification is GNU 4.2.0
-- The CXX compiler identification is GNU 4.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /srv/home/jenkins/xcc/install/tools/RG-2017.8-linux/XtensaTools/bin/xt-xcc
-- Found Python3: /usr/bin/python3.8 (found version "3.8.10") found components: Interpreter 

We are using 2 version of Xtensa Toolchain:

1. For SOF with XTOS version RG-2017.8-linux plus

-- The C compiler identification is GNU 4.2.0

This gives us the compilation problem. And we fixed it by EXTRA_CFLAGS="-std=gnu99 -fgnu89-inline"

2. For SOF with Zephyr version RI-2023.11-linux plus Clang 10.0.1 - This works FINE. No compilation problem.

-- The C compiler identification is Clang 10.0.1
-- The CXX compiler identification is Clang 10.0.1
-- The ASM compiler identification is Clang with GNU-like command-line
-- Found assembler: /home/nxf25322/work/repos/imx-audio-toolchain/Xtensa_Tool/install/tools/RI-2023.11-linux/XtensaTools/bin/xt-clang

We are planning to deprecate 1) and will already have a work around for the compilation issue so closing this PR!

[lyakh]

Enabling some more components I get this error:

cc1: warnings being treated as errors
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:46: warning: C99 inline functions are not supported; using GNU89
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:46: warning: to disable this warning use -fgnu89-inline or the gnu_inline function attribute
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:72: warning: C99 inline functions are not supported; using GNU89
/home/nxf25322/work/repos/sof/src/audio/drc/drc.c:95: warning: C99 inline functions are not supported; using GNU89
make[2]: *** [CMakeFiles/sof.dir/build.make:773: CMakeFiles/sof.dir/src/audio/drc/drc.c.o] Error 2

Do you see any problems like this with your private Cadence toolchain?

@dbaluta FWIW https://gcc.gnu.org/legacy-ml/gcc/2006-11/msg00006.html in short - inline without static isn't very well defined. So, only static functions can be safely made inline.