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

Add OS_ModuleLoad flags to indicate symbol table visibility #641

Closed
jphickey opened this issue Nov 3, 2020 · 2 comments · Fixed by #643 or #652
Closed

Add OS_ModuleLoad flags to indicate symbol table visibility #641

jphickey opened this issue Nov 3, 2020 · 2 comments · Fixed by #643 or #652
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Nov 3, 2020

Is your feature request related to a problem? Please describe.
On POSIX platforms with a real dynamic loader/shared object implementation, using the RTLD_GLOBAL flag to dlopen() can make it tricky or impossible to unload modules later. This flag makes the symbols globally available to satisfy other relocations, and unloading of the module will be deferred or prevented entirely as long as the runtime loader thinks a symbol is being used.

This ultimately causes a requirement failure on this platform as documented in nasa/cFE#952 - because the module isn't actually unloaded when dlclose() is called, and even though the new/replacement app module was loaded, CFE will end up restarting the original code, not the new (reloaded) code.

Loading a module with RTLD_LOCAL instead seems to prevent this issue - because the symbols are simply not made available for other modules/entities to use. This ensures that when the time comes to unload the module, nothing else is referencing the module, and dlclose() actually does unload it.

But this LOCAL flag cannot be used for all modules, because libraries do need their symbols added to the global table, or else it will not be possible to load apps that depend on those libraries

Describe the solution you'd like
Add a "flags" parameter onto the existing OS_ModuleLoad() API, so it becomes:

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

The "flags" parameter can be used to indicate the symbol visibility. A flag value of 0 should map to "global" - which is what the current implementation does - to make an easy transition for existing code.

In order to be able to look up an entry point in a module loaded with this option, this necessitates another new API:

int32 OS_ModuleSymbolLookup(osal_id_t module_id, cpuaddr *SymbolAddress, const char *SymbolName)

Which is the same as OS_SymbolLookup() but accepts a module ID value and operates on that module, rather than on the global scope. This should be the ID that was returned from the OS_ModuleLoad call.

For RTOS implementations that do not have this symbol visibility option they can ignore the flag, continue to map everything into the global symbol table as they currently do, and OS_ModuleSymbolLookup and OS_SymbolLookup become equivalent.

Additional context
Note that most all other OSAL "create" functions (tasks, queues, semaphores, etc) already have a "flags" parameter on the API, reserved for future use. Unfortunately, this flags parameter was not part of the original OS_ModuleLoad() API definition, making it an exception to the pattern. So by adding this, although it is a breaking change, it makes it more consistent with the rest of the APIs.

The alternative would be to define a separate OS_ModuleLoadWithFlags() API, but this pattern does not exist anywhere else, so it would continue to be an exception with respect to the overall OSAL API.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey self-assigned this Nov 3, 2020
jphickey added a commit to jphickey/osal that referenced this issue Nov 4, 2020
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 added a commit to jphickey/osal that referenced this issue Nov 4, 2020
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 added a commit to jphickey/osal that referenced this issue Nov 4, 2020
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.
@astrogeco
Copy link
Contributor

CCB 2020-11-04

  • Cannot determine why reference count is zero, dlclose is not unloading the module in POSIX
  • Unload issue shouldn't be a problem for libraries
  • Fundamental issue is "how to tell OSAL" about this

@ghost
Copy link

ghost commented Nov 4, 2020

I agree that this is the best path.. Break it on this major release. I don't know of any apps that directly load modules.. I thought SBN did, but it might load libraries through the cFE.

The only alternative I could think of is to keep the API as-is and modify it to load local modules. Then add a "Load Global Module" or "Load Shared Module" API. But that would probably confuse users, especially on an RTOS.
Since this is most likely only used by the cFE, the flags parameter solution is probably best.

jphickey added a commit to jphickey/osal that referenced this issue Nov 4, 2020
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.
astrogeco added a commit that referenced this issue Nov 10, 2020
jphickey added a commit that referenced this issue Nov 12, 2020
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.
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
jphickey added a commit to jphickey/osal that referenced this issue Aug 10, 2022
Ensure clean build, no warnings on string operations using GCC 9.3.0.

Some string ops were genuinely incorrect (particularly in UT) but some
were perfectly OK and handled correctly per the C spec.  In particular
the new "rules" that GCC9 warns about make the strncat library function
(and some others) somewhat off-limits even if used correctly.
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
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