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

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

@fredoh9

@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.

@xiulipan

@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()

@lgirdwood

@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?

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'

@lgirdwood

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/

@fredoh9

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

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

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

only when you manually hardcode -O3 in CMake

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)

@xiulipan

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.

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

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()

@marc-hb

@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?

@cujomalainey

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, ...)

@fredoh9

@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;

@marc-hb

@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

Sorry, wrong keyboard shortcut

@fredoh9

@fredoh9 we will need

#if GCC < 10 around the memset(), memmove() etc
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
	...

fixed by

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

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/

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

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

@marc-hb

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

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

xiulipan commented on Mar 1, 2021

lgirdwood commented on Mar 1, 2021

fredoh9 commented on Mar 1, 2021

fredoh9 commented on Mar 2, 2021

fredoh9 commented on Mar 2, 2021

lgirdwood commented on Mar 3, 2021

marc-hb commented on Mar 8, 2021

fredoh9 commented on Mar 9, 2021

marc-hb commented on Mar 9, 2021

lgirdwood commented on Mar 9, 2021

marc-hb commented on Mar 11, 2021

marc-hb commented on Mar 11, 2021

fredoh9 commented on Mar 11, 2021

marc-hb commented on Mar 11, 2021

lgirdwood commented on Mar 11, 2021

cujomalainey commented on Mar 11, 2021

cujomalainey commented on Mar 11, 2021

lgirdwood commented on Mar 12, 2021

fredoh9 commented on Mar 20, 2021

fredoh9 commented on Mar 21, 2021

marc-hb commented on Feb 1, 2023

lgirdwood commented on Feb 1, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions