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

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

Closed
jphickey opened this issue Sep 12, 2019 · 9 comments

Comments

@jphickey
Copy link
Contributor

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
Copy link
Contributor

skliper commented Sep 12, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

skliper commented Sep 12, 2019

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
Copy link
Contributor

skliper commented Sep 13, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

skliper commented Sep 13, 2019

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
Copy link
Contributor

skliper commented Sep 13, 2019

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

@jphickey
Copy link
Contributor Author

jphickey commented Nov 5, 2019

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
Copy link
Contributor

skliper commented Nov 5, 2019

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

skliper added a commit to skliper/cFE that referenced this issue Jan 7, 2020
Adds STRICT_NO_WARNINGS and OMIT_DEPRECATED
prep options for CI and as example build
skliper added a commit to skliper/cFE that referenced this issue Jan 8, 2020
Adds STRICT_NO_WARNINGS and OMIT_DEPRECATED
prep options for CI and as example build
jphickey added a commit to jphickey/cFE that referenced this issue Jan 8, 2020
Add extra compile options for mission scope and arch scope.

These are separated to support cross compile environments that
do not/cannot use the same flags on both builds.

For "mission" build the targets are never cross compiled, only
built for the native host machine.  It should be safe to assume
a compiler in the GCC family and the strict warnings should
_always_ be applicable here.

For "arch" build the options are compiler vendor dependent.  The
file as-supplied can only be used if all the target cross compilers
are in the same family and support the same warning options.
However, this file can be modified without affecting the options
used for the host side tools.
jphickey added a commit to jphickey/cFE that referenced this issue Jan 9, 2020
Add a _custom suffix to differentiate the customization
file from the base file in the cmake directory.
jphickey added a commit to jphickey/cFE that referenced this issue Jan 15, 2020
Include in the basic warning set.  Note that at this time the CFE
does not build cleanly on all architectures with this warning enabled,
pending resolution of issue nasa#313.
skliper added a commit that referenced this issue Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants