-
Notifications
You must be signed in to change notification settings - Fork 709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Validator] Check content of PSV0 part in validation #6859
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there is a lot of missing test coverage here. I don't see tests for the various errors. I also think the error messages don't seem to be particularly descriptive, so I'm not even sure how a compiler implementer would interpret the errors and know what was wrong.
aab094c
to
0eb3c2b
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
8da2af4
to
579f5b5
Compare
Replace the memcpy check for PSV0 part. Build DxilPipelineStateValidation object from data in PSV0 part. Then compare the content with information from input DxilModule. Second step for microsoft#6817
6268e44
to
f821b64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does two major things I don't think we should do:
- relaxes ordering requirement for signature elements relative to DxilModule (absolutely should not do this)
- relaxes ordering requirement for resources relative to DxilModule (I don't see any need or advantage to this)
I have other more minor comments, and I haven't completed reviewing some parts and the tests at this point.
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the new code consolidation. I was glad to see how well LoadViewIDStateFromPSV
subbed in for the SimpleViewID class. I still think the error messages are unwieldy, but I'm willing to defer to you there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One potential missing validation:
Where does it make sure the PSV part size is the expected size for the data for that PSV version based on the validator version? Otherwise it could contain extra data tacked on to the end which would get ignored by the validator.
Created #6924 to check PSV part size. |
Replace the memcpy check for PSV0 part.
Build DxilPipelineStateValidation object from data in PSV0 part. Then compare the content with information from input DxilModule.
With this change, the order of signature elements and resources could be different between the container and the DxilModule.
And the StringTable could be in different order as well.
Second step for #6817