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.

Sign up for GitHub

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

Jump to bottom

Add LTCG symbols: CDevice_KickOff variants, D3DDevice_MakeSpace, and XGSetSurfaceHeader #199

Merged
merged 3 commits into from
Oct 28, 2023
Merged

Add LTCG symbols: CDevice_KickOff variants, D3DDevice_MakeSpace, and XGSetSurfaceHeader #199

merged 3 commits into from
Oct 28, 2023

Conversation

jackchentwkh
Copy link
Contributor

@jackchentwkh jackchentwkh commented Oct 21, 2023 •
edited by RadWolfie
Loading

Add LTCG symbols:

  • CDevice_KickOff (thiscall)
  • CDevice_KickOff_4 (stdcall)
  • CDevice_KickOff_0_eax1 (optimized LTCG)
  • CDevice_KickOff_0_edx1 (optimized LTCG)
  • D3DDevice_MakeSpace (internal registers/instructions changed but the same as non-LTCG)
  • XGSetSurfaceHeader (non-LTCG)

these symbols are necessary for pushbuffer based rendering.

@github-actions github-actions bot added OOVPA Any OOVPA change relative needs-verification Require verification before approval D3D8LTCG OOVPA relative topic XGRAPHIC OOVPA relative topic labels Oct 21, 2023
PatrickvL
PatrickvL requested changes Oct 21, 2023
View reviewed changes
Copy link
Member

@PatrickvL PatrickvL left a comment

Choose a reason for hiding this comment

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

The new files should really mention your own name in the header comment block

@PatrickvL
Copy link
Member

Be aware that especially KickOff has likely been inlined in many titles - and since symbol scanning only returns one location it's currently not possible to locate (nor patch!) any other occurrences...

ergo720
ergo720 reviewed Oct 21, 2023
View reviewed changes
src/OOVPADatabase/D3D8LTCG/3925.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/XGraphic/3911.inl Outdated Show resolved Hide resolved
@RadWolfie
Copy link
Member

The new files should really mention your own name in the header comment block

@PatrickvL, signatures aren't copyrighted. Which they will get remove and replace with correct license. If going to keep the authors name, it is best to put it in central file which is the grouped signatures file, aka XXXX_OOPVA.inl files. See pull request #195

@PatrickvL
Copy link
Member

@RadWolfie note, I did not mention the term "copyright' on purpose ;)

RadWolfie
RadWolfie previously requested changes Oct 21, 2023
View reviewed changes
Copy link
Member

@RadWolfie RadWolfie left a comment

Choose a reason for hiding this comment

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

numerous of changes needed relative to the comments. I haven't test/review the signatures yet to verify anything amiss and/or list of titles missing these newly added symbol signatures. Plus review my branch(es) if any change is needed to work better.

src/OOVPADatabase/D3D8LTCG/3925.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/4039.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/4928.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/5558.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/5849.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/5558.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/XGraphic/5659.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/3925.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/D3D8LTCG/5558.inl Outdated Show resolved Hide resolved
src/OOVPADatabase/XGraphic/5659.inl Outdated Show resolved Hide resolved
@RadWolfie RadWolfie self-assigned this Oct 21, 2023
@RadWolfie
Copy link
Member

RadWolfie commented Oct 21, 2023 •
edited
Loading

@jackchentwkh, one other thing I noticed from our CI workflow checks. There are identical oovpa signatures being found and missing symbol entry for new symbols in unit test's database which is a requirement in order to properly test the signatures. See Add new founding symbol(s) or variant of symbol ABI section, step 4.

EDIT: Please ignore CI's Linux build failure, the cause of failure is from submodule's cmake file needs an update, thanks.

PatrickvL
PatrickvL requested changes Oct 21, 2023
View reviewed changes
Copy link
Member

@PatrickvL PatrickvL left a comment

Choose a reason for hiding this comment

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

Hey Jack, don't be intimidated by all the review comments; I think I can speak for the others as well, that it's only meant to make sure everything that's newly contributed with this pull request is going to get merged in the very best possible shape & form!

@jackchentwkh
Copy link
Contributor Author

per my last conversation with @RadWolfie , he will help me commit those necessary changes to merge this PR. So I will focus on other filed. currently I am working on a new symbol CMiniport_SoftwareMethod() which is necessary for D3DDevice_RunPushbuffer() to fixup return jump instructions and other pushbuffer fixups.

@github-actions github-actions bot added the D3D8 OOVPA relative topic label Oct 27, 2023
@github-actions github-actions bot added D3D8 OOVPA relative topic and removed D3D8 OOVPA relative topic labels Oct 27, 2023
@RadWolfie RadWolfie force-pushed the add_kickoff_ltcg branch 3 times, most recently from e455ce3 to 833d81c Compare October 27, 2023 15:07
@RadWolfie
Copy link
Member

RadWolfie commented Oct 27, 2023 •
edited
Loading

Fixup previous commit is made correction for titles older than build number are being detected and verified. Although, there is another function which is almost identical to CDevice_KickOff. With only difference is near end of function (first return) has "or" instruction to make it different from the other function. So, I had to rewrite the old and new signatures to use better detection. NOTE: Before 4034 does not have "or" instruction.

It is now ready for review since unit test did not report any issues nor missing detected known functions.

@RadWolfie RadWolfie dismissed their stale review October 27, 2023 15:18

resolved review remarks

@RadWolfie RadWolfie changed the title Add LTCG symbols: CDevice_KickOff_0_edx, CDevice_KickOff_0_ecx, D3DDevice_MakeSpace_0,XGSetSurfaceHeader Add LTCG symbols: CDevice_KickOff variants, D3DDevice_MakeSpace, and XGSetSurfaceHeader Oct 27, 2023
PatrickvL
PatrickvL approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@PatrickvL PatrickvL left a comment

Choose a reason for hiding this comment

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

Just one last question, no further comments

@RadWolfie
d3d8ltcg and xgraphic: fix previous commit conflicts and improve new …
aa6ea58 
…signatures

Renamed:
- CDevice_KickOff_0_ecx to CDevice_KickOff
- CDevice_KickOff_0_edx to CDevice_KickOff_0__LTCG_edx1
- D3DDevice_MakeSpace_0 to D3DDevice_MakeSpace

Removed:
- CDevice_KickOff_0__LTCG_edx1 (4928, 5849)
- CDevice_KickOff (5849)
- XGSetSurfaceHeader (5659)

All signatures are updated to reduce amount of signatures.
@RadWolfie
d3d8(ltcg): add/update CDevice_KickOff and D3DDevice_MakeSpace signat…
b070478 
…ures

Updated:
- CDevice_KickOff (3911, 4034, 4531, 5028, 5455)
- D3DDevice_MakeSpace (4034, 4134)

Added:
- CDevice_KickOff_4  (4432, 4531, 5028)
- CDevice_KickOff_0__LTCG_eax1 (5455)
@ergo720 ergo720 merged commit 0884e10 into Cxbx-Reloaded:master Oct 28, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ergo720 ergo720 ergo720 left review comments

@RadWolfie RadWolfie RadWolfie left review comments

@PatrickvL PatrickvL PatrickvL approved these changes

Assignees

@RadWolfie RadWolfie

Labels
D3D8LTCG OOVPA relative topic D3D8 OOVPA relative topic needs-verification Require verification before approval OOVPA Any OOVPA change relative XGRAPHIC OOVPA relative topic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackchentwkh @PatrickvL @RadWolfie @ergo720