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

[Win] LNK2005: bool const std::_Is_integral<bool> already defined #41372

Closed
llvmbot opened this issue May 26, 2019 · 16 comments
Closed

[Win] LNK2005: bool const std::_Is_integral<bool> already defined #41372

llvmbot opened this issue May 26, 2019 · 16 comments
Labels
bugzilla Issues migrated from bugzilla c++14

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented May 26, 2019

Bugzilla Link 42027
Resolution FIXED
Resolved on Jul 29, 2019 17:43
Version unspecified
OS Windows NT
Reporter LLVM Bugzilla Contributor
CC @BillyONeal,@zygoloid,@rnk,@StephanTLavavej

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.

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented May 26, 2019

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 26, 2019

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).
(but my main concern is this self-hosting issue)

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented May 26, 2019

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.)

@rnk
Copy link
Collaborator

rnk commented Jun 11, 2019

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Jun 11, 2019

I'm using VS 2019 Preview (16.2.0 Preview 1.0) (14.22.27706).

Here is aforementioned _Is_integral:

template
_INLINE_VAR constexpr bool _Is_integral = // determine whether cv-unqualified type _Ty is integral
_Is_any_of_v<_Ty, bool, char, signed char, unsigned char, wchar_t,
#if _HAS_CHAR8_T
char8_t,
#endif // _HAS_CHAR8_T
char16_t, char32_t, short, unsigned short, int, unsigned int, long, unsigned long, long long,
unsigned long long>;
#else // ^^^ workaround / no workaround vvv
template
_INLINE_VAR constexpr bool _Is_integral = false; // determine whether cv-unqualified type argument is integral

template <>
_INLINE_VAR constexpr bool _Is_integral = true;

template <>
_INLINE_VAR constexpr bool _Is_integral = true;

template <>
_INLINE_VAR constexpr bool _Is_integral = true;

... (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
#define _INLINE_VAR inline
#else // _HAS_CXX17
#define _INLINE_VAR
#endif // _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.

@rnk
Copy link
Collaborator

rnk commented Jun 11, 2019

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.

@BillyONeal
Copy link
Contributor

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
Visual C++ Libraries

@rnk
Copy link
Collaborator

rnk commented Jun 11, 2019

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?

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.

Is this some recent change after clang 8 to break this? (That's what we
currently test with)

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.

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Jun 11, 2019

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.

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.)

are we sure we want to give variable templates external linkage in C++14,
where inline variables aren't available?

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'.

@BillyONeal
Copy link
Contributor

Can a selectany constexpr variable be replaced by one with a different initializer at link time?

Yes, but under the usual inline penalty of ODR.

@rnk
Copy link
Collaborator

rnk commented Jun 11, 2019

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.

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.)

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.

are we sure we want to give variable templates external linkage in C++14,
where inline variables aren't available?

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'.

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'.

@rnk
Copy link
Collaborator

rnk commented Jun 12, 2019

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
template const int foo;
extern template <> const int foo;
const int *useit() { return &foo; }
// tu2.cpp
template const int foo;
template <> constexpr int foo = 42;

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.

@rnk
Copy link
Collaborator

rnk commented Jul 30, 2019

r363191 was reverted, but it seems this will be fixed in a soon-to-be-released version of the Visual C++ standard library.

@BillyONeal
Copy link
Contributor

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.

@zmodem
Copy link
Collaborator

zmodem commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#42843

@rnk
Copy link
Collaborator

rnk commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#45054

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla c++14
Projects
None yet
Development

No branches or pull requests

4 participants