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

III/VC cross-compile fixes #95

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open

III/VC cross-compile fixes #95

wants to merge 18 commits into from
+510 −2,120

Conversation

TheComputerGuy96
Copy link
Contributor

This PR might be too large when compared to the ModUtils one (so I can split it if needed)

Compiling with MSVC isn't tested either (maybe I should try installing VS on Wine to avoid this testing void?; GitHub Actions would also help)

Note: Adding a different build system is required to actually use this work (#31 mentions Premake which might be okay here but Meson is an option too and it's used by some popular PE libraries like DXVK) so I'm currently using some hacky Makefiles (I might push those to a branch later)

@CookiePLMonster
Copy link
Owner

CookiePLMonster commented Nov 7, 2024 •
edited
Loading

Before I start reviewing - is that not missing the inline assembly changes for San Andreas? That said, I would actually prefer the assembly changes and the rest split into two PRs.

Also as mentioned in #31, Actions will not happen because of the RW SDK dependency.

Some of those changes will also likely have to go under a macro, because SP needs to support v141_xp from VS2017 that has problems with static inline templated method pointers.

@TheComputerGuy96
Copy link
Contributor Author

Before I start reviewing - is that not missing the inline assembly changes for San Andreas? That said, I would actually prefer the assembly changes and the rest split into two PRs.

That's intentional because San Andreas has even bigger inline assembly changes (and this PR is probably big enough) and I'm currently having weird segfaults in that game (so support will be delayed for a future PR)

Actions will not happen because of the RW SDK dependency

So even adding an external RW SDK source in the Actions .yml file is as bad as vendoring the headers directly, right? If so then I guess REing the definitions required by the headers might be the only way

Also RW 3.7 SDK seems to work okay for all of the GTA SilentPatches (so I'm not sure why a precise version is needed)

Some of those changes will also likely have to go under a macro, because SP needs to support v141_xp from VS2017 that has problems with static inline templated method pointers.

I can work around that if the older compiler requires it

@CookiePLMonster
Copy link
Owner

CookiePLMonster commented Nov 7, 2024 •
edited
Loading

So even adding an external RW SDK source in the Actions .yml file is as bad as vendoring the headers directly, right? If so then I guess REing the definitions required by the headers might be the only way

Probably - it's a risk I choose not to take.

Also RW 3.7 SDK seems to work okay for all of the GTA SilentPatches (so I'm not sure why a precise version is needed)

At some point I remember it was problematic with one of the fixes, but maybe that was in another project. I vaguely recall that the SA fixes involving the anim interpolation broke with 3.7.

I can work around that if the older compiler requires it

I'll have to check if it's necessary on all templated static inlines, or only the method pointers. but something like #define STATIC_INLINE static inline and #define STATIC_INLINE static might be an okay solution.

Repository owner deleted a comment from CommandGenius Nov 7, 2024
@TheComputerGuy96
Copy link
Contributor Author

Some of the MSVC inline assembly statements do weird things (like doing a double pointer deference on a regular single-star pointer)

Is there any real reason for this?

@CookiePLMonster
Copy link
Owner

Do you have an example? Are you sure it's not a *&?

@TheComputerGuy96
Copy link
Contributor Author

Do you have an example? Are you sure it's not a *&?

This is one example: https://github.com/CookiePLMonster/SilentPatch/blob/dev/SilentPatchIII/SilentPatchIII.cpp#L199 (InstantHitsFiredByPlayer is defined as an int*)

@CookiePLMonster
Copy link
Owner

OK I think that may have been me being careless with the notation - the first mov is supposed to load the value in InstantHitsFiredByPlayer, then inc dereferences it.

@TheComputerGuy96
Copy link
Contributor Author

Anyway I just redid the MSVC inline assembly statements to be more logical (but they're of course untested)

Also there's some llvm-mingw/Clang fixes (now there's just some stubborn call instructions that won't work with the i constraint for some reason; I might make a bug report for that later)

@CookiePLMonster
Copy link
Owner

CookiePLMonster commented Nov 11, 2024 •
edited
Loading

Thanks! I'll have to verify those assembly statements later. I just hope they don't start mov'ing an address of the variable, but for that I think it'd have to be mov eax, offset X?

@CookiePLMonster
Copy link
Owner

CookiePLMonster commented Nov 12, 2024 •
edited
Loading

I checked the assembly changes and indeed, both mov eax, VAR and mov eax, [VAR] dereference it, so your changes are correct:
image

mov'ing the address of VAR requires mov eax, offset VAR, which makes sense.

What do you think about me (or you) submitting those MSVC assembly changes separately so then your PR can become a bit smaller and easier to review? I could submit this change as a PR too for you to review.

@TheComputerGuy96
Copy link
Contributor Author

What do you think about me (or you) submitting those MSVC assembly changes separately so then your PR can become a bit smaller and easier to review?

I'll do that fairly soon

@TheComputerGuy96 TheComputerGuy96 mentioned this pull request Nov 12, 2024
Merged
@TheComputerGuy96
Copy link
Contributor Author

Ironically up-to-date MSVC on Wine requires some of these cross-compile fixes too (despite not being GCC/Clang of course):

Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatch\Common.cpp(37,18): error C2065: 'RwIm2DRenderLine': undeclared identifier [Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatchIII\SilentPatchIII.vcxproj]
Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatch\RWGTA.cpp(55,8): error C2143: syntax error: missing ')' before '*' [Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatchIII\SilentPatchIII.vcxproj]

@CookiePLMonster
Copy link
Owner

Yeah, it's a combination of two things:

  1. VS2017 not being entirely C++17 conformant (static inline).
  2. Old Windows SDK 7.0 headers not being very strict with gating off the features (in this case, condition variables) behind a WINNT version check.

@TheComputerGuy96 TheComputerGuy96 force-pushed the iiivc-cross-fixes branch 3 times, most recently from a2cfac1 to 8871042 Compare November 14, 2024 23:44
@CookiePLMonster
Copy link
Owner

I merged #113 because it's safe and testable, but I am going to hold this one off until the next hotfix is released.

@TheComputerGuy96
Copy link
Contributor Author

Also the generated .rc files are fully UTF-16 (LE?) encoded which windres doesn't support at all (I'm currently converting those files to UTF-8 at compile-time which complicates the compile process a bit)

@TheComputerGuy96
Remove unused dllmain.cpp file
f33f5c1 
It seems to be used in the early days of SilentPatch (but it's
no longer included since the III/VC/SA code split and serves no purpose)
@TheComputerGuy96
Use lowercase names for Windows headers
c15fbc5 
This fixes missing header issues on a case-sensitive filesystem
with MinGW GCC
@TheComputerGuy96
Set Windows defines earlier
fe6f98c 
This avoids compile warnings on MinGW GCC (because standard C++
headers eventually import the Windows stuff)
@TheComputerGuy96
Use reinterpret_cast for function pointer casts
b5483bb 
MSVC (wrongly) allows those casts to succeed with static_cast:
https://stackoverflow.com/questions/74002657/why-cant-i-static-cast-a-void-to-a-pointer-to-function
(so adjust those casts for better compiler compatibility including MinGW GCC)
@TheComputerGuy96
SilentPatchIII/VC/SA: Add GCC/Clang-compatible safebuffers equivalent
5890527 
And switch to a common define for this attribute (this fixes
compile warnings on MinGW GCC)
@TheComputerGuy96
SilentPatchIII/VC: Remove extern from ppUserFilesDir variable definit…
04f7433 
…ions

This fixes compile warnings with MinGW GCC
@TheComputerGuy96
SilentPatchIII/VC/SA: Mark some function pointers as inline
1af97ee 
MinGW GCC's linker can't find them otherwise
@TheComputerGuy96
DDraw: Use a pragma for DirectDrawCreateEx export on MSVC
940c689 
The .def file is either going to have issues on MSVC or MinGW GCC
(so replace it with a pragma on MSVC)
@TheComputerGuy96
DDraw: Cast the memcpy() source argument
acc4af2 
This works around the MinGW GCC type strictness
@TheComputerGuy96
SilentPatch: Don't define certain Rw* functions if not needed
80b4fa1 
Redefining them can cause strange compile errors with MinGW GCC
@TheComputerGuy96
SilentPatch: Simplify RwIm2DRenderLine function pointer declaration
4ba23db 
MinGW GCC doesn't seem to unwind the layers of the macro define
properly (which causes it to not find the declaration type)
@TheComputerGuy96
SilentPatch: Move RwEngineInstance definition to headers
77f5e90 
MinGW GCC can't locate it in some files otherwise
@TheComputerGuy96
SilentPatch: Define a replacement for _stricmp() on non-MSVC
50b1c13 
MinGW GCC doesn't have this MSVC-specific function
@TheComputerGuy96
SilentPatch: Move a header include in SVF
d3e5495 
This makes sure the fixed-width integer types are included in SVF.h
@TheComputerGuy96
SilentPatch: Add missing cmath header include
1b174a3 
It's required for the modf() function (and it isn't implicitly
included on MinGW GCC)
@TheComputerGuy96
SilentPatchIII: Add a missing functional header include
d6f3ea8 
MinGW GCC doesn't implicitly include it either
@TheComputerGuy96
SilentPatchIII: Add GCC-style inline assembly statements
c2e58d3 
These almost work on llvm-mingw too (but there's some stubborn call instructions)
@TheComputerGuy96
SilentPatchVC: Add GCC-style inline assembly statements
815ee10 
This also includes a small wrapper to call a C++ function from GCC-style inline ASM

These statements almost work on llvm-mingw too (but there's some stubborn call instructions)
@CanerKaraca23
Copy link

Amazing work, thanks for your efforts.

Also, is it possible to upgrade the project to VS 2022?

@CookiePLMonster
Copy link
Owner

That's hopefully coming soon, there are just a few tiny incompatibilities stemming from how lax v141_xp is.

@TheComputerGuy96
Copy link
Contributor Author

is it possible to upgrade the project to VS 2022?

This project might be switched to Premake instead for cross-compile support (because it's already used by some other SilentPatches and because Meson might not be accepted) so the existing .sln/.vcxproj files will disappear (Premake does still have a VS generator though so you will still be able to build with VS)

I had some strange cross-compile issues with Premake though (so I might put a Git/fork version of Premake as a requirement instead of dealing with that weird stuff)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TheComputerGuy96 @CookiePLMonster @CanerKaraca23