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

Some symbols not making it into final core executable file #96

Closed
skliper opened this issue Sep 30, 2019 · 19 comments
Closed

Some symbols not making it into final core executable file #96

skliper opened this issue Sep 30, 2019 · 19 comments
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

In the current "cmake"-based build scripts, the final cfe core executable file is missing symbols that are not directly referenced in other parts of CFE. A notable example of this is {{{CFE_TBL_Register}}}.

Because these are defined in a shared library, the linker is simply not including them as they do not serve to define any unresolved symbols from the linker's point of view.

The {{{-rdynamic}}} option is already being used, but it does not apply since the CFE core code is in a static library.

There are several approaches to this problem:

  1. Use the {{{MODULE}}} library type instead of making a static library for the CFE core. This way the {{{-rdynamic}}} export option will be applied to all the cfe code as it should be and therefore all functions included in the final link. This is the correct, officially supported solution but it requires cmake v2.8.8+ (released 2012). However RHEL still includes a very old version of cmake from 2009 with their distribution so going this route forces users of RHEL to go to other sources for a newer cmake.

  2. Use the {{{--whole-archive}}} linker option to force the linker to include all objects from the CFE core, PSP, and OSAL libraries during the link. This works, but the {{{--whole-archive}}} is specific to the GNU ld linker and is unlikely to be supported on any other linkers. However, it appears that all targets supported by CFE use the GNU ld linker so this may not be a problem.

  3. Create a "fake" function that calls all external functions, which causes them to be undefined and therefore included in the link. This does not change linker options or build scripts so should work with ANY linker and the old version of cmake, but it wastes some memory as this function is still loaded into memory, and it will require maintenance to keep it up to date.

@skliper skliper added this to the 6.5.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 65. Created by jphickey on 2015-06-08T16:08:14, last modified: 2019-03-05T14:57:55

@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-08 16:10:48:

Implementing option 2 above for now. This is easy to do and it should not break any currently supported target as they all use GNU gcc/ld as the compiler and linker.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-08 16:25:59:

Pushed commit [changeset:f444663] for review

Branch is "trac-65-cfecore-whole-archive-link".

This adds the {{{--whole-archive}}} and {{{--no-whole-archive}}} linker options around CFE core, PSP, and OSAL which solves the problem for now.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-08 17:12:25:

Wow, yes, I've hit this on other platforms in other contexts.

The traversal from bunch-of-sources to bunch-of-objects
to fewer-libraries to one-big-binary is inherently a platform
specific set of transitions.

Option 1 does sound like the right thing. Is there a way that
we can prefer to use this option during the build, and only
fall back to another option if we have to?

I prefer Option 2 over Option 3. As a side note, only dragging
required objects out of a .a library is the classic way to
do these -- it's only .so where you have to bag the whole thing.

The downside of #34 is not only the fragile collection of calls
to otherwise unused code, but the further assumption that when the
linker pulls a .o from a .a file to resolve a symbol, it always
pulls the whole .o file. True for most if not all known platforms
but not by any means guaranteed.

I'll watch my Bamboo logs to see if any of our agents are
running versions of CMake older than 2.8.8

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-08 17:19:03:

At least one Bamboo Agent will trip over this.

{{{
build 08-Jun-2015 13:57:34 Host: f6-bamboo-r2.ndc.nasa.gov GNU/Linux x86_64
build 08-Jun-2015 13:57:34 Kernel: Linux 2.6.18-404.el5 #32 SMP Sat Mar 7 04:14:13 EST 2015
...
build 08-Jun-2015 13:57:34 CMake: /usr/bin/cmake
build 08-Jun-2015 13:57:34 cmake version 2.8.2
}}}

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-08 17:19:57:

Also pushed commit [changeset:c71f372] which represents Option 3 above, as I had an idea on how this could be done relatively cleanly and not be overly "fragile" in terms of future maintenance.

This is slightly more complicated than Option 2 but it does not hack the link line therefore should be compatible with linkers other than GNU.

This generates a table of function pointers to the undefined symbols so the overhead is minimized, but it will end up still using sizeof(void (*)(void)) of memory for each entry in the table. On the flip side, this object could potentially even be stripped from the final exe to recover that memory. But in the current form this is only one entry so this is only 4 (or 8) bytes and probably not worth the effort yet.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-08 17:32:37:

Just got a "CMake" failure from one of the RHEL agents
trying to build my OSAL test branch. I'll set up test
branches with the two fixes and fire them up.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-08 17:33:59:

Whoa, wait a sec, this is CFE, and my issue is
being unable to CMake in OSAL. Related?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-09 09:42:30:

The following is a complete list of symbols that had been missing from the cfe core executable, but are included when building using the commits listed here. This is generated by running "diff" on the symbol table of {{{core-cpu1}}} before and after merging this fix.

{{{
CFE_PSP_EepromPowerDown
CFE_PSP_EepromPowerUp
CFE_PSP_EepromWrite16
CFE_PSP_EepromWrite32
CFE_PSP_EepromWrite8
CFE_PSP_EepromWriteDisable
CFE_PSP_EepromWriteEnable
CFE_PSP_InitSSR
CFE_PSP_PortRead16
CFE_PSP_PortRead32
CFE_PSP_PortRead8
CFE_PSP_PortWrite16
CFE_PSP_PortWrite32
CFE_PSP_PortWrite8
CFE_PSP_WatchdogDisable
CFE_PSP_WatchdogEnable
CFE_PSP_WatchdogGet
CFE_PSP_WatchdogInit
CFE_PSP_WatchdogService
CFE_PSP_WatchdogSet
CFE_TBL_DumpToBuffer
CFE_TBL_GetAddress
CFE_TBL_GetAddresses
CFE_TBL_GetInfo
CFE_TBL_GetStatus
CFE_TBL_Load
CFE_TBL_Manage
CFE_TBL_Modified
CFE_TBL_NotifyByMessage
CFE_TBL_Register
CFE_TBL_ReleaseAddress
CFE_TBL_ReleaseAddresses
CFE_TBL_Share
CFE_TBL_Unregister
CFE_TBL_Update
CFE_TBL_Validate
}}}

Option 2 performs a "catch-all" while option 3 gives more fine grained control over which symbols to include. In the current form option 3 will include the TBL functions but leave out the PSP ones.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-07-21 11:19:52:

Another possible downside to {{{--whole-archive}}} (option 2) is that it precludes one from supplying overrides to common code.

Example:
Trac #115 and PSP trac [cfs_psp:3] allow the mission to specify modular extensions that are statically linked into the final exe. This is a very useful feature and it also allows the mission to "override" some functionality that normally is part of the PSP "common" code, e.g. EEPROM access, direct physical memory access, etc.

If using the {{{--whole-archive}}} for the PSP code then this results in duplicate symbols for these objects. But normally, without this option, only the first implementation of these functions in the link line is used and this can override the common functions later in the link line to get target-specific behavior.

Might be worth considering only using {{{--whole-archive}}} for the CFE core and possibly OSAL.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2015-07-24 16:20:31:

The gsfc_build tool setvars script and makefiles are setup to allow a user to build the CFE without including Table Services. How do these options support this?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-07-27 12:34:20:

As an aside, -rdynamic turns out to be necessary when building any
program that wants dlsym() to be able to resolve symbols defined by
that program, when building for posix targets. The "classic" Bamboo
build-and-test script for OSAL had to be updated to force this flag.

It might be a good idea to aim at -rdynamic and provide instructions
on how to make it happen with older versions of CMake ...?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sduran on 2015-07-27 17:28:26:

It sounds like this is only a cmake issue. I have never seen it with the default Makefiles. I'd vote for option 1, maybe 2, not 3.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-07-27 17:46:51:

Replying to [comment:12 sduran]:

It sounds like this is only a cmake issue. I have never seen it with the default Makefiles. I'd vote for option 1, maybe 2, not 3.

That is mostly a true statement - as Greg pointed out there was also a case in the classic build where -rdynamic also had to be added in order for dlsym() to work as expected in the unit tests.

But yes, this is mainly a CMake issue....

Some background:
The GSFC makefiles compile everything to .o files and then do "partial links" (using ld -r) to create larger .o files that contain objects from more than one compilation unit.

These larger .o files from CFE, PSP, and OSAL go to the final link line to produce the executable. Therefore if -rdynamic is used then it applies to all of CFE, PSP, and OSAL code.

CMake is very similar except it does not do the partial links, but builds static libraries instead for the intermediate OSAL, CFE, and PSP builds. Hence the slightly different linking semantics and why unreferenced code was getting pruned out.

With this linker-pruning behavior caveat aside, the intermediate static library approach is arguably preferable since it does not rely on any toolchain-specific behavior i.e. the way static libraries behave are relatively consistent even on non-GNU toolchains. The partial link behavior (ld -r) that the GSFC makefiles use to get around this a GNU ld-specific option so that would likely also be an issue in a non-GNU toolchain.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-07-28 17:07:29:

CCB review updates:

[changeset:242c3ab] updates the --whole-archive solution (option 2) to better allow for toolchain differences in the exact name/syntax of this option. A new boolean also allows the mission to override/disable this feature for a given cpu (e.g. if only static linking is used, the default library pruning logic is probably preferable).

[changeset:27a667b] updates the export symbol solution (option 3) to be a general purpose tool that a mission could use to ensure that any arbitrary symbol gets linked into the final exe. The symbol list is made into a cpu-specific file that is sourced from the mission definition directory, so it is entirely controlled by the mission configuration. An empty/nonexistant file (default) will have no effect on the build or link. The file is called <prefix>_export_symbols.inc and the same concatenation logic is used here so it can be cpu or platform specific in nature.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-07-30 15:15:51:

[changeset:242c3ab trac-65-cfecore-whole-archive-link] is included in [changeset:6ea42ba ic-2015-07-28]

[changeset:655d94e trac-65-export-symbol-table] is included in [changeset:6ea42ba ic-2015-07-28]

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-09-08 12:58:14:

... somehow was not closed after the 07-28 integration was complete.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2016-02-16 13:16:45:

Susie confirmed these tickets have been approved for CFE 6.5

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-05 14:57:55:

Milestone renamed

@skliper skliper closed this as completed Sep 30, 2019
@skliper skliper removed their assignment Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant