Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sharadhr
Copy link

@sharadhr sharadhr commented Aug 21, 2024

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.

- Added definitions for various SAL annotations as macros with empty values in CMakeLists.txt, and set them as `INTERFACE`
@sharadhr sharadhr changed the title Define SAL annotations as empty macros in CMake WIL::WIL target for MinGW and Clang Improve ergonomics of WIL::WIL CMake target for consumers Aug 21, 2024
@sharadhr sharadhr changed the title Improve ergonomics of WIL::WIL CMake target for consumers Improve ergonomics of WIL::WIL CMake target for consumers using LLVM/MinGW Aug 21, 2024
@dunhor
Copy link
Member

dunhor commented Aug 23, 2024

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.

@dunhor dunhor added the help wanted Extra attention is needed label Aug 23, 2024
@m417z
Copy link
Contributor

m417z commented Aug 23, 2024

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
Copy link
Member

dunhor commented Aug 23, 2024

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
Copy link
Contributor

m417z commented Aug 23, 2024

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
Copy link
Member

dunhor commented Aug 23, 2024

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
Copy link
Author

sharadhr commented Aug 23, 2024

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
Copy link

Biswa96 commented Sep 6, 2024

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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants