Fix crash on Windows with -j option

[chrchr-github]

https://sourceforge.net/p/cppcheck/discussion/general/thread/b806eec38c/

[chrchr-github]

No idea why Cygwin can't grok this,

[chrchr-github]

Seems like the Cygwin detection doesn't work, since the additional flags don't even show up.

[danmar]

Very quick fix!! 👍

[chrchr-github]

The idea of passing -mthreads came from here: https://stackoverflow.com/questions/20277625/why-does-this-usage-of-thread-local-crash
The flags are present now, but the executable still crashes.

[chrchr-github]

Do we have evidence that anyone even uses Cygwin? I was unable to build cppcheck on a fresh installation due to some missing defines.

[danmar]

Do we have evidence that anyone even uses Cygwin? I was unable to build cppcheck on a fresh installation due to some missing defines.

I don't know.

[chrchr-github]

Does anybody know what exact Cygwin setup the CI uses?

[chrfranke]

Do we have evidence that anyone even uses Cygwin?

I do frequently including a local build of cppcheck 2.7. Most or all of my cppcheck tickets were done with the Cygwin version. Cppcheck-2.7 could be build with a minor tweak: re-add #define THREADING_MODEL_FORK. Build of cppcheck-2.8 is broken due to other issues. The package in the Cygwin distro is unfortunately orphanded. I possibly will adopt it if it builds OOTB again.

[chrchr-github]

After messing around with defines and config.h, I was able to complete a build. But the executable didn’t crash, unlike the CI.

[chrchr-github]

Build of cppcheck-2.8 is broken due to other issues. The package in the Cygwin distro is unfortunately orphanded. I possibly will adopt it if it builds OOTB again.

So your build is broken, although we have Cygwin as a CI workflow? That's also interesting...

[chrfranke]

So your build is broken, although we have Cygwin as a CI workflow? That's also interesting...

Indeed. Which version of the Cygwin g++ toolchain and which make parameters do you use?

If _WIN32 is undefined (which is the case in Cygwin toolchain for ~10 years) and __CYGWIN__ is defined, the build obviously fails with "No threading model defined" in this section of config.h.

If this is fixed by either defining _WIN32 or USE_THREADS or removing ... && !defined(__CYGWIN__) ... (undoing decision from ticket 8973), other build errors occur with 2.8. The latter fix worked with 2.7. Sorry if I missed something. I didn't check current HEAD, but will do soon.

[chrfranke]

... other build errors occur with 2.8. ...

I take that partly back: make CYGWIN=1 FILESDIR=... works for 2.8 and also current HEAD (29402b4) if THREADING_MODEL_FORK is re-enabled with e.g.

--- lib/config.h.orig   2022-05-21 12:54:19.000000000 +0200
+++ lib/config.h        2022-07-03 14:13:55.919810500 +0200
@@ -111,5 +111,5 @@
 #define THREADING_MODEL_THREAD
 #define STDCALL
-#elif ((defined(__GNUC__) || defined(__sun)) && !defined(__MINGW32__) && !defined(__CYGWIN__)) || defined(__CPPCHECK__)
+#elif ((defined(__GNUC__) || defined(__sun)) && !defined(__MINGW32__)) || defined(__CPPCHECK__)
 #define THREADING_MODEL_FORK
 #else

A quick check of 2.8 with -j 5 on smartmontools code worked (currently we use 2.7 in our CI builds). Sorry for the noise which was somewhat unrelated to this PR.

[chrchr-github]

As far as I can tell, the CI installs Cygwin from a chocolatey package. I was testing with a fresh install from cygwin.org + gcc + g++.

[chrfranke]

The reason I asked was that Cygwin gcc is also available as a cross toolchain for Fedora which is typically much older. The chocolatey package only installs a recent Cygwin setup*.exe which should then always pull the most recent packages. I still wonder how your CI builds work for Cygwin (not for a MinGW* build on Cygwin), as it obviously (see above) fails in lib/config.h if _WIN32 is undefined and __CYGWIN__ is defined. So again my question: Which make Parameters does the CI build for Cygwin use?

[chrchr-github]

None, afaict:
https://github.com/danmar/cppcheck/blob/29402b4d1ab7cd9ed7bdc2c40271ff39cb3ce055/.github/workflows/CI-cygwin.yml#L33

[chrfranke]

Interesting. I will investigate this further and report the result separately as a ticket.

[firewave]

You can use gcc -dM -E - < /dev/null to get the built-in preprocessor defines.

According to some cleanups in https://gitlab.gnome.org/GNOME/libxml2/-/commit/2489c1d024906d0f72bbcf709aeb53a24602c219 _WIN32 should not be defined with Cygwin and was only for a limited time. But that was already noted in an earlier comment.

The Cygwin detection in the Makefile is also quite messy (since it is unfortunately). I once tried to make it easier but I can't remember how far I got and if I still got the changes somewhere.

The GitHub CI might also not be great for these "Linux on Windows" derivates. I had build failures with MinGW in the CI but it was building fine locally.

I have code locally which will provide the built-in define information in the build.

[chrchr-github]

I would even say that Cygwin detection does not work at all, see my earlier attempts in 54724ff

[firewave]

There's another GitHub action which installs Cygwin: https://github.com/cygwin/cygwin-install-action

I would even say that Cygwin detection does not work at all, see my earlier attempts in 54724ff

I think the detection is usually performed via uname. That approach obviously has portability issues.

Maybe we should try to build it with CMake and deprecate the (broken) make build for it to make things easier.

[firewave]

It seems that Cygwin installation is just extremely old or somehow broken.

Trying to build with MSYS2 also triggers the No threading model defined message. __CYGWIN__ is set for that as well as __MSYS__ - but not __WIN32. So it seems that one in the CI still uses a version which sets __WIN32 and __CYGWIN__.

MSYS2 also does not set WINNT in the make context.

A PR to enable a MSYS build will be posted some time in the future.

So we need to collect some more information from the Cygwin and MinGW jobs (compiler version, built-in defines, etc.) and then should probably disable them as they don't seem to represent what is current and go with local builds for now.

[chrfranke]

I would even say that Cygwin detection does not work at all, see my earlier attempts in 54724ff

The uname (--kernel-name) in the removed code didn't work because it includes the Windows flavor (here Win10 21H2). Using uname -o (--operating-system) should do the trick:

$ uname
CYGWIN_NT-10.0-19044

$ uname -o
Cygwin

[chrchr-github]

Yeah, but then why do Cygwin detection if those special flags aren't even needed for the CI build to succeed? The current Makefile is a mess.

It seems that Cygwin installation is just extremely old or somehow broken.

I agree.

[chrfranke]

You can use gcc -dM -E - < /dev/null to get the built-in preprocessor defines.

Add -xc++ to include c++only defines. Result on current cygwin with native and MinGW-w64 (cross-)compiler:

$ gcc -xc++ -dM -E - < /dev/null | egrep '(cplus|WIN32|CYGWIN|MINGW|VERSION)'
#define __cplusplus 201703L
#define __GXX_ABI_VERSION 1016
#define __VERSION__ "11.3.0"
#define __CYGWIN__ 1

$ x86_64-w64-mingw32-gcc -xc++ -dM -E - < /dev/null | egrep '(cplus|WIN32|CYGWIN|MINGW|VERSION)'
#define __MINGW32__ 1
#define __cplusplus 201703L
#define __WIN32__ 1
#define __GXX_ABI_VERSION 1016
#define __VERSION__ "11.3.0"
#define _WIN32 1
#define WIN32 1
#define __WIN32 1
#define __MINGW64__ 1

[chrchr-github]

From the CI:

#define _WIN32 1
#define __WIN32 1
#define __MINGW32__ 1
#define __cplusplus 201402L
#define __WIN32__ 1
#define __GXX_ABI_VERSION 1012
#define __VERSION__ "8.1.0"
#define WIN32 1
#define __MINGW64__ 1

[firewave]

From the CI:

#define _WIN32 1
#define __WIN32 1
#define __MINGW32__ 1
#define __cplusplus 201402L
#define __WIN32__ 1
#define __GXX_ABI_VERSION 1012
#define __VERSION__ "8.1.0"
#define WIN32 1
#define __MINGW64__ 1

So it isn't actually Cygwin. Or it invokes a totally different compiler.

It seems like they only carry the MinGW compiler now, but that should be at GCC 11.3.x: https://cygwin.com/pipermail/cygwin-announce/2022-May/010546.html

So MSYS is now more Cygwin than Cygwin - what the...

Also Cygwin 32-Bit will no longer be supported in the upcoming 3.4: https://cygwin.com/pipermail/cygwin-announce/2022-January/010439.html

https://github.com/cygwin/cygwin-install-action is using the actual Cygwin setup and not chocolatey. Maybe that should be used instead.

[chrchr-github]

The Cygwin build now uses THREADING_MODEL_FORK. There was some discussion a couple of years ago indicating that forking does not work so well: https://trac.cppcheck.net/ticket/8973
Maybe someone can verify this? @chrfranke

[firewave]

Is it really a Cygwin compiler? In one of the earlier attempts I saw that it's a MinGW compiler.

I also saw a hang with the MSYS build which is Cygwin based - no idea if it was with the thread or process code.

[chrchr-github]

Is it really a Cygwin compiler? In one of the earlier attempts I saw that it's a MinGW compiler.

I don't know what that means. The vanilla g++ that comes with Cygwin (apparently 8.1.0) produces a crashing binary. Installing the gcc-g++ package at least fixed that problem.

[firewave]

That means the compiler needs to have __CYGWIN__ defined. Otherwise it is not actually a Cygwin compiler.

[chrchr-github]

I think it is, since I was able to fix the build with 6b7538f

[firewave]

I think it is, since I was able to fix the build with 6b7538f

Alright. This whole thing is something to take a closer look at when the whole thread/process stuff is being simplified. The suppression/ErrorLogger stuff needs to be sorted out first and it's a bitch...

[danmar]

lgtm

[chrfranke]

The Cygwin build now uses THREADING_MODEL_FORK. There was some discussion a couple of years ago indicating that forking does not work so well: https://trac.cppcheck.net/ticket/8973 Maybe someone can verify this? @chrfranke

Short answer: The Cygwin fork() emulation works as expected.

Long answer: A POSIX emulation layer like Cygwin would be useless if fork() emulation is not reliable. Problems occur in most cases when the DLLs are not properly rebased (now done automatically by the setup tool) or too many installed DLLs exhaust the limited DLL address range on 32-bit Cygwin (which is now deprecated and will be retired soon). My local builds of recent cppcheck (with THREADING_MODEL_FORK fix in config.h) all worked as expected with -j option.

PS: You could also enable getloadavg() calls for Cygwin in ProcessExecutor::checkLoadAverage() because it meantime provides a reasonable emulation: https://sourceware.org/git/?p=newlib-cygwin.git;a=history;f=winsup/cygwin/loadavg.cc

[chrchr-github]

Thanks for the approval. Someone still has to merge it though...

[firewave]

Most or all of my cppcheck tickets were done with the Cygwin version.

So it seems like Cygwin is working quite okay. Do you think we can close https://trac.cppcheck.net/ticket/8973?

FYI a potential hang with -j and threads is being addressed in #4355,

[chrfranke]

So it seems like Cygwin is working quite okay. Do you think we can close https://trac.cppcheck.net/ticket/8973?

Yes. Build from current git HEAD uses THREADING_MODEL_FORK which works for me on 32-bit and 64-bit Cygwin without any problem.

[firewave]

So it seems like Cygwin is working quite okay. Do you think we can close https://trac.cppcheck.net/ticket/8973?

Yes. Build from current git HEAD uses THREADING_MODEL_FORK which works for me on 32-bit and 64-bit Cygwin without any problem.

Thanks. The ticket is now closed.