Skip to content
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

[d3d9] Implement several IDirect3DShaderValidator9 validations #4398

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

WinterSnowfall
Copy link
Contributor

@WinterSnowfall WinterSnowfall commented Oct 24, 2024

Fixes #2045.

(Working) rebase of @misyltoad's former d3d9-shader-val2 branch, which doesn't crash on the main menu with The Void. On top of it I've added a crude adaptation of @gofman's PS input counts validation. It seems to work fine in terms of actually doing what's expected of it , but it doesn't currently fix the crashing issues in the game.

A save file that can reproduce the crashing can be found here.

@WinterSnowfall
Copy link
Contributor Author

WinterSnowfall commented Oct 24, 2024

I've had to comment out:

      // + 1 to account for the opcode...
      uint32_t dwordLength = m_ctx->getInstructionContext().instruction.tokenLength + 1;
      if (cdw != dwordLength) {
        return ErrorCallback(pFile, Line, 0x2,
          D3D9ShaderValidatorMessage::BadInstructionLength,
          str::format("Instruction length specified for instruction (", cdw, ") does not match the token count encountered (", dwordLength, "). Aborting validation."));
      }

because on many occasions it seems like:

  • the dwordLength for mov, mul, mad and dp4 is -1 vs cdw
  • the dwordLength for tex is -2 vs cdw

That causes the game to crash before getting to the main menu. @misyltoad I hope you have a better memory than I do and maybe have a clue what's going on here?

Using: if (cdw < dwordLength) also works, but I'm not sure if that's the intended behavior here.

Nevermind, not even that does. The game crashes later on, so it seems like there's no guarantee that cdw and dwordLength are equal, though most of the time they are.

@WinterSnowfall WinterSnowfall force-pushed the d3d9-thevoid branch 5 times, most recently from 39aa686 to 2ca49a5 Compare October 24, 2024 22:26
@WinterSnowfall
Copy link
Contributor Author

@Blisto91 Temba, his arms wide.

Seems to not crash for me now in the problematic parts. Please also test when you can.

@WinterSnowfall WinterSnowfall force-pushed the d3d9-thevoid branch 4 times, most recently from 61e342b to 76d7f2c Compare October 25, 2024 20:29
@WinterSnowfall
Copy link
Contributor Author

Left the BadInstructionLength validations commented out and with a TODO, since I'm not sure what's going on there. Perhaps it's something we can improve upon in the future, if any game actually cares.

I'd welcome more testing, but at this stage it seems to be enough for The Void. The risk of breaking other stuff I think is rather minimal, since all other validations deal with shader type/structure validations and IDirect3DShaderValidator9 call order.

@WinterSnowfall WinterSnowfall marked this pull request as ready for review October 26, 2024 08:41
@WinterSnowfall WinterSnowfall marked this pull request as draft October 26, 2024 18:57
@WinterSnowfall
Copy link
Contributor Author

WinterSnowfall commented Oct 26, 2024

Act of War - Direct Action tries to use register input number 11 in a PS and expects that to work, otherwise it hangs before starting a game. Back to the drawing board, I guess.

Edit: The game uses input register index 11 in a V1.1 PS. I guess I should actually restrict the check to PS 3.0, as per intended design, duh.

@WinterSnowfall WinterSnowfall marked this pull request as ready for review October 26, 2024 20:06
@WinterSnowfall WinterSnowfall force-pushed the d3d9-thevoid branch 3 times, most recently from 8e2f2f3 to 899edb2 Compare October 26, 2024 22:45
@WinterSnowfall WinterSnowfall marked this pull request as draft October 27, 2024 12:59
@WinterSnowfall WinterSnowfall force-pushed the d3d9-thevoid branch 2 times, most recently from 337926c to 5154ae7 Compare November 9, 2024 22:40
@WinterSnowfall WinterSnowfall marked this pull request as ready for review November 11, 2024 19:43
@WinterSnowfall
Copy link
Contributor Author

Should be good to go post a review. It's been tested with quite a few games, and at this point I'm fairly confident it shouldn't regress anything. That being said, it's d3d9, so we won't know until it's merged.


// TODO: Consider switching this to debug, once we're
// confident the implementation doesn't cause any issues
Logger::warn(Message);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we expect this to get called in every game that uses the validation functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only in case of actual validation errors, however some of these (like in the case of The Void) are expected behavior. It would be useful to see them in logs however, at least for a while, until we're sure it's not causing unintended validations in other places.

Testing so far has been promising, but you never know, since this is like entirely undocumented.

src/d3d9/d3d9_shader_validator.h Show resolved Hide resolved
@WinterSnowfall WinterSnowfall force-pushed the d3d9-thevoid branch 2 times, most recently from 05658bf to 63c9339 Compare November 23, 2024 16:16
Copy link
Collaborator

@K0bin K0bin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly alright to me but I don't really like how inconsistent it is about re-using our Dxso compiler types in some places but not in others. I think being a bit more consistent about that and using them every where as much as possible would also make parts of it a bit more readable than using the raw bit masks and bit shifts.

src/d3d9/d3d9_shader_validator.h Show resolved Hide resolved
src/d3d9/d3d9_shader_validator.h Show resolved Hide resolved
src/d3d9/d3d9_shader_validator.h Outdated Show resolved Hide resolved
@K0bin
Copy link
Collaborator

K0bin commented Dec 4, 2024

We also need to test it with Sims 2, apparently the game calls this.

@WinterSnowfall
Copy link
Contributor Author

WinterSnowfall commented Dec 4, 2024

We also need to test it with Sims 2, apparently the game calls this.

In that regard @Blisto91 already did some tests and kept records. Perhaps he can share here. There are definitely games out there that use validation, so it's not as obscure as one may think.

Copy link
Collaborator

@K0bin K0bin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine now to me.

@WinterSnowfall
Copy link
Contributor Author

WinterSnowfall commented Dec 4, 2024

Will hold on this until:

  • @Blisto91 tests Sims 2, which allegedly passes a 0 cdw and could explode with this PR
  • I test the native reuse behavior of IDirect3DShaderValidator9 once an exception is hit (though I don't expect any surprises) Nevermind on the test, as it's an absolute pain to write. Forgot validators are very unofficial, without headers and undocumented. I'll trust in the original tests that @gofman wrote.

Co-Authored-By: Joshua Ashton <joshua@froggi.es>
Co-Authored-By: Paul Gofman <gofmanp@gmail.com>
@Blisto91
Copy link
Contributor

Blisto91 commented Dec 4, 2024

It has passed the Sims 2 and Sims Castaway Stories test

@doitsujin doitsujin merged commit d094053 into doitsujin:master Dec 5, 2024
4 checks passed
@WinterSnowfall WinterSnowfall deleted the d3d9-thevoid branch December 5, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[D3D9] The Void needs bits of IDirect3DShaderValidator9 implementation.
4 participants