Tooling - CMake - Export symbol when creating shared library on Windows by theblackunknown · Pull Request #930 · KhronosGroup/SPIRV-Tools

theblackunknown · 2017-10-29T08:12:01Z

Hello people,

Not sure if that be useful for anyone, but I am experimenting with this project and noticed that no symbols gets exported when creating a shared library on Windows.

So I modified build script and headers to select what should be exported or not.
The approach should be quite portable as I rely on CMake GenerateExportHeader module.

I haven*t touched travis or appveyor file to add windows shared library build to CI as I don't know what this is the recommended way to do so, but if merged it should go there to verify shared library correctly generate export symbol

Let me know what you think of it and if it worth a merge to upstream.

CLAassistant · 2017-10-29T08:12:07Z

All committers have signed the CLA.

dneto0 · 2017-10-30T18:27:10Z

Hi @Kangz can you comment? (You added the shared lib support).
As I recall, isn't there a less intrusive way to do this, via setting build defaults?

Kangz · 2017-10-30T18:38:41Z

source/CMakeLists.txt

+generate_export_header(${SPIRV_TOOLS}
+  BASE_NAME spirv-tools)

-  ${CMAKE_CURRENT_SOURCE_DIR}/util/bitutils.h


.h files are important to keep in the source list so that they are shown in IDEs like visual studio.

Kangz · 2017-10-30T18:39:30Z

source/CMakeLists.txt


