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.

Sign up for GitHub

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

Jump to bottom

Cppcheck fixes but its for rimage #46

Merged
merged 4 commits into from
May 19, 2021
Merged

Cppcheck fixes but its for rimage #46

merged 4 commits into from
May 19, 2021

Conversation

cujomalainey
Copy link
Contributor

No description provided.

@cujomalainey cujomalainey requested a review from lgirdwood May 4, 2021 00:25
@cujomalainey
Copy link
Contributor Author

@lgirdwood do you know why the workflows are not being recognized? Also @marc-hb I have a commit to update this to actions and off travis

@marc-hb
Copy link
Contributor

marc-hb commented May 4, 2021

Please try to copy/paste one of these two files into https://github.com/thesofproject/rimage/actions/new

For the main repo I think I remember nothing happened until I started using the web interface.

@cujomalainey
Copy link
Contributor Author

@marc-hb i think i figured it out, likely wont trigger new files when being pulled from an external fork for security reasons, since it ran fine in my repo

@cujomalainey
Copy link
Contributor Author

nevermind, added and deleted a commit and now it all works. i blame cosmic radiation

@marc-hb
Copy link
Contributor

marc-hb commented May 4, 2021

i blame cosmic radiation

I did not complete it but I did initiate the addition of a github action from the web interface... blame me?

@marc-hb marc-hb mentioned this pull request May 4, 2021
Closed
marc-hb
marc-hb reviewed May 4, 2021
View reviewed changes
@cujomalainey
Copy link
Contributor Author

i blame cosmic radiation

I did not complete it but I did initiate the addition of a github action from the web interface... blame me?

Seems to be a common issue https://github.community/t/workflow-dispatch-workflow-not-showing-in-actions-tab/130088/25

@marc-hb marc-hb requested review from fredoh9 and dcpleung May 4, 2021 04:23
marc-hb
marc-hb reviewed May 4, 2021
View reviewed changes
lgirdwood
lgirdwood reviewed May 6, 2021
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get this in rc2.

@cujomalainey
Copy link
Contributor Author

Sorry for the delay, got caught up in internal stuff

@cujomalainey cujomalainey force-pushed the cppcheck branch 3 times, most recently from bb83198 to edb8aaa Compare May 14, 2021 00:24
@cujomalainey
Copy link
Contributor Author

Travis is failing because checkpatch is mad that im using memcpy vs memcpy_s. Using the latter will require more surgery since size is not passed into the function. @lgirdwood what are your thoughts here

@cujomalainey cujomalainey requested review from lgirdwood and marc-hb May 14, 2021 00:57
marc-hb
marc-hb requested changes May 14, 2021
View reviewed changes
Copy link
Contributor

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below

lgirdwood
lgirdwood reviewed May 14, 2021
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its possible the testing used the version of rimage in $PATH. IIRC cmake check in $PATH first when it's looking for tools.

@cujomalainey
adsp_config: fix out of bounds error
f824680 
cppcheck find

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
@cujomalainey
ext_manifest: cppcheck: refactor to silence false positive
21aac06 
memory clearly has exit path through argument, factor out temporary
variable to silence cppcheck

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
@cujomalainey
manifest: close file on error
28e4852 
found via cppcheck

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
@cujomalainey
actions: add cppcheck and yamllint
740b5f5 
CI saves lives

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
fredoh9
fredoh9 approved these changes May 18, 2021
View reviewed changes
Copy link

@fredoh9 fredoh9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great fixes!

@cujomalainey cujomalainey requested a review from marc-hb May 18, 2021 00:24
@cujomalainey
Copy link
Contributor Author

FWIW i tried to update to memcpy_s but the docker container we use right now doesn't have the definition so it breaks it

marc-hb added a commit to marc-hb/sof that referenced this pull request May 18, 2021
@marc-hb
[TEST] test rimage pull request 46
3cf07eb 
See thesofproject/rimage#46

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb mentioned this pull request May 18, 2021
Closed
marc-hb
marc-hb approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rimage commit 740b5f5 tested in thesofproject/sof#4205 and everything is green (I hate to admit that submodules are very well supported by github)

FWIW i tried to update to memcpy_s but the docker container we use right now doesn't have the definition so it breaks it

Does any Linux distro have memcpy_s()?

@cujomalainey
Copy link
Contributor Author

cujomalainey commented May 18, 2021 •
edited
Loading

Rimage commit 740b5f5 tested in thesofproject/sof#4205 and everything is green (I hate to admit that submodules are very well supported by github)

FWIW i tried to update to memcpy_s but the docker container we use right now doesn't have the definition so it breaks it

Does any Linux distro have memcpy_s()?

Github does do an amazing job of handling the evil that is submodules.

I'm not sure what is going on with the container. It's Ubuntu 20.04. memcpy_s has been part of gcc since ~4.8 log shows 9.x. I even checked /usr/include/string.h and the signature was not there.

@marc-hb
Copy link
Contributor

marc-hb commented May 18, 2021 •
edited
Loading

Do you have memcpy_s on your Linux system and if yes then what distribution is it?

memcpy_s has been part of gcc since ~4.8

I think memcpy and similar functions are part of libc, not of any compiler. In fact we recently had an issue where the gcc -O optimizer was auto-generating calls to these functions that were not even explicit in the source code and that was causing the linker to fail: thesofproject/sof#3880 (comment)

In the past the glibc maintainer(s) have expressed very negative opinions about these extensions submitted by Microsoft.

dpkg -S /usr/include/string.h 
libc6-dev:amd64: /usr/include/string.h

@cujomalainey
Copy link
Contributor Author

Do you have memcpy_s on your Linux system and if yes then what distribution is it?

dpkg -S /usr/include/string.h 
libc6-dev:i386, libc6-dev:amd64: /usr/include/string.h

Only place I can see it is either in code that xcc compiles (aka SOF) or in one very obscure place I would not expect to be using any headers from. This is a fork of debian. Compiling a test program in the base OS also fails. So we can't assume its there it seems.

@cujomalainey cujomalainey requested a review from lgirdwood May 18, 2021 23:37
@lgirdwood
Copy link
Member

Travis checkpatch failure, build is good.

@lgirdwood lgirdwood merged commit 6f45b61 into thesofproject:main May 19, 2021
@cujomalainey cujomalainey deleted the cppcheck branch May 19, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgirdwood lgirdwood lgirdwood approved these changes

@fredoh9 fredoh9 fredoh9 approved these changes

@marc-hb marc-hb marc-hb approved these changes

@RanderWang RanderWang Awaiting requested review from RanderWang

@ktrzcinx ktrzcinx Awaiting requested review from ktrzcinx

@dcpleung dcpleung Awaiting requested review from dcpleung

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cujomalainey @marc-hb @lgirdwood @dcpleung @fredoh9