Skip to content

Prevent warning C4273 for functions defined in libunshield.h#227

Open
ScaledLizard wants to merge 2 commits intotwogood:mainfrom
ScaledLizard:main
Open

Prevent warning C4273 for functions defined in libunshield.h#227
ScaledLizard wants to merge 2 commits intotwogood:mainfrom
ScaledLizard:main

Conversation

@ScaledLizard
Copy link
Copy Markdown

When building libunshield as a Windows DLL with MSVC, several functions are declared with UNSHIELD_API in headers but defined without it in source files. This leads to:

warning C4273 (inconsistent DLL linkage)

This patch ensures that all functions declared with UNSHIELD_API
are consistently annotated in their definitions as well.

Also, adds missing include of "internal.h" in log.c so that UNSHIELD_API is available.

Tested with:

MSVC 2022 (DLL + static builds)
Linux (no change in behavior)

No functional changes for non-Windows platforms.

…ns should be defined and implemented with consistent UNSHIELD_API (MSVC DLL builds)
@twogood
Copy link
Copy Markdown
Owner

twogood commented Apr 2, 2026

@kratz00 could this also solve why #225 fails on mingw?

@kratz00
Copy link
Copy Markdown
Contributor

kratz00 commented Apr 2, 2026

My understanding is that this is only needed for the declaration of a function (usually *.h) not for the definition of a function (usually *.c). With my current knowledge these changes are not needed, the CMake files need changes.
I will test it later today and test my claim 😄

@ScaledLizard
Copy link
Copy Markdown
Author

ScaledLizard commented Apr 2, 2026

Thanks for taking a look 🙂

You're right that on many platforms (especially GCC/Clang on Linux), it is usually sufficient to annotate only the declarations in the headers. However, if the declaration uses UNSHIELD_API but the definition does not, MSVC reports:

warning C4273 (inconsistent DLL linkage)

Adding UNSHIELD_API to the definitions ensures that declaration and definition match, which resolves these errors. You can probably reproduce this only on MSVC (tested in version 2022).

Also, i tested compiling unmodified unshield on MinGW64, and that worked without problems, so #225 seems unrelated.

@twogood
Copy link
Copy Markdown
Owner

twogood commented Apr 2, 2026

I have some scars from C/C++ development in Windows back in the 1990s so I see this as perfectly reasonable by Microsoft standards :)

@kratz00
Copy link
Copy Markdown
Contributor

kratz00 commented Apr 2, 2026

@ScaledLizard I've added a workflow which is using MSVC to #225 - it does build without errors as is - some parts are missing, see last comment in #225

@ScaledLizard
Copy link
Copy Markdown
Author

Admittedly, this is only about warnings, but warnings can be confusing as well. Some warnings actually are errors. But no rush, happy easter.

@twogood
Copy link
Copy Markdown
Owner

twogood commented Apr 8, 2026

Admittedly, this is only about warnings, but warnings can be confusing as well. Some warnings actually are errors. But no rush, happy easter.

Zero warnings is the goal!

@kratz00
Copy link
Copy Markdown
Contributor

kratz00 commented Apr 8, 2026

@ScaledLizard #225 is nearly ready (just need to add openssl + zlib dll) to the artifacts, maybe you can give it a try and see if you can reproduce the issue reported here?

@ScaledLizard
Copy link
Copy Markdown
Author

@kratz00 wrote: @ScaledLizard #225 is nearly ready (just need to add openssl + zlib dll) to the artifacts, maybe you can give it a try and see if you can reproduce the issue reported here?

Sorry, I don't see the relation between issue #225 and #227. MinGW and MSVC are obviously completely different compilers. I'm not involved in issue #225 and I don't have a test setup for it, but I tried compiling unshield 1.6.2 with MinGW and that worked (file attached). Sorry, I'm not sure how to help with this.

unshield.zip

@kratz00
Copy link
Copy Markdown
Contributor

kratz00 commented Apr 8, 2026

#225 also adds a workflow building unshield using MSVC. It does compile without warnings (with the current set of compiler flags) and does so without the changes of this pull requests. It might be different usages of compiler flags, MSVC or something else, but so far I am still not convinced that the changes of this pull request are really needed 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants