microsoft / microsoft/DirectXShaderCompiler

Revise extra metadata error reporting in DxilMetadataHelper

Open
#5,723 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

tech-debt validation
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() and LoadDxilMetadata() 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.