DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.74k stars 231 forks source link

Fix cross-compiling "int $3" build error #380

Closed GravisZro closed 1 month ago

GravisZro commented 1 month ago

libmve/mveasm.cpp was using a macro to invoke int $3 instead of using the portable Int3() macro in pserror.h.

Pull Request Type

Description

Replace non-portable macro int3 with the portable macro Int3() from pserror.h. Fixes #311

Related Issues

Screenshots (if applicable)

Checklist

Additional Comments

I've cross-compiled a the libmve module but not the rest of the code because I don't have a port of SDL2 for that.

JeodC commented 1 month ago

Potentially dumb question, why not use debugbreak() as suggested in the issue? I also recommend tagging winterheart for review once this is prepared since the new libmve may not use asm--and therefore this change not needed in the long run.

GravisZro commented 1 month ago

Potentially dumb question, why not use debugbreak() as suggested in the issue?

The Int3() macro is a bit more sophisticated for debugging but it does actually end up calling the debug_break function.

I also recommend tagging winterheart for review

As far as I can tell, I don't have the ability to do that.

JeodC commented 1 month ago

Potentially dumb question, why not use debugbreak() as suggested in the issue?

The Int3() macro is a bit more sophisticated for debugging but it does actually end up calling the debug_break function.

I also recommend tagging winterheart for review

As far as I can tell, I don't have the ability to do that.

Done. Over in #289, libmve.asm is completely gone. I'm not sure how close we are to merging that though.

pzychotic commented 1 month ago

Uhm, I also changed that file in #354 for Windows x64 to compile, but opted for the debug_break() route. I wasn't aware, we have an issue for that. If the Int3() solution is preferable, I should revert my change in favor of this.

JeodC commented 1 month ago

Uhm, I also changed that file in #354 for Windows x64 to compile, but opted for the debug_break() route. I wasn't aware, we have an issue for that. If the Int3() solution is preferable, I should revert my change in favor of this.

Int3() is a simple macro that's strategically placed in the codebase to quickly identify issues that should only occur in rare circumstances. From my understanding, debug_break() should be preferred when using debug builds, but Int3() should be used in release builds for easier user-friendly reporting.

Of course, since Int3() just calls debug_break() anyway, I don't see why we shouldn't just use Int3().

JeodC commented 1 month ago

Actually, at the moment Int3() is rather unhelpful in comparison. It's defined at pserror.h where it calls debug_break() if it's available (which it usually will be). If not, then it just does the int3 messagebox. The mprintf statement, however, is the user-friendly part I was referring to. https://github.com/DescentDevelopers/Descent3/blob/main/lib/pserror.h#L222-L229

There are tons of mprintf statements scattered throughout the codebase. It used to output to a monochrome debug console: https://github.com/DescentDevelopers/Descent3/blob/main/ddebug/mono.h#L72 https://github.com/DescentDevelopers/Descent3/blob/main/ddebug/winmono.cpp#L370-L410

We have a logging module, DLOGGER, but I don't know to what extent it takes advantage of existing debug messages (in windows anyway).

Edit: It doesn't work for windows.

Reference #157 and https://github.com/DescentDevelopers/Descent3/commit/782d527de6e3a315c0b5880739729722f649a5e1

pzychotic commented 1 month ago

-DLOGGER works in Windows too, after we fixed it in #256 (https://github.com/DescentDevelopers/Descent3/commit/b59115460eb0d608a3fdd4bfca206dc56c23fc38). It outputs to the VS output window https://github.com/DescentDevelopers/Descent3/blob/cffeb20a3e09d8c869134a465555374b7a9b40ae/ddebug/winmono.cpp#L407 (or any other debugger supporting it) and to a log file https://github.com/DescentDevelopers/Descent3/blob/cffeb20a3e09d8c869134a465555374b7a9b40ae/ddebug/winmono.cpp#L404 when activated with -logfile.

JeodC commented 1 month ago

-DLOGGER works in Windows too, after we fixed it in #256 (https://github.com/DescentDevelopers/Descent3/commit/b59115460eb0d608a3fdd4bfca206dc56c23fc38).

It outputs to the VS output window https://github.com/DescentDevelopers/Descent3/blob/cffeb20a3e09d8c869134a465555374b7a9b40ae/ddebug/winmono.cpp#L407 (or any other debugger supporting it) and to a log file https://github.com/DescentDevelopers/Descent3/blob/cffeb20a3e09d8c869134a465555374b7a9b40ae/ddebug/winmono.cpp#L404 when activated with -logfile.

That explains it. I expected it to output to terminal when I ran the exe with it. We were going to have a rough writeup of how to use logging back when we had spdlog, but it never came about and -logfile slipped my mind.

Lgt2x commented 1 month ago

mveasm.cpp will be removed by #289, there is not point in fixing the current version of libmve

GravisZro commented 1 month ago

mveasm.cpp will be removed by #289, there is not point in fixing the current version of libmve

I don't understand why you closed this PR. I'm trying to resolve #311. If nothing else, it will fix the build problem for ARM.