cpu/esp: treat undefined reference as errors

[cladmi]

Contribution description

For the esp cpus, treat undefined reference as errors.

It was silently ignoring compilation errors.

Cleanup needed

This pull request should show compilation issues that are currently present and should be fixed to have this one merged.

I noticed that currently stdio_uart needs isrpipe that needs xtimer but without saying the dependency. This should show this issue when building everything and need to be handled in separate pull requests.

Testing procedure

Just define an extern void lala(void); function in an application and call it.

The compilation works in master and ignores the issue.
With this pull request it will correctly complain about it.

Issues/PRs references

Found while working on running tests with the esp32 no open pull request yet.

[kaspar030]

I noticed that currently stdio_uart needs isrpipe that needs xtimer but without saying the dependency.

That is because the dependency is kinda optional. xtimer is only needed for the functions providing a timeout. Adding xtimer as a hard dependency would include it unnecessarily in many cases.

[cladmi]

It should just be in a different module "isrpipe_with_timeout" and "at" would depend on this one.
Currently it finds "xtimer.h" because we always look the headers in "sys/include" but would fail if it had another include path, or requirements on variables not defined if the xtimer module is not there.

[cladmi]

List of found errors reported by the compilation. They would need to be solved in separate pull requests:

/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-fflush.o):(.literal+0x4): undefined reference to `pthread_setcancelstate'
/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-fflush.o): In function `_fflush_r':
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fflush.c:280: undefined reference to `pthread_setcancelstate'
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fflush.c:282: undefined reference to `pthread_setcancelstate'
/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-fclose.o): In function `_fclose_r':
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fclose.c:83: undefined reference to `pthread_setcancelstate'
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fclose.c:113: undefined reference to `pthread_setcancelstate'
/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-findfp.o):/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/findfp.c:240: more undefined references to `pthread_setcancelstate' follow
collect2: error: ld returned 1 exit status

"make" -C /tmp/dwq.0.014489729839360255/43d3ed27d6da8f802a0acf99feaa2fac/build/pkg/libhydrogen \
		  -f /tmp/dwq.0.014489729839360255/43d3ed27d6da8f802a0acf99feaa2fac/pkg/libhydrogen/Makefile.libhydrogen
/opt/esp/newlib-xtensa/xtensa-lx106-elf/lib/libc.a(lib_a-signal.o):(.literal+0x0): undefined reference to `_getpid_r'
/opt/esp/newlib-xtensa/xtensa-lx106-elf/lib/libc.a(lib_a-signal.o):(.literal+0x4): undefined reference to `_kill_r'
/opt/esp/newlib-xtensa/xtensa-lx106-elf/lib/libc.a(lib_a-signal.o): In function `_raise_r':
/root/newlib/xtensa-lx106-elf/newlib/libc/signal/../../../.././newlib/libc/signal/signal.c:149: undefined reference to `_getpid_r'
/root/newlib/xtensa-lx106-elf/newlib/libc/signal/../../../.././newlib/libc/signal/signal.c:149: undefined reference to `_kill_r'
collect2: error: ld returned 1 exit status

/tmp/dwq.0.5910872133452363/a3185cf12a8492eae7468da610d39dad/build/isrpipe.a(isrpipe.o): In function `isrpipe_read':
isrpipe.c:(.text+0xbc): undefined reference to `_xtimer_set'
isrpipe.c:(.text+0xc0): undefined reference to `xtimer_remove'
/tmp/dwq.0.5910872133452363/a3185cf12a8492eae7468da610d39dad/build/isrpipe.a(isrpipe.o): In function `isrpipe_read_timeout':
isrpipe.c:(.text+0x102): undefined reference to `_xtimer_set'
isrpipe.c:(.text+0x133): undefined reference to `xtimer_remove'
collect2: error: ld returned 1 exit status

/tmp/dwq.0.6828353026911386/06e44cffaa33732e8be6595f1af2233f/build/application_tests_pkg_libb2.a(main.o):(.text+0x18): undefined reference to `blake2b'
/tmp/dwq.0.6828353026911386/06e44cffaa33732e8be6595f1af2233f/build/application_tests_pkg_libb2.a(main.o): In function `test_blake2b':
main.c:(.text+0x34): undefined reference to `blake2b'
main.c:(.text+0x74): undefined reference to `blake2s'
main.c:(.text+0x8b): undefined reference to `blake2s'
collect2: error: ld returned 1 exit status

"make" -f /tmp/dwq.0.014489729839360255/ec374fe28928e0b0212f60adf67b888a/Makefile.base MODULE=lwip -C /tmp/dwq.0.014489729839360255/ec374fe28928e0b0212f60adf67b888a/build/pkg/lwip
make[2]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
/tmp/dwq.0.014489729839360255/ec374fe28928e0b0212f60adf67b888a/build/lwip_api.a(tcpip.o): In function `tcpip_callback':
tcpip.c:(.text+0x2dc): undefined reference to `sys_mbox_trypost_fromisr'
/tmp/dwq.0.014489729839360255/ec374fe28928e0b0212f60adf67b888a/build/lwip_api.a(tcpip.o): In function `tcpip_try_callback':
tcpip.c:(.text+0x30a): undefined reference to `sys_mbox_trypost_fromisr'
collect2: error: ld returned 1 exit status

/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-fflush.o):(.literal+0x4): undefined reference to `pthread_setcancelstate'
/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-fflush.o): In function `_fflush_r':
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fflush.c:280: undefined reference to `pthread_setcancelstate'
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fflush.c:282: undefined reference to `pthread_setcancelstate'
/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-fclose.o): In function `_fclose_r':
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fclose.c:83: undefined reference to `pthread_setcancelstate'
/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/fclose.c:113: undefined reference to `pthread_setcancelstate'
/opt/esp/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/libg.a(lib_a-findfp.o):/home/gs/esp/crosstool-NG/.build/src/newlib-2.2.0/newlib/libc/stdio/findfp.c:240: more undefined references to `pthread_setcancelstate' follow

Building application "gcoap_example" for "esp32-wrover-kit" with MCU "esp32".

/tmp/dwq.0.6828353026911386/966a85d8531a73733cd04833d3d7c638/build/nanocoap.a(nanocoap.o): In function `coap_build_hdr':
nanocoap.c:(.text+0x4fc): undefined reference to `coap_resources_numof'
/tmp/dwq.0.6828353026911386/966a85d8531a73733cd04833d3d7c638/build/nanocoap.a(nanocoap.o): In function `coap_build_reply':
nanocoap.c:(.text+0x500): undefined reference to `coap_resources'
collect2: error: ld returned 1 exit status

[gschorcht]

@cladmi
Undefined references to coap_resources_numof and coap_resources in examples/gcoap are probably not caused by the make build system of ESP32. Such symbels are defined in other COAP applications

./examples/nanocoap_server
./tests/riotboot_flashwrite
./tests/nanocoap_cli

but not in

./examples/gcoap

On the other side, the compilation of

make BOARD=arduino-due -C examples/gcoap

succeeds even though

find examples/gcoap/bin/arduino-due -type f -name '*.a' -exec nm -s {} \; | grep coap_resources
         U coap_resources
         U coap_resources_numof

shows that they are also undefined there ???

[cladmi]

@gschorcht sorry, I was not clear in my wording about this. The missing symbols are indeed not caused by the esp build system configuration. Everything is fine on that side.

In that case the fact that it is even stricter than the others is good as it helps find these inconsistencies :)

What I do not understand, is why the other linker tolerates this. It must be part of the garbage collection for unused functions. If we manage to enable the same behavior, it would allow being strict on undefined references without requiring to fix the undefined functions.

And then, fixing the inconsistencies would be separate tasks that could be done after instead of before.

[gschorcht]

@cladmi

What I do not understand, is why the other linker tolerates this. It must be part of the garbage collection for unused functions.

Indeed, it seems that the only functions that use coap_resources_numof and coap_resources are not required by examples/gcoap.

nm -s examples/gcoap/bin/arduino-due/gcoap_example.elf | grep handle_req
nm -s examples/gcoap/bin/arduino-due/gcoap_example.elf | grep coap_well_known_core_default_handler

give no result. Obviously, the linker does not complain about undefined symbols for functions that are not used. Makes sense. Why the ESP32 version of ld does complain, I don't know.

[gschorcht]

@cladmi
I made some changes in Makefile.include and in cpu/esp32/syscalls.c which allow to remove the -Wl,--warn-unresolved-symbols and fixes the undefined reference problems for the following symbols:

pthread_setcancelstate
_getpid_r
_kill_r
blake2b
blake2s
blake2s

The compilation for the following application now succeeds

tests/pkg_libhydrogen
tests/pkg_libb2

How do we manage the changes. Should I open a separate PR or should I contribute to your branch?

[gschorcht]

Concerning the undefined symbol sys_mbox_trypost_fromisr in lwip applications, it is the same problem. It is a symbol required by a function that is not used in lwip applications. On other platforms, the linker doesn't complain but on ESP32 it does.

[gschorcht]

@cladmi
Adding

CFLAGS += -ffunction-sections

which places each function in its own section seems to solve the undefined symbol problem during link time even though the undefined symbol and thus it's calling function remains in ELF binary

nm -u tests/lwip/bin/esp32-wroom-32/tests_lwip.elf
         U sys_mbox_trypost_fromisr

Thus the ESP32 linker behavior seems to be similar to that of other architectures.

[cladmi]

@gschorcht nice find I will enable it. Maybe some things need to be retested though. It also means I have a way to reproduce it with any platform I think.

[cladmi]

I enabled it and added you as co-author, I will still need to change the commits order but I am interested in seeing the build results with this one.

[gschorcht]

For the pthread_setcancelstate problem I had to change much more. I would like to provide my changes.

[gschorcht]

@cladmi I pushed my changes directly into your branch. I hope thats ok.

[gschorcht]

@cladmi BTW, with PR #11108 the make system for esp8266 will become very similar to that of esp32.

[gschorcht]

I would prefer to place it in front of -fdata-sections in the line above.

[cladmi]

Yep I will do.

[cladmi]

@gschorcht no problem with pushing the changes here
Are the changes you pushed required to enable -ffunction-sections or are they for fixing the "undefined reference" ?
Because I would prefer to split the ones related to undefined reference alone, I kept the isrpipe here for testing the build but need are a more general fix.

[gschorcht]

@cladmi This changes are only for fixing the undefined reference to pthread_setcancelstate.

The problem with pthread_setcancelstate was a bit weird. Usually, it was undefined when module pthread was not enabled, so that I provided a dummy function pthread_setcancelstate defined in cpu/esp32/syscalls.c in that case. But sometimes it was also undefined although module pthread was enabled. Therefore, I provided the dummy function in any case. However, if pthread modules was enabled, I got in some cases multiple defines.

The reason seemed to be that the system libraries were not in the same linker group as the other modules. However, placing them in the same linker group required to override some system functions defined in newlibc if module esp_idf_heap is enabled.

Finally, I would claim that it is not a fix of an undefined symbol but a fix of the make system (only for ESP32 at the moment). IMO, it should be part of this PR.

[cladmi]

Finally, I would claim that it is not a fix of an undefined symbol but a fix of the make system (only for ESP32 at the moment).

This is why I would prefer to merge them before this one :)
They fix something, when this one only changes the detection.

My changes only rely on "it does not provide compilation error" or maybe checking some build size.

Your fixes are more about reviewing the defined symbols so easier for me to review on their own as I am not sure about the consequences of my changes :D

However, putting the -ffunctions-sections did not solve the esp8266 issues so there may be something more.
I noticed that also the other architectures provide the option in both the CFLAGS and the LINKFLAGS but I do not understand why it should be that way…

[gschorcht]

@cladmi

However, putting the -ffunctions-sections did not solve the esp8266 issues so there may be something more.

I will take a look.

I noticed that also the other architectures provide the option in both the CFLAGS and the LINKFLAGS but I do not understand why it should be that way…

Using option -ffunction-sections allows the linker optimizations. The disadvantage is, that you are not realizing all symbol dependencies.

[gschorcht]

@cladmi The question for me is now, should I provide my changes as a separate PR even though they fix the linker problem (at least for ESP32) after removing
LINKFLAGS += -Wl,--warn-unresolved-symbols or do we leave my changes in this PR?

[gschorcht]

@cladmi

However, putting the -ffunctions-sections did not solve the esp8266 issues so there may be something more.

The following change in cpu/esp8266/Makefile.include will solve the compilation problems for esp8266, at least for tests/pkg_libhydrogen. The problem for tests/pkg_libb2 isn't clear to me.

--- a/cpu/esp8266/Makefile.include
+++ b/cpu/esp8266/Makefile.include
@@ -56,6 +56,7 @@ PSEUDOMODULES += esp_spiffs
 
 USEMODULE += esp
 USEMODULE += mtd
+USEMODULE += newlib_syscalls_default
 USEMODULE += periph
 USEMODULE += periph_common
 USEMODULE += ps

[cladmi]

@gschorcht I think it should be in separate pull requests. No need to rush anythings. I prefer that we understand why it fails and if the issue is in the esp or more in riot.

For libhydrogen if it needs to have the newlib_syscalls maybe it should set its dependency (how do we even set a dependency to the libc syscalls ?).
As newlib is always included for cortexm, mips and riscv, the newlib_syscalls_% dependency is always included.
And for the tests/pkg_libhydrogen the avr and the msp are disabled so it would basically hide the issue.
I think the issue would then be in RIOT.

[cladmi]

However, as everybody is already doing USEMODULE += newlib and so USEMODULE += newlib_syscalls_default if no other implementation is provided it is just the same if you put it directly by default in the esp cpus with a comment saying why you added it.

RIOT/Makefile.dep

Lines 377 to 386 in 299a190

	ifneq (,$(filter newlib,$(USEMODULE)))
	# allow custom newlib syscalls implementations by adding
	# newlib_syscalls_XXX to USEMODULE
	ifeq (,$(filter newlib_syscalls_%,$(USEMODULE)))
	USEMODULE += newlib_syscalls_default
	endif
	ifeq (,$(filter stdio_rtt,$(USEMODULE)))
	USEMODULE += stdio_uart
	endif
	endif

You could maybe also say USEMODULE += newlib but then you end up parsing

https://github.com/RIOT-OS/RIOT/blob/master/makefiles/libc/newlib.mk

Not sure if it is an issue or not with the current implementation.

[jcarrano]

Is this still waiting for another PR?

[cladmi]

I will rebase at least to see the current state.

[cladmi]

I think all these have been fixed by #11967 and #12133

The both removed the "-Wl,--warn-unresolved-symbols" that I wanted to get rid of with this one.

@gschorcht thank you for handling it :)