search

taisei-project / taisei

A free and open-source Touhou Project fangame
https://taisei-project.org/
Other
1.12k stars 97 forks source link

Embrace GNU extensions and abandon "strict" standard compliance. #318

Closed Akaricchi closed 3 years ago

Akaricchi commented 3 years ago

This PR abandons our previous commitment to standard C11 compatibility and introduces some preliminary cleanups that become possible thanks to certain GNU C extensions. We intend to support reasonably recent versions of GCC and clang, but no other compilers from now on. Here are some of the arguments in favor of this change, copypasted from the discord discussion:

Akaricchi commented 3 years ago

cc @makise-homura

makise-homura commented 3 years ago

I've checked this branch right now.

It still generates a lot of -Wstrict-prototypes, -Wunreachable-code, -Winvalid-builtin-arg, -Wdeprecated-declarations warnings (you may check attached build log to figure out what I'm talking about), but at least it compiles on lcc:1.25.09:Oct--1-2020:e2k-v4-linux. Guess "elbrus-check" is passed then.

(Actually build from scratch has failed due to the same problem I found several months ago, it's python-zstandard wasn't ported to e2k, and fixing this upstream is still in progress; but I've built it with my patch and installed, and once I did it, taisei build has finished successfully).

By the way: is there any way to test if taisei is runnable w/o running GUI? (IIRC there was something like "start up with null renderer, run a game for a couple of frames, then exit", but I can't find now how to do this). There are some issues with GUI on my test machine for now, so I can't test it with actual play. taisei-log.txt

StarWitch commented 3 years ago

@makise-homura You can run Taisei using the testing replay we have for CI. It'll start it "headless".

/path/to/taisei -R misc/ci/tests/test-replay.tsr
Akaricchi commented 3 years ago

@makise-homura

To expand on @StarWitch's answer on running Taisei in non-graphical mode: the general way to do this is via TAISEI_RENDERER=null or taisei --renderer=null. This will still create an SDL window though; use SDL_VIDEODRIVER=dummy if you want to avoid that. taisei -R or taisei --verify-replay does both of those things automatically, but it also disables audio playback and avoids calling the drawing code at all.

Akaricchi commented 3 years ago

Tested it with a 2-stage replay against master, no desyncs so far.

makise-homura commented 3 years ago

Uh, I finally broke my test machine at all several days ago, and just now got it running again (even with video, so I tried to playcheck taisei, and it goes nice). But...

/path/to/taisei -R misc/ci/tests/test-replay.tsr

Well. Taisei works pretty normal (still there's a couple of errors like it can't find res/shader/viewport.pp and res/shader/global.pp, but it looks like these resources are optional). But if I call it with -r /usr/src-remote/taisei/git/upstream/misc/ci/tests/test-replay.tsr, it quits just after a first replay frame is shown, without any significant error messages (just as if I played a normal game). Is it supposed to be like this (e.g. it's a replay for a couple frames)? If I call it with -R instead of -r, it produces a bunch of was not preloaded warnings (I think it's ok, as we didn't preload resources first, like we do in non-headless mode), but there's also Failed to load bgm 'stage1' from '<path unknown>' error message, and it looks really suspicious (in normal game, there's everything ok with first stage BGM, it plays normally). So there's two questions:

Also it says, like, Could not call method 'RegisterGame' on 'com.feralinteractive.GameMode' when taisei starts, and Could not call method 'UnregisterGame' on 'com.feralinteractive.GameMode' both because of The name com.feralinteractive.GameMode was not provided by any .service files. I guess it's ok, if there is no systemd in my OS?

* You can get around the `python-zstandard` problem for now with `meson configure -Dpackage_data=false`.

I've already did that with my patch to python-zstandard, but thanks, I'll keep this in mind.

* It seems like your compiler does not implement `#pragma GCC diagnostic` properly (or at all?). This is the reason for the `-Wstrict-prototypes` spam from `consideredharmful.h`, as well as some of the `-Wdeprecated-declarations` warnings.

Yes, it does not. EDG does not handle #pragma GCC diagnostic, it handles #pragma diag_suppress N to suppress some diagnostic, and #pragma diag_default N to revert to default behavior (see EDG manual 1.14.5). No pop or push sadly. For -Wdeprecated-declarations N is 1215, for -Wstrict-prototypes it is 3117, -Wunused-variable is 3145, -Wunreachable-code is 111, -Wclobbered is ignored, and -Wignored-attributes is not known to compiler. I'm not sure why -Wdeprecated is ignored in api.h while it has an effect afaik only for C++ files, not C. Is there a decision made to support only GCC/clang-style of diagnostics (that one with push/pop)? Or is it reasonable to implement, say, universal diagnostic mechanism? (I see it like bunch of argumentless macros like DISABLE_WARN_DEPRECATED_DECLARATIONS, DISABLE_WARN_STRICT_PROTOTYPES, DISABLEWARNWHATEVER_ELSE, and ENABLE_WARNINGS in place of current implementation which uses GCC-styled arguments).

* The `-Winvalid-builtin-arg` warning on `__builtin_assume_aligned` is misapplied: if anything, it should be `-Wunused-result`, according to the actual warning message. But even then, GCC and Clang do not generate such warnings for this builtin, because it makes no sense. I don't think this can be reasonably fixed in Taisei code.

That's really something strange. I guess I'd investigate this some time.

* At least some of the `-Wunreachable-code` warnings are actually valid, such as the one in `stage2/nonspells/midboss_nonspell_1.c`. I'm surprised neither gcc nor clang has caught those!

Afaik, gcc and clang have their unreachability detection not so precise, because they don't required to do so in most of the situations. For VLIW compiler, on other side, it's vital, because it has to build register maps, windows, stack regions and stuff, so it is required to look for execution paths very carefully. We have ever seen examples, where it's clear that code is unreachable, but at least gcc don't catch it.

Akaricchi commented 3 years ago

@makise-homura sorry for the delayed response

still there's a couple of errors like it can't find res/shader/viewport.pp and res/shader/global.pp, but it looks like these resources are optional

This is expected; I really need to tone down the error messages for those somehow. Those aren't provided by the game, they're for defining custom postprocessing shaders by the user (an undocumented half-baked feature).

But if I call it with -r /usr/src-remote/taisei/git/upstream/misc/ci/tests/test-replay.tsr, it quits just after a first replay frame is shown, without any significant error messages (just as if I played a normal game). Is it supposed to be like this (e.g. it's a replay for a couple frames)?

Yes, that replay is just a couple of frames (a very barebones smoke test).

If I call it with -R instead of -r, it produces a bunch of was not preloaded warnings (I think it's ok, as we didn't preload resources first, like we do in non-headless mode)

Indeed, preloads are disabled in headless.

but there's also Failed to load bgm 'stage1' from '' error message, and it looks really suspicious (in normal game, there's everything ok with first stage BGM, it plays normally).

That's also normal. This is because the "null" audio backend can't actually load anything.

How could I know replay has been desynced or played till the end without errors?

In the -R mode (which is an alias for --verify-replay, by the way), the game will quit with a non-zero exit code if it detects a desync. In normal -r/--replay mode, it'll begin spamming desync warnings to the log but will continue playing the replay (incorrectly). You'll also see a red (DESYNCED) label at the bottom left corner of the screen if you run the game with graphics on.

Also it says, like, Could not call method 'RegisterGame' on 'com.feralinteractive.GameMode' when taisei starts, and Could not call method 'UnregisterGame' on 'com.feralinteractive.GameMode' both because of The name com.feralinteractive.GameMode was not provided by any .service files. I guess it's ok, if there is no systemd in my OS?

I don't think systemd has anything to do with it, that's a DBus thing. I'm guessing you compiled with gamemoded support enabled, but the GameMode daemon isn't running, maybe? If it is running but you're still getting that error, it's probably best to file a bug on the GameMode repo.

Is there a decision made to support only GCC/clang-style of diagnostics (that one with push/pop)? Or is it reasonable to implement, say, universal diagnostic mechanism? (I see it like bunch of argumentless macros like DISABLE_WARN_DEPRECATED_DECLARATIONS, DISABLE_WARN_STRICT_PROTOTYPES, DISABLE_WARN_WHATEVER_ELSE, and ENABLE_WARNINGS in place of current implementation which uses GCC-styled arguments).

GCC does not have an equivalent to EDG's #pragma diag_default as far as I know, so you really can't abstract this in a compiler-agnostic manner. I think this is acceptable, given that EDG/LCC is closed source. I'm willing to merge EDG/LCC-specific fixes for stuff that doesn't compile, but you'll probably have to deal with the annoying warnings as long as your compiler doesn't support GCC-style pragmas, sorry.

Afaik, gcc and clang have their unreachability detection not so precise, because they don't required to do so in most of the situations. For VLIW compiler, on other side, it's vital, because it has to build register maps, windows, stack regions and stuff, so it is required to look for execution paths very carefully. We have ever seen examples, where it's clear that code is unreachable, but at least gcc don't catch it.

I think in that case, the compiler definitely should not warn on __builtin_unreachable() in unreachable places — the whole point of that builtin is to inform the compiler about an unreachable path. If the compiler can figure it out on its own, it should just ignore the hint, not warn about it.

Akaricchi commented 3 years ago

Missed this one

I'm not sure why -Wdeprecated is ignored in api.h while it has an effect afaik only for C++ files, not C.

I think that's just a typo, it was supposed to be -Wdeprecated-declarations. Either way, it's not even needed anymore; the enclosed code doesn't use anything deprecated now.

makise-homura commented 2 years ago

Ok, I get mostly everything. Now it's much more clear to me.

I'm guessing you compiled with gamemoded support enabled, but the GameMode daemon isn't running, maybe?

Yes, I think it's right: it has been used as a wrap/submodule IIRC, and I didn't explicitly turn it off, so looks like it is compiled with gamemode support.

GCC does not have an equivalent to EDG's #pragma diag_default as far as I know, so you really can't abstract this in a compiler-agnostic manner.

If we want to cover all the poosible ways, yes. But in cases where push/pop is used in the way of enable something and then disable it again (when you really know what you're disabling back to default), it may be an option. But also it may be an unneeded complexity, yes.

but you'll probably have to deal with the annoying warnings as long as your compiler doesn't support GCC-style pragmas, sorry.

Ok, got it. So those who use EDG compilers (or other that are incompatible with gcc in that manner) should just deal with it and force their compiler vendors to follow the 'industry standards' which is gcc/clang. At least EDG 6.3 already has a sort of draft diagnostic push/pop expressions, and it must have landed in lcc 1.26.09 (but I still have no OS distro that is compatible with 1.26.x, only 1.25.x).

the whole point of that builtin is to inform the compiler about an unreachable path. If the compiler can figure it out on its own, it should just ignore the hint, not warn about it.

Yes, by design it's right, but still there is a complete mess with whether the compiler could detect unreachable code, what should it do when encountering this builtin, and should it produce a warning or error in case it does not agree with this builtin use. That may be a massive research. Btw, there's only 4 places in Tasei now where this warning is issued: src/resource/resource.c:328, src/stages/stage1/draw.c:80, src/stages/stage2/nonspells/boss_nonspell_2.c:105 (here's no builtin used actually), and src/util/miscmath.c:413.

Actually, with -Ddeprecation_warnings=ignore, the only things remaining are:

Akaricchi commented 2 years ago

But in cases where push/pop is used in the way of enable something and then disable it again (when you really know what you're disabling back to default)

The problem is that it is impossible to really know. The user could have passed custom warning flags as c_args, and we must respect that. Even if we didn't want to support that, it's possible that we may change the defaults in the future (it happened before), and we shouldn't have to do that in more than one place (the meson.build file).

Yes, by design it's right, but still there is a complete mess with whether the compiler could detect unreachable code, what should it do when encountering this builtin, and should it produce a warning or error in case it does not agree with this builtin use. That may be a massive research.

__builtin_unreachable() is meant to override the compiler's analysis. If the compiler "disagrees", it should just assume that the programmer knows better, because that's what the builtin is for. If the usage is wrong and the code actually happens to be reachable at runtime, then that's undefined behavior (and 100% the programmer's fault); anything is allowed to happen here.

Consider this contrived example:

int foo(void);
int bar(void);

int foobar(int mode) {
    switch(mode) {
        case 0: return foo();
        case 1: return bar();
        default: __builtin_unreachable();
    }
}

Unless the mode argument is a known compile-time constant at every call site of foobar(), the compiler can't generally prove that the __builtin_unreachable() is indeed unreachable, but the program may still be valid. For example, mode may come from some external library function that always returns 0 or 1. The compiler can't know that, but the programmer can.

Now, a very smart compiler could emit a warning for something like foobar(42), where 42 is a compile-time constant, or generally if it can prove beyond a shadow of a doubt that the argument is neither 0 nor 1. GCC and clang don't do that; they'll happily emit broken (and divergent) code in this case — which is fine, because it's undefined behavior.

-Wstrict-prototypes spam: I hope it will vanish once functions from consideredharmful.h are not needed anymore, and the whole this file is deleted;

consideredharmful.h redeclares some standard library functions with the deprecated attribute to prevent people from accidentally using them; that's probably not going away. But we can probably declare them with their proper prototypes to avoid the whole -Wstrict-prototypes issue. This will be a requirement for C2x compatibility anyway.

Unknown pragma in refs.c stating to "Delete this file when no users left", so I guess it will be deleted some time in the future too;

#pragma message is a pretty common and simple extension; perhaps it's possible to get EDG to implement it? Though it's not very important at all.

And a ton of -Winvalid-builtin-arg that I don't exactly know what to do with:

I believe this is a compiler bug, this warning makes no sense. It should just disappear.