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.

Sign up for GitHub

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

Load global symbols #30

Closed
wants to merge 7 commits into from
Closed

Load global symbols #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3fa65b7
Load global symbols
ahcorde Nov 2, 2020
ba7dfa4
make linters happy
ahcorde Nov 2, 2020
fa3fc44
Fix some issues
ahcorde Nov 3, 2020
f0baa76
Fixed tests
ahcorde Nov 3, 2020
a8160b2
Defined RTLD_NONDELETE if not using glibc-2.2
ahcorde Nov 4, 2020
dd19257
Merge remote-tracking branch 'origin/ign-plugin1' into ahcorde/global…
ahcorde Feb 9, 2021
7ff136d
Merge branch 'main' into ahcorde/global/symbols
chapulina Mar 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,39 @@ of libraries designed to rapidly develop robot applications.
## Installation

See the [installation tutorial](https://ignitionrobotics.org/api/plugin/1.1/installation.html).

## Test

Run tests as follows:

make test

> Tests are automatically built. To disable them, run `cmake` as follows:

cmake .. -DBUILD_TESTING=false

### Test coverage

To run test coverage:

1. Install LCOV

sudo apt-get install lcov

1. Build with coverage

cd build/
cmake .. -DCMAKE_BUILD_TYPE=coverage
make

1. Run tests

make test

1. Generate coverage

make coverage

1. View results

firefox coverage/index.html
30 changes: 20 additions & 10 deletions loader/src/Loader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ namespace ignition
#endif

void *dlHandle = dlopen(_pathToLibrary.c_str(),
RTLD_NOLOAD | RTLD_LAZY | RTLD_LOCAL);
RTLD_NOLOAD | RTLD_LAZY | RTLD_GLOBAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think, @mxgrey , are we trying to do something that ign-plugin wasn't meant to do, or is this a reasonable change?

See gazebosim/gz-sensors#38 (comment) for the reason why this change is needed.


if (!dlHandle)
return false;
Expand Down Expand Up @@ -452,9 +452,18 @@ namespace ignition
// state gets cleared each time it is called.
dlerror();

// NOTE: We open using RTLD_LOCAL instead of RTLD_GLOBAL to prevent the
// symbols of different libraries from writing over each other.
void *dlHandle = dlopen(_full_path.c_str(), RTLD_LAZY | RTLD_LOCAL);
#ifndef RTLD_NODELETE
// This macro is not part of the POSIX standard, and is a custom addition to
// glibc-2.2, so we need create a no-op stand-in flag for it if we are not
// using glibc-2.2.
#define RTLD_NODELETE 0
#endif

// NOTE: We are using RTLD_GLOBAL, this may overwrite the symbols of
// different libraries.
void * dlHandle = dlopen(
_full_path.c_str(),
RTLD_LAZY | RTLD_GLOBAL | RTLD_NODELETE);

const char *loadError = dlerror();
if (nullptr == dlHandle || nullptr != loadError)
Expand Down Expand Up @@ -536,12 +545,6 @@ namespace ignition
const std::string& _pathToLibrary) const
{
std::vector<Info> loadedPlugins;

// This function should never be called with a nullptr _dlHandle
assert(_dlHandle &&
"Bug in code: Loader::Implementation::LoadPlugins was called with "
"a nullptr value for _dlHandle.");

const std::string infoSymbol = "IgnitionPluginHook";
void *infoFuncPtr = dlsym(_dlHandle.get(), infoSymbol.c_str());

Expand Down Expand Up @@ -636,6 +639,13 @@ namespace ignition
loadedPlugins.push_back(info.second);
}

if (loadedPlugins.size() == 0)
{
if (_dlHandle != nullptr) {
return LoadPlugins(nullptr, _pathToLibrary);
}
}

return loadedPlugins;
}

Expand Down
4 changes: 0 additions & 4 deletions test/integration/EnablePluginFromThis_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ TEST(EnablePluginFromThis, LibraryManagement)
{
ignition::plugin::PluginPtr longterm;

CHECK_FOR_LIBRARY(libraryPath, false);

{
ignition::plugin::Loader pl;
pl.LoadLib(libraryPath);
Expand All @@ -136,8 +134,6 @@ TEST(EnablePluginFromThis, LibraryManagement)
}

EXPECT_TRUE(weak.IsExpired());

CHECK_FOR_LIBRARY(libraryPath, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these checks are here to make sure the library is unloaded correctly. But @mxgrey's comment on the function suggests that it may not be reliable, so that could explain why you had to remove these. What do you think @mxgrey , should we remove the false checks, or adjust the CHECK_FOR_LIBRARY function somehow?

https://github.com/ignitionrobotics/ign-plugin/blob/07c20cbc61007bf0982583b3eecb7761ca5cf7c9/test/integration/utils.hh#L32-L42

}

/////////////////////////////////////////////////
Expand Down
2 changes: 0 additions & 2 deletions test/integration/WeakPluginPtr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ TEST(WeakPluginPtr, Lifecycle)
CHECK_FOR_LIBRARY(libraryPath, true);
}

CHECK_FOR_LIBRARY(libraryPath, false);

EXPECT_TRUE(weak.IsExpired());

ignition::plugin::PluginPtr empty = weak.Lock();
Expand Down
9 changes: 0 additions & 9 deletions test/integration/factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ TEST(Factory, LibraryManagement)
CHECK_FOR_LIBRARY(libraryPath, true);
}

CHECK_FOR_LIBRARY(libraryPath, false);

// Test that we can release from a ProductPtr, and the library will still
// remain loaded, and then it will unload correctly later, as long as we
// manually destruct with a ProductDeleter
Expand All @@ -192,12 +190,8 @@ TEST(Factory, LibraryManagement)
CHECK_FOR_LIBRARY(libraryPath, true);

ignition::plugin::ProductDeleter<SomeObject>()(obj);

CHECK_FOR_LIBRARY(libraryPath, false);
}

CHECK_FOR_LIBRARY(libraryPath, false);

// Test that if we release from a ProductPtr but do not delete it with a
// ProductDeleter, then its shared library will remain loaded until we call
// CleanupLostProducts().
Expand Down Expand Up @@ -238,9 +232,6 @@ TEST(Factory, LibraryManagement)

// Now there should be no more lost products, so this count is 0.
EXPECT_EQ(0u, ignition::plugin::LostProductCount());

// With the reference counts deleted, the library should automatically unload.
CHECK_FOR_LIBRARY(libraryPath, false);
}

/////////////////////////////////////////////////
Expand Down
55 changes: 19 additions & 36 deletions test/integration/plugin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,26 +31,7 @@
#include "../plugins/DummyPlugins.hh"
#include "utils.hh"

/////////////////////////////////////////////////
TEST(Loader, LoadBadPlugins)
{
std::vector<std::string> libraries = {
IGNBadPluginAPIVersionOld_LIB,
IGNBadPluginAPIVersionNew_LIB,
IGNBadPluginAlign_LIB,
IGNBadPluginNoInfo_LIB,
IGNBadPluginSize_LIB};
for (auto const & library : libraries)
{
ignition::plugin::Loader pl;

// Make sure the expected plugins were loaded.
std::unordered_set<std::string> pluginNames = pl.LoadLib(library);
EXPECT_TRUE(pluginNames.empty());
}
}

/////////////////////////////////////////////////
////////////////////////////////////////////////
TEST(Loader, LoadExistingLibrary)
{
ignition::plugin::Loader pl;
Expand Down Expand Up @@ -171,7 +152,6 @@ TEST(Loader, LoadExistingLibrary)
EXPECT_EQ(intBase->MyIntegerValueIs(), object.dummyInt);
}


/////////////////////////////////////////////////
class SomeInterface { };

Expand Down Expand Up @@ -502,12 +482,9 @@ TEST(PluginPtr, LibraryManagement)
CHECK_FOR_LIBRARY(path, true);
}

CHECK_FOR_LIBRARY(path, false);

// Test that we can transfer between plugins
{
ignition::plugin::PluginPtr somePlugin;
CHECK_FOR_LIBRARY(path, false);

{
ignition::plugin::PluginPtr temporaryPlugin = GetSomePlugin(path);
Expand All @@ -518,8 +495,6 @@ TEST(PluginPtr, LibraryManagement)
CHECK_FOR_LIBRARY(path, true);
}

CHECK_FOR_LIBRARY(path, false);

// Test that we can forget libraries
{
ignition::plugin::Loader pl;
Expand All @@ -528,8 +503,6 @@ TEST(PluginPtr, LibraryManagement)
CHECK_FOR_LIBRARY(path, true);

EXPECT_TRUE(pl.ForgetLibrary(path));

CHECK_FOR_LIBRARY(path, false);
}

// Test that we can forget libraries, but the library will remain loaded if
Expand All @@ -551,8 +524,6 @@ TEST(PluginPtr, LibraryManagement)
}

// Check that the library will be unloaded once the plugin instance is deleted
CHECK_FOR_LIBRARY(path, false);

// Check that we can unload libraries based on plugin name
{
ignition::plugin::Loader pl;
Expand All @@ -561,16 +532,13 @@ TEST(PluginPtr, LibraryManagement)
CHECK_FOR_LIBRARY(path, true);

pl.ForgetLibraryOfPlugin("test::util::DummyMultiPlugin");

CHECK_FOR_LIBRARY(path, false);
}

// Check that the std::shared_ptrs that we provide for interfaces will
// successfully keep the library loaded.
{
std::shared_ptr<test::util::DummyNameBase> interface;

CHECK_FOR_LIBRARY(path, false);
{
interface = GetSomePlugin(path)->QueryInterfaceSharedPtr<
test::util::DummyNameBase>();
Expand All @@ -584,8 +552,6 @@ TEST(PluginPtr, LibraryManagement)
CHECK_FOR_LIBRARY(path, true);
}

CHECK_FOR_LIBRARY(path, false);

// Check that mulitple Loaders can work side-by-side
{
ignition::plugin::Loader pl1;
Expand All @@ -602,8 +568,25 @@ TEST(PluginPtr, LibraryManagement)

CHECK_FOR_LIBRARY(path, true);
}
}

/////////////////////////////////////////////////
TEST(Loader, LoadBadPlugins)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a reason to move this test down here? Was it failing above?

{
std::vector<std::string> libraries = {
IGNBadPluginAPIVersionOld_LIB,
IGNBadPluginAPIVersionNew_LIB,
IGNBadPluginAlign_LIB,
IGNBadPluginNoInfo_LIB,
IGNBadPluginSize_LIB};
for (auto const & library : libraries)
{
ignition::plugin::Loader pl;

CHECK_FOR_LIBRARY(path, false);
// Make sure the expected plugins were loaded.
std::unordered_set<std::string> pluginNames = pl.LoadLib(library);
EXPECT_TRUE(pluginNames.empty());
}
}

/////////////////////////////////////////////////
Expand Down