Allow hidden visibility default on gcc/clang

[mvieth]

Can be enabled/disabled with the CMake option PCL_SYMBOL_VISIBILITY_HIDDEN. The main benefits are reduced library size and reduced risk of certain bugs. Some sources also claim that it can increase execution speed under certain conditions, but so far I did not test whether this is the case for PCL. This feature is still somewhat experimental (although I did a lot of testing), so for anyone reading this who got a linker error when this is enabled: please open an issue, it may be that PCL_EXPORTS is missing somewhere. I think for now, it is better to leave this disabled by default, and only encourage users who are fine with experimental features to enable it. In the future, we might then enable it for everyone.

Suggested read: https://gcc.gnu.org/wiki/Visibility

Range image

A commenter suggested a different solution, now only the four static members need to be marked with PCL_EXPORTS.
For gcc, I could only get it to work with the whole RangeImage class marked as PCL_EXPORTS (i.e. __attribute__ ((visibility ("default")))), otherwise I get the linking error undefined reference to vtable RangeImage.
For MSVC on the other hand, marking the whole class as PCL_EXPORTS (i.e. __declspec(dllexport)) seems to be problematic because when including range_image.h in another PCL module, RangeImage is also marked as dllexport so the compiler seems to expect a definition of the static members (lookup tables) in that module as well. In the future, it might be a good idea to move away from the static class member lookup tables, and perhaps use static local variables instead like in RGB2sRGB_LUT).

Supervoxel clustering

Moved the specializations of getPoint to supervoxel_clustering.h, using traits::HasColor. This was necessary to resolve some interdependencies between SupervoxelClustering, OctreePointCloudAdjacencyContainer, and OctreePointCloudAdjacency

Further changes

• removed PCL_EXPORTS on TransformationEstimationSVD again. I added it in a previous PR but noticed now that it was not necessary and caused warnings on MSVC.
• since I added PCL_EXPORTS to the static members in RangeImage, these change might fix also [windows] error linking pcl rangeimage LNK2020 LNK2001 LNK1120 #807 , but I did not test that
• in convolution.cpp, the explicit instantiation with RGB and PointXYZRGB is not necessary and even creates a warning that the visibility attributes change with this redefinition. The class is tested in https://github.com/PointCloudLibrary/pcl/blob/master/test/filters/test_convolution.cpp with both types
• in pcl_config.h.in, we now first check whether __cplusplus is defined (i.e. whether the program is a C++ program) before doing C++ version checks, because this file is now included in some C source code file via pcl_exports.h

Library size

in kilobytes	MinSizeRel, hidden symbols	MinSizeRel, hidden symbols off	Release, hidden symbols	Release, hidden symbols off
common	726	839	825	879
features	43023	49729	51947	57824
filters	8708	10326	10013	11194
io_ply	324	412	401	467
io	2454	3272	2985	3587
kdtree	1199	1565	1453	1754
keypoints	1674	2032	2040	2337
ml	125	146	188	196
octree	2593	2818	3252	3371
outofcore	81	85	97	101
people	22	22	29	29
recognition	5455	6821	6642	7592
registration	2108	2793	2428	2829
sample_consensus	13392	14776	16101	17036
search	2059	2501	2463	2850
segmentation	12893	16671	15067	18132
stereo	241	258	302	306
surface	6304	7174	8178	8747
tracking	3903	4578	4814	5378
visualization	1294	1648	1601	1865

Built with GCC 13.2. All CMake options except CMAKE_BUILD_TYPE and PCL_SYMBOL_VISIBILITY_HIDDEN are the default ones

[larshg]

This would close #1913 FYI.

[Osyotr]

Range image

You should move the key function (in this case virtual destructor) into a CPP file.
https://gcc.gnu.org/wiki/VerboseDiagnostics#missing_vtable

[mvieth]

Range image

You should move the key function (in this case virtual destructor) into a CPP file. https://gcc.gnu.org/wiki/VerboseDiagnostics#missing_vtable

Thanks for the suggestion, that seems to work. Then let's go with that solution since it does not require an if-else based on the compiler.

