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

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

Closed
2 tasks done
jphickey opened this issue Nov 30, 2022 · 3 comments · Fixed by #52
Closed
2 tasks done

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

jphickey opened this issue Nov 30, 2022 · 3 comments · Fixed by #52
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Nov 30, 2022

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

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

/* Global table variables for table pointers contained in LC_OperData */
LC_WDTEntry_t WDTable[LC_MAX_WATCHPOINTS];
LC_ADTEntry_t ADTable[LC_MAX_ACTIONPOINTS];
LC_WRTEntry_t WRTable[LC_MAX_WATCHPOINTS];
LC_ARTEntry_t ARTable[LC_MAX_ACTIONPOINTS];

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

skliper commented Nov 30, 2022

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 jphickey added the bug label Dec 1, 2022
@jphickey
Copy link
Contributor Author

jphickey commented Dec 1, 2022

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

jphickey commented Dec 1, 2022

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.

jphickey added a commit to jphickey/LC that referenced this issue Dec 1, 2022
Make the table objects in this header "extern" and instantiate them in
the C file instead.  This solves the duplicate symbol linker errors.
@jphickey jphickey self-assigned this Dec 1, 2022
dzbaker added a commit that referenced this issue Dec 1, 2022
Fix #51, externalize symbols in lc_test_utils.h
@dmknutsen dmknutsen added this to the Draco milestone Jan 18, 2023
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