Workaround for MSVC 16.3.* pre-c++17 type trait linkage

[erichkeane]

Summary

MSVC's type_trait header for the 16.3.* release in pre-c++17 mode
exposes explicitly specialized constexpr variables for _Is_integral,
is_void_v, and _Is_floating_point as not-inline (since that isn't
available in pre-C++17). The result is duplicate-symbols in any
program that includes <type_traits> more than once.

This patch works around this issue by making fairly specific cases (in
the system header, in MSVC mode, and in pre-c++17 mode) be weak_odr
linkage rather than External linkage.

[rnk]

I thought we agreed to remove the workaround that we had for this and just declare that the old MSVC STL release was buggy and users should update:
https://bugs.llvm.org/show_bug.cgi?id=42027
It's a point release that preserves ABI compatibility, so there's less motivation for us to add a compat hack.

In general, we should seriously think about sunsetting some MS compat hacks that aren't needed for new SDK/STL versions to save on clang maintenance costs. The complexity they add is surprisingly expensive. For example, -fdelayed-template-parsing is a recurring source of compat issues in clang-tools-extra (see the commit logs), and I would like to revive D66394 to get it disabled by default soon. (+@thakis)

[erichkeane]

In D70791#1762081, @rnk wrote:

I thought we agreed to remove the workaround that we had for this and just declare that the old MSVC STL release was buggy and users should update:
https://bugs.llvm.org/show_bug.cgi?id=42027
It's a point release that preserves ABI compatibility, so there's less motivation for us to add a compat hack.

In general, we should seriously think about sunsetting some MS compat hacks that aren't needed for new SDK/STL versions to save on clang maintenance costs. The complexity they add is surprisingly expensive. For example, -fdelayed-template-parsing is a recurring source of compat issues in clang-tools-extra (see the commit logs), and I would like to revive D66394 to get it disabled by default soon. (+@thakis)

Ah! I KNEW I had seen this bug before. I searched the bug database for a while for it.

I agree with sunsetting MS compat hacks as well, though I think we would need to support at least the most recentish versions. My test team tells me that this failure isn't fixed until the (yet to be released) 16.4 version of the MSVC, so at the moment we don't compile the 'current' version without -std=c++17. I don't see why r363191 was reverted, though I believe that this is both a less intrusive change, and is heavily specialized to only deal with this one case so hopefully it would not get reverted for the same reason.

[rnk]

My version of cl.exe claims to have version 19.23.28107 and my headers are in Tools/MSVC/14.23.28105/include, and I see that this is fixed in xtr1common. is_integral_v and the helpers are defined with implicit template instantiations. There are no fully specialized template definitions in that header. So, my hope is that this problem will go away as users run the VS updater. Although, for buildbots and users such as myself who rarely start the IDE, that may take longer than we'd like.

[erichkeane]

In D70791#1762155, @rnk wrote:

My version of cl.exe claims to have version 19.23.28107 and my headers are in Tools/MSVC/14.23.28105/include, and I see that this is fixed in xtr1common. is_integral_v and the helpers are defined with implicit template instantiations. There are no fully specialized template definitions in that header. So, my hope is that this problem will go away as users run the VS updater. Although, for buildbots and users such as myself who rarely start the IDE, that may take longer than we'd like.

Ah, I see! I have no idea what that version is unfortunately... 14.23 is just 16.3 I think, but no idea which of the _10_ sub releases that is (https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes). My tester claims to have a completely up to date version, but I'll have them check.

[erichkeane]

Microsoft recently released 16.4 and has ceased support for the 16.3 line, so it isn't really important to me anymore. I'd thought this was a 'hold our nose and do it' type fix, but I don't think that is justified any longer now that Microsoft doesn't even support it.

[STL_MSFT]

I would like to revive D66394 to get it disabled by default soon.

As I recently informed @thakis via email, our compiler documentation was incorrect (I've informed the doc team and they're going to publish a fixed article soon). MSVC's behavior defaults to Microsoft one-phase (delayed template lookup) as it always has, changes to Standard two-phase under /permissive-, and can be escape-hatched back to Microsoft one-phase with /permissive- /Zc:twoPhase-.

clang/lib/AST/ASTContext.cpp

case ExternalASTSource::EK_Always:
     return GVA_AvailableExternally;
 
   case ExternalASTSource::EK_ReplyHazy:
     break;
   }
   return L;
 }
 
+// The MSVC type_traits header defines a handful of type-trait constexpr
+// variables as non-inline in pre-C++17 mode which causes duplicate symbols.
+// In C++17 cases, they mark these as 'inline', which avoids this issue.
+// This fixup alters the linkage to be odr for this case.
+static bool isMSVCPreCxx17TypeTrait(const ASTContext &Context,
+                                    const VarDecl *VD) {
+  assert(VD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization &&
+         "Only valid for explicit specializations");
+  return VD->isConstexpr() && !VD->isInline() &&
+         !Context.getLangOpts().CPlusPlus17 &&
+         Context.getSourceManager().isInSystemHeader(VD->getLocation());
+}
+
 GVALinkage ASTContext::GetGVALinkageForFunction(const FunctionDecl *FD) const {
   return adjustGVALinkageForExternalDefinitionKind(*this, FD,
            adjustGVALinkageForAttributes(*this, FD,
              basicGVALinkageForFunction(*this, FD)));
 }
 
 static GVALinkage basicGVALinkageForVariable(const ASTContext &Context,
                                              const VarDecl *VD) {
static GVALinkage basicGVALinkageForVariable(const ASTContext &Context,
   }
 
   switch (VD->getTemplateSpecializationKind()) {
   case TSK_Undeclared:
     return StrongLinkage;
 
   case TSK_ExplicitSpecialization:
     return Context.getTargetInfo().getCXXABI().isMicrosoft() &&
-                   VD->isStaticDataMember()
+                   (VD->isStaticDataMember() ||
+                    isMSVCPreCxx17TypeTrait(Context, VD))
                ? GVA_StrongODR
                : StrongLinkage;
 
   case TSK_ExplicitInstantiationDefinition:
     return GVA_StrongODR;
 
   case TSK_ExplicitInstantiationDeclaration:
     return GVA_AvailableExternally;

clang/test/CodeGenCXX/Inputs/ms-constexpr-typetraits.h

+template <class>
+constexpr bool _Is_integral = false;
+
+template <>
+constexpr bool _Is_integral<bool> = true;
+
+template <class>
+constexpr bool _Is_floating_point = false;
+
+template <>
+constexpr bool _Is_floating_point<float> = true;
+
+template <class>
+constexpr bool is_void_v = false;
+
+template <>
+constexpr bool is_void_v<void> = true;

clang/test/CodeGenCXX/microsoft-type-traits-pre-cxx17.cpp

+// RUN: %clang_cc1 -emit-llvm -triple=x86_64-pc-win32 -std=c++14 -o - %s -isystem%S/Inputs | FileCheck %s
+#include <ms-constexpr-typetraits.h>
+
+// CHECK: @"??$_Is_integral@_N@@3_NB" = weak_odr dso_local constant
+// CHECK: @"??$_Is_floating_point@M@@3_NB" = weak_odr dso_local constant
+// CHECK: @"??$is_void_v@X@@3_NB" = weak_odr dso_local constant
+