mupen64plus / mupen64plus-core

Core module of the Mupen64Plus project
1.28k stars 257 forks source link

Issue with compile-time generation of asm defines #319

Closed inactive123 closed 7 years ago

inactive123 commented 7 years ago

@bsmiles32 and other interested parties -

https://github.com/mupen64plus/mupen64plus-core/blob/master/src/asm_defines/asm_defines.c

It would really be appreciated if you guys could start committing these generated files for all the specific toolchains/compilers/etc so that we don't have to rely on compile-time shenanigans to get files that are 'required' to be there by the sourcecode.

I consider it a really horrible and regressive design decision now that apparently certain required files are being 'generated' at compile-time now. Even worse that apparently it's also compiler/architecture bound. I wanted to upstream the mupen64plus cores but this move just now makes me have serious doubts as to whether it would even be worth pursuing the upstreamification any further. Hopefully we can avoid this.

fzurita commented 7 years ago

I don't think it's a bad idea to check those files in. I'm not sure how we would categorize them though. Under the tools folder, we could have a script to generate the files and people could submit pull requests to submit their own versions.

bsmiles32 commented 7 years ago

These generated files allow to easily share struct member offsets between C files and assembly files, which greatly improve maintenance of the project and allowed good refactorings to happen (for instance moving all emulation state inside the "device" struct, which is a good step toward removing global variables from emulation code). These files are not committed because they ARE compiler/os/compile flag dependent, so generating them is the "right way" to go. There is no way around that, it is a "limitation" of the C language. If you find a better way of sharing struct offsets between C and assembler files in a cross platform / cross architecture / cross os way please contribute it upstream. I don't consider it a regression as it greatly improved the overall maintenance of the project (no more manual error-prone offset fiddling) and I tell you that as someone who actually have to deal with/fight against the new_dynarec.

inactive123 commented 7 years ago

Well, this makes any libretro upstreamization completely impossible for me then.

I'd rather have globals if it means we don't have to generate a bunch of address definition headers in order to compile the base code. We can talk about globals being bad until the cows come home, but at the end of the day, requiring a bunch of files to be generated in order to get something to compile, and these files being OS/compiler-specific is completely and utterly terrible compared to how things were before. I'm sorry but I find this to be a very poor compromise that effectively slams the door shut on any hopes there is of ever getting libretro upstream. I hope you reconsider so I can reconsider as well because this really is a massive setback. I would like nothing better than to be on the same page with upstream but nearly all previous design issues I had with the upstream emulator which prevented upstreaming fell on deaf ears before, so I am especially worried that unless I loudly voice my disapproval of this, it effectively makes the point of no return inevitable.

I am afraid to say you did not make the codebase better by doing this, you made it much worse. This is constructive criticism.

fzurita commented 7 years ago

@twinaphex out of curiosity, what makes it so hard to generate files at compile time for libretro cores?

bsmiles32 commented 7 years ago

The files are auto-generated, the process is transparent for developers, so I don't get what annoys you. Committing "volatile" files is a no-no for me, but if you feel it's preferable for your fork feel free to do so.

Issues you raised previously against upstream are tracked in our issue tracker. If someone wants to address them they can. If they ar not addressed, it is not that we don't ear you, it is just that nobody has devoted time to do them. This is how free software works sorry. Also threatening us with by saying that you won't upstream your work, is not constructive. If you want things to happen upstream, make them happen upstream by sending PR, otherwise, maintenance and divergence from upstream is extra work for you, not us.

inactive123 commented 7 years ago

If you want things to happen upstream, make them happen upstream by sending PR, otherwise, maintenance and divergence from upstream is extra work for you, not us.

You made the codebase worse for pedantic 'code quality' dogmatic stances like 'no globals', which necessitated generating files now at compilation time because now you need to know the offset position to a variable inside a struct where previously this wasn't necessary. Now the onus is on me to have to come up with a different approach that doesn't require runtime generation of files? But it was working perfectly fine that way before you decided that globals had to be killed and didn't plan out in advance that this would then necessitate this frankly horrendous design decision.

Honestly, you guys are giving me the same brushing off like you guys did numerous times in past years, none of the design considerations we point out, you guys like to work with us on to help find a common solution. It's pretty clear you guys don't care, so why do I even bother at this stage? In case people wonder why the libretro port was never 'upstream', look no further than stuff like this right here, repeated x 1000 every year and always with the same response 'go figure out a solution yourself'. Oh well, time to stop caring then.

loganmc10 commented 7 years ago

Don't be so ridiculous, it's not a big deal, there is no reason why you can't implement this (https://github.com/mupen64plus/mupen64plus-core/blob/master/projects/unix/Makefile#L692-L694)

Besides, I don't get what the problem is, a year ago you wrote on libretro.com that ParaLLEl would have:

Completely rewritten CPU cores, both interpreter and dynarec. We want to fix the remaining CPU bugs that prevents Mupen64plus from being able to run certain games that PJ64 can, for instance. We also want to be able to move away from having two separate dynarec systems in Mupen64plus, where one is aimed at Atoms and ARM CPUs, and the other is meant for desktop x86, and neither is particulary fast.

Just do that, how hard could it be?

inactive123 commented 7 years ago

OK, so if you guys are all intent on shitting up Hacktarux's original codebase (we all know this entire emulator was mostly 99% written by one guy, after all, until he abandoned it) with these frankly obscene design decisions and we keep going in circles with zero willingness at all to strive for some common solutions, then we are done here. Have fun. I have no time for saltiness and little pot shots. Do what you like.

Regarding this certain guy above that follows me around anywhere and that thinks some fairly mediocre INI plugging makes him a 'core dev' now - everything that I wrote about these two divergent dynarec systems is true. I'ts a complete mess to have two divergent dynarec systems around. I understand that it's hard to unify them, but it honestly should be done at some point. I already merged the x86 and x64 duplicated files of the Hacktarux dynarec into one, to start with. Neither of these two dynarec systems you guys wrote BTW - one is written by Ari64, the other is written by Hacktarux. This entire emulator is mostly 99% written by Hacktarux, with the remaining 1% being you guys. LegendOfDragoon even agrees with me on the general slowness of both these dynarecs vs. say something like Project64.

We did more original work honestly on 'Mupen64plus' in Parallel RDP than any of you guys ever did in ages. None of you understand the RDP, none of you managed to move it onto hardware, so start paying some credit there where credit is due instead of brushing us off like this with zero respect being shown. We did more work for the sake of 'accurate RDP emulation' than any of you guys ever did, when the most you guys can claim credit for is rearranging the deck chairs on some scrappy HLE 'Glide64'-based video plugins made ages ago. Stop acting like you guys are in any way or shape the 'standard bearers' of anything N64 emulation-related when you can't back it up.

I have no issues with you guys being code maintainers over code you didn't even wite yourself, it happens all the time and a fair few cores we maintain are exactly like that. Hell, I will even give you that some of the commits have been improvements (not all though). However, it would behoove you to be more inclusive and collaborative towards other projects when fact is, you guys don't know how 80 to 90% of this codebase you are tooling away at even works, and that some of the design considerations you introduce are frankly regressive, and some of the design choices are still bad to this day. Like this 'main loop' that I've pointed out over and over again is just badly written and should get an entire makeover. See the Github issues raised about that. Zero fucks given to collaborate on anything or strive for a common solution in years. It takes two to tango and to collaborate on a project. When only one side gives a damn and it's shown time and time again that people are not willing to find a common ground, then perhaps it's time to move on.

bsmiles32 commented 7 years ago

@twinaphex Please keep civil and refrain from personal attack here. Yes the m64p codebase is not pretty, yes having 2 suboptimal dynarecs is not good and we'd like to improve the current situation, yes being able to break out of the emulation loop could be an improvement. But these changes are pretty hard to make in this decade old codebase. We instead prefer to tackle smaller problem first, and move along. Yes that takes more time, but we make progress.

You claim we didn't bring any value over vanilla mupen64, yet you forked from us not mupen64. In the parallel codebase I see mostly code directly taken from mupen64plus, except for a nice RDP and RSP implementation (congrats to Tiny Tiger) and DD support (Luigi Blood). Yet I fail to see any contribution of you (Twinaphex) upstream except for some code in glide64 some time ago. And yes I have some original work of my own in there which is not just refactoring other people code (which is in itself something... see the original codebase to understand what I mean) : I implemented many ucode in HLE which where never done before : musyx, variants of JPEG ucode, several missing audio commands (remember the GoldenEye missing sound).... So yeah I have I think a better understanding of the codebase and N64 inner working than what you make it seems.

So yeah, good luck with your project and stop spreading fud around.

inactive123 commented 7 years ago

@bsmiles32 You are the only one I have any regard left for in this team, and still do. I didn't mean to disparage the work you did, you are the only one who has actually managed to make real strides. That's why I hoped you would be reasonable and pragmatic in trying to find a solution for the issue I presented here, instead of just brushing it off and pretending it was not important.

EDIT: I edited the latter part of the comment here in hopes that cooler heads can prevail here and there is still a chance we can find common ground here and figure out a solution we can both agree on.

bsmiles32 commented 7 years ago

Could you elaborate more precisely why compile time generation of source code bother you so much. I mean :

  1. does it breaks on one platform you support that we don't ?
  2. Is it not as transparent as we think ?
  3. Would you prefer if generated files didn't pollute the source tree ?
  4. Or just personal preference ?

If it is 1, 2 or 3 we will make changes upstream based on your input. If it's 4, it is okay and allow me to disagree here. With some care it should be possible for you to keep the usage of global variables while not deviating too much from upstream.

For the record, compile time generation of files is common practice on many non trivial projets (for instance when defining a lexer+parser) and we're not the only project using this technique for sharing structure definition between ASM and C, the linux kernel does it too, but it's implementation relies on GCC-ism which cannot work on msvc. Also our implementation works on GCC, Clang,linux,Mac,msvc. Is it ugly ? Yes. Is the ugliness contained ? Yes (just define the struct member in asm_define and add the generated header where you need it). Does it affect runtime ? No.

inactive123 commented 7 years ago

Thanks for responding in a polite manner, I really appreciate that.

Back to your question, I'd say it's a mixture of 1, 2, and 3. For one, MAME ran into this pitfall many times before in the past where requiring to generate files at compile time would introduce all sorts of concurrency problems when wanting to compile with multiple jobs. But really, the main sticking point here is maintenance. The main problem with it is that it simply does not take care of itself when we have to add more and more platforms to the mix.

That some other non-trivial projects do it is a rather tough pill to swallow though considering that we didn't have to do this right now if these variables hadn't been made part of a struct, thus necessitating you now from needing to know their offset positions.

I'd say that if the compile-time definition headers are absolutely unavoidable from your view, at least commit these generated files to the repo so we don't have to rely on autogenerating them at startup. That'd be a start. After that, by closely studying how the offsets differ, surely you should be able to see some patterns at some point that would allow us to take care of some of these divergences with compile-time defines instead of having to resort to generate an entire header at compile-time.

bsmiles32 commented 7 years ago

The point you raise about concurrent builds can be resolved by generating the header files not inside the source tree, but in the build dir and sticking to one build dir per build. This will also avoid potential problems when building for different platform as they will be in separate build dir. That, I can try to make it in upstream.

However, there is litteraly no point in committing the generated header files as they are a mere compilation byproduct. This is also why putting them in the build dir is a better option than on the source tree. As for your suggestion of using compile time defines I don't see how to implement that and what it would solves that our compile-time generated header doesn't.

Without disrespect, I suspect (and I may be wrong) that you don't fully understand how this works so I invite you to try to compile our project on your machine and study how all this generation process works.

bsmiles32 commented 7 years ago

Fair enough. You don't have to backport my "dogshit" work in your fork.

inactive123 commented 7 years ago

Nothing I say seems to change your mind anyway, so you can do what you want to do, I'm not going to write a wall of text on it. It will be bad for the codebase from a maintenance and portability perspective, but oh well, everything I say seemingly falls on deaf ears anyway. Hopefully I am wrong and I am pleasantly surprised later on. I made a big issue out of it not to annoy people or to antagonize people, but because my intentions are sincere to try to prevent a codebase from going astray, and I feel this move done here is definitely a very big step backwards, and it could be hard to undo if enough time passes, leading to a point of no return.