Skip to content

[GEN][ZH] Replacements for rest of asm code #670

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zzambers
Copy link

@zzambers zzambers commented Apr 13, 2025

This makes rest of asm code MSVC only, to allow build on other compilers. So far mostly unimplemented for other compilers (apart from debug breaks). However this is all debug stuff and should not be critical to run the game afaik. I have introduced UNIMPLEMENTED_ERROR macro as placeholder for unimplemented code. (See dicussions for more about UNIMPLEMENTED_ERROR macro)

This does adds alternative code asm.

This was extracted from bigger change-set fixing build on MinGW: #547

@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Build Anything related to building, compiling labels Apr 13, 2025
@xezon xezon added this to the Code foundation build up milestone Apr 13, 2025
@zzambers
Copy link
Author

Just fixed message for UNIMPLEMENTED_ERROR

@zzambers
Copy link
Author

zzambers commented Apr 19, 2025

After doing bit more research, doing asm replacements turned out not that difficult:

  • I make use of RtlCaptureContext to get values of eip, esp and ebp. I think, it was not used by the game code, because it is WinXP+ and probably not supported by vc6. There are, however, some commented-out attempts to use GetThreadContext in the code. But Doc says:

    You cannot get a valid context for a running thread. Use the SuspendThread function to suspend the thread before calling GetThreadContext.

    Seems like GetThreadContext cannot be used to get context of current thread. This is probably reason why they abandoned approach with GetThreadContext and used inline assembly instead. However RtlCaptureContext should work.

  • _ReturnAddress from Utility/intrin_compat.h is used to obtain return address

  • For conversion from x87 10byte float I make use GCC __float80 type.

  • I removed, now unnecessary, UNIMPLEMENTED_ERROR macro :)

@zzambers zzambers changed the title [GEN][ZH] Rest of asm code made VC only [GEN][ZH] Replacements for rest of asm code Apr 19, 2025
@@ -86,6 +87,12 @@ _asm
mov eax, ebp
mov dword ptr [myebp] , eax
}
#else
Copy link

Choose a reason for hiding this comment

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

#elif _WIN32

Copy link
Author

@zzambers zzambers Apr 19, 2025

Choose a reason for hiding this comment

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

I would say basically whole file is only valid for _WIN32. Code here is very windows specific... To target other platforms/OSes, big parts of platform specific debug code would need to be written for scratch (for given platform).

Also code is currently for x86 (32-bit) only. It would need more fixes to support windows x86_64. Currently it would not even compile (but that is out of scope of this PR).

myeip = gsContext.Eip;
myesp = gsContext.Esp;
myebp = gsContext.Ebp;
#endif
Copy link

Choose a reason for hiding this comment

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

#else
#error not implemented
#endif

Copy link
Author

Choose a reason for hiding this comment

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

Same as higher, it would basically hold for file as a whole, which would currently not even compile on anything other than Windows x86 (32-bit).

Copy link
Author

Choose a reason for hiding this comment

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

That code in particular will also not compile on anything other that Windows x86, because it accesses x86 specific 32-bit registers ( Eip Esp Ebp).

@@ -76,6 +76,7 @@ void StackDump(void (*callback)(const char*))

DWORD myeip,myesp,myebp;

#ifdef _MSC_VER
Copy link

Choose a reason for hiding this comment

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

I think it is better to make this VS6 build specific.

#if defined(_MSC_VER) && _MSC_VER < 1300

Same for the various other ifdef's in this change.

Copy link
Author

@zzambers zzambers Apr 19, 2025

Choose a reason for hiding this comment

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

Probably true, I was just being conservative. This code will need to be properly tested, but I am now not in position to do so.. :/

Btw doc for RtlCaptureContext says:

Retrieves a context record in the context of the caller.

I have done some experiments and it seems that caller here means not caller of the RtlCaptureContext, but caller of function, which called RtlCaptureContext. So maybe context will be one frame off compared to asm. If that is the case, either skip value would need to be adjusted by one or dummy wrapper function would need to be created such as:

static void CaptureContextWrapper(CONTEXT *ctx) {
    RtlCaptureContext(ctx);
}

Copy link
Author

Choose a reason for hiding this comment

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

I tested this also with MSVC, seems that RtlCaptureContext really captures context one frame higher then, where it was called (see my experiment).

I think placing RtlCaptureContext in dummy wrapper function will probably be easiest and least invasive solution to match the asm behavior.

Copy link

@OmniBlade OmniBlade left a comment

Choose a reason for hiding this comment

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

I appreciate the effort to try and clean this up, but I'd be inclined to just dump the exception handler and go with an external library to handle this so we aren't trying to maintain a bunch of cross (or not so cross) platform code ourselves. Something like https://github.com/jeremy-rifkin/cpptrace has been suggested to me for light weight stack traces and we should consider a crashpad fork IMO for automated crash dump submissions from users once we dump VC6.

_asm
{
mov eax,value
fld tbyte ptr [eax]
fstp qword ptr [fpVal]
}
#else
__float80 fp80val;

Choose a reason for hiding this comment

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

This assumes a lot IMO, should be gated to GCC and X86 only as I'm not sure the type exists elsewhere and give an unimplemented error or message otherwise?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Anything related to building, compiling Minor Severity: Minor < Major < Critical < Blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants