-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[Win] LNK2005: bool const std::_Is_integral<bool> already defined #41372
Comments
This code is invalid, but it seems easy enough for us to support. We'll need to decide if we want to special-case the problematic declarations or give weak_odr linkage to all (const?) variable template explicit specialisations. |
Well, since it is invalid, maybe this can be treated as another non-standard Microsoft extension? But then I'd prefer to see a warning from clang so that Microsoft people can fix their STL (and compiler). |
Given that this only causes a failure at link time, it's unlikely we can warn on it. (We could warn if we find an strong external definition in a header file, but that's likely to have false positives for .def-style #includes and the like.) |
Can you elaborate on the self-hosting issue? When I look in xtr1common I don't see anything that would be problematic. All the fully specialized variable templates delegate to class templates, which use static data members, which clang marks selectany. |
I'm using VS 2019 Preview (16.2.0 Preview 1.0) (14.22.27706). Here is aforementioned _Is_integral: template template <> template <> template <> ... (similar to this random file on the Internet: https://github.com/Chuyu-Team/VC-LTL/blob/master/VC/14.21.27702/include/xtr1common#L166) In C++14 mode _INLINE_VAR (defined in <yvals_core.h>) expands to nothing: #if _HAS_CXX17 (https://github.com/Chuyu-Team/VC-LTL/blob/master/VC/14.21.27702/include/yvals_core.h#L386) The error: LNK2005: "bool const std::_Is_integral" (??$_Is_integral@_N@std@@3_NB) already defined My understanding is that this is not clang problem (_INLINE_VAR should probably expand to __declspec(selectany) in this case, idk), so please feel free to close this bug. |
It happens that I was building locally with VC version 14.20.27508, which defines _Is_integral as a class template. You said you were using the preview version of VS 2019, so I figured this was a bug I could report back to the VC team before they release it. But first, I wanted to check if I had the most recently released version of 2019 to confirm the issue wasn't there. Of course, now that I've run the updater, it has the problem, and my local build is completely broken. :-[ I guess I'll go back to 2017, which I happen to still have installed, until I fix this bug. |
Does clang support inline constexpr in this form in C++14 mode (presumably with a warning we can suppress) so we could turn inline on for clang unconditionally? Is this some recent change after clang 8 to break this? (That's what we currently test with) Billy O'Neal |
It looks like clang accepts inline variables as an extension under -Wc++17-extensions. I tried using __declspec(selectany) as an alternative, but it doesn't seem to work with constexpr. Clang doesn't seem to consider selectany globals to be constant expressions, even if they are marked constexpr.
Yes, r359260. Looking at that again, are we sure we want to give variable templates external linkage in C++14, where inline variables aren't available? MSVC doesn't implement this DR yet, so if we want to be compatible, we have to go back to our previous behavior. |
Yeah, if we're not sure which definition of a variable will be selected, we don't allow its value to be read from within the constant evaluator. That was intended to apply to attribute((weak)), but seems to catch this case too. Maybe it shouldn't? Can a selectany constexpr variable be replaced by one with a different initializer at link time? (But this probably isn't a fruitful line of inquiry, since if we can add 'selectany', we can add 'inline' instead.)
If we make C++14 and C++17 differ in this regard, then we'll effectively have an ABI difference between C++14 and C++17 forever. If we take the hit now, we only have pain in the short term. I think the short-term pain is worth it. (Also of note: any odr-use of _Is_integral in an inline function will result in an ODR violation with the old model.) So, degrading from 'external' to 'weak_odr' seems OK, but I wouldn't want to go all the way to 'internal'. |
Yes, but under the usual inline penalty of ODR. |
We give __declspec(selectany) things _odr linkage, unlike attribute((weak)) things, so I think we can allow them in constexpr contexts. I'll file something for that. I believe the name 'selectany' is a reference to 'IMAGE_COMDAT_SELECT_ANY', which means the linker should pick any definition, because they should all be functionally equivalent.
Should we make fully specialized variable templates weak_odr on all platforms or just on Windows? I can easily imagine that users will write this kind of code on Linux, compile it for C++14, and get link errors. What do you recommend that they write instead, assuming they don't have access to 'inline'? I suppose it's legal for them to add 'static' to explicitly opt into the old behavior, but it somehow feels wrong. I'm leaning towards making all fully specialized, constant, integer, variable templates weak_odr, everywhere, kind of like we do for explicit template instantiations. BTW, I fixed my local build by hacking _INLINE_VAR to expand to 'inline'. |
I made clang give constexpr explicit specializations of variable templates linkonce_odr linkage in r363191. This makes it so clang doesn't emit these _Is_integral symbols in every object file that transitively includes xtr1common, and if they are needed, they won't cause duplicate symbol errors. This has essentially the same effect as implicitly considering all constexpr variable templates as being marked 'inline'. It isn't strictly conforming, because you could do this: // tu1.cpp With my change, clang will not emit foo in tu2.cpp as written. But, for constexpr variable templates, I think this will be very uncommon. The point of constexpr is to be able to write constants and put them in headers to enable further optimization. |
r363191 was reverted, but it seems this will be fixed in a soon-to-be-released version of the Visual C++ standard library. |
Yes, Stephan fixed this one. |
mentioned in issue llvm/llvm-bugzilla-archive#42843 |
mentioned in issue llvm/llvm-bugzilla-archive#45054 |
Extended Description
Self-hosting on Windows (https://github.com/llvm/llvm-project/blob/master/llvm/utils/release/build_llvm_package.bat) is broken by this commit:
llvm-mirror/clang@4241f67
Another way to reproduce the bug:
// h.hpp
#pragma once
template
constexpr bool v = false;
template <>
constexpr bool v = true;
// unit1.cpp
#include "h.hpp"
// unit2.cpp
#include "h.hpp"
int main()
{
return v;
}
clang++ -std=c++14 unit1.cpp unit2.cpp
error LNK2005: "bool const v" (??$v@_N@@3_NB) already defined
I've no idea if it is OK code for C++14, but this is what MS STL does in its header.
The text was updated successfully, but these errors were encountered: