-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Updated GENie and changed Visual Studio target to Visual Studio 2022. (…
…#13000) * Updated to GENie 1181. * Applied local fix for GENie resource include directory bug, sent upstream as bkaradzic/GENie#572. * Set MSVC flags to use conformant preprocessor, standards conformance mode, and assume UTF-8 encoding.
- Loading branch information
1 parent
5b1adf3
commit dc8ba81
Showing
64 changed files
with
1,743 additions
and
2,705 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new genie you can't compile a 32bit MAME version anymore. In the file 3rdparty/genie/build/gmake.windows/genie.make
it used additionally the GCC flag -m64 for x86-64 architecture. This flags must be removed.
The other problem is a wrong "return" / typo in the file 3rdparty\genie\src\host\scripts.c, which creates a broken expat.make
in build\projects\windows\mameXXXX\gmake-mingw64-gcc.
I found this by comparing the old version of scripts.c with the new one.
If you replace 'result' with 'value' in 3rdparty\genie\src\host\scripts.c and line 225:
From
gsub("\"", "\\\"")\nreturn result
to
gsub("\"", "\\\"")\nreturn value
it creates a correct expat.make.
With these two changes i have successfully compiled a 32bit version of MAME without NOWERROR=1 !!!
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second change you describe would fundamentally change the meaning of the function (essentially disabling escaping of double quotation characters). The actual source is here (from actions/make/_make.lua), and it hasn't been changed in 6 years:
What is the actual problem you encountered before you made this change?
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone passed this to me fwiw:
Compiling 3rdparty/expat/lib/xmlparse.c...
: error: missing terminating " character [-Werror]
cc1.exe: all warnings being treated as errors
expat.make:316: recipe for target '../../../../mingw-gcc/obj/x64/Release/3rdparty/expat/lib/xmlparse.o' failed
make[2]: *** [../../../../mingw-gcc/obj/x64/Release/3rdparty/expat/lib/xmlparse.o] Error 1
makefile:34: recipe for target 'expat' failed
make[1]: *** [expat] Error 2
makefile:1126: recipe for target 'windows_x64' failed
make: *** [windows_x64] Error 2
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrick Mackinlay
If you compare the old 3rdparty/genie/build/gmake.windows/genie.make with the new one, you can
see that the old version uses $(MPARAM) instead of -m64. And the defintion of MPARAM
is in MAME's makefile at line 310:
ifeq ($(ARCHITECTURE),_x86)
MPARAM := -m32
else
ifeq ($(ARCHITECTURE),_x64)
MPARAM := -m64
Can you add $(MPARAM back so we can compile a 32bit version again.
For the 2nd problem please compare MAME's old scripts.c version with the new one.
This "return value" is in the old version.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I'm seeing this too. several 'make' attempts will get past it, but it's definitely an issue.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first issue regarding $(MPARAM) is a regression, and I'll fix it, no issue, please be a bit patient while I work on the other issue to minimize churn.
For the second issue, the root cause is GENie's escaping rules not handling a specific input here. Please note that scripts.c is a generated file, so comparing/modifying it doesn't make sense unless you also update the source (which is the various .lua files in the GENie actions folder). A better hack/fix in the short term is to remove/replace the space in the line mentioned above with something else that won't trigger escaping.
I can only assume that in the previous version of our GENie code, someone modified scripts.c (but not the .lua) to hack/avoid this issue, which is why I didn't notice the change. Again, please bear with me while I try to solve the problem properly.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tackling this problem!
Let's hope that the file will be generated correctly so that xmlrole.c can be compiled again :)
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully fixed here ae4e448.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works! Thx!
A missing hyphen... My head is spinning terribly ;)
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that building on Raspberry Pi OS fails since this commit (via bisect):
gcc complains about a wrong flag "-m64". This is obviously wrong for aarch64.
(BTW, I just installed a new Raspberry and wondered what I was doing wrong until I gave it a try with the previously working one.)
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it “obviously wrong”? The
-m32and-m64flags have been supported for selecting between 32-bit and 64-bit target ISA on numerous targets (SPARC, PowerPC, x86, etc.), why is ARM the odd one out? And if it doesn’t support-m32and-m64, how do you set the target ISA?dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understood from the options of gcc, the m32/64 options belong to x86/64, not aarch64.
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don’t belong to x86 – they’re available for most architectures that have 32-bit and 64-bit flavours:
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I did not design the gcc, I can just report what it says, and it's
"gcc: error: unrecognized command-line option '-m64'"
since this commit.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aarch64 has optional support for 32-bit in the instruction set and requires something particular with how dynamic objects are shared, making building two separate toolchains for both 32-bit and 64-bit a requirement.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, but building on aarch64 did work without a problem before this commit. I did it for each of the last releases.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I successfully compiled MAME on the Raspberry Pi with the "-m64" switches removed in gmake.linux/genie.make. Result is "ELF 64-bit LSB pie executable, ARM aarch64" as expected.
So it would be nice if you could add a check for aarch64 to omit the flag -m64, then everything seems fine again ... as far as I can see. I tried to use $(ARCH) for the check, but that did not work.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ${MPARPAM} worked because it did not add neither m64 nor m32 when on aarch64. This needs to be done for not only the genie.make for the Windows target but also for the Linux, FreeBSD, and Darwin targets.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here 383c16a. I can't test those other targets, so would appreciate testing and feedback from those who can. My apologies for the oversight in accidentally reverting this local change to the GENie upstream code and causing this problem.
dc8ba81There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks good now. Thanks.