Fix #641, add OS_ModuleLoad flags

[jphickey]

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.

[jphickey]

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]

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

[ghost]

Looks good.. Nothing needed in the RTEMS implementation ?

[jphickey]

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.

[skliper]

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

[astrogeco]

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]

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]

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?

[skliper]

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]

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]

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]

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]

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.