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

Integration Candidate: 2020-11-10 #652

Merged
merged 7 commits into from
Nov 16, 2020
Merged

Integration Candidate: 2020-11-10 #652

merged 7 commits into from
Nov 16, 2020

Conversation

astrogeco
Copy link
Contributor

@astrogeco astrogeco commented Nov 10, 2020

Describe the contribution

Fix #637, static module unload fix
Fix #641, add flags to OS_ModuleLoad

Testing performed
Bundle CI on astrogeco: https://travis-ci.com/github/astrogeco/cFS/builds/201769124

Expected behavior changes

PR #638 - Ensure that the handle is not NULL before invoking dlclose(). In particular the handle will be NULL for static modules. Shutdown after CTRL+C occurs normally (no segfault).

PR #643 - Add a "flags" parameter to OS_ModuleLoad() to indicate the desired symbol visibility:

  • GLOBAL (0, the default, and matches current behavior)
  • LOCAL which hides from other modules and prevents other modules from binding to symbols in this module, thereby ensuring/preserving the ability to unload in the future

CFE should use LOCAL flag for apps, and GLOBAL flags for libraries.

System(s) tested on

Additional context
Part of nasa/cFS#156

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
@jphickey

jphickey and others added 4 commits November 4, 2020 10:20
Mark static modules with a different type, and check that type at unload.
Only call unload low level implementation if type is dynamic.
Add flags to indicate symbol visibility for OS_ModuleLoad().

By default (flags=0) symbols will have global visibility,
which should be the same as existing behavior.
This restores the original behavior when using OS_SymbolLookup on POSIX,
so it doesn't return OS_ERR_NOT_IMPLEMENTED.

This is required for transitional purposes.  Will submit a separate bug
about use platform-dependent logic here.
Fix #637, fix OS_ModuleUnload for static modules
@astrogeco
Copy link
Contributor Author

@jphickey Got a test failure
https://travis-ci.com/github/astrogeco/cFS/jobs/432694405

/home/travis/build/astrogeco/cFS/cfe/fsw/cfe-core/src/es/cfe_es_apps.c: In function ‘CFE_ES_LoadModule’:

/home/travis/build/astrogeco/cFS/cfe/fsw/cfe-core/src/es/cfe_es_apps.c:371:22: error: too few arguments to function ‘OS_ModuleLoad’

         StatusCode = OS_ModuleLoad ( &ModuleId,

                      ^~~~~~~~~~~~~

In file included from /home/travis/build/astrogeco/cFS/osal/src/os/inc/osapi.h:115:0,

                 from /home/travis/build/astrogeco/cFS/cfe/fsw/cfe-core/src/inc/cfe.h:44,

                 from /home/travis/build/astrogeco/cFS/cfe/fsw/cfe-core/src/inc/private/cfe_private.h:32,

                 from /home/travis/build/astrogeco/cFS/cfe/fsw/cfe-core/src/es/cfe_es_apps.c:39:

/home/travis/build/astrogeco/cFS/osal/src/os/inc/osapi-os-loader.h:198:7: note: declared here

 int32 OS_ModuleLoad(osal_id_t *module_id, const char *module_name, const char *filename, uint32 flags);

       ^~~~~~~~~~~~~

cfe-core/CMakeFiles/cfe-core.dir/build.make:75: recipe for target 'cfe-core/CMakeFiles/cfe-core.dir/src/es/cfe_es_apps.c.o' failed

make[7]: *** [cfe-core/CMakeFiles/cfe-core.dir/src/es/cfe_es_apps.c.o] Error 1

CMakeFiles/Makefile2:6951: recipe for target 'cfe-core/CMakeFiles/cfe-core.dir/all' failed

make[6]: *** [cfe-core/CMakeFiles/cfe-core.dir/all] Error 2

Makefile:140: recipe for target 'all' failed

make[5]: *** [all] Error 2

CMakeFiles/native_default_cpu1-all.dir/build.make:57: recipe for target 'CMakeFiles/native_default_cpu1-all' failed

make[4]: *** [CMakeFiles/native_default_cpu1-all] Error 2

CMakeFiles/Makefile2:361: recipe for target 'CMakeFiles/native_default_cpu1-all.dir/all' failed

make[3]: *** [CMakeFiles/native_default_cpu1-all.dir/all] Error 2

CMakeFiles/Makefile2:208: recipe for target 'CMakeFiles/mission-all.dir/rule' failed

make[2]: *** [CMakeFiles/mission-all.dir/rule] Error 2

Makefile:227: recipe for target 'mission-all' failed

make[1]: *** [mission-all] Error 2

Makefile:120: recipe for target 'all' failed

make: *** [all] Error 2

The command "make" exited with 2.

@jphickey
Copy link
Contributor

@astrogeco make sure the dependency is also pulled into the build: nasa/cFE#1000

@astrogeco
Copy link
Contributor Author

astrogeco commented Nov 11, 2020 via email

@astrogeco
Copy link
Contributor Author

@jphickey doxygen error up next

You must fix doxygen warnings for "usersguide" before submitting a pull request

/home/travis/build/astrogeco/cFS/osal/src/os/inc/osapi-os-loader.h:198: warning: The following parameters of OS_ModuleLoad(osal_id_t *module_id, const char *module_name, const char *filename, uint32 flags) are not documented:

  parameter 'flags'

/home/travis/build/astrogeco/cFS/osal/src/os/inc/osapi-os-loader.h:148: warning: argument 'symbol_address' of command @param is not found in the argument list of OS_ModuleSymbolLookup(osal_id_t module_id, cpuaddr *SymbolAddress, const char *SymbolName)

/home/travis/build/astrogeco/cFS/osal/src/os/inc/osapi-os-loader.h:148: warning: argument 'symbol_name' of command @param is not found in the argument list of OS_ModuleSymbolLookup(osal_id_t module_id, cpuaddr *SymbolAddress, const char *SymbolName)

/home/travis/build/astrogec

@jphickey jphickey force-pushed the integration-candidate branch from b290347 to 8a9e736 Compare November 12, 2020 22:25
Add doxygen comment for flags parameter on OS_ModuleLoad

Also correct the capitalization of parameters to the
OS_ModuleSymbolLookup which caused them to be
reported as undocumented.
@jphickey
Copy link
Contributor

Doxygen issue is fixed on commit 3f5e8f3

(the force earlier was to just to revert an incomplete fix that I pushed too soon - its all good now).

@astrogeco astrogeco marked this pull request as ready for review November 13, 2020 20:46
@astrogeco astrogeco merged commit 7c1b59a into main Nov 16, 2020
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
jphickey added a commit to jphickey/osal that referenced this pull request Aug 10, 2022
Rather than CFE generating an osconfig.h file directly, it just
needs to select file(s) from the defs directory and pass them
to OSAL for the config.
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants