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

[BUG] Uninitialized variable caused the CMCocka test to fail. #8988

Closed
ShriramShastry opened this issue Mar 27, 2024 · 9 comments
Closed

[BUG] Uninitialized variable caused the CMCocka test to fail. #8988

ShriramShastry opened this issue Mar 27, 2024 · 9 comments
Labels
bug Something isn't working as expected

Comments

@ShriramShastry
Copy link
Contributor

Describe the bug
A clear and concise description of what the bug is.
What have you tried to diagnose or workaround this issue?
Please also read https://thesofproject.github.io/latest/contribute/process/bug-tracking.html for further information on submitting bugs.
error: ‘scale_inv’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
To Reproduce
Steps to reproduce the behavior: (e.g. list commands or actions used to reproduce the bug)
Run ./test/test-all-defconfigs.sh
Reproduction Rate
How often does the issue happen ? i.e. 1/10 (once in ten attempts), 1/1000 or all the time.
Does the reproduction rate vary with any other configuration or user action, if so please describe and show the new reproduction rate.
Always
Expected behavior
A clear and concise description of what you expected to happen.
All cmocka test's
Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)
Cmocka cannot be tested for every configuration.
Environment

  1. Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
    • Kernel: {SHA}
    • SOF: {SHA}
  2. Name of the topology file
    • Topology: {FILE}
  3. Name of the platform(s) on which the bug is observed.
    • Platform: {PLATFORM}

Screenshots or console output
If applicable, add a screenshot (drag-and-drop an image), or console logs
(cut-and-paste text and put a code fence (```) before and after, to help
explain the issue.

An error warning that suggested there might be a problem with the variable scale_inv 
being used before it was initialized was sent to the compilation process. The Q_MULTSR_32X32 macro,
which multiplies two 32-bit numbers, uses scale_inv without first determining whether
it has been initialized, which could lead to undefined behavior. Local variables in C are not
automatically initialized.

Please also include the relevant sections from the firmware log and kernel log in the report (and attach the full logs for complete reference). Kernel log is taken from dmesg and firmware log from sof-logger. See https://thesofproject.github.io/latest/developer_guides/debugability/logger/index.html

only assignment to scale_inv is conditionally done inside an if block

if (i == 0) {
    scale_inv = Q_SHIFT_LEFT(ONE_Q30 / scale, 14, 16);
    fb->scale_log2 = base2_logarithm((uint32_t)scale) - LOG2_2P16;
}

we 've ensure that scale_inv has a deterministic initial value

@ShriramShastry ShriramShastry added the bug Something isn't working as expected label Mar 27, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 30, 2024

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 30, 2024

Run ./test/test-all-defconfigs.sh

Please report which specific test fails and in which configuration.

@ShriramShastry
Copy link
Contributor Author

Here you go:

I'm adding the compilation error log that reflects the auditory file.

Consolidate compiler generated dependencies of target dct
[ 87%] Building C object test/cmocka/src/math/window/CMakeFiles/window.dir/__/__/__/__/__/src/math/decibels.c.o
[ 87%] Building C object test/cmocka/src/math/window/CMakeFiles/window.dir/__/__/__/__/__/src/math/base2log.c.o
In file included from /home/sangeetha02/work/sof/sof/src/math/auditory/auditory.c:7:0:
/home/sangeetha02/work/sof/sof/src/math/auditory/auditory.c: In function ‘psy_get_mel_filterbank’:
/home/sangeetha02/work/sof/sof/src/include/sof/audio/format.h:103:10: error: ‘scale_inv’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  ((((px) * (py) >> ((qx) + (qy) - (qp) - 1)) + 1) >> 1)
          ^
/home/sangeetha02/work/sof/sof/src/math/auditory/auditory.c:94:10: note: ‘scale_inv’ was declared here
  int32_t scale_inv;
          ^~~~~~~~~
[ 87%] Building C object test/cmocka/src/math/dct/CMakeFiles/dct.dir/dct.c.o
cc1: all warnings being treated as errors
test/cmocka/src/math/auditory/CMakeFiles/auditory.dir/build.make:91: recipe for target 'test/cmocka/src/math/auditory/CMakeFiles/auditory.dir/__/__/__/__/__/src/math/auditory/auditory.c.o' failed
make[2]: *** [test/cmocka/src/math/auditory/CMakeFiles/auditory.dir/__/__/__/__/__/src/math/auditory/auditory.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
[ 87%] Building C object test/cmocka/src/math/dct/CMakeFiles/dct.dir/__/__/__/__/__/src/math/trig.c.o
[ 88%] Building C object test/cmocka/src/math/dct/CMakeFiles/dct.dir/__/__/__/__/__/src/math/sqrt_int16.c.o
[ 88%] Building C object test/cmocka/src/math/dct/CMakeFiles/dct.dir/__/__/__/__/__/src/math/dct.c.o
Consolidate compiler generated dependencies of target mux_copy

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 1, 2024

I don't see this warning with gcc (GCC) 13.2.1 20230801. What toolchain is this?

@ShriramShastry
Copy link
Contributor Author

I am using /tools/xcc/install/tools/RI-2022.10-linux/XtensaTools/bin/

@lyakh
Copy link
Collaborator

lyakh commented Apr 2, 2024

at least in the current "main" the scale_inv variable is always initialised before use, so looks like not every compiler can detect it. But in fact indeed I think that function can be improved - if my assumption, that .slaney_normalize isn't expected to change during the execution of that function, it's a configuration parameter, right? Then indeed it doesn't make sense to re-initialise those 3 variables on each loop iteration, so maybe something like #8999

@ShriramShastry
Copy link
Contributor Author

Yup. I am OK with your proposal for the implementation https://github.com/thesofproject/sof/pull/8999.

The rationale behind the default value {ONE_Q16}, which is not {0}, is unclear to me.

Thank you

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 11, 2024

#8999 was merged, @ShriramShastry close?

BTW I noticed your links often don't work. For instance this one does not:

Yup. I am OK with your proposal for the implementation https://github.com/thesofproject/sof/pull/8999.

@ShriramShastry
Copy link
Contributor Author

Thanks for addressing bug. I am closing issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

3 participants