cast between incompatible function types

[Xeverous]

I get this on Windows while building the bootstrap project using MinGW-w64 with GCC 10.2.1

C:\mingw64\mingw64\bin\g++.exe -DFlextGL_EXPORTS -DMagnumGLObjects_EXPORTS -DNOMINMAX -DUNICODE -DWIN32_LEAN_AND_MEAN -D_UNICODE @CMakeFiles/MagnumGLObjects.dir/includes_CXX.rsp -O3 -DNDEBUG -std=c++11 -Wall -Wextra -Wold-style-cast -Winit-self -Werror=return-type -Wmissing-declarations -Wpedantic -fvisibility=hidden -fvisibility-inlines-hidden -Wzero-as-null-pointer-constant -Wdouble-promotion -fno-strict-aliasing -o CMakeFiles\MagnumGLObjects.dir\DebugOutput.cpp.obj -c [...]external\magnum\src\Magnum\GL\DebugOutput.cpp
[...]external\magnum\src\Magnum\Platform\Implementation\DpiScaling.cpp: In function 'bool Magnum::Platform::Implementation::isWindowsAppDpiAware()':
[...]external\magnum\src\Magnum\Platform\Implementation\DpiScaling.cpp:158:46: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'HRESULT (*)(HANDLE, PROCESS_DPI_AWARENESS*)' {aka 'long int (*)(void*, PROCESS_DPI_AWARENESS*)'} [-Wcast-function-type]
  158 |         auto* const getProcessDpiAwareness = reinterpret_cast<HRESULT(WINAPI *)(HANDLE, PROCESS_DPI_AWARENESS*)>(GetProcAddress(shcore, "GetProcessDpiAwareness"));
      |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]external\magnum\src\Magnum\Platform\Implementation\DpiScaling.cpp:169:36: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'BOOL (*)()' {aka 'int (*)()'} [-Wcast-function-type]
  169 |     auto const isProcessDPIAware = reinterpret_cast<BOOL(WINAPI *)()>(GetProcAddress(user32, "IsProcessDPIAware"));
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Second thing, I'm worried about the -fno-strict-aliasing. It basically means the code has undefined behavior in proper standard mode - does it come from some dependecy or is it magnum's addition?

[Amphaal]

About the warnings, and since this compiler behavior is expected, a quick workaround to silence them without obnoxious #pragma would be a preliminary reinterpret_cast<void*> (https://stackoverflow.com/a/38434088).

[Xeverous]

I see. So basically it's GetProcAddress's fault because it does not return FARPROC - IMO it should be declared to return void* so I like the proposed 2-step casting.

[mosra]

Hum. On the CI MinGW build, I don't get this warning but I suppose it's because there I use GCC 7 and this warning got added in GCC 8. It doesn't warn on MSVC or clang-cl, so I guess going with a GCC-specific #pragma is the way to go.

Can I ask you to test this patch? Thanks! :)

diff --git a/src/Magnum/Platform/Implementation/DpiScaling.cpp b/src/Magnum/Platform/Implementation/DpiScaling.cpp
index 262e972f6..25feecc7b 100644
--- a/src/Magnum/Platform/Implementation/DpiScaling.cpp
+++ b/src/Magnum/Platform/Implementation/DpiScaling.cpp
@@ -155,7 +155,17 @@ bool isWindowsAppDpiAware() {
        need. */
     HMODULE const shcore = GetModuleHandleA("Shcore.dll");
     if(shcore) {
+        /* GCC 8 adds -Wcast-function-type, enabled by default with -Wextra,
+           which causes this line to emit a warning on MinGW. We know what
+           we're doing, so suppress that. */
+        #if defined(CORRADE_TARGET_GCC) && __GNUC__ >= 8
+        #pragma GCC diagnostic push
+        #pragma GCC diagnostic ignored "-Wcast-function-type"
+        #endif
         auto* const getProcessDpiAwareness = reinterpret_cast<HRESULT(WINAPI *)(HANDLE, PROCESS_DPI_AWARENESS*)>(GetProcAddress(shcore, "GetProcessDpiAwareness"));
+        #if defined(CORRADE_TARGET_GCC) && __GNUC__ >= 8
+        #pragma GCC diagnostic pop
+        #endif
         PROCESS_DPI_AWARENESS result{};
         return getProcessDpiAwareness && getProcessDpiAwareness(nullptr, &result) == S_OK && result != PROCESS_DPI_UNAWARE;
     }
@@ -166,7 +176,17 @@ bool isWindowsAppDpiAware() {
        correctly. */
     HMODULE const user32 = GetModuleHandleA("User32.dll");
     CORRADE_INTERNAL_ASSERT(user32);
+    /* GCC 8 adds -Wcast-function-type, enabled by default with -Wextra, which
+       causes this line to emit a warning on MinGW. We know what we're doing,
+       so suppress that. */
+    #if defined(CORRADE_TARGET_GCC) && __GNUC__ >= 8
+    #pragma GCC diagnostic push
+    #pragma GCC diagnostic ignored "-Wcast-function-type"
+    #endif
     auto const isProcessDPIAware = reinterpret_cast<BOOL(WINAPI *)()>(GetProcAddress(user32, "IsProcessDPIAware"));
+    #if defined(CORRADE_TARGET_GCC) && __GNUC__ >= 8
+    #pragma GCC diagnostic pop
+    #endif
     CORRADE_INTERNAL_ASSERT(isProcessDPIAware);
     return isProcessDPIAware();
 }

About the strict aliasing -- I consider that a GCC wart, no other complier does such extreme assumptions about pointer aliasing. Back in the GCC 4 days, there was a version that pushed the strict aliasing optimizations too far, breaking random code, and making it work there involved a ton of work, so I just gave up and disabled it altogether since then. (There's other well-known libraries that are known to be broken with strict aliasing such as stb_image, but Magnum doesn't have a hard dependency on those, so it's not related to them.)

Moreover, from a language lawyer perspective, the new containers such as StridedArrayView wouldn't even be possible to implement without violating strict aliasing rules unless you are the Standard Library itself and are allowed to ignore them, so I find this part of standard just thoroughly broken and out-of-touch with reality and refuse to comply with it. What Linus said.

[Xeverous]

It doesn't warn on MSVC or clang-cl, so I guess going with a GCC-specific #pragma is the way to go.

I'm not sure but recent Clang versions could also have this warning. I like the idea of intermediate void* cast, because the core issue lies in GetProcAddress declared to return FARPROC which is not true. Unix equivalents of this function return void*.

Can I ask you to test this patch?

It works cleanly.

About the strict aliasing -- I consider that a GCC wart, no other complier does such extreme assumptions about pointer aliasing.

It's not really a wart because standard allows it. Of course I'm aware as some committee members said - compilers could match standard 100% perfectly and yet be useless in practice but for strict aliasing problems IMO there are better solutions than adding a flag that affects whole code:

• __attribute__((__may_alias__)) (C++ should have a standard attribute for it)
• type punning through unions: well, this is another gray area in the C++ standard, but C does allow it (in contrast to C++ which defines it as UB) and every compiler allows unions for type punning both in C and C++

Last question - is the -fno-strict-aliasing a PUBLIC or PRIVATE CMake propety? I'm mostly interested if it "poisons" other code that indirectly depends on magnum.

[mosra]

Unix equivalents of this function return void*.

Yep, and that produces another kind of warning that has to be suppressed, so we wouldn't really win anything in terms of amount of useless noise :)

It's not really a wart because standard allows it.

The standard also allows for chars that don't have 8 bits or floats that don't follow IEEE 754, but does it make practical sense to account for those in current real-world code? No. So, for me, strict aliasing is a GCC-specific source of problems.

attribute / unions

Those work for the tiny cases where one is reinterpreting a float as an int as such, but wouldn't be practical for the above-mentioned StridedArrayView and others (and that's the same problem as the proposed std::mdspan is hitting). For example:

struct Position {
    float x, y;
};

Containers::StridedArrayView1D<Position> data;
Containers::StridedArrayView1D<float> y = data.slice(&Position::y); // aliases data in a nontrivial way

The -fno-strict-aliasing is a flag applied private, doesn't poison code depending on Magnum. This might change in the future, though, depending on what problems I hit in user code.

---

Thanks for testing the patch! 👍 It's now running through CIs together with a larger batch of changes, will let you know when it lands in master.

[Xeverous]

Thanks for testing the patch!

Testing it was very trivial for me because I'm using the CMake subproject approach - I went for it from the start for 2 reasons:

• it's easier to bundle and prepare my project on different environments
• I plan to use incremental builds as much as possible
• I'm going for standalone + web-based builds both using ImGUI and it's somewhat experimental so I (and you) might expect more issues as I move forward

Also, big plus for the bootstrap project. It helped a lot.

Also, the strided array view you just posted gave me much better overview of what's possible with it than it's documentation.

[mosra]

Patch pushed in 19d1693. Sorry for the delay, it was among a bunch of other changes that took a while to get right.