Problems turning on -Werror

Coming from my branch of high-reliability embedded systems, we have an imperative that -Werror always be turned on. We accompany this with as much warning pedantry as the compiler allows.

While this runs into issues when using third party code as there is only so far that this can be controlled, it’s still a great ideal and any safety-critical systems really must compile without warnings.

I have been unable to use the -Werror flag (via. inserting CFLAGS += -Werror -Wfatal-errors into the top of deviceOS/?/?/build/arm-tools.mk, if anyone is interested) when compiling my app because of thousands of warnings which are coming from Particle’s toolkit. Some are trivial, such as extra ';' in the NRF SDK; but some are more architecturally important, such as enum values not being handled in a switch() in spark_wiring_ble.cpp[1]; or implicit casts from ‘float’ to ‘double’ in spark_wiring_fuel.cpp.

This is a large conversation, but I’d like to know what Particle devs think about turning on more warnings by default. It really helps a codebase’s clarity and stability to resolve all the various warnings. I can’t count the number of times I’ve found subtle bugs by resolving all warnings.

Thoughts?


[1] See https://github.com/rethinkdb/rethinkdb/issues/5177#issuecomment-169424548 for a nice read on why default should only be used to catch out-of-band enum values, and all defined enums should exhaustively be handled.

2 Likes

+1

For local gcc-arm builds (including Workbench local builds) I was able to do this for user firmware by adding a build.mk file in the src directory with the following contents:

# Standard behavior must be included here
INCLUDE_DIRS += $(SOURCE_PATH)/$(USRSRC)  # add user sources to include path
CPPSRC += $(call target_files,$(USRSRC_SLASH),*.cpp)
CSRC += $(call target_files,$(USRSRC_SLASH),*.c)

APPSOURCES=$(call target_files,$(USRSRC_SLASH),*.cpp)
APPSOURCES+=$(call target_files,$(USRSRC_SLASH),*.c)

# Custom stuff can be added here

# Make all warnings fatal
EXTRA_CFLAGS += -Werror -Wfatal-errors

There’s still a little boilerplate at the top that’s unfortunate, but using EXTRA_CFLAGS is much better than having to replicate the actual CFLAGS values. And it doesn’t require modifying the Device OS source.

Note that the build.mk file must be in src, not the top level where project.properties is.

Also, if you edit build.mk, it does not automatically force rebuild so you may need to clean application.

It does not work with the cloud compilers (CLI or cloud compile from workbench).

1 Like

I share your concerns. In addition to -Werror -Wfatal-errors, -Wenum and -Wswitch-enum. I would also like to make sure that my application code can at least compile with--std=c++14. I do not like the idea of implicitly depending on GNU extensions.

Apart from EXTRA_CFLAGS, you can rein-in the chaos in your application code to some degree:

  • Write your own Makefile to build your application code against mocks of the firmware API and whatever libraries you might be using.
    • Basically, treat GCC and / or clang as a linter.
    • Could also serve as the entry point for unit tests that do not depend on particle.
    • Allows you to mock things out to the desired level of detail you need.
    • You can also build for different combinations of compiler, optimization level, and target arch. Each of these can influence what gets flagged as a warning, and even whether test cases pass.
  • Wherever possible, use enum class in your application code, rather than typedef enum.
    • You get the benefits of -Wenum and -Wswitch-enum, plus enum variants don’t pollute the global namespace.

Unfortunately globally enabling -Werror is best done at the start of a project. That they got this far without having done that is unfortunate, but it’s also a relatively obscure feature of GCC.

Having gone through the process of enabling these flags for an existing project, I can tell you that it’s no small effort, and scrubbing all the transitive dependencies that the Particle FW might pull could take a very long time.

1 Like

As long are you are targeting Device OS 1.2.1 or later, gcc is used in --std=c++14 mode, not c++11.

IIRC, it’s --std=gnu++14. Which enabled some behavior I don’t want.

You are correct, it’s actually gnu++14.

Thank you, it was very nice to have a canonical example. I had achieved something similar, only by using CFLAGS += -Werror. It's relieving to know to use EXTRA_CFLAGS instead.

I would love to encourage you guys to turn on -Werror in DeviceOS builds. The warning flags there are easy to clean up and as @emdash points out the earlier you do it the easier it is. It's great that I can use a local build.mk to clean up my personal code, but for the best in high-reliability you really want this project wide. It's doubly important for DeviceOS because it's the underpinning of the entire ecosystem and bugs or unintended behavior there are particularly damaging.