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

Linux/MacOSX port of DriverLoader #10

Merged
merged 12 commits into from
Apr 25, 2016
Merged
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ target_link_libraries(ViveLoaderLib
PUBLIC
OpenVRDriver osvr::osvrUtil linkable_into_dll
PRIVATE
filesystem_lib JsonCpp::JsonCpp)
filesystem_lib JsonCpp::JsonCpp ${CMAKE_DL_LIBS}) # ${CMAKE_DL_LIBS} is set to empty string, when system doesn't provide dlfcn. For example, Windows.
target_include_directories(ViveLoaderLib PUBLIC ${CMAKE_CURRENT_BINARY_DIRECTORY} PRIVATE ${Boost_INCLUDE_DIRS})

# Build the plugin
Expand Down
4 changes: 3 additions & 1 deletion ChaperoneData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <iostream>
#include <map>
#include <sstream>
#include <algorithm>

#ifdef _WIN32
#define WIN32_LEAN_AND_MEAN
Expand Down Expand Up @@ -100,6 +101,7 @@ getFile(std::string const &fn,
return os.str();
}
#else
#include <cstring> // strerror

std::string
getFile(std::string const &fn,
Expand All @@ -119,7 +121,7 @@ getFile(std::string const &fn,
}
std::ostringstream os;
std::string temp;
while (std::getline(os, temp)) {
while (std::getline(s, temp)) {
os << temp;
}
return os.str();
Expand Down
5 changes: 5 additions & 0 deletions DisplayDescriptor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

// Internal Includes
#include "DisplayDescriptor.h"
#include "osvr/Util/PlatformConfig.h"

// Library/third-party includes
#include <json/reader.h>
Expand All @@ -37,6 +38,10 @@
#include <cmath>
#include <iostream>

#if defined(OSVR_LINUX) || defined(OSVR_MACOSX)
#include <float.h> // FLT_EPSILON
#endif

namespace osvr {
namespace vive {

Expand Down
35 changes: 21 additions & 14 deletions DriverLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#define WIN32_LEAN_AND_MEAN
#define NO_MINMAX
#include <windows.h>
#elif defined(OSVR_LINUX) || defined(OSVR_MACOSX)
#include <dlfcn.h>
Copy link
Member

Choose a reason for hiding this comment

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

Is dlfcn.h how you open a .dylib on Mac OS X, or just how you open a .so? The terribly confusing part is that I know OS X has both of those... (@godbyk? @d235j?)

Update: It looks like it's correct for loading whatever CMAKE_SHARED_MODULE_SUFFIX is on Mac. Can't work worse than it does right now, so that's good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I load libraries the same way in my project. Works perfect on both OSes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth having the same code in separate OSVR_LINUX and OSVR_MACOSX branches or should they be conflated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a mistype.

#endif

namespace osvr {
Expand All @@ -48,10 +50,8 @@ namespace vive {
/// Platform-specific handle to dynamic library
#if defined(OSVR_WINDOWS)
HMODULE driver_ = nullptr;
#elif defined(OSVR_MACOSX)
#error "Implementation incomplete!"
#elif defined(OSVR_LINUX)
#error "Implementation incomplete!"
#elif defined(OSVR_MACOSX) || defined(OSVR_LINUX)
void *driver_ = nullptr;
#endif

/// Destructor: should contain platform-specific code to unload dynamic
Expand All @@ -61,10 +61,10 @@ namespace vive {
if (driver_) {
FreeLibrary(driver_);
}
#elif defined(OSVR_MACOSX)
#error "Implementation incomplete! Unload dynamic library here!"
#elif defined(OSVR_LINUX)
#error "Implementation incomplete! Unload dynamic library here!"
#elif defined(OSVR_MACOSX) || defined(OSVR_LINUX)
if (driver_) {
dlclose(driver_);
}
#endif
}
};
Expand All @@ -91,12 +91,19 @@ namespace vive {
throw CouldNotLoadEntryPoint();
}
factory_ = reinterpret_cast<DriverFactory>(proc);
#elif defined(OSVR_MACOSX)
#error \
"Implementation incomplete! Load dynamic library here and retrieve entry point function here!"
#elif defined(OSVR_LINUX)
#error \
"Implementation incomplete! Load dynamic library here and retrieve entry point function here!"
#elif defined(OSVR_LINUX) || defined(OSVR_MACOSX)
impl_->driver_ = dlopen(driverFile.c_str(), RTLD_NOW | RTLD_GLOBAL);
if (!impl_->driver_) {
reset();
throw CouldNotLoadDriverModule(dlerror());
}

auto proc = dlsym(impl_->driver_, ENTRY_POINT_FUNCTION_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to call dlerror() first here unconditionally? I'm looking at some of the code that loads OSVR plugins and it looks like that's what I do https://github.com/OSVR/libfunctionality/blob/master/src/libfunctionality/LoadPluginLibdl.h#L62

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dlerror() returns NULL when is no error. But in that case, this exception will be never thrown.

Also, calling dlerror() without taking returned pointer to string -- is really bad idea.
POSIX's libdl gives really nice reports through dlerror(). For example, in case loading 64 bit library in 32 bit app gives "Wrong elf class ELFCLASS64". So user can know where's the problem. And developer, ofc. :)

if (!proc) {
reset();
throw CouldNotLoadEntryPoint(dlerror());
}
factory_ = reinterpret_cast<DriverFactory>(proc); \
Copy link

@phiresky phiresky Apr 20, 2016

Choose a reason for hiding this comment

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

Why is there a backslash at the end of this line (line 108)? Seems to break the build for me

#endif
}

Expand Down
19 changes: 16 additions & 3 deletions DriverLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
#define INCLUDED_DriverLoader_h_GUID_882F2FD5_F218_42BE_3088_31CF712EC455

// Internal Includes
// - none
#include <osvr/Util/PlatformConfig.h>
#include <InterfaceTraits.h>

// Library/third-party includes
// - none
Expand All @@ -40,14 +41,26 @@
namespace osvr {
namespace vive {
struct CouldNotLoadDriverModule : std::runtime_error {
Copy link
Member

Choose a reason for hiding this comment

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

I actually meant leave both constructors - a default constructor and one that takes an extra error message, with no platform ifdefs in this header at all...

CouldNotLoadDriverModule()
CouldNotLoadDriverModule(const char *errString = nullptr)
#if defined(OSVR_LINUX) || defined(OSVR_MACOSX)
Copy link
Member

Choose a reason for hiding this comment

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

To have these defines, you need to include <osvr/Util/PlatformDefines.h> - but I'd just leave those constructors available on all platforms. Just because only mac and linux use them doesn't mean having them will hurt the Windows build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, is Windows can give a error string.
But yes, there is can be a checking for NULL pointer. :)

: std::runtime_error(
"Could not load driver module: " +
std::string(errString)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably add a space between the hard-coded C-string and the appended error string.

#else
: std::runtime_error("Could not load driver module.") {}
#endif
};

struct CouldNotLoadEntryPoint : std::runtime_error {
CouldNotLoadEntryPoint()
CouldNotLoadEntryPoint(const char *errString = nullptr)
#if defined(OSVR_LINUX) || defined(OSVR_MACOSX)
: std::runtime_error(
"Could not load entry point function from driver: " +
std::string(errString)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space here, too.

#else
: std::runtime_error(
"Could not load entry point function from driver.") {}
#endif
};

struct CouldNotGetInterface : std::runtime_error {
Expand Down
1 change: 1 addition & 0 deletions FindDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include <shlobj.h>
#else
#include <cstdlib> // for getenv
#include <fstream> // std::ifstream
#endif

#undef VIVELOADER_VERBOSE
Expand Down
2 changes: 1 addition & 1 deletion SearchPathExtender.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ namespace vive {
#ifdef _MSC_VER
_putenv_s(SEARCH_PATH_ENV, val.c_str());
#else // not microsoft runtime specific
auto newValue = SEARCH_PATH_ENV + "=" + val;
auto newValue = SEARCH_PATH_ENV + std::string("=") + val;
// Have to allocate new string because it becomes part of the
// environment.
char *newString = static_cast<char *>(malloc(newValue.size() + 1));
Expand Down