C++ visibility support #1040

jslee02 · 2018-03-22T17:57:04Z

This PR implements the control of binary symbols visibility in DART inspired by flexible-collision-library/fcl#233.

DART_EXPORT preprocessor is added to any class, struct, and function that needs to be exposed publicly to third-party software. I could compile all the tests, examples, and tutorials without any issue, but it would be good to test DART with other third-party software like AIKIDO for sure.

DART_HIDE_ALL_SYMBOLS CMake option is also added, which is OFF by default, for the case that we want to hide all the symbols as DART is built as static library.

Caveat:
There is a preprocessor name conflict between the existing DART_DEPRECATED(version) and another preprocessor generated by generate_export_header(), which is necessary to mark visibility of deprecated APIs. To resolve this, I changed the name to DART_DEPRECATED2 so that the preprocessor for deprecated API becomes DART_DEPRECATED2_EXPORT. I'm open to better workaround.

Alternatively, we could remove DART_DEPRECATED(version) and use one generated by generate_export_header(). The version information can be provided by using Doxygen comment (e.g., /// \deprecated Deprecated in <version>. Please use <other_api> instead).).

This PR is related to #1005.

Before creating a pull request

Format new code files using clang-format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md
Add comment to the export preprocessors

mxgrey · 2018-03-22T18:18:46Z

This is a good start, but I have a few notes:

I changed the name to DART_DEPRECATED2 so that the preprocessor for deprecated API becomes DART_DEPRECATED2_EXPORT.

What I've done in the past is:

Put the auto-generated header into detail/export.h (e.g. dart/detail/export.h)
Name the auto-generated macros as DETAIL_<BASE>_<MACRO_NAME>
Manually write an export.h file (e.g. dart/export.h)
Have the manually-written export.h file use the macros that were defined in dart/detail/export.h, e.g.:

#include <dart/detail/export.h>

/* explain the macro here */
#define DART_EXPORT \
    DETAIL_DART_EXPORT

/* explain the macro here */
#define DART_DEPRECATED(v) \
    DETAIL_DART_DEPRECATED
...

This also gives us the opportunity to add documentation to each macro, which may be helpful for end users.

I would suggest we consider naming the export macro as DART_API instead of the default DART_EXPORT, since I think calling it API makes it more explicit about its purpose. We can do that by adding EXPORT_MACRO_NAME DART_API to the generate_export_headers command.

And a final note: We need to create a unique export header for each library in the project, which means each of the optional components that aren't part of libdart need their own unique export header with their own unique export macro name.

mxgrey · 2018-03-22T18:23:08Z

Revising my last comment: Since we would need a bunch of the "manually written" export.h with slight variations for each library, we can create an export.h.in and configure it for each of the components.

jslee02 · 2018-03-22T18:30:59Z

Having another layer of export header sounds good to me.

And a final note: We need to create a unique export header for each library in the project, which means each of the optional components that aren't part of libdart need their own unique export header with their own unique export macro name.

Could you let me know why do we need different configurations for each component?

mxgrey · 2018-03-22T18:37:19Z

The definition of the export macro will be different depending on whether the header is being included by the library that is defining the class or by a library that is using the class.

To illustrate how things would go wrong, let's say we use the same export macro for libdart-gui-osg as libdart. Then when we compile libdart-gui-osg and the compiler comes across the declaration of Skeleton in dart/dynamics/Skeleton.hpp, then the compiler will think that libdart-gui-osg wants to define and export the symbol of dart::dynamics::Skeleton. This will cause undefined reference and/or other linker errors, since libdart-gui-osg does not have access to the definition of dart::dynamics::Skeleton.

jslee02 · 2018-03-22T20:42:52Z

@mxgrey That makes sense to me. I have a follow-up question though.

Although gui-osg component uses different preprocessors (e.g., DART_GUI_OSG_API), I think the component still will try to export the symbol of dart::dynamics::Skeleton because Skeleton.hpp includes dart/export.hpp so that DART_API is defined to export the symbol. How could we prevent this situation? Should we redefine all the export preprocessor except the preprocessors of the component not to try to export symbols of other components?

mxgrey · 2018-03-22T21:00:04Z

I think the component still will try to export the symbol of dart::dynamics::Skeleton because Skeleton.hpp includes dart/export.hpp so that DART_API is defined to export the symbol.

This is not correct.

When you use the generate_export_header function from cmake, it will add a private compile definition (something along the lines of dart_EXPORTS) to the library for which it's generated (this is why the first argument of the function needs to be the library target for which you're generating the export header). That compile definition, which is only given to libdart during compilation, will make it so that the DART_API macro expands to __declspec(dllexport) (or whatever the platform-specific export attribute is).

When libdart-gui-osg is compiled, it will not be given the dart_EXPORTS compile definition, so the DART_API macro will expand to __declspec(dllimport) (or whatever the platform-specific import attribute is).

mxgrey · 2018-03-22T21:03:52Z

So the short version is, cmake takes care of your concern automatically.

jslee02 · 2018-03-22T23:06:42Z

@mxgrey Thanks for the explanation! I learned a lot while working on this PR.

