Error even w/warnings disabled for no-return fcns by earlephilhower · Pull Request #8495 · esp8266/Arduino

earlephilhower · 2022-02-22T17:05:06Z

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 · 2022-02-22T17:06:26Z

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]

mcspr

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 · 2022-02-22T20:53:27Z

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 · 2022-02-23T01:54:09Z

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.

… into safetydance

earlephilhower · 2022-02-23T02:35:53Z

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 · 2022-02-25T21:06:08Z

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 · 2022-02-25T22:23:40Z

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?

d-a-v

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 · 2022-03-01T17:45:15Z

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

* 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

earlephilhower requested review from d-a-v and mcspr February 22, 2022 17:05

Merge branch 'master' into safetydance

5886d52

mcspr approved these changes Feb 22, 2022

View reviewed changes

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 added 2 commits February 22, 2022 18:30

Use different lists for GCC vs G++

086b9c9

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

Merge branch 'safetydance' of https://github.com/earlephilhower/Arduino…

bea71f1

… into safetydance

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

d-a-v approved these changes Feb 28, 2022

View reviewed changes

Make separate file for each level, add readme

ee7fb2a

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

mcspr reviewed Mar 2, 2022

View reviewed changes

tools/warnings/README.md Outdated Show resolved Hide resolved

tools/warnings/README.md Outdated Show resolved Hide resolved

Address review comments, only adjusts G++/None

5520015

earlephilhower force-pushed the safetydance branch from 4695043 to 5520015 Compare March 3, 2022 04:32

Merge branch 'master' into safetydance

3d979e1

d-a-v approved these changes Mar 3, 2022

View reviewed changes

mcspr merged commit 46190b6 into esp8266:master Mar 3, 2022

earlephilhower deleted the safetydance branch March 4, 2022 05:00

igrr mentioned this pull request May 27, 2022

Enable -Wextra in CI builds espressif/arduino-esp32#6814

Closed

1 task

mcspr mentioned this pull request Jun 20, 2022

Error compiling for board NodeMCU 1.0 (ESP-12E Module)on one Windows-based machine but is able to compile on other Windows-based machines #8611

Closed

6 tasks

mcspr mentioned this pull request Jan 26, 2023

Frequent Exception 0 Crashes with v3.1.0 and v3.1.1, but none with Core <v3.0.2 #8830

Closed

6 tasks

chibidev mentioned this pull request Jan 27, 2023

Language server error related to generated compilerPath in c_cpp_properties.json for esp8266/Arduino microsoft/vscode-arduino#1567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Error even w/warnings disabled for no-return fcns#8495
mcspr merged 7 commits intoesp8266:masterfrom
earlephilhower:safetydance

earlephilhower commented Feb 22, 2022

Uh oh!

earlephilhower commented Feb 22, 2022 •

edited

Loading

Uh oh!

mcspr left a comment

Uh oh!

earlephilhower commented Feb 22, 2022

Uh oh!

earlephilhower commented Feb 23, 2022

Uh oh!

earlephilhower commented Feb 23, 2022

Uh oh!

mcspr commented Feb 25, 2022

Uh oh!

earlephilhower commented Feb 25, 2022

Uh oh!

d-a-v left a comment

Uh oh!

earlephilhower commented Mar 1, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

earlephilhower commented Feb 22, 2022

Uh oh!

earlephilhower commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcspr left a comment

Choose a reason for hiding this comment

Uh oh!

earlephilhower commented Feb 22, 2022

Uh oh!

earlephilhower commented Feb 23, 2022

Uh oh!

earlephilhower commented Feb 23, 2022

Uh oh!

mcspr commented Feb 25, 2022

Uh oh!

earlephilhower commented Feb 25, 2022

Uh oh!

d-a-v left a comment

Choose a reason for hiding this comment

Uh oh!

earlephilhower commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

earlephilhower commented Feb 22, 2022 •

edited

Loading

earlephilhower commented Mar 1, 2022 •

edited

Loading