[BUG] Build error with GCC10 in mux.c: missing memmove, memset and memcpy

[xiulipan]

Describe the bug
After #3848 merged, when build SOF with latest GCC10 binary, got below error on all cavs platform

/home/sof/work/xtensa-apl-elf/lib/gcc/xtensa-apl-elf/10.2.0/../../../../xtensa-apl-elf/bin/ld: CMakeFiles/sof.dir/src/audio/mux/mux.c.o: in function `mux_prepare':
/home/sof/work/sof.git/src/audio/mux/mux.c:657: undefined reference to `memmove'
/home/sof/work/xtensa-apl-elf/lib/gcc/xtensa-apl-elf/10.2.0/../../../../xtensa-apl-elf/bin/ld: CMakeFiles/sof.dir/src/audio/mux/mux.c.o: in function `mux_mix_check':
/home/sof/work/sof.git/src/audio/mux/mux.c:62: undefined reference to `memmove'

To Reproduce

# get the latest Docker image with GCC 10
docker pull thesofproject/sof:20210228
docker tag thesofproject/sof:20210228 sof
# build with docker image in sof repo folder
./scripts/docker-run.sh ./scripts/xtensa-build-all.sh apl

Reproduction Rate
10/10

Expected behavior
Build pass without error

Impact
GCC10 docker image could not be used in CI

[xiulipan]

@fredoh9 @lgirdwood I am not sure if this is GCC compiler issue, SOF code issue or Docker build issue. Will try to reproduce with local GCC compiler.

[lgirdwood]

@xiulipan it looks like we are assigning a > 32bit variable and hence GCC will try and call the C library. Quickest way to solve is to define memmove()

[fredoh9]

@lgirdwood do we need to add memmove.S like memcopy.S (src/arch/xtensa/hal/memcopy.S). Do you have one?
or can we add memcopy.c from open source implementation?

[fredoh9]

After resolving memmove error, now linker complains memset().

/home/sof/work/xtensa-apl-elf/lib/gcc/xtensa-apl-elf/10.2.0/../../../../xtensa-apl-elf/bin/ld: CMakeFiles/bootloader.dir/__/__/platform/intel/cavs/boot_loader.c.o: in function `strcmp':
/home/sof/work/sof.git/src/platform/intel/cavs/boot_loader.c:206: undefined reference to `memset'
/home/sof/work/xtensa-apl-elf/lib/gcc/xtensa-apl-elf/10.2.0/../../../../xtensa-apl-elf/bin/ld: CMakeFiles/bootloader.dir/__/__/platform/intel/cavs/boot_loader.c.o: in function `bbzero':
/home/sof/work/sof.git/src/platform/intel/cavs/boot_loader.c:79: undefined reference to `memset'

[fredoh9]

After resolving memset(), now I'm able to build build_apl_gcc/sof-apl.ri. And I tried with byt cnl icl jsl tgl tgl-h, all are working fine. So it looks memmove() and memset() are only the problem.

@lgirdwood I made a quick workaround on src/audio/mux/mux.c and src/platform/intel/cavs/boot_loader.c respectively. I copied the function from gcc library below.
I'm not sure where I should put those functions properly. Could you advise me?

https://github.com/gcc-mirror/gcc/blob/master/libgcc/

[lgirdwood]

@fredoh9 you cant copy due to license, can you re-implement both functions and add them. Look at existing memcopy_s for guidance. Thanks

[marc-hb]

Interesting, when compiling with gcc8 -O3 I noticed this:

[ 98%] Linking C executable bootloader
CMakeFiles/bootloader.dir/__/__/platform/intel/cavs/boot_loader.c.o: In function `strcmp':
boot_loader.c:(.text+0x58): undefined reference to `memset'
CMakeFiles/bootloader.dir/__/__/platform/intel/cavs/boot_loader.c.o: In function `boot_primary_core':
boot_loader.c:(.text+0x1df): undefined reference to `memset'
collect2: error: ld returned 1 exit status

[fredoh9]

GCC8 also have same problem? I didn't notice. Let me try with GCC8 local setup.

[marc-hb]

only when you manually hardcode -O3 in CMake

[lgirdwood]

GCC8 also have same problem? I didn't notice. Let me try with GCC8 local setup.

Not worth wasting time with GCC -O3 (xtensa optimisation bugs)

[marc-hb]

There's no optimization bug with xtensa-gcc 8.1, it is rather almost as you said:

@xiulipan it looks like we are assigning a > 32bit variable and hence GCC will try and call the C library.

The source location is wrong but the rest of the error message makes sense and the behavior is documented. By trial and error I isolated the missing memset issue to bbzero() (maybe -g3 + disassembly would have been faster?[*]). This one character patch is enough to fix avoid the memset error with both gcc8.1 -O3 and gcc9.2 -O3:

--- a/src/platform/intel/cavs/boot_loader.c
+++ b/src/platform/intel/cavs/boot_loader.c
@@ -73,7 +73,7 @@ static inline void bbzero(void *dest, size_t bytes)
 	int i;
 
 	for (i = 0; i < (bytes >> 2); i++)
-		d[i] = 0;
+		d[i] = 1;
 
 	dcache_writeback_region(dest, bytes);
 }

@fredoh9 can you test this one character change with gcc10? Of course this is not a fix but just a demonstration of the issue which is: even in freestanding mode (no libc) gcc feels free to optimize code into a memset or into a memcpy

Some references:

• http://cs107e.github.io/guides/gcc/

It maybe a bit surprising to learn that even when compiling in freestanding mode, gcc can emit a call to memcpy or memset

• https://gcc.gnu.org/onlinedocs/gcc/Standards.html

Most of the compiler support routines used by GCC are present in libgcc, but there are a few exceptions. GCC requires the freestanding environment provide memcpy, memmove, memset and memcmp.

• https://stackoverflow.com/questions/6410595/getting-gcc-to-compile-without-inserting-call-to-memcpy

• Same issue with clang: https://www.raspberrypi.org/forums/viewtopic.php?t=219687

• https://stackoverflow.com/questions/48676760/how-to-prevent-auto-optimization-from-for-loop-to-memset-in-visual-studio

[*] EDIT: no, objdump --source can barely make sense of heavily optimized code.

[marc-hb]

BTW boot_loader.c includes (directly or indirectly) src/arch/xtensa/include/arch/string.h which has a static inline int arch_memset_s() function that expects a missing memset(). However that is not the root-cause (at least not this time...) because boot_loader.c does not use arch_memset_s() so it gets optimized away.

I did not look for other arch_memset_s() users and where they get their memset()

[fredoh9]

@marc-hb if I changed "d[i] = 1;" in boot_loader memset error is gone. interesting experimental. (thanks you for good reading about gcc also)
Do you have any suggestion on my PR #3911?

[marc-hb]

I did not look for other arch_memset_s() users and where they get their memset()

Initial commit c0dfb4e added memset and memcpy in src/lib/lib.c. How about moving these functions to static inlines in string.h?

cc: @cujomalainey of clang "fame" (fd398c9, 149adda, ...)

[lgirdwood]

@fredoh9 can you apply this test, then rebuild after a build clean. I've altered the CT-ng C library config for GCC10 so we can now take the code from the C library. You may need to rename the other funcs too (and build clean)

diff --git a/src/arch/xtensa/CMakeLists.txt b/src/arch/xtensa/CMakeLists.txt
index 1ae3f88b9..1d58be9b3 100644
--- a/src/arch/xtensa/CMakeLists.txt
+++ b/src/arch/xtensa/CMakeLists.txt
@@ -216,7 +216,7 @@ else()
        target_link_libraries(sof_static_libraries INTERFACE reset)
 endif()
 
-target_link_libraries(sof_ld_flags INTERFACE "-Wl,-Map=sof.map")
+target_link_libraries(sof_ld_flags INTERFACE "-Wl,-Map=sof.map -lc")
 target_link_libraries(sof_ld_flags INTERFACE "-T${PROJECT_BINARY_DIR}/${platform_ld_script}")
 
 add_custom_target(
diff --git a/src/lib/lib.c b/src/lib/lib.c
index 09a1adec6..8de101451 100644
--- a/src/lib/lib.c
+++ b/src/lib/lib.c
@@ -9,6 +9,8 @@
 #include <stddef.h>
 #include <stdint.h>
 
+void *memset1(void *s, int c, size_t n);
+
 /* Not needed for host or Zephyr */
 #if !CONFIG_LIBRARY && !__ZEPHYR__
 /* used by gcc - but uses arch_memcpy internally */
@@ -19,7 +21,7 @@ void *memcpy(void *dest, const void *src, size_t n)
 }
 
 /* generic memset */
-void *memset(void *s, int c, size_t n)
+void *memset1(void *s, int c, size_t n)
 {
        uint8_t *d8 = s;
        uint8_t v = c;

[cujomalainey]

@marc-hb thanks for the FYI, once we have a solution here I will test it against the fuzzer build to see if we will break that

[cujomalainey]

Sorry, wrong keyboard shortcut

[lgirdwood]

@fredoh9 we will need

1. #if GCC < 10 around the memset(), memmove() etc
2. a similar GCC version check in Cmakefile to add -lc to the ld_flags

So here's the GCC output if anyone is interested, a nice recusive crash picked up by qemu here.

/* generic memset */
void *memset(void *s, int c, size_t n)
{
ff2cb3d4:	004136               	entry	a1, 32
	uint8_t *d8 = s;
	uint8_t v = c;
	int i;

	for (i = 0; i <	n; i++)
ff2cb3d7:	948c                	beqz.n	a4, ff2cb3e4 <memset+0x10>
		d8[i] = v;
ff2cb3d9:	04cd                	mov.n	a12, a4
ff2cb3db:	74b030               	extui	a11, a3, 0, 8
ff2cb3de:	20a220               	or	a10, a2, a2
ff2cb3e1:	ffff25               	call8	ff2cb3d4 <memset>  <<<< WRONG, confusion over this being a local branch or external call

	return s;
}
ff2cb3e4:	f01d                	retw.n
	...

[fredoh9]

fixed by

• compiler: use libc versions of memset, memcpy for GCC10 onwards #3930

[fredoh9]

for the information, Latest #3938 patch is compiled with GCC 10 toolchains.

• https://sof-ci.01.org/sofpr/PR3938/build8369/build/

-- Preparing Xtensa toolchain
-- The C compiler identification is GNU 10.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /home/sof/work/xtensa-apl-elf/bin/xtensa-apl-elf-gcc
-- Found Git: /usr/bin/git (found version "2.17.1") 

QEMU boot test also passed.

• https://sof-ci.01.org/sofpr/PR3938/build8369/boottest/

[marc-hb]

Very scary volatile / memcpy / -fno-builtin-memcpy bug and fix in Zephyr:

• intel_adsp: bbzero/bmemcpy with picolibc fix zephyrproject-rtos/zephyr#54314

[lgirdwood]

@marc-hb good find !
@mwasko @marcinszkudlinski fyi - may want to cherry-pick this Zephyr commit for mtl.