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

Error even w/warnings disabled for no-return fcns #8495

Merged
merged 7 commits into from
Mar 3, 2022

Conversation

earlephilhower
Copy link
Collaborator

A function whose prototype says it will return a value but doesn't
is undefined behaviour in C++. GCC 10 will generate code that crashes
in this case.

In warnings==None mode, insterad of turning off all warnings with
-w, explicitly list all G++ possible warnings except for the
no-return warning which catches this programming error.

A function whose prototype says it will return a value but doesn't
is undefined behaviour in C++.  GCC 10 will generate code that crashes
in this case.

In warnings==None mode, insterad of turning off all warnings with
`-w`, explicitly list all G++ possible warnings except for the
`no-return` warning which catches this programming error.
@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Feb 22, 2022

With this PR, the following code generates a compile error even with File->Prefs->Warnings->None

int test() {
    Serial.println("test");
}
    
void setup() {
  test();
}

void loop() {
}

output:

...
Compiling sketch...
/home/earle/Arduino/hardware/esp8266com/esp8266/tools/python3/python3 -I /home/earle/Arduino/hardware/esp8266com/esp8266/tools/signing.py --mode header --publickey /home/earle/Arduino/sketch_feb21a/public.key --out /tmp/arduino_build_467286/core/Updater_Signing.h
/home/earle/Arduino/hardware/esp8266com/esp8266/tools/xtensa-lx106-elf/bin/xtensa-lx106-elf-g++ -D__ets__ -DICACHE_FLASH -U__STRICT_ANSI__ -D_GNU_SOURCE -DESP8266 -I/home/earle/Arduino/hardware/esp8266com/esp8266/tools/sdk/include -I/home/earle/Arduino/hardware/esp8266com/esp8266/tools/sdk/lwip2/include -I/home/earle/Arduino/hardware/esp8266com/esp8266/tools/sdk/libc/xtensa-lx106-elf/include -I/tmp/arduino_build_467286/core -c @/home/earle/Arduino/hardware/esp8266com/esp8266/tools/gcc-no-warnings -Werror=return-type -Os -g -free -fipa-pta -mlongcalls -mtext-section-literals -fno-rtti -falign-functions=4 -std=gnu++17 -MMD -ffunction-sections -fdata-sections -fno-exceptions -DMMU_IRAM_SIZE=0x8000 -DMMU_ICACHE_SIZE=0x8000 -DNONOSDK22x_190703=1 -DF_CPU=80000000L -DLWIP_OPEN_SRC -DTCP_MSS=536 -DLWIP_FEATURES=1 -DLWIP_IPV6=0 -DARDUINO=10816 -DARDUINO_ESP8266_WEMOS_D1MINI -DARDUINO_ARCH_ESP8266 "-DARDUINO_BOARD=\"ESP8266_WEMOS_D1MINI\"" -DFLASHMODE_DIO "-DSTASSID=\"STA\"" "-DSTAPSK=\"PSK\"" -I/home/earle/Arduino/hardware/esp8266com/esp8266/cores/esp8266 -I/home/earle/Arduino/hardware/esp8266com/esp8266/variants/d1_mini /tmp/arduino_build_467286/sketch/sketch_feb21a.ino.cpp -o /tmp/arduino_build_467286/sketch/sketch_feb21a.ino.cpp.o
/home/earle/Arduino/sketch_feb21a/sketch_feb21a.ino: In function 'int test()':
sketch_feb21a:3:5: error: no return statement in function returning non-void [-Werror=return-type]
    3 |     }
      |     ^
cc1plus: some warnings being treated as errors
exit status 1
no return statement in function returning non-void [-Werror=return-type]

Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

I guess this will also work :)
Since there are no -Wall or -Wextra, can the list be reduced to only contain stuff tagged [enabled] from the gcc -Q --help=warnings output?

@earlephilhower
Copy link
Collaborator Author

It's a lot of lines, but really just the output of

./tools/xtensa-lx106-elf/bin/xtensa-lx106-elf-g++ --help=warnings | awk '{print $1}' | sed 's/-W/-Wno-/' | grep -v = | grep -v -- -f > tools/gcc-no-warnings

and about 3 minutes manually removing warnings that aren't C/C++ compatible (GCC gives a warning for each non-C warning, so just do a compile and scan and dd in vi.) So for GCC 10.3, I'd rather just leave it as-is with everything. If we go to 11.2 in the future, we can revisit and I can add a grep '[enabled]' to the pipeline.

@earlephilhower earlephilhower changed the title Error even w/warnings disabled for no-return fcns WIP: Error even w/warnings disabled for no-return fcns Feb 23, 2022
@earlephilhower
Copy link
Collaborator Author

Some more checks show that I have some C++ warnings disabled on the 4-5 C code files we have, so this isn't quite there yet. Let me try @mcspr's suggestion and see if the common set of enabled ones is legit for both C and C++.

G++ and GCC have different warning options, so use different lists.
@earlephilhower earlephilhower changed the title WIP: Error even w/warnings disabled for no-return fcns Error even w/warnings disabled for no-return fcns Feb 23, 2022
@earlephilhower
Copy link
Collaborator Author

Latest push has differing C and C++ warnings files for Mode==None. Tested all warning modes with complete restart between (to ensure core is rebuilt).

A slightly-hackish way is used to allow the compiler type to be used as a parameter in compiler.warning_flags. The GCC or G++ compile recipe now appends gcc/g++ immediately after the warning_flags. So you end up with @file-name-gcc or @file-name-g++. For the Default...All levels, finish the warning flags with a -D__unused_define= so you end up with a harmless -D__unused_define={gcc,g++} on the command line.

@mcspr
Copy link
Collaborator

mcspr commented Feb 25, 2022

I wonder if it would be worth to go all-in at this point

diff --git a/platform.txt b/platform.txt
index 39414b98..c0fa81b2 100644
--- a/platform.txt
+++ b/platform.txt
@@ -21,11 +21,11 @@ runtime.tools.mkdir={runtime.platform.path}/tools/mkdir.py
 runtime.tools.cp={runtime.platform.path}/tools/cp.py
 runtime.tools.eboot={runtime.platform.path}/bootloaders/eboot/eboot.elf

-compiler.warning_flags=@{runtime.platform.path}/tools/no-warnings-
-compiler.warning_flags.none=@{runtime.platform.path}/tools/no-warnings-
-compiler.warning_flags.default=-D__COMPILER=
-compiler.warning_flags.more=-Wall -D__COMPILER=
-compiler.warning_flags.all=-Wall -Wextra -D__COMPILER=
+compiler.warning_flags={runtime.platform.path}/tools/warnings/none
+compiler.warning_flags.none={runtime.platform.path}/tools/warnings/none
+compiler.warning_flags.default={runtime.platform.path}/tools/warnings/default
+compiler.warning_flags.more={runtime.platform.path}/tools/warnings/more
+compiler.warning_flags.all={runtime.platform.path}/tools/warnings/all

 build.lwip_lib=-llwip_gcc
 build.lwip_include=lwip/include
@@ -68,18 +68,18 @@ compiler.cpreprocessor.flags=-D__ets__ -DICACHE_FLASH -U__STRICT_ANSI__ -D_GNU_S
 compiler.libraries.ldflags=

 compiler.c.cmd=xtensa-lx106-elf-gcc
-compiler.c.flags=-c {compiler.warning_flags}gcc -std=gnu17 {build.stacksmash_flags} -Os -g -free -fipa-pta -Werror=return-type -Wpointer-arith -Wno-implicit-function-declaration -Wl,-EL -fno-inline-functions -nostdlib -mlongcalls -mtext-section-literals -falign-functions=4 -MMD -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags} {build.mmuflags} {build.non32xferflags}
+compiler.c.flags=-c @{compiler.warning_flags}-gcc -std=gnu17 {build.stacksmash_flags} -Os -g -free -fipa-pta -Werror=return-type -Wpointer-arith -Wno-implicit-function-declaration -Wl,-EL -fno-inline-functions -nostdlib -mlongcalls -mtext-section-literals -falign-functions=4 -MMD -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags} {build.mmuflags} {build.non32xferflags}

 compiler.S.cmd=xtensa-lx106-elf-gcc
 compiler.S.flags=-c -g -x assembler-with-cpp -MMD -mlongcalls "-I{runtime.tools.xtensa-lx106-elf-gcc.path}/include/"

-compiler.c.elf.flags=-g {compiler.warning_flags}gcc -Os -nostdlib -Wl,--no-check-sections -u app_entry {build.float} -Wl,-static "-L{compiler.sdk.path}/lib" "-L{compiler.sdk.path}/lib/{build.sdk}" "-L{build.path}" "-L{compiler.libc.path}/lib" "-Tlocal.eagle.flash.ld" -Wl,--gc-sections -Wl,-wrap,system_restart_local -Wl,-wrap,spi_flash_read
+compiler.c.elf.flags=-g @{compiler.warning_flags}-gcc -Os -nostdlib -Wl,--no-check-sections -u app_entry {build.float} -Wl,-static "-L{compiler.sdk.path}/lib" "-L{compiler.sdk.path}/lib/{build.sdk}" "-L{build.path}" "-L{compiler.libc.path}/lib" "-Tlocal.eagle.flash.ld" -Wl,--gc-sections -Wl,-wrap,system_restart_local -Wl,-wrap,spi_flash_read

 compiler.c.elf.cmd=xtensa-lx106-elf-gcc
 compiler.c.elf.libs=-lhal -lphy -lpp -lnet80211 {build.lwip_lib} -lwpa -lcrypto -lmain -lwps -lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 {build.stdcpp_lib} -lm -lc -lgcc

 compiler.cpp.cmd=xtensa-lx106-elf-g++
-compiler.cpp.flags=-c {compiler.warning_flags}g++ {build.stacksmash_flags} -Os -g -free -fipa-pta -Werror=return-type -mlongcalls -mtext-section-literals -fno-rtti -falign-functions=4 {build.stdcpp_level} -MMD -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags} {build.mmuflags} {build.non32xferflags}
+compiler.cpp.flags=-c @{compiler.warning_flags}-g++ {build.stacksmash_flags} -Os -g -free -fipa-pta -Werror=return-type -mlongcalls -mtext-section-literals -fno-rtti -falign-functions=4 {build.stdcpp_level} -MMD -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags} {build.mmuflags} {build.non32xferflags}

 compiler.as.cmd=xtensa-lx106-elf-as

where files are tools/warnings/${warnings-name}-${compiler-name}

@earlephilhower
Copy link
Collaborator Author

I wonder if it would be worth to go all-in at this point ...snip...

I thought about doing it that way at first, and while on the +ve side it gets rid of the kludge -Dunusedstuff I think it obfuscates this since the files themselves will be a max of 2 lines. More files == more places to lose things in...

I could go either way on this, though. Thoughts, @d-a-v?

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

It is worth adding a ref here to #8421.
I'm OK for either way,

More files == more places to lose things in...

... and it would be nice in both case to have a script which regenerates the warning-list files. I think it can come later with a compiler update.

Thank you both

@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Mar 1, 2022

Thanks for the decisiveness, @d-a-v . 😆

Hold off on merging this. After consideration, I think @mcspr 's suggestion is cleaner. You would have to go through extra files to figure out WTH is going on, but you wouldn't have the random -Dblah which is probably worse.

I'll do an update w/separate option files for all modes shortly. Updated

The readme now includes the exact commands required to regenerate the
none-XXX files, no manual editing needed.
tools/warnings/README.md Outdated Show resolved Hide resolved
tools/warnings/README.md Outdated Show resolved Hide resolved
@mcspr mcspr merged commit 46190b6 into esp8266:master Mar 3, 2022
@earlephilhower earlephilhower deleted the safetydance branch March 4, 2022 05:00
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* Error even w/warnings disabled for no-return fcns

A function whose prototype says it will return a value but doesn't
is undefined behaviour in C++.  GCC 10 will generate code that crashes
in this case.

In warnings==None mode, insterad of turning off all warnings with
`-w`, explicitly list all G++ possible warnings except for the
`no-return` warning which catches this programming error.

* Use different lists for GCC vs G++

G++ and GCC have different warning options, so use different lists.

* Make separate file for each level, add readme

The readme now includes the exact commands required to regenerate the
none-XXX files, no manual editing needed.

* Address review comments, only adjusts G++/None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants