nasa / LC

The Core Flight System (cFS) Limit Checker (LC) application.
Apache License 2.0
30 stars 21 forks source link

Items instantiated in header causes duplicate definitions and link errors #51

Closed jphickey closed 1 year ago

jphickey commented 1 year ago

Checklist (Please check before submitting)

Describe the bug The unit test header file lc_test_utils.h instantiates objects directly in the header file, which breaks if it is ever included in more than one C file.

To Reproduce Build LC with unit tests enabled, get lots of linker errors:

usr/bin/ld: libcoverage-lc_internal-stubs.a(lc_test_utils.c.o):/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:39: multiple definition of `WDTable'; CMakeFiles/coverage-lc-lc_action-testrunner.dir/lc_action_tests.c.o:/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:39: first defined here
/usr/bin/ld: libcoverage-lc_internal-stubs.a(lc_test_utils.c.o):/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:40: multiple definition of `ADTable'; CMakeFiles/coverage-lc-lc_action-testrunner.dir/lc_action_tests.c.o:/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:40: first defined here

Expected behavior Build should work?

Code snips https://github.com/nasa/LC/blob/543cf6b8906cd976d33f14d3907567d6bc8c8cd9/unit-test/utilities/lc_test_utils.h#L38-L43

System observed on: Ubuntu

Additional context I was just trying to build LC "out of the box" - not modified in any way - and it failed badly. Not sure how this ever built or passed any validation testing with this the way it was.

Reporter Info Joseph Hickey, Vantage Systems, Inc.

skliper commented 1 year ago

Strange, CI has been passing for the build/run of the tests and I periodically build/run all the draco updated apps... I also just updated to main everywhere and build all the tests with no issues. Ubuntu 20.04. Ideas on what might be different?

EDIT - it's possible I'm behind on my *_defs... EDIT1 - just copied the latest sample_defs, added the 10 apps to the build and build still worked fine EDIT2 - Looks like it's a GCC 10+ thing... where it now defaults to -fno-common

Note I added the flag locally and the 9 other Draco updated apps compile, so it's just an LC issue. Ubuntu 20.04 is gcc 9.4.0, hopefully the upgrade to 22.04 can be soon.

jphickey commented 1 year ago

Yes, I investigated this a bit and found that it GCC 9.x (w/binutils 2.34) put this symbol by default into a common section, and thus does not trigger the error because it silently merges the duplicate symbols into one. But if you add -fno-common to the same build it will fail.

The newer gcc/binutils seem to have made the -fno-common the default (Which I agree with, I've been tripped up by the unexpected common-ness of global symbols, particularly for CFE/CFS - if more than one app/library by some chance defines the same global symbol name, it will share the single instance between them, corrupting eachother). We also had to add -fno-common to the RTEMS build at one point because it broke something else without it.

Anyway - I think this adequately explains why it didn't fail validation with the way it was, but it needs to be corrected because its very wrong to instantiate objects in a header.

jphickey commented 1 year ago

It does raise the point though - we should probably consider addding -fno-common to the arch_build_custom.cmake for all CI workflows. We shouldn't be relying on common sections in dynamic modules, and their presence can mask real issues and cause others.

FWIW, I don't think VxWorks or RTEMS loaders support common sections at all, so if these tests were built on either of those platforms I'm fairly sure this would fail there too.