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

Fix #641, add OS_ModuleLoad flags #643

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Nov 4, 2020

Describe the contribution
Add a "flags" parameter to OS_ModuleLoad() and use this to indicate the desired symbol visibility - either GLOBAL (0, the default, and matches current behavior) or LOCAL, which means symbols are hidden from other modules.

This is intended as a "hint" to OSAL as to how the module will be used - LOCAL 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, which are typically not ever unloaded.

Fixes #641

Testing performed
Build and run unit test
Sanity check CFE (update/patch required update to CFE to add flags parameter on API call)

Expected behavior changes
No impact to behavior if flags are passed as 0.

System(s) tested on
Ubuntu 20.04

Additional context
Related to nasa/cFE#952

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

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.
@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 4, 2020
@jphickey
Copy link
Contributor Author

jphickey commented Nov 4, 2020

This can be reviewed if there is time.... mainly looking for feedback regarding addition of the flags parameter.

This changeset to OSAL is fully functional (contains all unit test updates, all passing, etc) but if accepted it will need a corresponding patch to CFE to pass in a value for flags.

@astrogeco astrogeco added CCB-20201104 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Nov 4, 2020
@astrogeco astrogeco requested review from skliper and a user November 4, 2020 17:18
@astrogeco
Copy link
Contributor

CCB 2020-11-04 APPROVED

  • BREAKING CHANGE,
    • though there shouldn't be many apps using this.
    • this will also break apps that are loading things incorrectly
  • VxWorks doesn't have a way to do the local module symbol table lookup because of the flat namespace
  • PIPE DREAM: If we run cFS and apps within a single RTP then it would be possible to do the local module lookup in VxWorks

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

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.. Nothing needed in the RTEMS implementation ?

@jphickey
Copy link
Contributor Author

jphickey commented Nov 4, 2020

Looks good.. Nothing needed in the RTEMS implementation ?

Correct! AFAIK RTEMS has a flat namespace, like VxWorks - so no change is needed for its "dlopen()" call. It will ignore the flag.
For symbol lookups it uses the os-impl-posix-dl-symtab.c implementation so it is covered that way.

I built and sanity checked my RTEMS 4.11.3, seems to boot and run fine, so long as you've also merged nasa/cFE#1000.

Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a functional test? If not, should add to go with the new APIs. Could be done as a separate issue.

@astrogeco
Copy link
Contributor

Is there a functional test? If not, should add to go with the new APIs. Could be done as a separate issue.

@jphickey will you add these to this PR? If not can you open a new PR for them?

@jphickey
Copy link
Contributor Author

Is there a functional test? If not, should add to go with the new APIs. Could be done as a separate issue.

@jphickey will you add these to this PR? If not can you open a new PR for them?

I don't think we need to add anything more? The current functional tests (in unit-tests) was updated to invoke the function with flags set both ways, so it is part of the functional test. This also calls the new function (OS_ModuleSymbolLookup) - so its all in there.

But we cannot verify symbol visibility because it won't necessarily be consistent across platforms (i.e. the underlying OS may or may not actually implement the capability). So as a result the only thing we can actually vaildate in functional test remains the current operation.

@astrogeco
Copy link
Contributor

But we cannot verify symbol visibility because it won't necessarily be consistent across platforms (i.e. the underlying OS may or may not actually implement the capability). So as a result the only thing we can actually vaildate in functional test remains the current operation.

Sounds good to me, I'll merge this then. @skliper do we need to document this testing limitation anywhere else other than in this thread?

@astrogeco astrogeco changed the base branch from main to integration-candidate November 10, 2020 22:01
@astrogeco astrogeco merged commit 8a9e736 into nasa:integration-candidate Nov 10, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Nov 10, 2020
@skliper
Copy link
Contributor

skliper commented Nov 12, 2020

The functional test should report if the underlying OS supports the functionality or not (basically check the result, if it works report as such and if it doesn't report that it's not supported). It's not a "Failure" for it not to be supported, but should report.

@skliper
Copy link
Contributor

skliper commented Nov 12, 2020

Could also make it one of the tests that require analysis to fully pass... worth a check in the end to confirm it's the expected result. It would be an issue if it's expected to work and doesn't.

@jphickey
Copy link
Contributor Author

The functional test should report if the underlying OS supports the functionality or not (basically check the result, if it works report as such and if it doesn't report that it's not supported). It's not a "Failure" for it not to be supported, but should report.

This flag is really just a hint to the OSAL about how the app plans to use the module being loaded. The OSAL may or may not actually do anything differently. Since functional tests only use public APIs, there is no way for a functional tests to determine if its supposed to have an effect or not.

The expected pattern is:

  • When using OS_ModuleLoad() flags=GLOBAL -> app then looks up symbols with OS_SymbolLookup()
  • When using OS_ModuleLoad() flags=LOCAL -> app then looks up symbols with OS_ModuleSymbolLookup()

And this is the pattern used/implemented by the functional tests now, and as such it is confirmed to work correctly.

But the issue is that an app doing this "wrongly" i.e. using LOCAL load flags with OS_SymbolLookup() or GLOBAL load flags with OS_ModuleSymbolLookup() is not guaranteed to produce an OSAL error that can be checked for in a test. It might trigger an error, if the OS has a local symbol namespace and can do this. But for VxWorks and RTEMS it will likely still work OK. That would be a bug in the app though, not in the OSAL.

So while we could add an OSAL test to call the "wrong" combinations of these functions - we'd have to pass the test either way - or at least it will be an MIR result regardless of what OSAL returned. So I don't see much value in doing this.

@skliper
Copy link
Contributor

skliper commented Nov 12, 2020

I misinterpreted "But we cannot verify symbol visibility..." to mean it isn't being checked functionally for the correct behavior. As long as the functional test is confirming the correct behavior for the correct combination I'm good with it (no need for testing incorrect combination).

@jphickey
Copy link
Contributor Author

I misinterpreted "But we cannot verify symbol visibility..." to mean it isn't being checked functionally for the correct behavior. As long as the functional test is confirming the correct behavior for the correct combination I'm good with it (no need for testing incorrect combination).

Right - by that statement I only meant that if a module is loaded w/LOCAL - we cannot verify that its symbols are not also visible via the global lookup function. They might be, and its not a failure if they are.

jphickey added a commit to nasa/cFS that referenced this pull request Nov 12, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Nov 16, 2020
* Add nasa/cFE#984

* Add nasa/cFE#980

* Add nasa/cFE#867

* Add nasa/osal#638 and update cfe due to rebase

* Add nasa/cFE#987

* Add nasa/to_lab#64 and nasa/sample_app#104

* Add nasa/osal#643

* Add nasa/cFE#1000

* Add nasa/ci_lab#58

* Add doxygen fixes for nasa/osal#643

* Add nasa/cFE#1013

* Add nasa/cFE#1011

* Add nasa/ci_lab#61

* Add nasa/sample_app#109

* Bump versions and point to submodules main

Co-authored-by: Joseph Hickey <joseph.p.hickey@nasa.gov>
@jphickey jphickey deleted the fix-641-moduleload-flags branch December 3, 2020 17:26
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OS_ModuleLoad flags to indicate symbol table visibility
3 participants