Cherry picked fixes #1351
Merged
Cherry picked fixes #1351
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Otherwise some compilation units may use the definitions of inline glslang functions from the Vulkan SDK's copy of glslang instead of the glslang we're using, and if the necessary linker warnings aren't enabled and set as errors (they're not), this can cause ABI issues. I noticed this after encountering stack corruption in glslang::GlslangToSpv as the Vulkan SDK version 1.3.296 provides glslang 15.0.0, where glslang::SpvOptions is ten bytes, whereas I was using glslang 14.3.0, where it's only nine bytes, so initialising defaultOptions would spill one byte past the object's end. Fixing the include order ensures all TUs use the right function implementation and the linker won't randomly pick the wrong one.
We unconditionally use binary mode when reading them as we use the header to differentiate between a .vsgt and a .vsgb, and can't read the header without opening the stream first. If the stream mode is mismatched, then this can cause problems. On Unix, there are no symptoms as the only difference is whether a trailing line feed will be inserted if the file is closed and doesn't end with one - some Unices treat all files that don't end in a line feed as binary - and `AsciiOutput::write` adds one after the closing `}` anyway. On Windows, it causes a bigger problem as automatic line ending conversion only happens in text mode - binary streams read and write carriage returns and line feeds as-is, but text streams convert a CRLF sequence to just the line feed when reading and convert a lone line feed to a CRLF sequence when writing. Because of this, when reading a multiline string in a .vsgt file, like a shader listing, on Windows, the CRLF in the file would be read into memory as-is, but if the same object was written back, then the CR would be written, and then the LF would be written as CRLF, leaving CRCRLF in the file. As well as being generally gross, most software for Windows attempts to accommodate files with weird line endings, so will interpret this as a 90s Mac line ending followed by a Windows line ending, and display it as two line breaks. The alternative would be to conditionally enable binary mode after reading the header, but that would mean the stream would need to be closed and reopened, then the header skipped, which would be a nuisance. Developers writing software for Windows are generally ready to deal with explicit carriage returns in strings, so it's fine to load them. Also, this approach is consistent with the text and glsl loaders.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry-picked fixes from my colour correctness branch.
Prepend glslang onto the list of linked libraries so that we prefer the include paths from the version we're linking over the include paths for the Vulkan SDK, which may provide headers for an ABI-incompatible version of glslang, potentially leading to memory corruption etc.
Use binary IO mode when writing
.vsgtfiles, like we already do when loading them, so that on Windows, we don't leave CRLF as-is on load but convert the CR and the LF both to a CRLF sequence on save. Fixing this avoids doubling all line breaks when loading and resaving a.vsgtfile.Clear the SPIR-V vector when recompiling shaders so that the new shader binary doesn't get appended onto the old one, which causes invalid opcode errors.