projects/sound-open-firmware: New fuzzing framework #10342
Conversation
There was a problem hiding this comment.
any reason we are tied to a particular release?
There was a problem hiding this comment.
Just a general intuition that Zephyr SDK updates tend to be high churn and need some manual attention anyway. Also as I understand the setup, docker isn't going to be sensitive to things like remotely-pulled "latest" links and we'd need to manually rebuild the image anyway if the SDK changed underneath it.
There was a problem hiding this comment.
Just a general intuition that Zephyr SDK updates tend to be high churn and need some manual attention anyway.
Confirmed. SDK "flag day" learned the hard way this week: thesofproject/sof#7614
There was a problem hiding this comment.
Is there a way would could "inject" the version into this script? Updating things in this repo at any level of regularity is not simple.
There was a problem hiding this comment.
https://docs.docker.com/engine/reference/builder/#arg ?
It could also be a more dynamic ENV that defaults to 0.16.1 (why 0.16.0 BTW?)
There was a problem hiding this comment.
Uh... because that line of the script predates the .1 release and I missed it? :)
I'll dig up a latest link (I'm like 80% sure we have one) and use that. It won't hurt anything, and if nothing else it's a good opportunity to figure out which SDK subsets we really need so the image doesn't have to pull all the architecture toolchains we don't care about.
DonggeLiu
left a comment
DonggeLiu
left a comment
There was a problem hiding this comment.
LGTM, if you could address the questions from others : )
|
Updated the SDK URL (and also replaced it with the vastly smaller host-tools-only version). There's no clean way to download "latest" without traversing a directory, but at least this will fail and require manual update (which is statistically probably anyway) every time the contents of the /latest/ path change. |
There was a problem hiding this comment.
We don't update copyright years.
|
de-update copyright date |
There was a problem hiding this comment.
Dunno whether this matters here or not but we use a Docker image for that reason; to track ALSA: https://github.com/thesofproject/sof/blob/main/scripts/docker_build/sof_builder/Dockerfile
There was a problem hiding this comment.
Yeah, that's mostly a drive-by comment. But it seems like there's space for SOF to specify "we are based on this version of alsa-lib/alsa-utils as of your current tree". West would be handy, but then we'd have to rebuild alsa along with the firmware, and that's probably a pain.
There was a problem hiding this comment.
Yeah, that's mostly a drive-by comment. But it seems like there's space for SOF to specify "we are based on this version of alsa-lib/alsa-utils as of your current tree".
So maybe this comment is saying either too much or too little. Because the longer story is that only building topologies requires the latest and greatest ALSA, that's pretty much the only job the SOF "ALSA container" is used for. For building the kernel or the firmware a cutting edge ALSA has never been required in my experience.
So do you really need the latest ALSA here? Maybe not.
There was a problem hiding this comment.
You will get a 2x speed up with this recent and wonderful git option. I started carpet bombing SOF CI with it and it's been fantastic
| RUN cd sof && west update | |
| RUN cd sof && west update --fetch-opt=--filter=tree:0 |
It's better than --depth because it breaks neither git describe nor git merge-base thesofproject/linux#2556
More info in zephyrproject-rtos/west#638 and others
There was a problem hiding this comment.
Hm... getting fatal: --filter can only be used when extensions.partialClone is set when I try this? Maybe this is too bleeding edge for the git in the 20.04 image? Or maybe I just need to read some more docs. CI optimization is pretty far from my happy zone.
There was a problem hiding this comment.
Maybe this is too bleeding edge for the git in the 20.04 image?
Yes: I can reproduce with git 2.25.1 in Ubuntu 20.04 but not with git 2.34.1 in Ubuntu 22.04.
This happens only when trying to fetch with the --filter optimization on an existing, not already "filtered" clone (and only on 20.04).
This happens because west clones missing modules in two steps: 1. git initialization of a "non-filtered". 2. git fetch --filter=... (boom)
A tested workaround is git config remote.upstream.partialclonefilter tree:0 between 1. and 2. but of course you can't inject any command between west steps 1. and 2.
https://git-scm.com/docs/repository-version
Ubuntu 22.04 is not affected because git fetch with version 2.34.1 automagically adds the git config above.
Do you really need to use Ubuntu 20.04? We recently upgraded everything to 22.04 and we experienced a few bugs but absolutely nothing related to toolchains or building anything.
There was a problem hiding this comment.
You will get a 2x speed up with this recent and wonderful git option.
Finally woke up and realized this is run at image creation time; to sort-of "warm up a git cache". So while this git optimization trick is very good to know it's totally off-topic here, sorry the distraction. Resolving this comment.
EDIT: I can't resolve comments here for some reason. My shame will stay on display.
There was a problem hiding this comment.
@marc-hb its because we are not part of the repo
There was a problem hiding this comment.
Can this be overriden on some --arg command line? If yes maybe say it for the Docker noobs among us.
There was a problem hiding this comment.
At the docker level, yes. docker build --build-arg SDK_VER=0.16.2 would have the expected effect. I have no idea if that's wired meaingfully to the oss-fuzz infrastructure or not.
Really this is just here to define a local variable for clarity. Docker has two variable tyes: ARG works like this and can be set externally, but only works within the dockerfile itself. ENV cannot be set externally, but also persists into the default environment variables of processes spawned in the container, which is undesirable here.
There was a problem hiding this comment.
Bleh if the versions weren't part of the release file name this would be much easier. Seems like a common problem.
There was a problem hiding this comment.
I think we will have to leave this as it, not much hope for fixing it without some additional logic
There was a problem hiding this comment.
Nit: the -J hasn't been needed for ages: auto-detect. Except when using a pipe?
There was a problem hiding this comment.
Do you really want to use a pipe? I mean without set -o pipefail? Downloads are the commands the most likely to fail in this script, aren't they?
Consider how I landed here :-)
There was a problem hiding this comment.
This seems unnecessary, bash supports multilines strings?
| BASE_CFG=`cat <<EOF | |
| BASE_CFG=' | |
| -DCONFIG_ASSERT=y | |
| ... |
There was a problem hiding this comment.
The recommended and shellcheck-compliant way to do this is using a bash array:
BASE_CFG=(
-DCONFIG_ASSERT=y
-DCONFIG_SYS_HEAP_BIG_ONLY=y
...
)
declare -p BASE_CFG
west build -p -b native_posix ./app -- "${BASE_CFG[@]}"shellcheck saves lives; use it always. It's also a great... teacher.
You can't go wrong with arrays, they're always safe.
http://mywiki.wooledge.org/BashGuide/Arrays
http://mywiki.wooledge.org/Quotes
|
Hm... looks like the target list isn't as autodetecty as I thought it was. Need to figure out the yaml scheme to get it to conform to what the runner thinks it should be. Will update the shellisms. |
|
One more spin. Update shell fu per @marc-hb. Declare the right set of architectures/sanitizers in yaml. |
-Wl,EL is a linker option, not a compiler option and clang does not
like it at compilation time; it fails like this:
```
cd smex
cmake -B build -DCMAKE_C_COMPILER=clang
make -C build
clang-15: error: -Wl,-EL: 'linker' input unused
[-Werror,-Wunused-command-line-argument]
```
Reported by @andyross in google/oss-fuzz#10342
oss-fuzz does not need smex at all but this one-line fix is just
faster and simpler than a bigger CMake re-architecture just for
oss-fuzz.
Also simplify this for clang compatibility:
```
error: unknown warning option '-Wimplicit-fallthrough=3'; did you mean
'-Wimplicit-fallthrough'? [-Werror,-Wunknown-warning-option]
```
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
SOF has a new fuzzing framework that leverages the underlying Zephyr OS's native_posix integration to get a more complete build than just the protocol layer. In theory all the non-hardware-specific code is reachable from the fuzzer inputs. Also adds support for the newer "IPC4" protocol as a separate target. Signed-off-by: Andy Ross <andyross@google.com>
|
Yet one more, because @marc-hb pointed out that ALSA isn't actually required for a SOF firmware build anymore (no topology files for fuzzing). And also because CI seemed to be indefinitely stuck... |
-Wl,EL is a linker option, not a compiler option and clang does not
like it at compilation time; it fails like this:
```
cd smex
cmake -B build -DCMAKE_C_COMPILER=clang
make -C build
clang-15: error: -Wl,-EL: 'linker' input unused
[-Werror,-Wunused-command-line-argument]
```
Reported by @andyross in google/oss-fuzz#10342
oss-fuzz does not need smex at all but this one-line fix is just
faster and simpler than a bigger CMake re-architecture just for
oss-fuzz.
Also simplify this for clang compatibility:
```
error: unknown warning option '-Wimplicit-fallthrough=3'; did you mean
'-Wimplicit-fallthrough'? [-Werror,-Wunknown-warning-option]
```
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
Is this ready to merge? |
|
Should be, yes. |
|
Yes please go ahead :) |
Just a stable-v2.2 test. Sample failure on the main branch: https://github.com/thesofproject/sof/actions/runs/5053499443/ Probably since google/oss-fuzz#10342 ? Signed-off-by: Marc Herbert <marc.herbert@intel.com>
jonathanmetzman
left a comment
jonathanmetzman
left a comment
There was a problem hiding this comment.
The build is broken because our tooling (srcmap) breaks on this: https://oss-fuzz-build-logs.storage.googleapis.com/index.html#sound-open-firmware
| # Environment: Zephyr has its own toolchain selection mechanism, and | ||
| # the oss-fuzz environment generates warnings and confuses things when | ||
| # building some of the (un-fuzzed) build-time tools that aren't quite | ||
| # clang-compatible. | ||
| export ZEPHYR_TOOLCHAIN_VARIANT=llvm | ||
| unset CC | ||
| unset CCC | ||
| unset CXX | ||
| unset CFLAGS | ||
| unset CXXFLAGS |
There was a problem hiding this comment.
Not really a fan of this. LibFuzzer is bundled with clang and using your own clang can cause many other problems. I guess if you do this, we can't help you with build issues.
There was a problem hiding this comment.
I may even revert this
There was a problem hiding this comment.
This isn't being used to build the fuzzed binary, which is grabbing clang off of PATH. It's there because there's a build-time tool in SOF (that actually isn't used except on real firmware) that doesn't build with clang due to routine warning collisions. AFAICT @marc-hb fixed this upstream, not sure if that patch merged or not.
There was a problem hiding this comment.
OK, false alarm, this seems fine.
There was a problem hiding this comment.
AFAICT @marc-hb fixed this upstream, not sure if that patch merged or not.
Yes it's merged:
| WORKDIR $SRC | ||
|
|
||
| # SOF itself (also pulls in Zephyr + modules via west) | ||
| RUN west init -m https://github.com/thesofproject/sof sof |
There was a problem hiding this comment.
Could you use git instead? This is breaking some of our tooling that finds revisions (it assumes git or svn usage like all other users).
There was a problem hiding this comment.
West is a git wrapper, analogous to repo. It's possible, but error prone, to manually clone all the relevant repositories. The structure isn't complicated, $SRC/sof is the top level directory which contains $SRC/sof/sof, which is the git repo corresponding to the project itself, and $SRC/sof/zephyr, and a few other bits under $SRC/sof/modules/*.
What are the requirements here? Can I symlink you something to the top level that matches what you want to see?
There was a problem hiding this comment.
OK, I see it. srcmap assumes that the git archive was pulled from a remote with the name "origin". This is true by default with an unadorned git clone. It's not required, and in fact west allows multiple remotes with names like "thesofproject" or "upstream".
FWIW: a straightforward fix to srcmap, which replaces the assumption that "there is always a remote named origin" with "we only care about the first remote listed" is something like (untested):
--- a/infra/base-images/base-builder/srcmap
+++ b/infra/base-images/base-builder/srcmap
@@ -35,7 +35,7 @@ fi
for DOT_GIT_DIR in $(find $PATHS_TO_SCAN -name ".git" -type d); do
GIT_DIR=$(dirname $DOT_GIT_DIR)
cd $GIT_DIR
- GIT_URL=$(git config --get remote.origin.url)
+ GIT_URL=$(git config --get remote.$(git remote | head -1).url)
GIT_REV=$(git rev-parse HEAD)
jq_inplace $SRCMAP ".\"$GIT_DIR\" = { type: \"git\", url: \"$GIT_URL\", rev: \"$GIT_REV\" }"
doneIt's worth pointing out that these are still just assumptions. In the general case it's not possible to look at a .git directory and ask the question "from where was this pulled?". Maybe it would be better to allow projects to specify the srcmap themselves for edge cases like this?
Is there a way for me to run srcmap locally in a way that corresponds to your build failures to test this? I'm digging around and getting a little lost.
There was a problem hiding this comment.
Alternatively, I could update the project code to go in and deliberately add a "origin" remote that copies the one already there. That's more code and won't help anyone else, but might be more palatable?
There was a problem hiding this comment.
I think the origin trick would work. If you're using the infra/helper.py shell command to build/debug, you can run the srcmap command (inside our docker container) to figure out if this will work or not.
There was a problem hiding this comment.
FWIW west list has format options, for instance:
west list -f '{url}'
https://github.com/thesofproject/rimage
https://github.com/thesofproject/tomlc99
https://github.com/zephyrproject-rtos/zephyr
https://github.com/zephyrproject-rtos/hal_xtensa
https://github.com/zephyrproject-rtos/lz4
https://github.com/zephyrproject-rtos/mbedtls
https://github.com/zephyrproject-rtos/mipi-sys-t
...
There was a problem hiding this comment.
Global fix up at #10383
Will look at the project-side workaround now. Honestly didn't even know about west list, but that's cleaner than find $SRC -name .git for sure.
There was a problem hiding this comment.
jonathanmetzman
left a comment
jonathanmetzman
left a comment
There was a problem hiding this comment.
The build is broken because our tooling (srcmap) breaks on this: https://oss-fuzz-build-logs.storage.googleapis.com/index.html#sound-open-firmware
|
@andyross you should have access to the dashboard now, please look into this |
5 participants
[First submission to oss-fuzz. Not sure if I'm checking all the correct boxes to get this submitted successfully. Be gentle.]
SOF has a new fuzzing framework that leverages the underlying Zephyr OS's native_posix integration to get a more complete build than just the protocol layer. In theory all the non-hardware-specific code is reachable from the fuzzer inputs.
Also adds support for the newer "IPC4" protocol as a separate target.