-set(SPIRV_SOURCES
-  ${spirv-tools_SOURCE_DIR}/include/spirv-tools/libspirv.h
+include(GenerateExportHeader)


Why not make your own export header, it is simple to do and makes the code more clear imho.

Kangz · 2017-10-30T18:39:59Z

include/spirv-tools/optimizer.hpp

  // ignores all messages from the library. Use SetMessageConsumer() to supply
  // one if messages are of concern.
-  explicit Optimizer(spv_target_env env);
+  SPIRV_TOOLS_OPT_EXPORT explicit Optimizer(spv_target_env env);


You can make the whole class SPIRV_TOOLS_OPT_EXPORT instead of every function individually.

Yes, that was my recollection. (I haven't had to do this in some time...)
That is much preferred: mark an entire class rather than individual methods.

Kangz · 2017-10-30T18:42:41Z

@dneto0 the changes are a bit more intrusive then they could if whole classes were exported and the export headers were written manually (vs. generated inside CMake).

Exporting symbols / classes individually is the preferred way to do this in the Chromium style. Since this is not Chromium, the alternative is to have the following CMake code:

set_target_properties(${Target} PROPERTIES
                      WINDOWS_EXPORT_ALL_SYMBOLS true
                      C_VISIBILITY_PRESET visible)

dneto0 · 2017-10-30T20:04:34Z

@Kangz : Thanks for the advice/confirmation.

My summary:

I strongly prefer not to export all symbols.
The files in include/spirv-tools/* are all meant to be the export interface of the various libraries. It bears a review to double-check before applying export annotations.

theblackunknown · 2017-10-31T02:12:01Z

I totally agree on your conclusion that those changes are quite intrusive, at the cost of having finer-grained control over what should be exposed or not.
As @Kangz mentionned using CMake WINDOWS_EXPORT_ALL_SYMBOLS and [LANG]_VISIBILITY_PRESET should be equivalent and less intrusive if we want to sacrifice the finer-grained control.
If we go that way also CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and CMAKE_[LANG]_VISIBILITY_PRESET at the root CMakeLists.txt to set the default value for every subsequent library resulting in less changes.

I can try experimenting with those, in the meantime let me know what are your thoughts on this

EDIT It seems to have been used to port project on Windows

theblackunknown · 2017-10-31T02:50:32Z

So CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS works pretty well and it has no if you are not on Windows.
One downside is that linker is called everytime you trigger a cmake --build with generator "Visual Studio Code 15 2017".
It is not that long but definitively annoying when I try to have a quick iteration cycle.

NOTE Using Ninja or NMake Makefiles this problem does not show up

dj2 · 2018-08-27T13:48:13Z

Is this PR still valid, or can we close it?

Kangz · 2018-08-27T13:50:30Z

It should be closed, I fixed it in a PR that was based on this one.

theblackunknown · 2018-08-27T14:03:55Z

I am fine to have it closed, I didn't touched it in a while.
Thanks @Kangz to have fixed it, I'll give it a try !

Roll third_party/glslang/ 9db7278..f9d08a2 (6 commits) KhronosGroup/glslang@9db7278...f9d08a2 $ git log 9db7278..f9d08a2 --date=short --no-merges --format='%ad %ae %s' 2019-06-18 cepheus Bump revision. 2019-06-17 cepheus AST/SPV: Fix KhronosGroup#930: translate uvec4 <-> uint64 for SubgroupGeMask et. al. 2019-06-18 cepheus Bump revision. 2019-06-17 cepheus SPV: Add a switch for favoring non-NaN operands in min, max, and clamp. 2019-06-17 cepheus Bump revision. 2019-02-17 jbolz Add Float16/Int8/Int16 capabilities for private variables and function parameters Roll third_party/googletest/ 176eccfb8..ee32b72e1 (12 commits) google/googletest@176eccf...ee32b72 $ git log 176eccfb8..ee32b72e1 --date=short --no-merges --format='%ad %ae %s' 2019-06-18 misterg Googletest export 2019-06-17 misterg Googletest export 2019-06-17 misterg Fixing CI break by going to bazel 0.26.1 2019-06-17 misterg Revert "testing, explicitly specify compiler" 2019-06-17 absl-team Googletest export 2019-06-17 misterg Googletest export 2019-06-17 misterg revert travis.yml, irrelevant 2019-06-17 misterg bazel 0.26.1 2019-06-17 misterg bazel 0.26.1 2019-06-17 misterg testing with bazel 0.26.1 2019-06-17 misterg testing with bazel 0.26.1 2019-06-17 misterg testing, explicitly specify compiler Roll third_party/spirv-cross/ 146dc453b..05ea05509 (2 commits) KhronosGroup/SPIRV-Cross@146dc45...05ea055 $ git log 146dc453b..05ea05509 --date=short --no-merges --format='%ad %ae %s' 2019-06-18 post Make sure args.msl22 is set in test_shaders.py. 2019-06-18 post MSL: Fix path check in test_shaders.py. Roll third_party/spirv-tools/ 59983a6..001e823 (5 commits) KhronosGroup/SPIRV-Tools@59983a6...001e823 $ git log 59983a6..001e823 --date=short --no-merges --format='%ad %ae %s' 2019-06-18 afdx Add fuzzer pass to obfuscate constants. (KhronosGroup#2671) 2019-06-17 33432579+alan-baker Handle volatile memory semantics in upgrade (KhronosGroup#2674) 2019-06-17 33432579+alan-baker Validate Volatile memory semantics bit (KhronosGroup#2672) 2019-06-17 33432579+alan-baker Disallow stores to UBOs (KhronosGroup#2651) 2019-06-17 dneto Another fix uint -> uint32_t (KhronosGroup#2676) Created with: roll-dep third_party/effcee third_party/glslang third_party/googletest third_party/re2 third_party/spirv-cross third_party/spirv-headers third_party/spirv-tools

- remove useless std::move on a const std::string variable - remove redundant return at end of void function

Tooling - CMake - Compatible with shared library build

15d7036

Tooling - CMake - More export when building tools

e7997de

Kangz reviewed Oct 30, 2017

View reviewed changes

dj2 closed this Aug 27, 2018

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024

Apply clang-tidy advice (KhronosGroup#930)

011996b

- remove useless std::move on a const std::string variable - remove redundant return at end of void function

Conversation

theblackunknown commented Oct 29, 2017

Uh oh!

CLAassistant commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dneto0 commented Oct 30, 2017

Uh oh!

Kangz Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

Kangz Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

Kangz Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

dneto0 Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

Kangz commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dneto0 commented Oct 30, 2017

Uh oh!

theblackunknown commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theblackunknown commented Oct 31, 2017

Uh oh!

dj2 commented Aug 27, 2018

Uh oh!

Kangz commented Aug 27, 2018

Uh oh!

theblackunknown commented Aug 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Oct 29, 2017 •

edited

Loading

Kangz commented Oct 30, 2017 •

edited

Loading

theblackunknown commented Oct 31, 2017 •

edited

Loading