Compiling error in ResourceLimits.cpp on macOS

olegded commented 3 years ago

Hi Robert,

I'm seeing the following error while compiling latest master on macOS

Excess elements in scalar initializer

on the line 143 in https://github.com/vsg-dev/VulkanSceneGraph/blob/master/src/vsg/vk/ResourceLimits.cpp:

/* .limits = */ {
    /* .nonInductiveForLoops = */ 1,
    /* .whileLoops = */ 1,
    /* .doWhileLoops = */ 1,
    /* .generalUniformIndexing = */ 1,
    /* .generalAttributeMatrixVectorIndexing = */ 1,
    /* .generalVaryingIndexing = */ 1,
    /* .generalSamplerIndexing = */ 1,
    /* .generalVariableIndexing = */ 1,
    /* .generalConstantMatrixVectorIndexing = */ 1,
}};

As far as I understand the error, it's not macOS-specific but is caused by the failed check on the line 139, cpp file:

#ifdef GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT
        /*.maxDualSourceDrawBuffersEXT =*/1,
#endif

Corresponding header file defines maxDualSourceDrawBuffersEXT unconditionally, so it tries to assign the TLimits struct to maxDualSourceDrawBuffersEXT integer value if GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT is not defined.

How To Reproduce Steps to reproduce the behavior:

Check out latest code
Try to compile it on macOS

I think it can be reproduced on any platform or compiler by undefining GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT.

Expected behavior Error-free compiler run.

Desktop (please complete the following information):

OS: macOS catalina v. 10.15.7
Xcode v. 12.3
clang v. 12.0.0

I'm not sure what the best solution would be. GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT is an optional macro definition. I tried to set the value of maxDualSourceDrawBuffersEXT to 0 in the case GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT is not defined. At least I got running sample, e.g. vsgimgui:

robertosfield commented 3 years ago

I don't have macOS to test so will need you to help resolve the issue.

What version of the VulkanSDK are you using? What version of glslang does it have?

robertosfield commented 3 years ago

FYI, the GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT #define "should" be provided by CMake system. The relevant code is in VulkanSceneGraph/src/vsg/CMakeLists.txt:

if (glslang_FOUND) set(LIBRARIES ${LIBRARIES} PUBLIC ${glslang_LIBRARIES})

set(EXTRA_DEFINES ${EXTRA_DEFINES} HAS_GLSLANG)

include(CheckCXXSourceRuns)

set(CMAKE_REQUIRED_LIBRARIES ${glslang_LIBRARY})
set(CMAKE_REQUIRED_INCLUDES ${glslang_INCLUDE_DIR})

if (NOT GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT)
    check_cxx_source_runs("
        #include <glslang/Include/ResourceLimits.h>

        int main()
        {
            TBuiltInResource resource;
            resource.maxDualSourceDrawBuffersEXT = 0;
            return 0;
        }
    " GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT)
endif()

# check whether glslang/build_info.h exists
if (GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT)
    set(EXTRA_DEFINES ${EXTRA_DEFINES} "GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT")
endif()

install(
    FILES vsg_glslangConfig.cmake
    DESTINATION lib/cmake/vsg_glslang
)

endif()

robertosfield commented 3 years ago

So it would seem the CMake script isn't working as intended. When I implemented it I tried it on a range of VulkanSDK versions with different glslang versions, as well as stand alone glslang builds. I was only able to test under Linux though.

olegded commented 3 years ago

I have installed the latest VulkanSDK available from the LunarG website. VERSIONS.txt file reports

7a313093b5c4af964d50a5a64e73d7df6152ea3f : https://github.com/KhronosGroup/Vulkan-Loader.git
7cf6e361ee511723ae58fc17ffe88f16965be42b : https://github.com/KhronosGroup/Vulkan-ValidationLayers.git
9e9d7c24ed8b321815f77f31c100bbf9462193b4 : https://github.com/KhronosGroup/Vulkan-Tools.git
fda985a6a3f1a29233558976c809656ebc7b92f8 : https://github.com/LunarG/VulkanTools.git
49de6604b0395057e7d3b7ce7001ed29b25708f7 : https://www.github.com/KhronosGroup/MoltenVK.git
c594de23cdd790d64ad5f9c8b059baae0ee2941d : https://github.com/KhronosGroup/Glslang.git
b27b1afd12d05bf238ac7368bb49de73cd620a8e : https://github.com/KhronosGroup/SPIRV-Tools
f027d53ded7e230e008d37c8b47ede7cd308e19d : https://github.com/KhronosGroup/SPIRV-Headers
6d10da0224bd3214c9a507832e62d9fb6ae9620d : https://github.com/KhronosGroup/SPIRV-Cross.git
35ca8c7971db2de65bb4f46fdcb54cbaf36e6c4c : https://github.com/google/shaderc.git

I found the corresponding CMake code but still interpreted it as an optional value according to the implementation.

I will try to find out what is going wrong with CMake.

robertosfield commented 3 years ago

Any chance you didn't do a clean CMake run and it didn't rerun the check_cxx_source_runs()?

olegded commented 3 years ago

Yeah, looks like a small CMake hiccup, clearing cache and re-running helped. I still would say there is a small issue in the impl:

CMake may or may not define it due to whatever reason, e.g. wrong include path or incompatible Vulkan SDK version
Header defines the struct unconditionally
Impl sets the value inside the struct based on the defined macro and fails to compile if the macro is not set

Not sure which solution would be the best one

More prominent error message/warning in CMake

Always assigning some value to maxDualSourceDrawBuffersEXT

#ifdef GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT
/*.maxDualSourceDrawBuffersEXT =*/1,
#else
/*.maxDualSourceDrawBuffersEXT =*/0,
#endif

Do nothing and let the compiler fail, but then it's not really conditional

robertosfield commented 3 years ago

The issue isn't whether 1 or 0 should be passed, but whether that parameter exists in the struct, in the glslang lib, that's what the cmake test attempts to define.

I wonder whether we get get cmake to detect a change in VulkanSDK version and the re-run the GLSLANG_ResourceLimits_maxDualSourceDrawBuffersEXT test.

FYI, I've spent several days over the last year trying to come up with a solution for glslang moving target for this struct. glslang has been changing how version so you can't easily just check a header version and decide whet to do. Some poor decisions on glslang project have made this all more awkward than it should have been.

olegded commented 3 years ago

I would say we should then do nothing and let the compiler fail. I will close the issue.

robertosfield commented 3 years ago

On Wed, 13 Jan 2021 at 17:11, olegded notifications@github.com wrote:

I would say we should then do nothing and let the compiler fail. I will close the issue.

Which compile? The CMake test build or the normal source code build?

Having a normal build fail would be OK if the solution is easy to find. It might be that this issue just occurs when you move from an older VulkanSDK to a newer one, in which case it'll be a rare occurrence for users that have been working with the VSG for a while, but something that wouldn't affect new users.

Assuming this is all reasonable reading of the situation then just closing the Issue should be fine. If further errors occur then we at least know about the nature of it and how to resolve it.

vsg-dev / VulkanSceneGraph

Compiling error in ResourceLimits.cpp on macOS #237