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

[SKIP SOF-TEST] Add new -i3/-i4 flags + fuzz_*features.conf files to add more CONFIG_ when fuzzing #9409

Merged
merged 3 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/workflows/build_all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
strategy:
matrix:
# Keep these names short due to questionable Github UI choices
IPC: [ipc3, ipc4]
IPC: [3, 4]

steps:
- name: add i386 arch
Expand Down Expand Up @@ -65,5 +65,6 @@ jobs:
cd workspace
clang --verbose
set -x
sof/scripts/fuzz.sh -b -- -DEXTRA_CFLAGS='-Werror' -DEXTRA_CXXFLAGS='-Werror' \
-DEXTRA_CONF_FILE='stub_build_all_${{ matrix.IPC }}.conf'
sof/scripts/fuzz.sh -b -i '${{ matrix.IPC }}' -- \
-DEXTRA_CFLAGS='-Werror' -DEXTRA_CXXFLAGS='-Werror' \
-DEXTRA_CONF_FILE='stub_build_all_ipc${{ matrix.IPC }}.conf'
8 changes: 2 additions & 6 deletions .github/workflows/ipc_fuzzer.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ jobs:
fail-fast: false
matrix:
# Keep these names short due to questionable Github UI choices
IPC: [IPC3, IPC4]
IPC: [3, 4]

steps:
- name: add i386 arch
Expand Down Expand Up @@ -77,14 +77,10 @@ jobs:
cd workspace
clang --verbose
set -x
case '${{ matrix.IPC }}' in
IPC3) cmake_arg='-DCONFIG_IPC_MAJOR_3=y' ;;
IPC4) cmake_arg='-DCONFIG_IPC_MAJOR_4=y' ;;
esac
duration="${{inputs.fuzzing_duration_s}}"
duration="${duration:-301}" # pull_request has not 'inputs.' :-(
# Note libFuzzer makes a difference between -jobs and -workers (capped at nproc/2)
sof/scripts/fuzz.sh -o fuzz-stdout.txt -t "$duration" -j"$(nproc)" -- "$cmake_arg"
sof/scripts/fuzz.sh -i '${{ matrix.IPC }}' -o fuzz-stdout.txt -t "$duration" -j"$(nproc)"

- name: Upload stdout
uses: actions/upload-artifact@v4
Expand Down
1 change: 1 addition & 0 deletions app/configs/fuzz_IPC3_features.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# See main file fuzz_features.conf
4 changes: 4 additions & 0 deletions app/configs/fuzz_IPC4_features.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# See main file fuzz_features.conf

CONFIG_COMP_UP_DOWN_MIXER=y
CONFIG_COMP_ARIA=y
48 changes: 48 additions & 0 deletions app/configs/fuzz_features.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# The goal of this file is to:
# 1. Fuzz more code
# 2. Reduce the gap between fuzzed SOF and the real thing.

# KConfig warnings are NOT fatal so you must always INSPECT build logs when changing
# .conf files. See https://github.com/thesofproject/sof/issues/9386

# Note 1. is not as simple as enabling as many CONFIG_ as possible. Enabling some CONFIG_
# can technically _disable_ some code paths. But the opposite is more common so let's add
# more.

# In the longer term we should have some more elaborate configuration framework to reduce
# duplicate/diverge between fuzzing and production but also across product
# generations. Something like config fragments including each other? Not something as
# complicated as Yocto fragments but something more flexible than
# https://docs.zephyrproject.org/latest/build/kconfig/setting.html#initial-conf
#
# Discuss in https://github.com/thesofproject/sof/issues/9386

# Many of these features are too far from IPC to make any fuzzing difference. But as long
# as they reduce the size of the textual difference between build-fuzz/zephyr/.config and
# build-xxx/zephyr/.config, they make that manual comparison easier which is still a win.


CONFIG_COUNTER=y

CONFIG_PROBE=y

CONFIG_CRYPTO=y

CONFIG_LOG_TIMESTAMP_64BIT=y
CONFIG_MM_DRV=y

CONFIG_DMA=y

CONFIG_DAI=y

CONFIG_PM_DEVICE=y
CONFIG_POWER_DOMAIN=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we actually want / need to fuzz-test Zephyr Kconfig options or only SOF?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a long comment at the top of this file... too long? :-)
Does line 20 answer the question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I was confusing things a bit. For some reason I decided that for fuzzing tests we also randomise .config, which isn't the case, is it? Our fuzzing .config is fully deterministic so it can be fixed as close to the production one as only possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's the idea.

Even if we wanted to do it somewhere, kconfiglib.py has unfortunately no "randconfig" ability. At least not the last time I checked.


CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y
CONFIG_COMP_CROSSOVER=y
CONFIG_COMP_DRC=y
CONFIG_COMP_KPB=y
Copy link
Member

Choose a reason for hiding this comment

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

everything should be one here, even if its only a stub, can you turn everything on?

Copy link
Collaborator Author

@marc-hb marc-hb Aug 29, 2024

Choose a reason for hiding this comment

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

I ran into some Kconfig warnings when trying to enable more stuff and I'm running a bit out of SOF time... the main purpose of this PR is really to set the scripting "framework" in place so people who know better can come and just tweak the CONFIG_... / features. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

yep that is fine

Copy link
Collaborator Author

@marc-hb marc-hb Aug 29, 2024

Choose a reason for hiding this comment

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

A long time ago, I tried to find some -Werr option in kconfiglib.py but there is none, at least none for this. Because most people don't look at build logs, I'm afraid it means that CONFIG_ warnings and messes will never stop creeping in, see some examples in:

This comment is NOT fuzzer-specific unfortunately.


CONFIG_MATH_LUT_SINE_FIXED=y
CONFIG_MATH_EXP=y
CONFIG_MATH_IIR_DF2T=y
25 changes: 20 additions & 5 deletions scripts/fuzz.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Usage:
$0 -b -- -DEXTRA_CONF_FILE=stub_build_all_ipc4.conf -DEXTRA_CFLAGS="-O0 -g3" ...
$0 -t 500 -- -DEXTRA_CONF_FILE=stub_build_all_ipc3.conf ...

-i4 Appends: -- -DCONFIG_IPC_MAJOR_4=y + fuzz_IPC4_features.conf
-i3 See above
-p Delete build-fuzz/ first ("pristine")
-b Do not run/fuzz: stop after the build.
-t n Fuzz for n seconds.
Expand Down Expand Up @@ -85,10 +87,12 @@ main()
local BUILD_ONLY=false
local FUZZER_STDOUT=/dev/stdout # bashism
local TEST_DURATION=3
local IPC

# Parse "$@". getopts stops after '--'
while getopts "hj:po:t:b" opt; do
while getopts "i:hj:po:t:b" opt; do
case "$opt" in
i) IPC="$OPTARG";;
h) print_help; exit 0;;
j) if [ "$OPTARG" -eq 0 ]; then JOBS=$(nproc); else JOBS="$OPTARG"; fi;;
p) PRISTINE=true;;
Expand All @@ -106,16 +110,27 @@ main()
# https://docs.zephyrproject.org/latest/build/kconfig/setting.html#initial-conf
local conf_files_list='prj.conf;boards/native_sim_libfuzzer.conf'

conf_files_list+=';configs/fuzz_features.conf'
if [ -n "$IPC" ]; then
conf_files_list+=";configs/fuzz_IPC${IPC}_features.conf"
fi

# Note there's never any reason to delete fuzz_corpus/.
# Don't trust `west build -p` because it is not 100% unreliable,
# especially not when doing unusual toolchain things.
if $PRISTINE; then rm -rf build-fuzz/; fi

# When passing conflicting -DVAR='VAL UE1' -DVAR='VAL UE2' to CMake,
# the last 'VAL UE2' wins. Previous ones are silently ignored.
local cmake_args=( -DCONF_FILE="$conf_files_list" )
if [ -n "$IPC" ]; then
cmake_args+=( "-DCONFIG_IPC_MAJOR_$IPC=y" )
fi

cmake_args+=( "$@" )

(set -x
# When passing conflicting -DVAR='VAL UE1' -DVAR='VAL UE2' to CMake,
# the last 'VAL UE2' wins. Previous ones are silently ignored.
west build -d build-fuzz -b native_sim "$SOF_TOP"/app/ -- \
-DCONF_FILE="$conf_files_list" "$@"
west build -d build-fuzz -b native_sim "$SOF_TOP"/app/ -- "${cmake_args[@]}"
)

if $BUILD_ONLY; then
Expand Down