Fix compilation on MSVC

[ndiddy99]

• Fix section macro in settings.h so it compiles on MSVC.
• Fix non-C89 compliant variable declaration in co_derive.

Tested on Visual Studio 2022 and Visual C++ 2008.

[Screwtapello]

Thanks for your PR!

I'm a little surprised; we have MSVC in our CI setup, and it hasn't encountered problems before:

https://github.com/higan-emu/libco/blob/9b76ff4c5c7680555d27c869ae90aa399d3cd0f2/.github/workflows/ci.yml#L165-L190

Do you know if there's some way we could enhance our CI system to detect such problems in future?

[ndiddy99]

For the section problem, it looks like your script only builds libco with LIBCO_MPROTECT declared, so the MSVC-specific section declaration stuff in settings.h never gets exercised.

For the C89 problem, it doesn't look like there's a way to force C89 variable declarations on newer MSVC. I only caught it when I tried building the project with Visual C++ 2008. Microsoft added C99 variable declarations to the "Legacy MSVC" standard in Visual C++ 2013. There is the "/Za" option to turn off Microsoft extensions and force strict C89, but that causes compilation errors as some of the Windows headers assume Microsoft extensions are on. My recommendation for catching C89 issues would be adding -std=c89 -Wpedantic to the line in build.sh/build.bat that builds libco.c, so the Linux builds will catch them. Alternatively, you could try shoehorning an old version of MSVC into your build system, but that seems like a pain in the ass.

[Alcaro]

That could be troublesome, since long long isn't a thing in C89.

[ndiddy99]

Thanks for pointing that out, I forgot about that. Maybe -std=c89 -Wdeclaration-after-statement would work better?

It looks like Microsoft decided to call their 64-bit variable type __int64 rather than long long like everyone else, but I think you can ignore that unless someone wants to both use old MSVC and compile for 64-bit (targeting Windows XP x64 Edition or something).

[Screwtapello]

I edited the CI script to reproduce the problem with the stringification, then cherry-picked your fix and verified that it fixed the problem, so I've pushed that as a new commit. I'll look into the C89 stuff next.

[Screwtapello]

I added the -std=c89 -Wpedantic configuration for the Linux CI and got it to build and now it produces warnings: "ISO C90 does not support ‘long long’" as mentioned above, "ISO C90 forbids mixed declarations and code" which this PR fixes, and one more:

In file included from ../../libco.c:13:
../../amd64.c: In function ‘co_derive’:
../../amd64.c:132:15: warning: ISO C forbids conversion of object pointer to function pointer type [-Wpedantic]
  132 |     co_swap = (void (*)(cothread_t, cothread_t))co_swap_function;
      |               ^

co_swap_function is the char array used to store the custom stack-swapping machine code. I guess this is illegal because it makes the code non-portable to Harvard architectures or something.

Ultimately, I think the main reason Near was so careful to aim for C89 was for the benefit of MSVC, so perhaps -std=c89 -Wdeclaration-after-statement is the best way forward after all.

[Alcaro]

Yeah, the C spec doesn't guarantee data and function pointers have the same size. Sometimes a function pointer must also contain a pointer to the target dll's data section, because there's no absolute or pc-relative addressing mode, only register-relative. I think Itanium does that.

But afaik POSIX demands they are, in fact, the same size (and even if it doesn't, they are on all platforms I'm aware of). On Itanium, I believe that's implemented by function pointers actually being pointers to structs, containing the globals pointer and the actual function pointer.

So yeah, just ignore that warning. Strict C89 compliance is just a pedantic waste of time, what matters is whether it works in the compilers you care about.

[ndiddy99]

Strict C89 compliance is just a pedantic waste of time, what matters is whether it works in the compilers you care about.

Agreed, it was a dumb suggestion. Sorry about that!

[Screwtapello]

I have merged e18e09d634d612a01781168ad4d76be10a7e3bad which adds C89 checks to the CI system, and fixes the other affected implementations in addition to amd64 and x86. Thanks for the PR, even if I didn't merge your commits exactly!