microsoft / wil

Windows Implementation Library
MIT License
2.57k stars 234 forks source link

Improve ergonomics of `WIL::WIL` CMake target for consumers using LLVM/MinGW #467

Open sharadhr opened 3 weeks ago

sharadhr commented 3 weeks ago

Relevant to #117 and #454 (the latter of which I feel is incomplete). In that PR users had to #define away SAL annotations used by WIL as macros with empty definitions, as well as add -fms-extensions to their command-line.

When consumers (e.g. Azure SDK for C++) use WIL with CMake and compile with Clang targeting MinGW, they will have to re-define these macros for every project that uses WIL, which will be repetitive and unintuitive.

This PR resolves this, at least for CMake.

Now, consumers may simply use target_link_libraries(my_target PRIVATE WIL::WIL) and these macros and options will be appended to every command-line invocation when building my_target, which considerably simplifies matters.

The function-like macros are added with target_compile_options because target_compile_definitions does not support function-like macros.

dunhor commented 2 weeks ago

While I don't see anything necessarily worrisome here, I also don't have much experience with MinGW (and no experience with MinGW with WIL), so I'm hoping others with more experience can comment here. From past issues/PRs, it seems @m417z or @Berrysoft might be able to provide feedback on this approach.

m417z commented 2 weeks ago

I don't personally use CMake, so I don't mind, but I think that while it can be a valid short term approach, the right solution here is to have these definitions included into MinGW (e.g. sal.h).

BTW the comment is not precise, as not all the defined macros are SAL annotations.

dunhor commented 2 weeks ago

the right solution here is to have these definitions included into MinGW

That sounds like it's probably the right approach to me. @sharadhr any issues with pursuing the Windows SDK specific changes in MinGW itself? That would just leave WIL_NO_SLIM_EVENT and -fms-extensions. And even then, I'm curious if the slim event inclusion could be deduced in-code with preprocessor checks.

m417z commented 2 weeks ago

Perhaps WIL_NO_SLIM_EVENT can become unnecessary if ReadAcquire, WriteRelease are added into MinGW. Interestingly, they are defined somewhere (for a specific tool?), but not in the main include files.

Edit: Alternatively, WIL can be modified to use std::atomic. That can also help get rid of InterlockedDecrementNoFence and InterlockedIncrementNoFence.

dunhor commented 2 weeks ago

Alternatively, WIL can be modified to use std::atomic. That can also help get rid of InterlockedDecrementNoFence and InterlockedIncrementNoFence

It could maybe be conditionally updated to do so, however WIL cannot in general assume access to the STL

sharadhr commented 2 weeks ago

That sounds like it's probably the right approach to me. @sharadhr any issues with pursuing the Windows SDK specific changes in MinGW itself?

Good point. I could submit a request/patch to the mailing list. I have already submitted some, and they were merged in not too long after.

I could also ask for ReadAcquire and WriteRelease (and the entire associated mechanisms) to be added to the public MinGW headers, too.


In all honesty, this entire effort is to get the Azure SDK for C++ compiling on MinGW, and that entire stack exposed just how incomplete MinGW is. For instance, <webservices.h> is not available, either.

Biswa96 commented 1 week ago

Could you provide steps to reproduce any compiler error with mingw-w64 toolchain? It would help to add the required macros in mingw-w64. Please reply to this email https://sourceforge.net/p/mingw-w64/mailman/message/58813213/