Fixed warnings issued by new xt-clang compiler

[aborisovich]

New Xtensa xt-clang driver issues new warnings that were not present before when using xt-xcc driver.
Fixed warnings related to enum cast to other enum. Silenced warnings related to C/C++ interoptability.

Signed-off-by: Andrey Borisovich andrey.borisovich@intel.com

[paulstelian97]

Your change can hide a genuine bug (at least from a superficial analysis); see review comments.

I could probably approve this if given a good answer in that situation.

[paulstelian97]

Why do we need to have a conversion between different enums in the first place?

[lgirdwood]

@aborisovich which enums are impacted here ? Sounds like we may need an alignment.

[aborisovich]

Well, when sof/lib/dai.h is included, depth is a uint32_t type member of dai_plat_fifo_data struct.
However when we compile with IPC4 config option, header src\include\ipc4\base-config.h is used and depth is enum ipc4_bit_depth type what causes the warning:

implicit conversion from enumeration type 'enum ipc4_bit_depth' to different enumeration type 'enum sof_ipc_frame' [-Wenum-conversion]

[aborisovich]

Yes, as @paulstelian97 indicated this looks like a serious bug.
Enums are implicitly cast to numerical values so what we do here,
is trying to assign one of the values from this set { 8, 16, 24, 32, 64, 65 } (values of enumerators of enum ipc4_bit_depth) as enumerator with such index to enum sof_ipc_frame which has enumerator values { 0, 1, 2, 3, 4 }.
I'm quite positive this is undefined behavior.

[aborisovich]

Converted back to draft, we need to verify & fix the case with enum casting.

[aborisovich]

Resolved issue with enum casting - converting back to normal PR.
Resolution: removed assignment from incompatible enumerator value:
frame_fmt = cfg->base_cfg.audio_fmt.depth &
frame_fmt = cd->sink_format
Motivation: assignment is not needed. Actually frame_fmt variable is not needed at all in the body of the:
static void build_config(struct comp_data *cd, struct module_config *cfg)
function. audio_stream_fmt_conversion function writes to frame_fmt as output, so this assignments are pointless.
Existence of frame_fmt is actually pointless too, as cd->source_format and corresponding cd->sink_format could be used
as arguments to audio_stream_fmt_conversion function.

However, this PR is not about refactoring but fixing the warnings issues by xt-clang.
Why warnings are issued by clang? Because they can.
Some explanation from C standard draft ISO/IEC 9899:TC3:

Annex I
(informative)
Common warnings
An implementation may generate warnings in many situations, none of which are
specified as part of this International Standard. The following are a few of the more
common situations.
(...)

• A value is given to an object of an enumerated type other than by assignment of an
  enumeration constant that is a member of that type, or an enumeration object that has
  the same type, or the value of a function that returns the same enumerated type
  (6.7.2.2).

Research done:

• debugged build_ut/test/cmocka/src/audio/selector/selector_test unit test with GDB.
  Unfortunately, the test is not calling static void build_config(...) function that is a part of the static int selector_init(...) function, what is completely bizarre situation (unit tests not calling code under test?)
• debugged some end-to-end tests on MTL from "TestDmicMicselPeakvolHdaPipe" family hoping to resolve related bugs but fixing enum assignments did not bring any improvement.

[marc-hb]

Should this file be in a separate commit?

[marc-hb]

Even if it's not a functional change, it very much looks like one.

[aborisovich]

Let's focus on what it actually is and not what it looks to be. As some people say "don't judge the book by the cover" :-)

[kv2019i]

@aborisovich You do need to explain this in the git commit in some way why is it ok to remove these two lines. This is not unheard of to have code that doesn't do anything, but expectation is that you explain in commit to explain why this is the case.

[aborisovich]

ok sure, I'll add some proper message

[kv2019i]

Commit msg needs a bit more rationale...

[kv2019i]

@aborisovich You do need to explain this in the git commit in some way why is it ok to remove these two lines. This is not unheard of to have code that doesn't do anything, but expectation is that you explain in commit to explain why this is the case.

[aborisovich]

You do need to explain this in the git commit in some way why is it ok to remove these two lines. This is not unheard of to have code that doesn't do anything, but expectation is that you explain in commit to explain why this is the case.

Provided more description to commit message. Is this ok now @kv2019i ?

[paulstelian97]

I'm fine with the commit, just a few minor things with the message (that I can allow).

First, "interoperability" is the right spelling.

The other thing is I would have appreciated some blank lines between the individual paragraphs on the commit description.

[aborisovich]

First, "interoperability" is the right spelling.

Thanks for caching this, fixed the spelling. Added some line breaks too.

[paulstelian97]

Re-approving, looks good now. I'd suggest for future work to eventually look at the enum issue you uncovered in a previous version of this.

[kv2019i]

This is racing with #7058 but as this now has tests passing and reviews are ok, let's have this go in first. @aiChaoSONG you probably need to rebase.

One fail in https://sof-ci.01.org/sofpr/PR7096/build4004/devicetest/index.html in the suspend-resume case, but seems unrelated to this PR. Proceeding with merge.