Limit set of exported symbols #583
Conversation
|
At least 4. should not be used via public API imho as these functions are used internally to have rolling file names. There is no point in exposing that via the public API as clients should not write files with the dlt library. |
The symbols I listed are not only public (user-facing) API. However, they have to be exported at the moment because some console tools expect them to be there. This is because Generally I'd propose to extract these details into a separate static library with private headers. To continue with this, however, I'd need some basic commitment to this PR's general idea by any one of the project responsible. |
|
Hello @Felix-El , As this anyhow breaks ABI-compatiblity, I would also like to have the functionname #2 to #4 renamed. There is a version bump in cmake. So DLT will not be compilable on older platforms (e.g. Ubuntu 18.04). Can you make this changes backward compatible? E.g. by using this feature only on recent cmake versions? |
Yus, totally agree.
Fine for me.
Is it maybe an option to just copy https://github.com/Kitware/CMake/blob/v3.12.0/Modules/GenerateExportHeader.cmake into this repo? License is BSD 3-Clause so should not be an issue. I would just need to verify this generates fine even with CMake as old as 3.3. |
Summary: This commit deals with issues potentially introduced in code linked to shared libdlt. The library previously exported all symbols by default, up to ~60 of which were not prefixed 'dlt*' and could conflict with symbols in the consumer code. After applying this commit only symbols prefixed 'dlt*' can be observed using $ nm --dynamic --defined-only ./build/src/lib/libdlt.so | cut -f3 -d' ' | sort which drastically reduces the chance of such conflicts. Brief: - explicit symbol export using DLT_EXPORT macro as provided by CMake's GenerateExportHeader module - rename get_filename_ext to dlt_get_filename_ext - rename getFileSerialNumber to dlt_get_file_serial_number - hide dlt_daemon's original main() in unit tests to avoid multiple definition on some platforms - add CMake 3.12's GenerateExportHeader to repository for backward-compatibility with pre-3.12 CMake (i.e. Ubuntu 18.04)
|
Hello everyone, I have just pushed an updated version which addresses above discussion points.
Since there are many more API to revise for "should they be public?" I deemed it enough to rename the few non-dlt* prefixed exported functions so we have everything under a dlt* "namespace" for the time being. This already reduces the risk of conflict extremely and further improvement can be done after merging this. Reminder: Already the original commit removed up to ~60 symbols from the shared library's export table which were non-namespaced private implementation details. Only few symbols were under discussion. |
|
Hello @Bichdao021195 |
|
Hi @minminlittleshrimp With cat ./after , i can observed that |
|
@michael-methner, @minminlittleshrimp how can we get this merged? |
Patch for #582. This disables unconditional export of symbols from the shared library
libdltin favor of an opt-in approach.libdltare unaffected by this change (BUILD_SHARED_LIBS=OFF)The sorted list of remaining symbols is:
So some functions need to be revised:
dlt_buffer_*defined insrc/shared/dlt_common.cThese functions are required to be exported but they are not declared in any header.
get_filename_extdeclared ininclude/dlt/dlt_common.hIt is not prefixed with dlt. Should it really be exported?
getFileSerialNumberdefined insrc/lib/dlt_filetransfer.cThis function should be renamed to have a dlt prefix. It is also required to be exported but not declared in any header.
multiple_files_buffer_*declared ininclude/dlt/dlt_multiple_files.hThese functions appear to be public API, too but are not prefixed with dlt.