Fix MSVC warning C4273 "inconsistent dll linkage" #30491
Conversation
When using CMake, the user can select the MSVC runtime library to be: 1) Statically-linked (with the corresponding `x64-windows-static` vcpkg triplet) or 2) Dynamically-linked (with the corresponding `x64-windows` vcpkg triplet) In the latter case, the compiler emits the C4273 warning. As the "Necessary on some platforms" comment does not apply to MSVC, skip the declaration for MSVC.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Friendly ping:
|
|
utACK 7703884 |
|
utACK 7703884. |
There was a problem hiding this comment.
Microsoft says on Windows it's coming from Stdlib.h, which something else must be including. (Well, Microsoft says it's called _environ, but I'm pretty sure the linker fixes that up for us).
From what I can tell, this will simply fail to link if it's not found somewhere. Also, this is just another opportunistic entropy source for us, so setting aside a Debian+OpenSSL-type whoopsie, I don't think it's a huge deal if this is null anyway.
So.. as long as this doesn't break anything, fine by me.
utACK 7703884
Though, I don't like the slippery slope notion here. Should we go ahead and disable runtimes/link modes we don't support? Just because CMake enables it doesn't mean we want to start supporting it.
I agree. It seems that rather than just porting what we currently build for MSVC, and adding support for new things later (once we've decided what to do), we've instead ported master + assumed support for a number of other things that we don't otherwise test/maintain, or use to ship releases. Just because CMake might support various different Windows linkage models, or runtimes, etc, I think there's still a discussion to be had around if we as a project actually want to actively support all these things, want to test them in CI etc. Hopefully we can spend some more time improving the release builds we actually ship to Windows users, i.e #30210, before we worry too much about supporting the matrix of different ways to use Visual Studio to compile Bitcoin Core. |
7703884 Fix MSVC warning C4273 "inconsistent dll linkage" (Hennadii Stepanov) Pull request description: Broken out of bitcoin#30454. When using CMake, the user can select the MSVC runtime library to be: 1) Statically-linked (with the corresponding `x64-windows-static` vcpkg triplet) or 2) Dynamically-linked (with the corresponding `x64-windows` vcpkg triplet) In the latter case, the compiler emits the [C4273](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4273) warning. As the "Necessary on some platforms" comment does not apply to MSVC, skip the declaration for MSVC. The MSVC build system in the master branch supports the statically-linked runtime only: https://github.com/bitcoin/bitcoin/blob/ed739d14b58b5e772a65b85bb421703963b06852/build_msvc/common.init.vcxproj.in#L65 ACKs for top commit: sipa: utACK 7703884 sipsorcery: utACK 7703884. theuni: utACK 7703884 Tree-SHA512: a42e1a0d48973217462e703c418f3e9ef9cb5236267c1bf32912aacaf68976cdd2b9229168523f7c2a99ee3f2fb1bf8add4f342796bdb1e4063ca026b761db51
7703884 Fix MSVC warning C4273 "inconsistent dll linkage" (Hennadii Stepanov) Pull request description: Broken out of bitcoin#30454. When using CMake, the user can select the MSVC runtime library to be: 1) Statically-linked (with the corresponding `x64-windows-static` vcpkg triplet) or 2) Dynamically-linked (with the corresponding `x64-windows` vcpkg triplet) In the latter case, the compiler emits the [C4273](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4273) warning. As the "Necessary on some platforms" comment does not apply to MSVC, skip the declaration for MSVC. The MSVC build system in the master branch supports the statically-linked runtime only: https://github.com/bitcoin/bitcoin/blob/ed739d14b58b5e772a65b85bb421703963b06852/build_msvc/common.init.vcxproj.in#L65 ACKs for top commit: sipa: utACK 7703884 sipsorcery: utACK 7703884. theuni: utACK 7703884 Tree-SHA512: a42e1a0d48973217462e703c418f3e9ef9cb5236267c1bf32912aacaf68976cdd2b9229168523f7c2a99ee3f2fb1bf8add4f342796bdb1e4063ca026b761db51
2f751ed fixup! Merge bitcoin#30567: qt, build: Drop `QT_STATICPLUGIN` macro (pasta) 142245d Merge bitcoin#29733: build, macos: Drop unused `osx_volname` target (fanquake) 02f81e5 Merge bitcoin#23511: require glibc 2.18+ (pasta) 9f0e4ae Merge bitcoin#29706: depends: set two CMake options globally (fanquake) be07bbe Merge bitcoin#28846: depends: fix libmultiprocess build on aarch64 (fanquake) 0dea194 Merge bitcoin#28856: depends: Build the `native_capnp` and `capnp` packages with CMake (fanquake) a23eee1 partial Merge bitcoin#23619: build: Propagate user-defined flags to host packages (fanquake) 7cdacdc Merge bitcoin#30513: depends: Bump `libmultiprocess` for CMake fixes (merge-script) 4f44750 Merge bitcoin#30491: Fix MSVC warning C4273 "inconsistent dll linkage" (merge-script) 5ba1309 Merge bitcoin#30567: qt, build: Drop `QT_STATICPLUGIN` macro (merge-script) Pull request description: ## Issue being fixed or feature implemented Batch of more PRs that I found during make work ## What was done? ## How Has This Been Tested? hasn't yet ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 2f751ed UdjinM6: utACK 2f751ed Tree-SHA512: 1d8433daaf8dc8c8f04beca1cf0281f0dc29a623e5e8ed941bcb556568d72d8ce0ac5b5c001b10645fdffaa4e7083b76d61075049b2418bb8dd9b5ba0f53a8a9
Broken out of #30454.
When using CMake, the user can select the MSVC runtime library to be:
x64-windows-staticvcpkg triplet) orx64-windowsvcpkg triplet)In the latter case, the compiler emits the C4273 warning.
As the "Necessary on some platforms" comment does not apply to MSVC, skip the declaration for MSVC.
The MSVC build system in the master branch supports the statically-linked runtime only:
bitcoin/build_msvc/common.init.vcxproj.in
Line 65 in ed739d1