07151129 / sh3proxy

BSD 3-Clause "New" or "Revised" License
29 stars 1 forks source link

stability issues #9

Closed argvx closed 7 years ago

argvx commented 8 years ago

It seems this fix has many stability issues, tested on 7 PC's (DEP disabling: no effect), 6x - Win7 x64 and 1 - Win10Pro. It works only on 3 of them (except on one requires compatibility mode set for sh3.exe), on Win10 PC - intro video freeze, anything else in game seems working ok, another one - 0xc000001d error (no error if disabled Patch option in .ini, but process just dies then), others - process just dies (no errors reported). It works ok without sh3proxy on all of them.

PS: no issues with sh2proxy at all.

07151129 commented 8 years ago

Win10 PC - intro video freeze, anything else in game seems working ok

So you can skip the video? Do other videos freeze (e.g. credits)? sh3proxy doesn't touch video playback at all.

another one - 0xc000001d error (no error if disabled Patch option in .ini, but process just dies then)

Which option exactly are you disabling? Patch is a name for a set of options.

Does the error message contain the current instruction pointer (eip/rip CPU register value)? Unfortunately, Windows error messages are not informative at all.

others - process just dies (no errors reported)

A good way to find out why would be to disable all sh3proxy options that can be disabled and see if the game still works. If it does, try enabling them by one until you find the one that breaks the game. I suggest you also use v0.7 for this, since UseCWD patch can be disabled there. Also try setting the video resolution in sh3proxy.ini equal to the native display resolution.

Unfortunately, I can't fix any of these issues unless I know exactly what patches break the game. I've also had the game die silently, but this only happened on a virtual machine, so I blame its file sharing. On every live machine I've tested, I had no issues at all.

argvx commented 8 years ago
  1. yes, video can be skipped, credits not tested (and if don't skip it for a long time, game will crash)
  2. Enable video patches (this one), and no instruction pointers..
  3. on machines where process just silently dies it doesn't matter which options is disabled at all.

The game also was tested on vmware ws 12, OS: win10pro - same problem with video playback as on real hardware, other OS not tested. v0.7 have the same problem, and yes.. i tested every option.

07151129 commented 8 years ago

Does your sh3.exe have the same crc32 checksum (372ae792)? (You can check it using e.g. http://code.kliu.org/hashcheck/.)

argvx commented 8 years ago

yes, crc32 is 372ae792 MD5(sh3.exe)= cabab17c9db0b2e16f6281912ca48cf8 SHA256(sh3.exe)= a51f956bd5be21fd704c4d19cf0674a175d002081da43b1d377be83226c36e5e

argvx commented 8 years ago

Well, i found root's of all problems.. First, function WRAP_FN_IFACE, in some cases incorrect path is passed to LoadLibrary (causing process to silently die), changing code to look like this fixes this issue:

if (!origDLL) { char path[MAX_PATH], fullPath[MAX_PATH]; char name[] = WRAP_DLL_NAME".dll"; memset(fullPath, 0, sizeof(fullPath)); memset(path, 0, sizeof(path)); GetSystemDirectory(path, MAX_PATH); snprintf(fullPath, MAX_PATH, "%s\\%s", path, name); origDLL = LoadLibrary(fullPath); if (!origDLL) ExitProcess(1); }

Second problem comes from clampPow2() function, this cause 0xc000001d error on some systems.

07151129 commented 8 years ago

Thanks for debugging.

changing code to look like this fixes this issue:

So I suppose memsetting path to 0 fixed it? Then it must be GetSystemDirectoryA not writing NUL to terminate the string. Wine uses strcpy, so now it makes sense why this never failed with wine.

Second problem comes from clampPow2() function, this cause 0xc000001d error on some systems.

Looks like gcc generated misaligned code, will look into this.

argvx commented 8 years ago

Second problem is this popcnt asm code is not portable in clampPow2() function. Here is portable version of popcount:

static int
popcount(unsigned int x)
{
#define INIT1(X)                                \
     ((((X) & (1 << 0)) != 0) +                  \
      (((X) & (1 << 1)) != 0) +                  \
      (((X) & (1 << 2)) != 0) +                  \
      (((X) & (1 << 3)) != 0) +                  \
      (((X) & (1 << 4)) != 0) +                  \
      (((X) & (1 << 5)) != 0) +                  \
      (((X) & (1 << 6)) != 0) +                  \
      (((X) & (1 << 7)) != 0))
#define INIT2(X)   INIT1(X),  INIT1((X) +  1)
#define INIT4(X)   INIT2(X),  INIT2((X) +  2)
#define INIT8(X)   INIT4(X),  INIT4((X) +  4)
#define INIT16(X)  INIT8(X),  INIT8((X) +  8)
#define INIT32(X) INIT16(X), INIT16((X) + 16)
#define INIT64(X) INIT32(X), INIT32((X) + 32)

    static const uint8_t popcount[256] = {
        INIT64(0), INIT64(64), INIT64(128), INIT64(192)
    };

    return (popcount[x & 0xff] +
            popcount[(x >> 8) & 0xff] +
            popcount[(x >> 16) & 0xff] +
            popcount[x >> 24]);
}

static inline
int clampPow2(int val, int min, int max) {
    int cnt = 0;

    /* __asm__ ("popcnt %1, %0;" : "=r"(cnt) : "r"(val)); */
    cnt = popcount(val);

    if (cnt > 1) /* Must be pow2 */
        val &= (1 << (8 * sizeof(int) - __builtin_clz(val) - 1));

    /* Clamp */
    if (val > max)
        val = max;
    if (val < min)
        val = min;
    return val;
}

it is slower a bit, but it works fine on every tested system.

07151129 commented 8 years ago

Second problem is this popcnt asm code is not portable in clampPow2() function.

Not portable to what? I was assuming generic x86, which is what users are limited to anyway. It hardly affects performance, so I might rewrite it as a simple loop then.

argvx commented 8 years ago

I mean that popcnt instruction is not supported by any CPU.

07151129 commented 8 years ago

Oh, you're right, I missed the fact it's a part of SSE4.2.

07151129 commented 7 years ago

Can you test whether this build works? It still fails to start for me on Windows, unless I run sh3.exe from command line.

argvx commented 7 years ago

why there is no source code? Well, it's not working.. anyway.. I use my fixed version with some enhancements (auto detect screen resolution, cheats, etc..)

07151129 commented 7 years ago

Can you publish the patches so I can merge them?

I didn't publish the changes yet since the fixes might not work. I've replaced popcnt and made path in WRAP_FN static, but I am not sure if this fixes actually the issue, as I can't even get logging working on Windows. Using your fix for WRAP_FN seems to be helping, though, so I will merge that (I don't have time to investigate what is really going on there).

argvx commented 7 years ago

here is the diff from my version

07151129 commented 7 years ago

Nice, thanks. I'll integrate this soon.