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.

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions Generals/Code/GameEngine/Source/Common/System/StackDump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

_asm
{
MYEIP1:
Expand All @@ -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 •
edited
Loading

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).

RtlCaptureContext(&gsContext);
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).



MakeStackTrace(myeip,myesp,myebp, 2, callback);
Expand Down Expand Up @@ -340,6 +347,7 @@ void FillStackAddresses(void**addresses, unsigned int count, unsigned int skip)
gsContext.ContextFlags = CONTEXT_FULL;

DWORD myeip,myesp,myebp;
#ifdef _MSC_VER
_asm
{
MYEIP2:
Expand All @@ -351,6 +359,12 @@ _asm
mov dword ptr [myebp] , eax
xor eax,eax
}
#else
RtlCaptureContext(&gsContext);
myeip = gsContext.Eip;
myesp = gsContext.Esp;
myebp = gsContext.Ebp;
#endif
memset(&stack_frame, 0, sizeof(STACKFRAME));
stack_frame.AddrPC.Mode = AddrModeFlat;
stack_frame.AddrPC.Offset = myeip;
Expand All @@ -360,17 +374,6 @@ stack_frame.AddrFrame.Mode = AddrModeFlat;
stack_frame.AddrFrame.Offset = myebp;

{
/*
if(GetThreadContext(thread, &gsContext))
{
memset(&stack_frame, 0, sizeof(STACKFRAME));
stack_frame.AddrPC.Mode = AddrModeFlat;
stack_frame.AddrPC.Offset = gsContext.Eip;
stack_frame.AddrStack.Mode = AddrModeFlat;
stack_frame.AddrStack.Offset = gsContext.Esp;
stack_frame.AddrFrame.Mode = AddrModeFlat;
stack_frame.AddrFrame.Offset = gsContext.Ebp;
*/

Bool stillgoing = TRUE;
// unsigned int cd = count;
Expand Down
25 changes: 14 additions & 11 deletions GeneralsMD/Code/GameEngine/Source/Common/System/StackDump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ void StackDump(void (*callback)(const char*))

DWORD myeip,myesp,myebp;

#ifdef _MSC_VER
_asm
{
MYEIP1:
Expand All @@ -86,6 +87,12 @@ _asm
mov eax, ebp
mov dword ptr [myebp] , eax
}
#else
RtlCaptureContext(&gsContext);
myeip = gsContext.Eip;
myesp = gsContext.Esp;
myebp = gsContext.Ebp;
#endif


MakeStackTrace(myeip,myesp,myebp, 2, callback);
Expand Down Expand Up @@ -340,6 +347,7 @@ void FillStackAddresses(void**addresses, unsigned int count, unsigned int skip)
gsContext.ContextFlags = CONTEXT_FULL;

DWORD myeip,myesp,myebp;
#ifdef _MSC_VER
_asm
{
MYEIP2:
Expand All @@ -351,6 +359,12 @@ _asm
mov dword ptr [myebp] , eax
xor eax,eax
}
#else
RtlCaptureContext(&gsContext);
myeip = gsContext.Eip;
myesp = gsContext.Esp;
myebp = gsContext.Ebp;
#endif
memset(&stack_frame, 0, sizeof(STACKFRAME));
stack_frame.AddrPC.Mode = AddrModeFlat;
stack_frame.AddrPC.Offset = myeip;
Expand All @@ -360,17 +374,6 @@ stack_frame.AddrFrame.Mode = AddrModeFlat;
stack_frame.AddrFrame.Offset = myebp;

{
/*
if(GetThreadContext(thread, &gsContext))
{
memset(&stack_frame, 0, sizeof(STACKFRAME));
stack_frame.AddrPC.Mode = AddrModeFlat;
stack_frame.AddrPC.Offset = gsContext.Eip;
stack_frame.AddrStack.Mode = AddrModeFlat;
stack_frame.AddrStack.Offset = gsContext.Esp;
stack_frame.AddrFrame.Mode = AddrModeFlat;
stack_frame.AddrFrame.Offset = gsContext.Ebp;
*/

Bool stillgoing = TRUE;
// unsigned int cd = count;
Expand Down
9 changes: 7 additions & 2 deletions GeneralsMD/Code/Libraries/Source/debug/debug_debug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <stdio.h>
#include <string.h>
#include <new> // needed for placement new prototype
#include <Utility/intrin_compat.h>

// a little dummy variable that makes the linker actually include
// us...
Expand Down Expand Up @@ -270,11 +271,15 @@ bool Debug::SkipNext(void)
// do not implement this function inline, we do need
// a valid frame pointer here!
unsigned help;
#ifdef _MSC_VER
_asm
{
mov eax,[ebp+4] // return address
mov help,eax
};
#else
help = (unsigned)_ReturnAddress();
#endif
curStackFrame=help;

// do we know if to skip the following code?
Expand Down Expand Up @@ -386,7 +391,7 @@ bool Debug::AssertDone(void)
}
break;
case IDRETRY:
_asm int 0x03
__debugbreak();
break;
default:
((void)0);
Expand Down Expand Up @@ -654,7 +659,7 @@ bool Debug::CrashDone(bool die)
}
break;
case IDRETRY:
_asm int 0x03
__debugbreak();
break;
default:
((void)0);
Expand Down
6 changes: 6 additions & 0 deletions GeneralsMD/Code/Libraries/Source/debug/debug_except.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,18 @@ void DebugExceptionhandler::LogFPURegisters(Debug &dbg, struct _EXCEPTION_POINTE
double fpVal;

// convert from temporary real (10 byte) to double
#ifdef _MSC_VER
_asm
{
mov eax,value
fld tbyte ptr [eax]
fstp qword ptr [fpVal]
}
#else
__float80 fp80val;
Copy link

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?

memcpy(&fp80val, value, 10);
fpVal = (double) fp80val;
#endif

dbg << " " << fpVal << "\n";
}
Expand Down
8 changes: 8 additions & 0 deletions GeneralsMD/Code/Libraries/Source/debug/debug_stack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ int DebugStackwalk::StackWalk(Signature &sig, struct _CONTEXT *ctx)
{
// walk stack back using current call chain
unsigned long reg_eip, reg_ebp, reg_esp;
#ifdef _MSC_VER
__asm
{
here:
Expand All @@ -371,6 +372,13 @@ int DebugStackwalk::StackWalk(Signature &sig, struct _CONTEXT *ctx)
mov reg_ebp,ebp
mov reg_esp,esp
};
#else
CONTEXT ctx2;
RtlCaptureContext(&ctx2);
reg_eip = ctx2.Eip;
reg_ebp = ctx2.Ebp;
reg_esp = ctx2.Esp;
#endif
stackFrame.AddrPC.Offset = reg_eip;
stackFrame.AddrStack.Offset = reg_esp;
stackFrame.AddrFrame.Offset = reg_ebp;
Expand Down