Sample makefile should use strict compiler flags and treat warnings as errors

[jphickey]

Describe the bug
The default makefiles / CMake scripts do not enforce any strict compiler warning flags and do not treat warnings as errors. This means issues like #22 can more easily slip past.

To Reproduce
Building the rc-6.7.0 branch as a "release" (-O3) using the default settings/sample config/makefile wrapper by preparing as:

make SIMULATION=native BUILDTYPE=release prep

Then run make and you do get some warnings, at least when using gcc 7.x and above:

[ 56%] Building C object cfe_core_default_cpu1/CMakeFiles/cfe_core_default_cpu1.dir/src/es/cfe_es_task.c.o
/home/joe/code/cfecfs/github/cfe/fsw/cfe-core/src/es/cfe_es_task.c: In function ‘CFE_ES_TaskInit’:
/home/joe/code/cfecfs/github/cfe/fsw/cfe-core/src/es/cfe_es_task.c:374:64: error: array subscript is below array bounds [-Werror=array-bounds]
        strncat(EventBuffer, VersionBuffer, sizeof(EventBuffer)-strlen(EventBuffer-1));
                                                                ^~~~~~~~~~~~~~~~~~~~~
/home/joe/code/cfecfs/github/cfe/fsw/cfe-core/src/es/cfe_es_task.c:380:64: error: array subscript is below array bounds [-Werror=array-bounds]
        strncat(EventBuffer, VersionBuffer, sizeof(EventBuffer)-strlen(EventBuffer-1));
                                                                ^~~~~~~~~~~~~~~~~~~~~

However, the build continues and completes the process with no error results.

Expected behavior
The build should stop, because warnings are problems that need to be resolved. If the build does not stop then it is very easy to not notice these issues.

System observed on:
Ubuntu 18.04 (64-bit), kernel 5.0.0-23-generic, gcc 7.4.0

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

[skliper]

Just looked at OSAL building verbose, and it didn't include -Wall by default (doesn't look like elf2cfetbl has it either). Is the line setting it in osal/CMakeLists.txt working as intended? Should this be a top level setting (along with the rest of the useful flags) instead of buried in each element's recipe?

[jphickey]

The problem was, historically, not every tool built successfully with -Wall -Werror so it was only turned on for those individual tools that were known to be clean. This is why it is a bit piecemeal; some tools use -Wall, some do not. Also, note that OSAL source files might be built several times with different flags (notably, for the target, for the host, and for unit test, if enabled).

Now that we have cleaned up warnings in most tools, this could possibly be made better.

[skliper]

A cleanup would be good.

My take (from your email suggestion) is at minimum "-Wall –std=c99 –pedantic –Wstrict-prototypes –Wwrite-strings" everywhere by default (or at least all flight code), all the time set from a top level (hopefully something in sample_defs or from the default top Makefile).

I'd prefer if -Werror was optional such that we can script a CI build of the community set of apps with and without it included. I want to avoid having to edit files for the various CI tests and I suspect not all apps will be warning free.

[skliper]

Addendum to previous comment - other threads have mentioned -Wall and -Werror does break some elements, which bolsters the use case for -Werror being easily configurable such that we can apply this change easily without breaking everything (report all warnings, make -Werror optional).

[jphickey]

My idea is to add these flags to the Makefile.sample file, by conditionally setting the OSAL_USER_C_FLAGS variable (e.g. a ?= assignment).

This way, if you do set that variable in your prep step, your setting will override the value in the makefile. This way its easy to take it out in case of building some legacy code where -Werror doesn't work, or in a CI build script that includes legacy code.

[skliper]

Note it doesn't work as I'd expect now

make prep

reports

-- OSAL Compile Definitions: -Wall   -D_XOPEN_SOURCE=600

Yet a

make VERBOSE=1

snippit for an OSAL element shows:

[  3%] Building C object osal/CMakeFiles/osal.dir/src/os/shared/osapi-printf.c.o
cd /home/jhageman/cFS/cFS-GitHub/build/cpu1/osal && /usr/bin/gcc  -D_LINUX_OS_ -I/home/jhageman/cFS/cFS-GitHub/build/inc -I/home/jhageman/cFS/cFS-GitHub/build/cpu1/inc -I/home/jhageman/cFS/cFS-GitHub/osal/src/os/inc -I/home/jhageman/cFS/cFS-GitHub/osal/src/os/shared -I/home/jhageman/cFS/cFS-GitHub/osal/ut_assert/inc  -D_XOPEN_SOURCE=600    -D_LINUX_OS_ -g   -o CMakeFiles/osal.dir/src/os/shared/osapi-printf.c.o   -c /home/jhageman/cFS/cFS-GitHub/osal/src/os/shared/osapi-printf.c

Note the missing -Wall. My hunch is the flags are getting set in osal/CMakeLists.txt and reported:

# Set up the final set of C flags for the build.  This is the sum of what the toolchain needs,
# what the BSP/PSP has added and anything else that the user has asked for.
osal_assemble_compiler_flags(${OSAL_SELECTED_OSTYPE} ${OSAL_SELECTED_BSPTYPE})

# At a mimimum, also compile with -Wall to show extra warnings.  Only do this if nothing
# added it already (prevents adding this twice in case the User/BSP/PSP already specified it)
if (NOT CMAKE_C_FLAGS MATCHES "-Wall")
  set(CMAKE_C_FLAGS "-Wall ${CMAKE_C_FLAGS}")
endif(NOT CMAKE_C_FLAGS MATCHES "-Wall")

message(STATUS "OSAL Compile Definitions: ${CMAKE_C_FLAGS}")
message(STATUS "OSAL Link Libraries: ${OSAL_LINK_LIBS}")

and then exported to parent scope, but then in setting up the UNIT TEST SUPPORT LIBRARIES, the flags are getting reset to not include Wall.

I think there is more cleanup required beyond just the proposed modification.

[skliper]

Although that's OSAL, so likely a topic of a different issue.

[jphickey]

I've come to the conclusion that this would probably work a lot better if OSAL stopped setting CMAKE_C_FLAGS directly. Years ago there was some desire to make things work with very old versions of CMake that were shipped with RHEL 5, which I think was version 2.6.4. But we no longer test with anything nearly that old.

In newer versions CMake has added features to help manage the use of different compiler flags in different targets/directories and thereby avoid directly setting the CMAKE_C_FLAGS variable.

Unfortunately RHEL7, which is still in active use, provides only CMake v2.8.12. But even this version has most of the target-oriented commands that would help, as it is a lot newer than the RHEL5 version (target_compile_definitions, target_compile_options, etc).

[skliper]

Sounds great to me. Could you prioritize this update? It's currently complicating improvements in work for our CI.