I've addressed your feedback in the last commits.

test_Collision fails due to a reason that is not relevant to the changes of this PR. In a nutshell, the current implementation of Singleton doesn't work for multiple binary modules (related SO post). I'll handle this in a future PR.

mxgrey · 2018-03-23T16:14:27Z

cmake/DARTMacros.cmake

+  )
+
+  # Generate final export header
+  set(header_guard_name ${base_name_upper}_EXPORT_HPP_)


This would probably be much cleaner if we created a export.hpp.in and then called configure_file on it.

Agree. I wrote this lines because I thought each component requires individual export.hpp.in, but this is not true.

mxgrey · 2018-03-23T16:18:50Z

cmake/DARTMacros.cmake

+  generate_export_header(${target_name}
+    EXPORT_MACRO_NAME ${base_name_upper}_DETAIL_API
+    EXPORT_FILE_NAME detail/export.h
+    DEPRECATED_MACRO_NAME ${base_name_upper}_DETAIL_DEPRECATED


Nitpick: I like to put the DETAIL in front of DART so it gets excluded from autocomplete when users start typing DART....

That's true, but I thought we use DART_ prefix to avoid possible name conflicts. At the same time, I don't see the possibility of name confliction with DETAIL_DART_~.

Let me change it to DETAIL_DART_~ as I don't have a strong preference here.

jslee02 · 2018-04-29T12:37:38Z

@mxgrey I've fixed build and test errors. Could you do a second review?

mxgrey · 2018-04-30T17:17:02Z

dart/common/Deprecated.hpp

@@ -42,7 +43,7 @@
 //==============================================================================

 #if defined(__GNUC__) || defined(__clang__)
-  #define DART_DEPRECATED(version) __attribute__ ((deprecated))
+  #define DART_DEPRECATED(version) DETAIL_DART_DEPRECATED
  #define DART_FORCEINLINE __attribute__((always_inline))
 #elif defined(_MSC_VER)
  #define DART_DEPRECATED(version) __declspec(deprecated)


This should also be redirected to DETAIL_DART_DEPRECATED to be consistent with the other definition.

mxgrey

I believe the export macro should not be applied to any templates. It should only be applied to fully-defined classes (which does include explicit template instantiations, but it does not include any templates with unset parameters).

I've marked a few lines with the comment Example to point out what I mean.

The reason incomplete templates can't be marked as exported is because it prevents a user from instantiating that template with their own custom parameters. Instead of automatically instantiating the template with whatever parameters the user requests, the compiler will assume that a suitable class with those parameters was already instantiated by the DLL that exported the header. This will typically result in a linking error.

mxgrey · 2018-04-30T17:27:30Z

It looks like the lines that I marked with Example didn't make it into the review for some reason. In any case, template definitions shouldn't be given the export macro.

jslee02 · 2018-04-30T17:35:16Z

I haven't fully investigated but without the export macro to the template classes caused test failures of test_Collision and test_Skeleton for different reasons.

test_Collision failed due to creating different static instances for collision factory; FCL and DART collision detectors are registered to one factory, but Bullet and FCL are registered to each different factory.

test_Skeleton failed due to that null is returned from here. (edit: This only happens on macOS)

mxgrey · 2018-04-30T17:50:01Z

test_Collision failed due to creating different static instances for collision factory

I think that's because this class is never marked as exported. I'm not sure what the correct way to export that would be. Maybe we need to explicitly instantiate the SingletonFactory class. In this specific case, we could decide that we do not want users to be able to use our Singleton template for any of their own template arguments (and therefore put the DART_API macro on it), but we can't apply that reasoning to all of our templates, or else DART won't be extensible.

test_Skeleton failed due to that null is returned from here.

That might be a trickier issue. It's not immediately clear to me why that would happen.

jslee02 · 2018-04-30T21:24:36Z

Marking a static member of a template class would be also dangerous? test_Collision can pass with this.

mxgrey · 2018-04-30T23:47:26Z

Yes, I think that would also be dangerous, because the definition is actually given in the implementation header.

My understanding of the export macro is that it effectively tells the user's compiler "Don't try to create a definition for this symbol, because the definition will be provided by the library that exported this header", and then it tells the user's linker "Don't look for a definition of this symbol within the library that we just compiled; instead, look for it in one of the libraries that we are linking to".

I think if we "export" that static member, then users will get a linking error for it whenever they pass a custom class type into Singleton<T>.

jslee02 added 4 commits March 21, 2018 22:02

Add export.h and hide symbols by default

97468c3

Apply export macros

7e82bcd

Add DART_HIDE_ALL_SYMBOLS option

9129ad6

Code format

a38ca13

jslee02 added the pr: review needed label Mar 22, 2018

jslee02 added this to the DART 6.4.0 milestone Mar 22, 2018

jslee02 requested a review from mxgrey March 22, 2018 17:57

jslee02 mentioned this pull request Mar 22, 2018

Submit DART port to vcpkg #1005

Closed

Export missing ASSIMP definitions -- for test

4dd81fd

jslee02 added 2 commits March 22, 2018 12:59

Rename DART_EXPORT to DART_API

e3fd90e

Use manually written export.hpp

024ac5a

jslee02 added 2 commits March 22, 2018 15:47

Use unique export preprocessors per component

9141345

Update export preprocessor per components accordingly

c5eb4be

jslee02 added 5 commits March 22, 2018 16:19

Minor cleanup

12aec1a

Update changelog

e3f17f2

Fix installation of export headers

cc638e0

Fix use of CMake variable

ed9a5df

Fix install path of export.h

d8de131

mxgrey reviewed Mar 23, 2018

View reviewed changes

jslee02 added 2 commits March 23, 2018 10:01

Use template file for export.hpp

11330ed

Change naming pattern: DART_DETAIL_ --> DETAIL_DART_

cec1d7c

Add missing DART_API

73e98ec

jslee02 removed the pr: work-in-progress label Apr 28, 2018

jslee02 and others added 5 commits April 28, 2018 04:10

Fix accidentally pushed commit

ff49098

Update CHANGELOG.md

7dce12d

Fix warnings per clang 9.1.0

98444dd

Add more missing DART_API

5f9fd41

Merge remote-tracking branch 'origin/release-6.5' into build/visibility

91d089e

jslee02 added the pr: will merge soon label Apr 29, 2018

mxgrey reviewed Apr 30, 2018

View reviewed changes

Use DETAIL_DART_DEPRECATED

ee0063c

mxgrey requested changes Apr 30, 2018

View reviewed changes

jslee02 added pr: work-in-progress and removed pr: will merge soon labels Apr 30, 2018

Add missing header

9facf40

Merge branch 'release-6.5' into build/visibility

a521359

jslee02 modified the milestones: DART 6.5.0, DART 6.6.0 May 12, 2018

jslee02 changed the base branch from release-6.5 to release-6.6 May 12, 2018 15:49

jslee02 removed this from the DART 6.6.0 milestone Jul 19, 2018

jslee02 added the help wanted Indicates wanting help on an issue or pull request label Aug 12, 2018

jslee02 mentioned this pull request Sep 19, 2018

Automatically update CollisionGroups #1112

Merged

5 tasks

jslee02 closed this Jan 12, 2019

traversaro mentioned this pull request Sep 18, 2020

Support BUILD_SHARED_LIBS set to ON on Windows/MSVC #1501

Closed

jslee02 deleted the build/visibility branch July 22, 2021 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ visibility support #1040

C++ visibility support #1040

jslee02 commented Mar 22, 2018 •

edited

Loading

mxgrey commented Mar 22, 2018 •

edited

Loading

mxgrey commented Mar 22, 2018 •

edited

Loading

jslee02 commented Mar 22, 2018

mxgrey commented Mar 22, 2018 •

edited

Loading

jslee02 commented Mar 22, 2018 •

edited

Loading

mxgrey commented Mar 22, 2018

mxgrey commented Mar 22, 2018

jslee02 commented Mar 22, 2018

mxgrey Mar 23, 2018 •

edited

Loading

jslee02 Mar 23, 2018

mxgrey Mar 23, 2018

jslee02 Mar 23, 2018

jslee02 commented Apr 29, 2018

mxgrey Apr 30, 2018

mxgrey left a comment

mxgrey commented Apr 30, 2018

jslee02 commented Apr 30, 2018 •

edited

Loading

mxgrey commented Apr 30, 2018 •

edited

Loading

jslee02 commented Apr 30, 2018

mxgrey commented Apr 30, 2018

C++ visibility support #1040

C++ visibility support #1040

Conversation

jslee02 commented Mar 22, 2018 • edited Loading

mxgrey commented Mar 22, 2018 • edited Loading

mxgrey commented Mar 22, 2018 • edited Loading

jslee02 commented Mar 22, 2018

mxgrey commented Mar 22, 2018 • edited Loading

jslee02 commented Mar 22, 2018 • edited Loading

mxgrey commented Mar 22, 2018

mxgrey commented Mar 22, 2018

jslee02 commented Mar 22, 2018

mxgrey Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

jslee02 Mar 23, 2018

Choose a reason for hiding this comment

mxgrey Mar 23, 2018

Choose a reason for hiding this comment

jslee02 Mar 23, 2018

Choose a reason for hiding this comment

jslee02 commented Apr 29, 2018

mxgrey Apr 30, 2018

Choose a reason for hiding this comment

mxgrey left a comment

Choose a reason for hiding this comment

mxgrey commented Apr 30, 2018

jslee02 commented Apr 30, 2018 • edited Loading

mxgrey commented Apr 30, 2018 • edited Loading

jslee02 commented Apr 30, 2018

mxgrey commented Apr 30, 2018

jslee02 commented Mar 22, 2018 •

edited

Loading

mxgrey commented Mar 22, 2018 •

edited

Loading

mxgrey commented Mar 22, 2018 •

edited

Loading

mxgrey commented Mar 22, 2018 •

edited

Loading

jslee02 commented Mar 22, 2018 •

edited

Loading

mxgrey Mar 23, 2018 •

edited

Loading

jslee02 commented Apr 30, 2018 •

edited

Loading

mxgrey commented Apr 30, 2018 •

edited

Loading