[larshg]

Should/can we export the entire range_image class instead of a bunch of "selected" members?

[mvieth]

Should/can we export the entire range_image class instead of a bunch of "selected" members?

When I tested that, it lead to problems with MSVC (see also what I wrote above under "Range Image" but crossed out because I switched to a different solution). Basically, when range_image.h is included in another PCL module (e.g. io) and the whole class is marked as PCL_EXPORTS, then MSVC expects a definition of RangeImage's four static members in io as well (like the definition in range_image.cpp). In that case, there seems to be nothing that indicates that RangeImage is exported from common, and not io. I am actually wondering if it wouldn't be "more correct" to have multiple macros PCL_COMMON_EXPORTS, PCL_IO_EXPORTS, PCL_FILTERS_EXPORTS, etc instead of just PCL_EXPORTS for all modules. Perhaps we could look into this in the future.

[Osyotr]

I am actually wondering if it wouldn't be "more correct" to have multiple macros PCL_COMMON_EXPORTS, PCL_IO_EXPORTS, PCL_FILTERS_EXPORTS, etc instead of just PCL_EXPORTS for all modules.

Every library must have its own PCL_*_EXPORTS. And don't forget __declspec(dllimport).
Consider using CMake's generate_export_header (https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html)

[Osyotr]

Should/can we export the entire range_image class instead of a bunch of "selected" members?

Basically, when range_image.h is included in another PCL module (e.g. io) and the whole class is marked as PCL_EXPORTS, then MSVC expects a definition of RangeImage's four static members in io as well (like the definition in range_image.cpp).

Exporting the whole class should work on all platforms. I suspect that the reason why it doesn't work is because of the wrong exporting logic on Windows (see my previous comment).

[mvieth]

Every library must have its own PCL_*_EXPORTS.

That was my impression as well while reading into MSVC's exporting system. But if that is the case, why have there never been any problems with the current shared PCL_EXPORTS (except for not being able to export the whole RangeImage class in this PR), even though there are many classes that are defined in one PCL module library and used in another?
Edit: I just checked and OpenCV seems to also use just one CV_EXPORTS for all libraries: https://github.com/opencv/opencv/blob/master/modules/core/include/opencv2/core/cvdef.h#L426

I would suggest to merge this pull request as soon as possible since I have been working on this for over a year and it would be nice to have this feature finally merged. Assuming of course, that we all agree that there is no regression compared to the master branch, no new bugs are introduced, nothing is made worse.
After that, we can again discuss replacing PCL_EXPORTS by PCL_*_EXPORTS and exporting the whole RangeImage class.

[larshg]

I think its good to merge!

I wonder why there have been no issues with the Exceptions and why those need export macro, but I guess they just haven't been used on windows 😄

I have fiddled around with cmakes generate_export_header and found out that it adds: __declspec (dllimport) to the else case. When I compiled with that and using ie PointCloud in a subproject I got all kinds of linking errors.

And seemingly you shouldn't export templated classes (kinda makes sense though 😁) - but the various instantiations that we make, can be exported.
Ie. when I removed the export macro to PointCloud it compiled with the newly added __declspec (dllimport).

Also as of what I understood, static member variables are not allowed to have an initial value, but its advised to set the value in a .cpp file and it could be done in, ie. a initialize method. I haven't tried it yet though (that might be applicable to the range_image?)

[larshg]

Hmm I just tried to use an pcl::PCLException and it compiles fine without the export flags. Also according to https://stackoverflow.com/questions/64506244/how-do-i-export-class-that-fully-defined-in-header-file - since it fully defined in the header file, there is nothing to export?

[larshg]

Can we retest without PCL_EXPORT for all the exceptions?

[mvieth]

Can we retest without PCL_EXPORT for all the exceptions?

I added PCL_EXPORTS to the exceptions because the section "Problems with C++ exceptions" in https://gcc.gnu.org/wiki/Visibility warned that all user defined exceptions must be marked as visibility ("default"). This might be a GCC-specific problem, and it might be irrelevant if the exceptions are marked as dllexport on MSVC. I am also not sure how to test (on GCC) whether everything still works if I remove PCL_EXPORTS from the exceptions.

[larshg]

Hm okay. I just tested with an application using PCLException, but not the example with libB throwing a exception defined in LibA and handled in libC 😅

[mvieth]

I tested with GCC and printed the symbols in pcl_common with the nm tool. When the exceptions are marked with PCL_EXPORTS:

00000000001b4468 V typeinfo for pcl::PCLException
00000000001b44a8 V typeinfo for pcl::BadArgumentException
00000000001b4450 V typeinfo for pcl::ComputeFailedException
00000000001b4978 V typeinfo for pcl::KernelWidthTooSmallException
00000000001b49e8 V typeinfo for pcl::UnorganizedPointCloudException
000000000010e3b0 V typeinfo name for pcl::PCLException
000000000010f560 V typeinfo name for pcl::BadArgumentException
000000000010e390 V typeinfo name for pcl::ComputeFailedException
00000000001253a0 V typeinfo name for pcl::KernelWidthTooSmallException
0000000000127660 V typeinfo name for pcl::UnorganizedPointCloudException
00000000001b4428 V vtable for pcl::PCLException
00000000001b4480 V vtable for pcl::BadArgumentException
00000000001b4400 V vtable for pcl::ComputeFailedException
00000000001b4950 V vtable for pcl::KernelWidthTooSmallException
00000000001b49c0 V vtable for pcl::UnorganizedPointCloudException

Without PCL_EXPORTS:

00000000001b34a8 d typeinfo for pcl::PCLException
00000000001b34e8 d typeinfo for pcl::BadArgumentException
00000000001b3490 d typeinfo for pcl::ComputeFailedException
00000000001b39b8 d typeinfo for pcl::KernelWidthTooSmallException
00000000001b3a28 d typeinfo for pcl::UnorganizedPointCloudException
000000000010d3b0 r typeinfo name for pcl::PCLException
000000000010e560 r typeinfo name for pcl::BadArgumentException
000000000010d390 r typeinfo name for pcl::ComputeFailedException
00000000001243a0 r typeinfo name for pcl::KernelWidthTooSmallException
0000000000126660 r typeinfo name for pcl::UnorganizedPointCloudException
00000000001b3468 d vtable for pcl::PCLException
00000000001b34c0 d vtable for pcl::BadArgumentException
00000000001b3440 d vtable for pcl::ComputeFailedException
00000000001b3990 d vtable for pcl::KernelWidthTooSmallException
00000000001b3a00 d vtable for pcl::UnorganizedPointCloudException

The second column is the symbol type, a lowercase letter means that the symbol is local, while an uppercase letter means that it is global (external). This indicates that marking the exceptions with PCL_EXPORTS (so visibility ("default") on GCC) does make a difference on GCC, and it would likely not work without (at runtime, in specific circumstances). And yes, on MSVC it does not seem necessary to mark the exceptions with dllexport, but it does not seem to cause any problems either.

[larshg]

I have tested both on windows and ubuntu (24.04 dev docker), where I make a small application that calls code, that makes an exception in the common .dll/.so.
It works both with and without PCL_EXPORT for the exceptions, even though when I was in ubuntu I tried compile the application with clang and the library with GCC.
It is probably difficult to fully ensure all kind of mixes and from a general readup on exceptions, it is advised to use the same compiler, when throwing exceptions across dll/.so boundaries. Alternatively is to provide only a C interface....
From what I can gather, the EXPORT is not necessary in the following cases:

• Class is fully defined in header file
• Class/method is templated

Generally though, I think we would get some linker errors, if we add the declspec(__dllImport) to the two macros, but it can make the code a bit faster on windows:
https://learn.microsoft.com/en-us/cpp/build/importing-function-calls-using-declspec-dllimport?view=msvc-170

However, if Whole Program Optimization is enabled, this can be achieved as well (which is normally the case, except if we compile with cuda/gpu modules atm, since that had a bug with an old thrust library).

I tried with the declspec(__dllImport) as well and it doesn't come with any linking errors with the Exceptions, so I'm fine with keeping them then.