[CXX-1251] Investigate whether inline functions should be marked with BSONCXX_API Created: 10/Mar/17 Updated: 27/Oct/23 Resolved: 07/Apr/17 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | API |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Samuel Rossi (Inactive) | Assignee: | David Golden |
| Resolution: | Works as Designed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
Currently, we don't mark inline functions with BSONCXX_API. We should determine if this is correct, and if not, fix it. |
| Comments |
| Comment by David Golden [ 07/Apr/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
After offline discussion, we've concluded that the current approach is fine. | |||||||||||||||||||||||||||||||||||||||||
| Comment by David Golden [ 14/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
MSFT on exporting symbols via .def, including symbol mangling: https://msdn.microsoft.com/en-us/library/d91k01sh.aspx | |||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 14/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
SO discussion: http://stackoverflow.com/questions/42789289/how-can-i-override-a-class-scoped-declspecdllexport-annotation-on-a-per-me | |||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 14/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
Right, if we had the equivalent of -fvisibility-inlines-hidden on Windows, this would be less of an issue. The problem appears to be that MSVCs annotations are simply insufficient to express what we want here. Once you have marked a class for export, there appears to be no selective way to reverse its application on select members. Note also that merely tagging the methods of the class, rather than the class itself, is likely insufficient, because that would not export implicit members or typeinfo associated state, which almost certainly does need to be exported. | |||||||||||||||||||||||||||||||||||||||||
| Comment by David Golden [ 14/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
More references: | |||||||||||||||||||||||||||||||||||||||||
| Comment by J Rassi [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
Indeed, XCXX_PRIVATE expands to nothing when compiling with VS2015. Here's the full contents of the export header, as the Evergreen builds currently generate them:
I suppose then that we should either stop declaring any methods within XCXX_API class definitions as inline (as David suggests above), or halt the practice of keeping them out of the ABI. | |||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
rassi - You are correct as to the intention of marking XCXX_INLINE functions with XCXX_PRIVATE. However, I have a suspicion that on windows, export.h defines XCXX_PRIVATE as an empty expansion, because I don't think there is a way to say "don't export this", within an exported class with MSVC. Can someone with a windows build take a look at what shows up for MONGOCXX_PRIVATE in the export.h header? | |||||||||||||||||||||||||||||||||||||||||
| Comment by Mark Benvenuto [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
Yes, David your interpretation is correct. You only need to mark methods in classes with export if the class is not marked with declspec. | |||||||||||||||||||||||||||||||||||||||||
| Comment by J Rassi [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
I think that Drew's description of the driver's "current practice" of making inline functions explicitly not considered part of the ABI includes the practice of marking methods that are inlined in class definitions with XCXX_INLINE (which limits their visibility by including XCXX_PRIVATE as part of the macro expansion). src/mongocxx seems to adhere to this practice rather consistently, but I notice we're missing BSONCXX_INLINE in a few places in the bsoncxx library (see the key_context constructor, for example). I found that one and some others through grep (note: contains many false positives):
| |||||||||||||||||||||||||||||||||||||||||
| Comment by David Golden [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
Well, we're already marking classes with XCXX_API, so all inline functions in the class definition are already exported. The blogs.msdn article gives this example:
Do you think we should just not use any inline functions? That would avoid the problems entirely. | |||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
I think this needs careful consideration. I think that there is a case to be made that we should not mark inline functions with XCXX_API, because if we do so, they become part of the ABI, since it means per the above links hat the consuming code might decide not to inline the function, and then instead rely on the version caused to be exported from the library by being tagged dllexport. That would represent a divergence from the current practice, where inline functions are explicitly not considered part of the ABI. | |||||||||||||||||||||||||||||||||||||||||
| Comment by David Golden [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
More discussion:
My take is that we need to mark static inline functions with the API macros because if the compiler doesn't inline them for some reason, we still need them to be marked for export/import. However if a class has a declspec annotation, then any inlines in the class definition get the same annotation so we don't it in that circumstance. mark.benvenuto, does this match with your understanding? | |||||||||||||||||||||||||||||||||||||||||
| Comment by Samuel Rossi (Inactive) [ 13/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
I'm not quite sure what to do for this ticket based on the above link. Does this imply that we should be marking inline functions with BSONCXX_API/MONGOCXX_API, or that we shouldn't? | |||||||||||||||||||||||||||||||||||||||||
| Comment by David Golden [ 11/Mar/17 ] | |||||||||||||||||||||||||||||||||||||||||
|
Here's what I think is the definitive reference for this question: https://msdn.microsoft.com/en-us/library/xa0d9ste.aspx |