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

Add errors on invalid/missing function return type #8165

Merged
merged 3 commits into from
Jun 24, 2021

Conversation

earlephilhower
Copy link
Collaborator

GCC 10.x seems to have a knack for crashing when a function which is declared
to return a value does not. Add a warning, present on all builds, when this
is the case. For more info see
#8160

Thanks to @hreintke for the tip

GCC 10.x seems to have a knack for crashing when a function which is declared
to return a value does not.  Add a warning, present on all builds, when this
is the case.  For more info see
esp8266#8160

Thanks to @hreintke for the tip
@dok-net
Copy link
Contributor

dok-net commented Jun 23, 2021

This warning is basically a must-have on the ESPs. Is there a way to treat it as an error? Because that is what it really is on this architecture, returning from non-void function without a return statement. @earlephilhower That said, then looked at your PR, you have already upgraded it to error, correct? Thanks, this will help me with 3rd party PRs to my libraries as well.

@mcspr
Copy link
Collaborator

mcspr commented Jun 23, 2021

This warning is basically a must-have on the ESPs. Is there a way to treat it as an error? Because that is what it really is on this architecture, returning from non-void function without a return statement.

Notice it is not just -W, but -Werror. Turns out gcc can selectively pick warnings to be treated as errors

@dok-net
Copy link
Contributor

dok-net commented Jun 23, 2021

@me-no-dev In reference to the now closed discussion about espressif/arduino-esp32#5310, would you see any value in this and get it inserted into the right places, instead of the simple warning, that I am already getting for such misuses (warning: no return statement in function returning non-void [-Wreturn-type])?

@me-no-dev
Copy link
Collaborator

@dok-net I will accept the same change in the ESP32 repo 😄

@dok-net
Copy link
Contributor

dok-net commented Jun 24, 2021

@me-no-dev Before moving over there (sorry ESP8266 guys for talking about ESP32 here), you recently said:

all of the changes that you have made are to files that are auto generated by the lib builder. They will not stick. The lib builder get's the flags from IDF's compile process.

The files were platform.txt, and tools/platformio-build-esp32*.py.
Predictably, now I am confused, shall I PR a change to these files in the ESP32 repo or not? Mind you, I am not familiar with "the lib builder" or the IDF.

@me-no-dev
Copy link
Collaborator

just -Werror=return-type can be added to platform.txt. Nothing else from your previous PR

@dok-net
Copy link
Contributor

dok-net commented Jun 24, 2021

@earlephilhower This is from ESP32,

compiler.warning_flags.more=-Wall -Werror=all
compiler.warning_flags.all=-Wall -Werror=all -Wextra

What do you think about -Werror=all for flags.more and flags.all, respectively? I can't image there is any difference between ESP8266 and ESP32 that requires this difference? Of course this is before this PR, but I'm just about to duplicate that to ESP32, so I could use some guidance on whether to make this the same.

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.

Also need to update tools/platformio-build.py

CFLAGS=[
"-std=gnu17",
"-Wpointer-arith",
"-Wno-implicit-function-declaration",
"-Wl,-EL",
"-fno-inline-functions",
"-nostdlib"
],
CCFLAGS=[
"-Os", # optimize for size
"-mlongcalls",
"-mtext-section-literals",
"-falign-functions=4",
"-U__STRICT_ANSI__",
"-D_GNU_SOURCE",
"-ffunction-sections",
"-fdata-sections",
"-Wall",
"-free",
"-fipa-pta"
],
CXXFLAGS=[
"-fno-rtti",
"-std=gnu++17"
],

I guess CCFLAGS is the correct choice, based on underlying build system docs:
https://scons.org/doc/latest/HTML/scons-user/apa.html

CCFLAGS
General options that are passed to the C and C++ compilers.

@earlephilhower earlephilhower added this to the 3.0.1 milestone Jun 24, 2021
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.

5 participants