Shared library use not first-class functionality, not tested

[jengelh]

Compilation fails in commit e7df8e0; it used to previously work in a515250 (where essentially no libHLSL.so was generated).
Please specify the dependencies of HLSL properly.

/usr/bin/cmake . '-DCMAKE_SHARED_LINKER_FLAGS=-Wl,--no-undefined' -DBUILD_SHARED_LIBS:BOOL=ON

-- The C compiler identification is GNU 8.1.1
-- The CXX compiler identification is GNU 8.1.1
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Google Mock was not found - tests based on that will not build
-- spirv-tools not linked - illegal SPIRV may be generated for HLSL
-- Configuring done
-- Generating done

$ make VERBOSE=1
[...]
/usr/bin/c++ -fPIC  -Wl,--no-undefined -shared -Wl,-soname,libHLSL.so -o libHLSL.so CMakeFiles/HLSL.dir/hlslAttributes.cpp.o CMakeFiles/HLSL.dir/hlslParseHelper.cpp.o CMakeFiles/HLSL.dir/hlslScanContext.cpp.o CMakeFiles/HLSL.dir/hlslOpMap.cpp.o CMakeFiles/HLSL.dir/hlslTokenStream.cpp.o CMakeFiles/HLSL.dir/hlslGrammar.cpp.o CMakeFiles/HLSL.dir/hlslParseables.cpp.o 
/usr/lib64/gcc/x86_64-suse-linux/8/../../../../x86_64-suse-linux/bin/ld: CMakeFiles/HLSL.dir/hlslParseHelper.cpp.o: in function `glslang::HlslParseContext::initializeExtensionBehavior()':
hlslParseHelper.cpp:(.text+0x59a): undefined reference to `glslang::TParseVersions::initializeExtensionBehavior()'
/usr/lib64/gcc/x86_64-suse-linux/8/../../../../x86_64-suse-linux/bin/ld: CMakeFiles/HLSL.dir/hlslParseHelper.cpp.o: in function `glslang::HlslParseContext::parseShaderStrings(glslang::TPpContext&, glslang::TInputScanner&, bool)':
[...]

[johnkslang]

As stated in #1421, maintenance of dynamic library builds is currently a non goal: It has no testing and most input had been to use glslang only with static linking. Negative judgments of a tool against goals it does not have does not help.

That being said, if there are users that want to contribute fixes for this space, or otherwise enable its support, I am happy to help fix glslang per those contributions. Or, if it is truly a general need, figure out how to more officially add and test this need.

File locations, and many other legacy constructs adopted into glslang from its history, are likely relied on by users and so we need to cautious about changing things around (much as I'd like to). So, contributions that work without breaking things will be easier to get in.

[johnkslang]

Let's get down to specifics, and stay on topic with this issue.

[The referenced commits (e7df8e0 and a515250) don't seem to have much to do with the reported behavior change, and some of the other criticisms seem triggered by guesses.]

The error glslang::TParseVersions::initializeExtensionBehavior() seems more to do with the addition of a virtual method override, which passed all the Linux, Mac, and Windows build bots. On the other hand, commit e7df8e0 added use of the validator and seems completely unrelated.

With the issue being about something that is not tested by the normal configuration, it will help to:

1. identify when things really broke
2. identify what settings you are using that detect the break, despite the normal settings being happy

[jengelh]

Oh - I did not intend to mean e7df8e0 was the culprit; I merely tested these two commits, so the a51..e7d range is still subject to a bisection.

[johnkslang]

This is pending any contribution or constructive suggestion of how to advance to better support of shared linkage objects.

[v-fox]

With the issue being about something that is not tested by the normal configuration, it will help to:

1. identify when things really broke

2. identify what settings you are using that detect the break, despite the normal settings being happy

The "settings we are using that detect the break, despite the normal settings being happy" is what all distributions consider proper practice, such as these automated openSUSE builds (see build logs on the right). Notice how I'm the only person on OBS who even bothered trying building vulkan, spirv-{headers,tools}, glslang, shaderc past the breakage spiral. That is because you:

• don't version your API and put proper SO-versions in libs;
• don't define API dependencies, let alone build ones;
• put untested half-backed crap into master branches that doesn't belong there;
• have no concept of release cycle and feature-targeting from roadmap goals (you are NOT supposed to merge commits of entire features BEFORE testing them out in THEIR ENTIRETY in a development branch that is synced with master, especially if you haven't even defined their dependencies !);
• probably, don't even have roadmap or any idea what exactly are you trying to achieve;
• have no concept of coding style, any idea of good & bad coding & maintenance practices.

This is pending any contribution or constructive suggestion of how to advance to better support of shared linkage objects.

Because of that and your insistence on staying in your "safe-space" echo-chamber that as much of "constructive suggestions" that you're going to get. No one wants to deal with that and with you. Bisecting your code is even bigger fool's errand than trying to compile supposed "release" shapshots with your abnormal "normal configuration".

PS: same thing happened in distributions with support for horrible mess of android-tools, by the way. Although, with Mesa being dependant on vulkan, maintainers will persevere somehow this time.

[johnkslang]

I see several straw man fallacies, some religion, and can guess how you might jump to some of the non-religious conclusions, and yes there is a partially complete process of adapting a Windows' static-linking design to a Linux SO design (contributed by others), while not breaking existing use (which currently works on Max/Linux/Windows).

Rather than argue religiously in the abstract, it will be best to focus on specific issues and constructive action.

Is there a specific action to improve the semantic versioning being used?
Can you contribute the dependencies you want to see?
Is something currently in master that should not be?

(Note some large Khronos-provided features were developed and tested in a fork before appearing here, so appearances are different than reality.)

[jengelh]

Is there a specific action to improve the semantic versioning being used?

That semantic version should be added to the filename; instead of producing (just) libHLSL.so, it should be called libHLSL.so.7. AIUI cmake has something like that in the form of set_target_properties(HLSL PROPERTIES VERSION 7.9.2888 SOVERSION 7).

[akien-mga]

I can understand the frustration of package maintainers trying to build glslang as a shared library, as that's what I'm doing now and I spent a couple hours working on fixes in that direction already.

Yet I don't think language as seen in #1484 (comment) is constructive, and throwing a bunch of complaints on a generic issue isn't either.

So back to issue at end (the OP: #1484 (comment)):

This happens because the glslang and HLSL both have a weak dependency on each other (cyclic dependency). HLSL doesn't specify it, which causes this link error when preventing underlinking issues using the link flags -Wl,--as-needed -Wl,--no-undefined.

This patch would solve the underlinking issue (i.e. make HLSL's dependency on glslang explicit):

diff --git a/hlsl/CMakeLists.txt b/hlsl/CMakeLists.txt
index f918d7a1..5696afd8 100644
--- a/hlsl/CMakeLists.txt
+++ b/hlsl/CMakeLists.txt
@@ -27,6 +27,8 @@ if(WIN32 AND BUILD_SHARED_LIBS)
     set_target_properties(HLSL PROPERTIES PREFIX "")
 endif()
 
+target_link_libraries(HLSL glslang)
+
 if(WIN32)
     source_group("Source" FILES ${SOURCES} ${HEADERS})
 endif(WIN32)

But then another issue comes up:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "glslang" of type SHARED_LIBRARY
    depends on "HLSL" (weak)
  "HLSL" of type SHARED_LIBRARY
    depends on "glslang" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

Cyclic dependencies are not allowed in shared libraries, so there's no easy fix. The proper fix would be to remove the cyclic dependency, probably by removing glslang's dependency on HLSL somehow.

In the meantime, the workaround for distro packagers is to forego their distro's library policy and let the underlinking go through (so remove -Wl,--no-undefined from the link flags).

[akien-mga]

With the attached patch, HLSL links properly, but glslang then has its own linking issues. They're a lot less than what HLSL complains about though (hundreds of glslang symbol calls).

/usr/bin/ld: CMakeFiles/glslang.dir/MachineIndependent/ShaderLang.cpp.o: in function `(anonymous namespace)::CreateBuiltInParseables(TInfoSink&, glslang::EShSource)':
/home/akien/Mageia/Sandbox/_rpm/BUILD/glslang-7.10.2984/glslang/MachineIndependent/ShaderLang.cpp:89: undefined reference to `glslang::TBuiltInParseablesHlsl::TBuiltInParseablesHlsl()'
/usr/bin/ld: CMakeFiles/glslang.dir/MachineIndependent/ShaderLang.cpp.o: in function `ShInitialize':
/home/akien/Mageia/Sandbox/_rpm/BUILD/glslang-7.10.2984/glslang/MachineIndependent/ShaderLang.cpp:1276: undefined reference to `glslang::HlslScanContext::fillInKeywordMap()'
/usr/bin/ld: CMakeFiles/glslang.dir/MachineIndependent/ShaderLang.cpp.o: in function `ShFinalize':
/home/akien/Mageia/Sandbox/_rpm/BUILD/glslang-7.10.2984/glslang/MachineIndependent/ShaderLang.cpp:1378: undefined reference to `glslang::HlslScanContext::deleteKeywordMap()'
/usr/bin/ld: CMakeFiles/glslang.dir/MachineIndependent/ShaderLang.cpp.o: in function `(anonymous namespace)::CreateParseContext(glslang::TSymbolTable&, glslang::TIntermediate&, int, EProfile, glslang::EShSource, EShLanguage, TInfoSink&, glslang::SpvVersion, bool, EShMessages, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)':
/home/akien/Mageia/Sandbox/_rpm/BUILD/glslang-7.10.2984/glslang/MachineIndependent/ShaderLang.cpp:116: undefined reference to `glslang::HlslParseContext::HlslParseContext(glslang::TSymbolTable&, glslang::TIntermediate&, bool, int, EProfile, glslang::SpvVersion const&, EShLanguage, TInfoSink&, std::__cxx11::basic_string<char, std::char_traits<char>, glslang::pool_allocator<char> >, bool, EShMessages)'

commit 9dde31a801343121c9aec1803f6410dea91cda51
Author: Rémi Verschelde <rverschelde@gmail.com>
Date:   Mon Dec 10 13:56:03 2018 +0100

    CMake: Properly link HLSL against glslang
    
    The HLSL library was using glslang symbols without linking against it
    formally. glslang on the contrary was linking against HLSL for no reason.
    
    Underlinking error spotted with GCC's '-Wl,--as-needed -Wl,--no-undefined'.

diff --git a/glslang/CMakeLists.txt b/glslang/CMakeLists.txt
index 5f51476b..ba7268c4 100644
--- a/glslang/CMakeLists.txt
+++ b/glslang/CMakeLists.txt
@@ -92,10 +92,6 @@ if(WIN32 AND BUILD_SHARED_LIBS)
     set_target_properties(glslang PROPERTIES PREFIX "")
 endif()
 
-if(ENABLE_HLSL)
-    target_link_libraries(glslang HLSL)
-endif()
-
 if(WIN32)
     source_group("Public" REGULAR_EXPRESSION "Public/*")
     source_group("MachineIndependent" REGULAR_EXPRESSION "MachineIndependent/[^/]*")
diff --git a/hlsl/CMakeLists.txt b/hlsl/CMakeLists.txt
index f918d7a1..5696afd8 100644
--- a/hlsl/CMakeLists.txt
+++ b/hlsl/CMakeLists.txt
@@ -27,6 +27,8 @@ if(WIN32 AND BUILD_SHARED_LIBS)
     set_target_properties(HLSL PROPERTIES PREFIX "")
 endif()
 
+target_link_libraries(HLSL glslang)
+
 if(WIN32)
     source_group("Source" FILES ${SOURCES} ${HEADERS})
 endif(WIN32)

[jengelh]

Cyclic dependencies are not allowed in shared libraries, so there's no easy fix.

Generally speaking, there are two trivial (or let's say "default") ways of addressing such.

1. Merge the two libraries into one build target, or
2. Finding the intersection (usually the cycle in the graph) and factoring that out into a third library, then required by the other two.

While --no-undefined is a possible workaround, that would only work for ELF and not for PE platforms as I understand it.

[ben-clayton]

#2271 fixes building of shared libraries.

#2277 (draft) starts to tackle versioning, including SONAME.

[arcady-lunarg]

This is becoming first class functionality now and is even tested in CI to some extent, though Windows doesn't yet have testing in CI and there isn't ABI compatibility testing yet either. We'll track the remaining few things that need to be done under #2283.