Move code from a header to a .c file by lyakh · Pull Request #1005 · thesofproject/sof

lyakh · 2019-02-13T14:47:37Z

compile- and run-tested on Up^2 with a couple of days old "master" branch. The current "master" doesn't build for me - "8d408eb cmake: add dist target" broke it.

jajanusz · 2019-02-13T14:50:22Z

compile- and run-tested on Up^2 with a couple of days old "master" branch. The current "master" doesn't build for me - "8d408eb cmake: add dist target" broke it.

git clean -fdx, but please read this first: https://explainshell.com/explain?cmd=git+clean+-fdx

I guess you just have some artifacts from the old buildsystem maybe .tarball-version in repo?

lyakh · 2019-02-13T17:05:48Z

Is this the reason for this build failure:

16:17:11,589 INFO  - /localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-xcc    CMakeFiles/pipeline_new.dir/pipeline_new.c.o CMakeFiles/pipeline_new.dir/pipeline_mocks.c.o CMakeFiles/pipeline_new.dir/pipeline_mocks_rzalloc.c.o CMakeFiles/pipeline_new.dir/__/__/__/__/__/src/audio/pipeline.c.o  -o pipeline_new -T/quickbuild/workspace/24733/SOF_FW/build_ut/test/cmocka/memory_mock.x /localdisk/tools/cmocka/build_X6H3CNL_2017_8/install/lib/libcmocka-static.a 
16:17:11,601 INFO  - /localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-xcc    CMakeFiles/mixer.dir/mixer_test.c.o CMakeFiles/mixer.dir/mock.c.o CMakeFiles/mixer.dir/comp_mock.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/buffer.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/mixer.c.o  -o mixer -T/quickbuild/workspace/24733/SOF_FW/build_ut/test/cmocka/memory_mock.x /localdisk/tools/cmocka/build_X6H3CNL_2017_8/install/lib/libcmocka-static.a -lm 
16:17:11,621 WARN  - CMakeFiles/pipeline_new_allocation.dir/__/__/__/__/__/src/audio/pipeline.c.o: In function `idc_send_msg':
16:17:11,621 WARN  - pipeline.c:(.text+0x353): undefined reference to `arch_idc_send_msg'
16:17:11,628 INFO  - test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new_allocation.dir/build.make:129: recipe for target 'test/cmocka/src/audio/pipeline/pipeline_new_allocation' failed
16:17:11,628 INFO  - make[2]: Leaving directory '/quickbuild/workspace/24733/SOF_FW/build_ut'
16:17:11,628 WARN  - make[2]: *** [test/cmocka/src/audio/pipeline/pipeline_new_allocation] Error 2
16:17:11,628 INFO  - CMakeFiles/Makefile2:1154: recipe for target 'test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new_allocation.dir/all' failed
16:17:11,628 WARN  - make[1]: *** [test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new_allocation.dir/all] Error 2
16:17:11,628 WARN  - make[1]: *** Waiting for unfinished jobs....

? This seems to be not a "normal" build, but something cmocka-related. How do I reproduce it locally or why is it failing?

jajanusz · 2019-02-13T19:03:37Z

@lyakh
https://thesofproject.github.io/latest/developer_guides/unit_tests.html
You need xt-* (xt-xcc/xt-run) toolchain for that, do you have it or only gcc ?

? This seems to be not a "normal" build, but something cmocka-related. How do I reproduce it locally or why is it failing?

Yes, it's unit-test build, if you changed location of symbols they will not run until you update them

lgirdwood

@lyakh can you check CI

lyakh · 2019-02-13T20:11:50Z

@jajanusz ah, at least now I see what's failing, thanks! I don't have xt-xcc, but it seems relatively easy to "guess" what has to be done to fix this. Only it's a pity it takes so long to get a result... But anyway, I'll probably need a couple of iterations with the CI to get this right, sorry about the electricity bill :-)

jajanusz · 2019-02-13T20:28:23Z

@jajanusz ah, at least now I see what's failing, thanks! I don't have xt-xcc, but it seems relatively easy to "guess" what has to be done to fix this. Only it's a pity it takes so long to get a result... But anyway, I'll probably need a couple of iterations with the CI to get this right, sorry about the electricity bill :-)

