search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.43k stars 6.39k forks source link

Wrong quotation mark position generation with Zephyr SDK and Newlib #39940

Open TheSilentDawn opened 2 years ago

TheSilentDawn commented 2 years ago

Describe the bug The Zephyr SDK generates incorrect link command when using newlibc.

To Reproduce

  1. cmake -B build/peripheral_hr -DBOARD=nrf52840dk_nrf52840 -G"Unix Makefiles" samples/bluetooth/peripheral_hr/ -D=CONFIG_LOG_PROCESS_THREAD_STACK_SIZE=2048 -DCONFIG_NO_OPTIMIZATIONS=y -DCONFIG_NEWLIB_LIBC=y
  2. check the generated file ./build/peripheral_hr/zephyr/CMakeFiles/zephyr_final.dir/link.txt and ./build/peripheral_hr/zephyr/CMakeFiles/zephyr_prebuilt.dir/link.txt

Impact Wrong linking command

Logs and console output The option in the link.txt looks like this

-L"/home/usr/zephyr-sdk-0.13.1/arm-zephyr-eabi/arm-zephyr-eabi"/lib/thumb/v7e-m/nofp

The quotation mark position should at the end

Environment (please complete the following information):

stephanosio commented 2 years ago

Technically, this is not a bug because it works and is syntactically valid. As for how sane it looks, I suppose we could do better.

https://github.com/zephyrproject-rtos/zephyr/blob/1836f10bb701f2b20f18bc59ec943cc39f57a9fc/cmake/compiler/gcc/target.cmake#L89

In general, we should refrain from adding quotation marks in the intermediate path variables, and only add them where the path variable is actually "consumed."

https://github.com/zephyrproject-rtos/zephyr/blob/21d1ad3762302b3e461953df59430c77e0709274/lib/libc/newlib/CMakeLists.txt#L25-L27

In that sense, the above should be changed to set(LIBC_LIBRARY_DIR_FLAG "-L\"${LIBC_LIBRARY_DIR}\"").

But I am no CMake expert, and @tejlmand can probably provide better insight.

tejlmand commented 2 years ago

In general, we should refrain from adding quotation marks in the intermediate path variables, and only add them where the path variable is actually "consumed."

... In that sense, the above should be changed to set(LIBC_LIBRARY_DIR_FLAG "-L\"${LIBC_LIBRARY_DIR}\"").

Completely agree. We should do this at the final place where it's consumed.

TheSilentDawn commented 2 years ago

Thanks for your prompt reply

nashif commented 6 months ago

@tejlmand does this require any action, i.e. apply the change proposed above?