microsoft / microsoft/DirectXShaderCompiler
Revise extra metadata error reporting in DxilMetadataHelper
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 3.7k
- Forks
- 900
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 44
Description
Situation
When DxilMetadataHelper encounters extra tags in extended attributes, these should not be treated as a fatal error, since drivers using DxilMetadataHelper should be able to safely ignore extra tags as long as they support the shader model. However, these should be recognized by the version of the validator against which the shader was validated. That means tags unrecognized by the validator need to result in a validation failure.
The way this is handled today is to DXASSERT to attempt to catch this if it happens at development time, then set a m_bExtraMetadata flag which will let the validator know extra metadata was defined.
There are two problems with this approach. First, the presence of the DXASSERT prevents tests from testing this code path for the validator, without breaking on debug (or assert enabled) builds. Second, the extra metadata error provides no context for where this extra metadata was found, requiring a debugging session to find the source of the problem.
Proposal
I propose using a different mechanism to capture the situation with extra metadata, which can capture information about the location of the metadata, enabling the validator to emit a more useful error message. No assert would be used, but since reporting would be centralized, you could still insert a breakpoint to easily debug it if needed.
Since extended metadata lists are used in a variety of places, more than just the type of extended metadata needs to be reported in order to better pinpoint the extra metadata value. Thus, I also propose a mechanism of capturing context while loading different lists or objects, so when an error occurs, a path to that metadata is captured for the validator to emit later.
This mechanism should also be light-weight, avoiding unnecessary overhead for other users of DxilMetadataHelper when it doesn't need to capture error details for the validator.
This mechanism can also be used beyond the validator, for instance, when loading modules for linking or optimization, we should likely warn about unrecognized metadata that will be ignored and will not be preserved through the deserialization/serialization process.
Proposed Rough Design
Interface for DxilValidation to use:
- Modifications to
TryGetDxilModule()andLoadDxilMetadata()to add optional pointer to output error list.
MetaErrorContext:
- capture a string for context at this point on the stack
- has pointer to prior context and head pointer to create context stack
- used in RAII pattern to push error context:
auto errCtx = PushErrorContext("SRVs");
DxilMetadataHelper:
- at various locations, push error context
- report unknown metadata tag through centralized method, which constructs the final message from the context and the additional tag information passed.
- When optional error list output pointer is not supplied, skip context capture, and skip processing and string construction early to avoid needless overhead.
Implementation
There's an implementation started on this branch:
https://github.com/tex3d/DirectXShaderCompiler/tree/metadata-error-reporting
It's code-complete, barring any desired design changes, but tests still need to be written.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Review the metadata-error-reporting branch and the proposed TryGetDxilModule() and LoadDxilMetadata() changes first. Add tests for extra metadata validation without assertion failures and for reporting useful metadata context. Done means the validator rejects unrecognized tags with actionable location details while callers without an error-list output retain the lightweight path.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers, testing-qa
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100