KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
3.07k stars 839 forks source link

RFC: Changes to the glslang API/ABI #3320

Closed arcady-lunarg closed 1 month ago

arcady-lunarg commented 1 year ago

Given the difficulty of maintaining ABI compatibility when all the internals of the compiler are exposed as a public API, I propose the following changes to the way that glslang is built.

  1. The final result of the build of the compiler is one library, libglslang.so or libglslang.a as appropriate for the type of build, and the 'glslang' binary. Likewise, for spirv-remap there would be one library, libSPVRemapper.so (or libSPVRemapper.a) and the spirv-remap binary.
  2. The only header files installed by the build process would be:
    Public/ResourceLimits.h
    Public/ShaderLang.h
    Public/resource_limits_c.h
    Include/glslang_c_interface.h
    Include/glslang_c_shader_types.h
    Include/ResourceLimits.h
    MachineIndependent/Versions.h
    SPIRV/GlslangToSpv.h
    SPIRV/disassemble.h
    SPIRV/SPVRemapper.h (for the remapper)
  3. The only entrypoints exposed by libglslang will be the ones declared in the above header files (except the last one, which declares the entry points for libSPVRemapper). The API and ABI for these will be kept stable, with the SOVERSION being incremented anytime there is a backward-incompatible change. We will add a CI hook to use libabigail to ensure that no backwards-incompatible changes are made.
  4. If for some reason you want to use the internals of glslang (for example, just the AST, or just the SPIR-V emitting backend), you are welcome to a) include glslang as a subproject in your project, and b) link against it statically. These internals are subject to binary-incompatible changes at any time, though we will try to maintain source compatibility.
arcady-lunarg commented 1 year ago

CC @rhabacker @jengelh @ben-clayton @haasn @SpaceIm as people whose names have appeared in issues related to shared library builds, I would be curious to hear your opinions on this.

haasn commented 1 year ago

This would work for my use case. I only need TShader and GlslangToSpv, pretty much.

jengelh commented 1 year ago

Not (actively) maintaining compatibility like right now is a possibility too; the file just needs to be marked such that it appears to be "different" often enough so the ELF loader can reject possibly-incompatible library<>program combinations.

exempli gratia
# can't rely on .git/ dir existing, so no git commit id available
cs=$(find * -type f ( -name "*.[ch]" -o -name "*.cc" ) -exec md5sum {} + | cut -b 1-32)
echo "$cs { global: *; };" >x.ver
gcc -o libglslang.so -Wl,--version-script=x.ver *.o
rhabacker commented 1 year ago

If for some reason you want to use the internals of glslang (for example, just the AST, or just the SPIR-V emitting backend), you are welcome to a) include glslang as a subproject in your project, and b) link against it statically.

I think that this is what @robertosfield does with https://github.com/vsg-dev/glslang for vsg.

rhabacker commented 1 year ago

The reason for this usage was

ensures that all platforms have the same shader compilation features available at all times.

see https://github.com/vsg-dev/VulkanSceneGraph/discussions/738

Would this requirement also be covered by the topics mentioned at https://github.com/KhronosGroup/glslang/issues/3320#issue-1867345751 ?

robertosfield commented 1 year ago

Simplifying the public interface of gllang and making it easier maintain are worthy goals, the suggested changes look solid to me.

FYI, the only public headers that the VulkanSceneGraph directly includes are:

#if VSG_SUPPORTS_ShaderCompiler
#    include <SPIRV/GlslangToSpv.h>
#    include <glslang/Public/ResourceLimits.h>
#    include <glslang/Public/ShaderLang.h>
#endif

The code itself: https://github.com/vsg-dev/VulkanSceneGraph/blob/master/src/vsg/utils/ShaderCompiler.cpp#L22

Currently the VSG uses a local clone of glslang simply because it compiles the library directly so I fixed dozens of compile warnings that otherwise would have swamped the VSG. Perhaps if glslang is clean enough and consistently distributed enough we can drop the whole local compile workaround that I had to implement to keep things compiling across platforms.

arcady-lunarg commented 1 year ago

Yes, we are interested in fixing the compiler warnings as well, and will welcome PRs fixing warnings and turning on warning flags. I have a private branch somewhere fixing various warnings but you are more than welcome to submit your own for the warnings you care about.

arcady-lunarg commented 2 months ago

I forgot that this issue was still open, but for anyone following it, it's worth mentioning that libglslang has been made into the primary library for everything in glslang as of #3698, and libSPIRV.so as well as libMachineIndependent.a are now just stubs. This will make it easier to enforce proper symbol hiding. #3705 added some visibility annotations to the functions in the SPIRV/ directory, and there will likely be another PR to add some more visibility annotations elsewhere. After that, we will try to provide a stable ABI within a glslang major version.

arcady-lunarg commented 1 month ago

The final changes to enable symbol visibility went out in #3732 and all the pieces of glslang should be buildable as shared libraries now, with the intention to maintain a stable ABI as defined by the functions and classes tagged with GLSLANG_EXPORT in the publicly installed headers. We still don't have tests to check that we don't break the ABI, but those are next on the list.

arcady-lunarg commented 1 month ago

This work has now been completed: the set of installed headers has been pruned to those that define the public API, and when glslang is built as a shared library, only symbols that are part of the API and tagged with GLSLANG_EXPORT are exported from the shared library. There are a couple of exceptions, which are tagged with GLSLANG_EXPORT_FOR_TESTS and thus end up exported from the shared libraries but are not in the public headers and thus not considered part of the public API or ABI.