fix: Add MINGW compiler macro for MSYS2 Clang64 environment#5209
fix: Add MINGW compiler macro for MSYS2 Clang64 environment#5209feihongmeilian wants to merge 2 commits intopocoproject:mainfrom
Conversation
In the MSYS2 clang64 compilation environment, although the compiler is Clang, it is based on the MinGW runtime underneath. In this case, the Windows SEH syntax with __try/__except in the code is incompatible, which leads to compilation failures. By detecting the MINGW32/MINGW64 macros under the Clang compiler branch and defining POCO_COMPILER_MINGW, subsequent code can skip the __try/__except related logic to ensure compilation compatibility.
|
Note: In my opinion, a better approach would be to separate platform detection (MINGW32/MINGW64) from compiler detection, and define POCO_COMPILER_MINGW in a standalone check instead of duplicating the logic inside each compiler branch. This would make the code cleaner and easier to maintain. However, since I don't have access to enough platforms and environments for full validation, I'm applying this minimal fix for now to resolve the compilation issue on MSYS2 Clang64. The separated detection logic can be refactored in a future change after more comprehensive testing. |
| #include <vector> | ||
|
|
||
| #ifdef _WIN32 | ||
| #ifdef _WIN32 && defined(_MSC_VER) |
There was a problem hiding this comment.
Bug: #ifdef only accepts a single identifier — it does not support expressions. The && defined(_MSC_VER) part will either be silently ignored or cause a preprocessor error depending on the compiler/mode.
// Current (broken):
#ifdef _WIN32 && defined(_MSC_VER)
// Should be:
#if defined(_WIN32) && defined(_MSC_VER)That said, see my top-level review comment — I believe this change (and all the other __declspec guard changes) should actually be reverted entirely.
| #include <stdio.h> | ||
|
|
||
| #ifdef _WIN32 | ||
| #ifdef _WIN32 && defined(_MSC_VER) |
There was a problem hiding this comment.
Bug: Same #ifdef vs #if syntax error as in cpptrace.hpp. #ifdef does not support compound expressions.
// Current (broken):
#ifdef _WIN32 && defined(_MSC_VER)
// Would need to be:
#if defined(_WIN32) && defined(_MSC_VER)| #endif | ||
| #else | ||
| #if defined(_DLL) || defined(_USRDLL) | ||
| #if defined(_DLL) || defined(_USRDLL) && defined(_MSC_VER) |
There was a problem hiding this comment.
Bug: operator precedence. In the C preprocessor && binds tighter than ||, so this evaluates as:
#if defined(_DLL) || (defined(_USRDLL) && defined(_MSC_VER))
The defined(_DLL) path is completely unguarded by _MSC_VER, which is almost certainly not the intent. If this change were to be kept, it should be:
#if (defined(_DLL) || defined(_USRDLL)) && defined(_MSC_VER)
|
|
||
|
|
||
| #if defined(_WIN32) | ||
| #if defined(_WIN32) && defined(_MSC_VER) |
There was a problem hiding this comment.
Regression: This breaks MinGW plugin loading. With this change, MinGW builds will fall through to the __GNUC__ visibility branch, which uses ELF-style __attribute__((visibility("default"))). That attribute has no effect on Windows — plugins built with MinGW will fail to export the required ClassLoader entry points.
__declspec(dllexport) is correct and supported by all Windows compilers (MSVC, GCC-MinGW, Clang-MinGW). The original #if defined(_WIN32) guard was right here.
| // defined with this macro as being exported. | ||
| // | ||
| #if (defined(_WIN32) || defined(__CYGWIN__)) && defined(POCO_DLL) | ||
| #if defined(_WIN32) && defined(_MSC_VER) && defined(POCO_DLL) |
There was a problem hiding this comment.
Regression: The original guard included defined(__CYGWIN__). This change silently drops Cygwin DLL export/import support. The same issue applies to NetSSL_OpenSSL/include/Poco/Net/NetSSL.h and NetSSL_Win/include/Poco/Net/NetSSL.h.
| #if __has_include(<cxxabi.h>) | ||
| #define POCO_HAVE_CXXABI_H | ||
| #endif | ||
| #if defined (__MINGW32__) || defined (__MINGW64__) |
There was a problem hiding this comment.
👍 This addition is correct and valuable. Currently, MSYS2 Clang64 gets POCO_COMPILER_CLANG but not POCO_COMPILER_MINGW, which means MinGW-specific workarounds (e.g. avoiding __try/__except SEH) do not trigger. This is the right fix for the core issue described in this PR.
|
|
||
| #ifdef _WIN32 | ||
| #ifdef _WIN32 && defined(_MSC_VER) | ||
| #define CPPTRACE_EXPORT_ATTR __declspec(dllexport) |
There was a problem hiding this comment.
Maintenance concern: This is a vendored upstream header (cpptrace). Changes here will be overwritten on the next vendor update. If a fix is needed, it would be better handled via a patch file or build-system defines that avoid modifying the vendored source directly.
matejk
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for working on MSYS2 Clang64 support! The diagnosis is correct — Clang64 in MSYS2 targets MinGW but doesn't support MSVC-specific __try/__except SEH syntax, and POCO_COMPILER_MINGW isn't being set in that environment.
However, the approach of gating all __declspec(dllexport/dllimport) behind _MSC_VER is fundamentally incorrect and will break shared library builds for all non-MSVC Windows compilers.
Critical issues
-
__declspec(dllexport/dllimport)works on all Windows compilers — GCC-MinGW and Clang-MinGW fully support__declspec. Restricting it to MSVC-only means MinGW/Clang shared library builds will produce DLLs with no exported symbols. -
Preprocessor syntax errors — Two files use
#ifdef X && defined(Y)which is invalid (#ifdeftakes a single token). See inline comments oncpptrace.hppandctrace.h. -
Operator precedence bug —
sqlparser_win.hhasdefined(_DLL) || defined(_USRDLL) && defined(_MSC_VER)where&&binds tighter than||, leaving the_DLLpath unguarded. -
Cygwin support silently dropped — Three files previously included
__CYGWIN__in their guards; this is removed without discussion. -
ClassLibrary.h regression — MinGW plugin builds will fall through to the GCC visibility attribute branch, which has no effect on Windows.
-
Vendored headers modified —
cpptrace.hppandctrace.hare upstream vendored files; direct edits will be lost on the next update.
What should change
- Keep the
POCO_COMPILER_MINGWaddition inPlatform.h(this is the correct fix) - Revert all
__declspecguard changes —__declspec(dllexport/dllimport)is correct for all Windows compilers - Fix the actual issue — guard
__try/__exceptSEH usage sites with!defined(POCO_COMPILER_MINGW)ordefined(POCO_COMPILER_MSVC)where SEH is used - Split out the ODBCVER
0x0300→0x0350bump into a separate PR for cleaner history
|
I added the check for _MSC_VER because when compiling POCO, the JSON library depends on the Foundation library. Without the _MSC_VER check, the code would use the __declspec syntax. The following code exists in Foundation\include\Poco\Dynamic\Struct.h: It is mandatory to add Foundation_API before the template classes within the POCO_OS_FAMILY_WINDOWS block as well; otherwise, the function definitions cannot be found. However, MSVC (Microsoft Visual C++) behaves correctly in this scenario: it automatically adds Foundation_API to template classes, so the missing Foundation_API in the Windows branch does not cause issues for pure MSVC builds. |
|
I am not very familiar with submitting changes to GitHub. For each submission, I end up forking the repository again from scratch. Should I close this current pull request (PR), re-fork the repository, and then create three separate branches in the forked repository to submit my changes individually? |
|
You can add more commits to the branch you created for the PR and push them. |
|
@feihongmeilian Thanks for the detailed explanation about the However, I believe this was already resolved by PR #5202 (merged Feb 12, before this PR was created). That PR changed all the #if defined(POCO_OS_FAMILY_WINDOWS)
extern template class Struct<std::string>; // no _API macro
#else
extern template class Foundation_API Struct<std::string>;
#endifto: #if defined(POCO_OS_FAMILY_WINDOWS) && defined(Foundation_EXPORTS)
extern template class Struct<std::string>; // only when building the DLL
#else
extern template class Foundation_API Struct<std::string>; // consumers + non-Windows
#endifThis means MinGW consumers now always take the Given that, the
Could you rebase on current |
|
I've reverted the commit regarding symbol exports and split the ODBCVER modification into a new PR. However, after updating my local branch to main, I'm still encountering compilation failures with Clang in MSYS2. I may need to explore alternative approaches to fix this issue going forward. |
|
@matejk I have concerns about the appropriateness of the following implementation: When POCO_OS_FAMILY_WINDOWS is defined and Foundation_EXPORTS is set, the code path extern template class Structstd::string; will be taken. Could this result in missing symbol exports from the DLL when the function is used? This approach centralizes the export/import control in the Foundation_API macro itself (e.g., setting it to __declspec(dllexport), __declspec(dllimport) or an empty value based on compilation context), eliminating scattered conditional judgments and ensuring consistent symbol handling across all platforms and build scenarios. |
|
@feihongmeilian The current pattern is correct. The When building the DLL (
When consuming the DLL (
Your suggestion to unconditionally use Regarding Clang on Windows: both Clang-CL and Clang/MinGW support If you're still seeing build failures with Clang64 after rebasing on current |
In the MSYS2 clang64 compilation environment, although the compiler is Clang, it is based on the MinGW runtime underneath. In this case, the Windows SEH syntax with __try/__except in the code is incompatible, which leads to compilation failures.
By detecting the MINGW32/MINGW64 macros under the Clang compiler branch and defining POCO_COMPILER_MINGW, subsequent code can skip the __try/__except related logic to ensure compilation compatibility.