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

fix: fix symbol export on Windows + use target properties #922

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aminya
Copy link

@aminya aminya commented Oct 27, 2022

This changes the export definition to only happen when the tinyxml2 itself is built as a shared library.

It fixes an issue where the tinyxml2 symbols are exported even when it is compiled statically and privately linked into another shared library. This allows the compiler to inline these symbols. Otherwise, the symbols of tinyxml2 are exported in the final shared library.

BillyONeal pushed a commit to microsoft/vcpkg that referenced this pull request Oct 29, 2022
…27514)

* [tinyxml2]: do not force export the symbols when building statically

leethomason/tinyxml2#922

* [tinyxml2]: check for TINYXML2_EXPORT on non windows

* [tinyxml2] avoid redefinition of TINYXML2_LIB
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@leethomason
Copy link
Owner

@aminya any thoughts / responses to @alexreinking comments?

@aminya
Copy link
Author

aminya commented Jan 15, 2023

I already discussed many things in
#922 (comment)

@alexreinking
Copy link
Contributor

I already discussed many things in (comment)

@aminya - but you have not replied to my review comments, which were written more recently. In particular, the DEFINE_SYMBOL change is absolutely irrelevant.

@aminya
Copy link
Author

aminya commented Jan 17, 2023

TINYXML2_EXPORT is the gateway to forcing the symbol in the final shared library. What do you mean it is irrelevant?

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
Comment on lines 41 to 42
DEFINE_SYMBOL "TINYXML2_EXPORT" # only used when tinyxml2_SHARED_LIBS/BUILD_SHARED_LIBS is ON

Copy link
Contributor

Choose a reason for hiding this comment

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

So now there's an extra newline?

Suggested change
DEFINE_SYMBOL "TINYXML2_EXPORT" # only used when tinyxml2_SHARED_LIBS/BUILD_SHARED_LIBS is ON
DEFINE_SYMBOL "TINYXML2_EXPORT"

Let's keep this change functional, please. We can discuss adding proper documentation for this later.

@alexreinking
Copy link
Contributor

@aminya - I tried to reproduce the issue you were facing. Please tell me if any of the steps I followed differ from yours.

Step 0: create a working directory

$ cd ~
$ mkdir test
$ cd test

Now we have somewhere to work.

Step 1: download, patch, and build tinyxml2

$ git clone https://github.com/leethomason/tinyxml2.git
$ cd tinyxml2
$ sed -i 's/\(__GNUC__ >= 4\)/defined(TINYXML2_EXPORT) \&\& \1/g' tinyxml2.h

That last line applies the same change you made to tinyxml2.h. The CMakeLists.txt file is untouched.

Now we'll build tinyxml2 as a static library, with PIC enabled since it will be part of a shared library later.

$ cmake -G Ninja -S . -B build -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=NO -DCMAKE_POSITION_INDEPENDENT_CODE=YES
$ cmake --build build/
$ cmake --install build --prefix ../_local

We're installing it to ~/test/_local, which will be convenient later.

Step 2: create a simple project:

Create two files in ~/test:

CMakeLists.txt:

cmake_minimum_required(VERSION 3.23)
project(example)

find_package(tinyxml2 REQUIRED)

set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN YES)

add_library(example SHARED example.cpp)
target_link_libraries(example PRIVATE tinyxml2::tinyxml2)

example.cpp:

#include <tinyxml2.h>
using namespace tinyxml2;

__attribute__((visibility("default")))
int example()
{
	static const char* xml = "<element/>";
	XMLDocument doc;
	doc.Parse( xml );

	return doc.ErrorID();
}

The build forces the example library to be shared, and it links to tinyxml2. We also hide symbols by default, but expose example as our one API.

Step 3: build the project

$ cmake -G Ninja -S . -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_PREFIX_PATH=$PWD/_local
$ cmake --build build

Here we set CMAKE_PREFIX_PATH to point to the patched tinyxml2 we just built.

Now we can see that the only exported symbol is example.

$ nm -D build/libexample.so | grep ' T ' | c++filt
00000000000035b0 T example()

We only care about the exported (capital), text ("T") symbols.


Step 3b: trying again

If I repeat all the same steps as before but without the patch to tinyxml2.h, then I see this:

alex@alex-ubuntu:~/test$ nm -D build/libexample.so | grep ' T ' | c++filt
0000000000009280 T example()
000000000000ffd0 T tinyxml2::XMLComment::ParseDeep(char*, tinyxml2::StrPair*, int*)
000000000000be70 T tinyxml2::XMLComment::XMLComment(tinyxml2::XMLDocument*)
... dozens more ...

This shows that no changes to the CMakeLists.txt are necessary, but your change to the header file is good and correct.

@aminya
Copy link
Author

aminya commented Jan 17, 2023

Thanks for the example. I moved the setting of the visibility properties of the tinyxml2 target to avoid any confusion.

@alexreinking
Copy link
Contributor

Please revert the changes to CMakeLists.txt as they do not change the functionality. Keep only the change to tinyxml2.h. Then I will approve the PR. ATTN @leethomason

@leethomason
Copy link
Owner

Closing. Can re-open if the thread picks back up.

@aminya
Copy link
Author

aminya commented Nov 21, 2023

@leethomason This was ready to be merged. Are you closing this because of a simple documentation comment?

@leethomason
Copy link
Owner

@alexreinking had requested removing the changes to cmakelist.txt. There are still changes - were those intended? Is there any reason not to revert them?

@aminya
Copy link
Author

aminya commented Nov 22, 2023

@alexreinking had requested removing the changes to cmakelist.txt. There are still changes - were those intended? Is there any reason not to revert them?

Yes, they were intended. The CMake hidden variable was being set globally, but this makes it specific to the tinyxml target.

@leethomason leethomason reopened this Nov 23, 2023
@alexreinking
Copy link
Contributor

Yes, they were intended. The CMake hidden variable was being set globally, but this makes it specific to the tinyxml target.

@aminya -- I'm not sure what you mean by "globally". The variables would be local to the project, so if one were to FetchContent or add_subdirectory tinyxml2, they would not see these variables. So they are not global in that sense (as they would be were they set as cache variables).

Furthermore, this build has only one library (tinyxml2) which is affected by the two visibility variables you set. The only other target is xmltest, which is an executable; the visibility variables do not meaningfully affect executables unless ENABLE_EXPORTS is set, which is not the case here.

Thus the changes to the CMakeLists.txt are totally unnecessary and should not be included.

@dg0yt
Copy link

dg0yt commented Nov 26, 2023

With 240e5c1, the PR lost the if(BUILD_SHARED_LIBS) guard which was the key contribution IIUC.

@aminya
Copy link
Author

aminya commented Nov 26, 2023

Thus the changes to the CMakeLists.txt are totally unnecessary and should not be included.

I suggest you read the Effective Modern Cmake as it is the recommended approach by the CMake experts.

https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1#think-in-terms-of-targets-and-properties

@aminya aminya changed the title fix: do not force export the symbols when building statically fix: fix symbol export on Windows + use target properties Nov 26, 2023
@aminya
Copy link
Author

aminya commented Nov 26, 2023

With 240e5c1, the PR lost the if(BUILD_SHARED_LIBS) guard which was the key contribution IIUC.

I checked the documentation of CMake. Based on the doc, that check is done automatically for a target.

@dg0yt
Copy link

dg0yt commented Nov 26, 2023

I checked the documentation again now.

  • IMO the variables are perfect modern CMake: They determine the default value for the target properties.
    So moving the value from the variable to the main target makes no difference.
  • Modern CMake (cmake_minimum_required version >= 3.3) applies the property to all targets (CMP0063).
    So if you want to have different visibility settings for static libraries, you must use an explicit guard.

@alexreinking
Copy link
Contributor

alexreinking commented Nov 28, 2023

I suggest you read the Effective Modern Cmake as it is the recommended approach by the CMake experts.

@aminya -- Please do not resort to insults. I posted a demonstration above showing that your header edit alone (which is good and worth merging!) is sufficient to address the symbol export issue. Please re-examine this and either:

  1. Show steps to reproduce the issue without the CMake edits (to demonstrate they are indeed necessary), or
  2. Remove the CMake edits

@dg0yt - thank you for reading the documentation. The variables are indeed fine. They have the added benefit of applying to new targets within this project should the need ever arise; this reduces the risk of accidental symbol mismanagement.

So if you want to have different visibility settings for static libraries, you must use an explicit guard.

IIUC - we actually don't want this. We want the non-API symbols to be hidden in static libraries. The header fix makes exactly the API symbols visible (by checking TINYXML2_EXPORT) when building statically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants