Fixing warnings that occur with stricter warning settings

[robertosfield]

In order to provide runtime support for compiling shader to SPIRV within the VulkanSceneGraph project I've been using to the glslang libraries. I've been doing this four about 4 years now and have to come up with some awkward workarounds for the various versions of glslang + spirv-tools that may existing on different systems, some systems may even have multiple versions installed and suck us into the vortex of dll hell.

Over the past week I've tried a few different approaches to smooth over this issue and the most workable and reliable path seems to be compiling glslang directly as part of the VSG library. It's a bit perverse I know but it looks to be working well across the various platforms. This change simplifies the VSG codebase and build requirements - I've been able to remove ~500 lines of VSG code/cmake scripts that were required to implement the workarounds, and should ensure end users of the VSG get a consistent experience across all platforms.

What I have found when compiling glslang within the VSG is that more strict warnings that we enable have thrown up many warnings from compiling glslang source code. As the VSG normally compiles warning free having a forest of warnings appear due to integrating 3rd party code is a bit of show stopper and if we are to continue down the direct integration route it's something I need to resolve. So I've forked glslang and had bash at resolving the warnings. The fork is:

https://github.com/vsg-dev/glslang

The VSG pulls this in as a submodule and compiles the source files that I've defined in a glslang/build_vats.cmake file. It felt like a bit of long short doing this, but it's actually worked surprisingly smoothly - just the warnings have been a big issue.

To address the warnings I've created a series of branches that address different classes of warnings:

https://github.com/vsg-dev/glslang/tree/shadows_fixes https://github.com/vsg-dev/glslang/tree/fallthrough_fixes https://github.com/vsg-dev/glslang/tree/fixes_for_uninitialized_warnings

I am still working on a few more warnings so this work isn't complete yet, but mostly done. I'm testing things with g++ 11.3.0, and the compile options I've added from the VSG side are:

add_compile_options(-Wall -Wparentheses -Wno-long-long -Wno-import -Wreturn-type -Wmissing-braces -Wunknown-pragmas -Wmaybe-uninitialized -Wshadow -Wunused -Wno-misleading-indentation -Wextra)

Most of the warning fixes are just tightening up a few ambiguities that probably won't affect runtime behavior, there are a few fixes that are likely bug fixes though so definitely worth pulling into the main glslang. To do this warning fixes work I had to trawl through a wide range of glslang code, and this is the first time I've dived into the source code base so there is a chance that I've misunderstood things and my resolution of issues highlighted by the warnings could be wrong. To make sure I haven't introduced some regression ever single line changed will need a review.

There was a few practices in the code base that I was troubled by, the compiler warnings were saying jikes too, so I think it would be worthwhile making stricter warning standard for glslang so help raise a red flag in code early in it's development, rather than perhaps years down the line when 3rd parties are trying to figure out the intent of the original programmer. After getting over the initial bump of fixing old code, work going forwards can be much more sane.

If necessary I'll maintain my own glslang fork for the purposes of including as part of the VSG build, but I'd rather keep any refinements I make, such as warning fixes, merged with the main glslang repo. Once I've completed my warning fixes sprint I ot the VSG community to complete build testing of changes I can generate PR for the above and further warning fixes.

I should wrap up my work tomorrow so can't provide the changes this week or hold off or make changes based on suggestions, just let me know how you'd like me to proceed.

Thanks, Robert.

[robertosfield]

I have fixed a few more warning and checked them in, much smaller set of warnings with these than previous changes.

https://github.com/vsg-dev/glslang/tree/type_conversion_fixes https://github.com/vsg-dev/glslang/tree/unused_function_fixes

I'm happy to walk through the changes.

I have tested the VSG's vsgconv utility that is able to compile .glsl shaders to .spv files when using the glslang in VuikanSDK-1.3.239.0 vs the VSG built integrating the glslang I have forked with the above warning fixes. The .spv files were identical, while not proof the warnings are without error it is at least a small bit of comfort.

[robertosfield]

I have also added CMake autogenerated files to .gitignore so that git status now just reports the changed files:

https://github.com/vsg-dev/glslang/blob/main/.gitignore

[arcady-lunarg]

Thanks for trying to fix all those warnings. When you make the changes, it would be a good idea to also run the same testing that is run in CI. You need to follow the steps in the README to checkout a copy of googletest into the External/ directory and then, once you reconfigure/rebuild glslang, you can use the "test" build target to run the full test suite, which is reasonably comprehensive. Once you've made sure that those tests all pass, we would be happy to review your PRs.

[robertosfield]

I haven't tried running the tests yet, I will do that once I have a quite half hour, but this might take till next week as right now I'm busy working towards a stable release of the VulkanSceneGraph.

[AnyOldName3]

For anyone who finds this issue, I did some work on this a few weeks ago as https://github.com/KhronosGroup/glslang/pull/3517, and the consensus among the glslang team was that -Wshadow was too draconian, so glslang will continue not using it. The reason it's causing bother for the VulkanSceneGraph is that it's setting warning flags globally instead of just for its own targets, so they end up affecting its dependencies, too. This is generally a bad idea, and you're only supposed to set compiler flags globally through CMake if they affect linking so really do need to be global. Possible solutions would be changing the VSG's CMake to not set these flags globally, or changing it so that it sets them globally only after adding glslang. I intend to do this once the other problems I've discussed in https://github.com/KhronosGroup/glslang/issues/3509 are resolved.