Fix use of relative include paths in libspirv.h about spirv-tools HOT 11 CLOSED

No need for any local modifications to spirv.h (quite the opposite, in fact), but SPIRV-Tools is tightly coupled to a specific SPIR-V revision. If we created ambiguity about where spirv.h comes from, I'm worried we'd expose ourselves to the very unpleasant risk of version drift. We've already had the issue crop up with dEQP's use of SPIRV-Tools.

Therefore, even if we removed "headers" from the path, the enclosing project would either still need to set up the include search path to the SPIRV-Tools copy of spirv.h OR to constantly monitor for version drift. Seems like an unappetizing prospect.

from spirv-tools.

After filing this issue I hit an issue with this that made me question why the headers are included in the public libspirv header at all. The spirv headers in the khronos registry all use the same ifndef guard, meaning that including libspirv.h which pulls in spirv.h as well as including the C++11 spirv.hpp11 will cause issues.
It seems like the only use of the headers in the public libspirv header is SpvOp opcode; in spv_parsed_instruction_t. This being a uint32_t would remove the need to expose which headers are being used at all, or shipping the headers with the library. Given the version drift concerns it seems ideal to remove the includes (since it's a C-style enum it's not getting proper type checking that benefits from the typedef anyway). So long as consumers of the library have opcodes that match those used by the library (which should never change now, given the finalized spec), things will work. This is implicitly what would happen with anyone using the library via FFI to python/etc.

Usability of the library without installation (via git submodule/etc) would be easier and cleaner if projects were not required to add two separate include paths to non-root-level paths within the repository and couldn't run into these potential conflicts and versioning issues. There are many ways to solve this, I merely wanted to raise the issue :)

from spirv-tools.

Good catch on the header guards -- that's something to be addressed.

Long term, of course, the plan is for C++ clients to use a new C++ client envisioned by issue #2 and not need to include libspirv.h.

from spirv-tools.

I'll see about fixing the header guards upstream.

from spirv-tools.

As for the inclusion of spirv.h at all:

We plan to expose more functionality in libspirv.h, e.g. looking up strings for enum values, and possibly vice versa. For example, see requests from #4.
It's generally useful for SPIR-V manipulators. So we'll want the standard enum definitions.

Yes, we also plan to expose a C++ API. That should help some of this.

from spirv-tools.

Thanks for fixing the guards, that'll be helpful!

@dneto0: I do believe there's a conflict with that approach, though.

• For consumers of SPIRV-Tools by way of binaries (as/dis/val) none of this matters.
• For consumers of SPIRV-Tools by internal use of the non-public API the requirement to include the SPIRV-Tools headers (even indirectly) makes sense. The SPIRV-Tools will need to be kept in sync with the consuming library with respect to spirv.h version.
• For consumers of SPIRV-Tools by way of the public API (libspirv.h or whatever C++ equivalent is created) this is an issue.

Basically, if SPIRV-Tools exposes a particular version of spirv.h in its public API it is making any consuming code use that exact version of spirv.h. This means that if I had some C library that included libspirv.h I would not be able to have my own spirv.h anywhere in a way that would not cause fragile builds. I would have to use the SPIRV-Tools version, even if I didn't need or want it. If my code was also pulling in a few other libraries written in the same fashion I'd be in the #include equivalent of DLL hell.

This causes issues like:

• mylib includes libspirv.h and the spirv.h from the khronos registry, both v1.0
• SPIRV-Tools upstream switches to v1.1 of spirv.h
• mylib updates SPIRV-Tools to get some bug fixes
• mylib now sometimes uses the v1.0 spirv.h (when libspirv.h is not included first) and sometimes uses v1.1 (when libspirv.h is included first)

Or:

• mylib includes libspirv.h and the spirv.h from the khronos registry, both v1.0
• mylib switches to v1.1 of spirv.h
• mylib and libspirv.h both now use the v1.1 spirv.h (when spirv.h is included first) and other times v1.0

Of course, real projects are more complex:

• mygame includes mylib (which includes SPIRV-Tools), bgfx, glslang, etc who all expose their own spirv.h versions
• good luck figuring out what SpvOp means in any given file

So if you're concerned with version drift then including your own headers/ copy doesn't solve it and actually makes things more difficult on consumers. Removing the "headers/" from the includes and always taking what is on the include path at least makes things predictable (in that I won't get a random spirv.h version depending on include orders).

The version drift concerns only really impact attempting to use older headers than SPIRV-Tools is using. The SPV_VERSION/SPV_REVISION can be used to statically assert that a header >= the required version is used. I'd hope the SPIRV maintainers would prevent the release of a new header that changes opcode values and breaks every binary-serialized shader in existence :)

And, as I mentioned, anyone using the libspirv.h public API through an FFI (lua, python, .net, etc) will not be using your spirv.h header so any reliance on that exact header version in non-forward-compatible ways will make using the library in those environments impossible. When using an FFI this is often checked with an init method that takes the version and performs a >= check (spvContextCreate would be a good place to do this).

Of course, if I was using spirv.h v1.1 and generating opcodes that an older version of SPIRV-Tools I was using couldn't handle I'm going to have a bad time, but this is more about not requiring complex projects made of multiple nested libraries to be forced to sync with each other simultaneously at all times.

(this whole issue is why in google-land we only allow one version of each header to exist and use absolute paths for everything. Doesn't work too well unless all code is in one repo, though :)

from spirv-tools.

Ok.

Part 1: I agree the "headers/" part of the include path is a bit strange. Other Khronos APIs tend to reserve a directory to themselves, e.g. gl/glut.h or CL for OpenCL (See https://www.khronos.org/registry/cl/api/2.1/opencl.h). Even though I'd be tempted to have "spirv/spirv.h", "spirv/OpenCL.std.h" and "spirv/GLSL.std450.h" I don't think that's been established for SPIR-V. So we could just strip the "headers" part.

Part 2: What is a friendly way to establish the include directory in CMake? Right now both include/ and external/include are added as PUBLIC include dirs. Studying the CMake docs, I think this is right for the spirv.h, since that header is required to compile SPIR-V Tools itself, and also required for correct usage of the SPIR-V Tools public API due to the use of the SpvOp enum.
(The GLSL.std450.h and OpenCL.std.h are not needed for the public API, so their includes should probably be removed from libspirv.h.)

So the proposed fix is:

• Change libspirv.h from #include "headers/spirv.h" to #include "spirv.h"
• Change the target_include_directories in CMakeLists.txt so it includes ${CMAKE_CURRENT_SOURCE_DIR}/external/include/headers instead of ${CMAKE_CURRENT_SOURCE_DIR}/external/include
• Opportunistically remove the inclusion of OpenCL.std.h and GLSL.std450.h from libspirv.h

Does that look right to you?

Do you think we should force the standard headers into a "spirv" directory, addressing Part 1 above?

The open issues are:

• Clients must not accidentally include the wrong spirv.h
• In the scenarios you've listed above, the clients have a fighting chance at getting what they want. Any complexity due to version skew is entirely up to them to resolve. But they have the expressive power to do so. At least you have the ability to non-interfering components that use different versions of spirv.h

from spirv-tools.

Oo good point about a spirv/ directory. It's consistent with the other Khronos APIs so maybe that's the best course; every API I'm seeing in the registry (ES, EGL, etc) has a directory prefix of their name or of KHR.

The CMake changes seem legit. For consumers who are not using CMake/your CMakeLists.txt it's their job to either add that include path or have already added a path with spirv.h to their include list (which they are likely doing already or would have had to do with the old system as well). In many cases someone wouldn't have to do anything - just #include "full_path_to/libspirv.h" and then already have spirv.h somewhere and be done, no include wrangling required. Cool!

For the version issue, adding some static checks (done with preprocessor macros and #error) to verify that a minimum spirv.h version is present would provide useful diagnostic information to people in the case of their header being out of date (no missing enums, but a "SPIRV-Tools requires a spirv.h of at least version X").

As an aside, it seems like Vulkan uses the fact that an #include with quotes will first look for files side-by-side with the given file before going to include paths. If you had the files you wanted side-by-side (or in a subtree) then you could exploit that. It doesn't help the header-guard exclusion of the shared spirv.h header, but if you find yourself adding more headers in the future it's useful!

from spirv-tools.

We are now most of the way there:

All headers are under {SPIRV source dir}/include, with subdirectories for spirv (official headers), and spirv-tools (public API).
Also, libspirv.h no longer #include's the official spirv headers.

But still that means that a client project using the public API will likely inadvertently pick up the public headers from the SPIRV-Tools project, since your -I directive.

The SPIR-V headers are now published in https://github.com/KhronosGroup/SPIRV-Headers
We plan to switch SPIRV-Tools to require that you have that project checked out, and SPIRV-Tools will use the headers from there, and will pick up the specific versions it's coded against. (As of this morning SPIR-V has 1.1 and 1.0 headers for the core instruction set.)

from spirv-tools.

PR #186 aims to switch to use https://github.com/KhronosGroup/SPIRV-Headers.

from spirv-tools.

I think all issues have been resolved:

• SPIRV-Tools itself builds against an external copy of SPIRV-Headers
• libspirv.h no longer includes spirv.h. (We changed opcode to use uint16_t. It weakened the type safety, yes, but I think we deemed that ok.)

Closing

from spirv-tools.