You can try to play with UTs as it may be helpful later, however it may be challenging to fix when you use UTs first time. Feel free to merge that: lyakh#1 +You can feel like a maintainer ;)

It should automatically update also this PR, I guess it can be the first PR in this repo with multiple commiters, so may be interesting as example for others - for example when one PR depends on another PR

lyakh · 2019-02-13T20:52:51Z

@jajanusz thanks a lot! Looking at the patch it would take me a bit more than a couple of rounds ;-) I thought I would have to create a mock up for the missing symbol.

jajanusz · 2019-02-13T20:57:56Z

@jajanusz thanks a lot! Looking at the patch it would take me a bit more than a couple of rounds ;-) I thought I would have to create a mock up for the missing symbol.

You can create stubs when dependencies are 1 level, but I guess there are some spagetthi inside so it's easier to strip unused references

jajanusz

Please change commit message from 'Move code from a header to a .c file ' to topic: what's changes format

lyakh · 2019-02-14T07:20:24Z

@jajanusz sorry, what do you mean "what's changes format?" The patch isn't introducing any functional changes, it only moves code to a .c file to prevent it from being built multiple times. The only thing I can imagine that might be worth adding to the description is what code has been moved, i.e. maybe adding an "idc:" prefix, is that what you meant?
CI servers seem to have problems again :-/

jajanusz · 2019-02-14T08:01:36Z

@jajanusz sorry, what do you mean "what's changes format?" The patch isn't introducing any functional changes, it only moves code to a .c file to prevent it from being built multiple times. The only thing I can imagine that might be worth adding to the description is what code has been moved, i.e. maybe adding an "idc:" prefix, is that what you meant?
CI servers seem to have problems again :-/

Yes. Something like idc: move implementation from header to .c file. I think all our commit use similar format (it's also in kernel)

lyakh · 2019-02-14T10:25:39Z

@jajanusz sure, can do that, but I've just pushed one more similar PR, and that one moves several headers... So there such a prefix would be difficult

lyakh · 2019-02-14T13:40:05Z

@jajanusz There we go: separate commits, with prefixes, one cmacka problem fixed by you, more expected... I'll close the other PR.

jajanusz · 2019-02-14T13:45:11Z

@lyakh It looks like there are some fixes needed for GCC compilers. I'll fix unit tests after you fix compilation for travis/jenkis cis

lgirdwood

Please just keep this to larger functions.

lgirdwood · 2019-02-14T17:53:59Z

please keep the small functions inline, this way we can avoid unnecessary calls and window exceptions on xtensa

@lgirdwood should be better now

src/arch/xtensa/smp/include/arch/idc.h has way too much code for a header, it is included by platform/idc.h on various platforms, and that header in turn is included by bultiple .c files, which builds the code multiple times into the firmware. Move it to a C file. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Many functions in src/include/sof/panic.h are too large to be inlined and repeated every time the header is included. Move them out to src/lib/panic.c Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Move large functions from src/arch/xtensa/include/arch/task.h to src/arch/xtensa/smp/init.c to avoid building them multiple times. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Move large functions from src/include/sof/wait.h to src/lib/wait.c to avoid building them multiple times. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

lyakh · 2019-02-15T08:10:30Z

@jajanusz presumably your UT fixes have to go in simultaneously with this PR?

jajanusz · 2019-02-15T09:39:51Z

@jajanusz presumably your UT fixes have to go in simultaneously with this PR?

Yes they have to be in the same PR. Otherwise QB CI check will fail what is merge-blocker. I'll try to add commit for fixing UTs later today

lyakh · 2019-02-15T12:45:00Z

@jajanusz that would be great, thanks! Otherwise we might have to rebase again, and then again, and... :-)

jajanusz · 2019-02-17T18:35:10Z

@jajanusz that would be great, thanks! Otherwise we might have to rebase again, and then again, and... :-)

This one was easy: lyakh#2 ;p

Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

lgirdwood · 2019-02-18T17:49:04Z

@lyakh CI scan

lyakh · 2019-02-19T13:26:33Z

@lyakh CI scan

@lgirdwood Nice, all the issues I've looked at so far are (logically) not mine - I only copied them over from .h to .c. That being said, many of them are real and have to be fixed. Some are also false positives, e.g. an infinite loop in the panic() implementation ;-) What I'm wondering though: hasn't the complete SOF source base been verified by KW? It seems to be a bit strange to me to use it for new submissions without having applied it to the existing code.
Anyway, what would be the proposed way to fix this? The easiest way for me would be to add patches on top - either in a new patch series (would make it even easier to not have to rebase a couple more times, but would possibly hide some bugs), or as additional patches in this series. But the "correct" way would be to first fix bugs, and then move the code... But that would make it complex, because you only see those bugs when you try to move code around.
I think the best option would be adding patches to this PR on top of the current ones - that way we make sure that we don't miss any bugs. Would that be acceptable?

lgirdwood · 2019-02-19T16:48:55Z

@lyakh yes incremental should be fine.

lyakh requested review from jajanusz, lbetlej, lgirdwood, mmaka1 and plbossart as code owners February 13, 2019 14:47

lgirdwood approved these changes Feb 13, 2019

View reviewed changes

jajanusz mentioned this pull request Feb 13, 2019

fix idc for smp lyakh/sof#1

Merged

jajanusz approved these changes Feb 13, 2019

View reviewed changes

jajanusz suggested changes Feb 13, 2019

View reviewed changes

lyakh force-pushed the idc branch from e1512d8 to e8b4d37 Compare February 14, 2019 13:38

lyakh requested review from bardliao, libinyang, tlauda and xiulipan as code owners February 14, 2019 13:38

lyakh mentioned this pull request Feb 14, 2019

Move code from multiple headers to .c files #1007

Closed

lyakh force-pushed the idc branch from e8b4d37 to 3a85d24 Compare February 14, 2019 14:49

lyakh requested a review from RanderWang as a code owner February 14, 2019 14:49

lgirdwood requested changes Feb 14, 2019

View reviewed changes

lyakh and others added 5 commits February 15, 2019 08:37

panic: move most functions from the header to a .c file

3e06542

Many functions in src/include/sof/panic.h are too large to be inlined and repeated every time the header is included. Move them out to src/lib/panic.c Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

task: move most functions from a header to a new .c file

d235631

Move large functions from src/arch/xtensa/include/arch/task.h to src/arch/xtensa/smp/init.c to avoid building them multiple times. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

wait: move most functions from the header to a new .c file

9c96785

Move large functions from src/include/sof/wait.h to src/lib/wait.c to avoid building them multiple times. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

tests: fix idc for smp

9e92719

Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

lyakh force-pushed the idc branch from 3a85d24 to 9e92719 Compare February 15, 2019 07:47

tests: add panic symbols for alloc

071dd5d

Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

lgirdwood approved these changes Feb 19, 2019

View reviewed changes

lgirdwood merged commit 5ccbab2 into thesofproject:master Feb 19, 2019

lyakh deleted the idc branch February 19, 2019 17:27

lyakh mentioned this pull request Feb 20, 2019

More cosmetic fixes and clean up #1025

Merged

Uh oh!

Conversation

lyakh commented Feb 13, 2019

Uh oh!

jajanusz commented Feb 13, 2019

Uh oh!

lyakh commented Feb 13, 2019

Uh oh!

jajanusz commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh commented Feb 13, 2019

Uh oh!

jajanusz commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Feb 13, 2019

Uh oh!

jajanusz commented Feb 13, 2019

Uh oh!

jajanusz left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh commented Feb 14, 2019

Uh oh!

jajanusz commented Feb 14, 2019

Uh oh!

lyakh commented Feb 14, 2019

Uh oh!

lyakh commented Feb 14, 2019

Uh oh!

jajanusz commented Feb 14, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

lyakh Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

lyakh commented Feb 15, 2019

Uh oh!

jajanusz commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Feb 15, 2019

Uh oh!

jajanusz commented Feb 17, 2019

Uh oh!

lgirdwood commented Feb 18, 2019

Uh oh!

lyakh commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Feb 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jajanusz commented Feb 13, 2019 •

edited

Loading

jajanusz commented Feb 13, 2019 •

edited

Loading

jajanusz commented Feb 15, 2019 •

edited

Loading

lyakh commented Feb 19, 2019 •

edited

Loading