Don't crash when a selectany symbol would get common linkage

[thakis]

Summary

Don't crash when a selectany symbol would get common linkage

Things can't both be in comdats and have common linkage, so never give things in comdats common linkage. Common linkage is only used in .c files, and the only thing that can trigger a comdat in c is selectany from what I can tell. Fixes PR23243.

Also address an over-the-shoulder review comment from rnk by moving the hasAttr<SelectAnyAttr>() in Decl.cpp around a bit. It only makes a minor difference for selectany on global variables, so it goes well with the rest of this patch.

[rnk]

lgtm

I think the only other way to get shouldBeInCOMDAT to return true is with static data members of class templates, but those never get common linkage anyway.

[thakis]

r235053, thanks

lib/AST/Decl.cpp

VarDecl::isThisDeclarationADefinition(ASTContext &C) const {
   //   [...] causes storage to be reserved for that object.
   // Note: that applies for all non-file-scope objects.
   // C99 6.9.2p1:
   //   If the declaration of an identifier for an object has file scope and an
   //   initializer, the declaration is an external definition for the identifier
   if (hasInit())
     return Definition;
 
-  if (hasAttr<AliasAttr>())
+  if (hasAttr<AliasAttr>() || hasAttr<SelectAnyAttr>())
     return Definition;
 
   // A variable template specialization (other than a static data member
   // template or an explicit specialization) is a declaration until we
   // instantiate its initializer.
   if (isa<VarTemplateSpecializationDecl>(this) &&
       getTemplateSpecializationKind() != TSK_ExplicitSpecialization)
     return DeclarationOnly;
 
-  if (!hasAttr<SelectAnyAttr>() && hasExternalStorage())
+  if (hasExternalStorage())
     return DeclarationOnly;
 
   // [dcl.link] p7:
   //   A declaration directly contained in a linkage-specification is treated
   //   as if it contains the extern specifier for the purpose of determining
   //   the linkage of the declared name and whether it is a definition.
-  if (!hasAttr<SelectAnyAttr>() && isSingleLineLanguageLinkage(*this))
+  if (isSingleLineLanguageLinkage(*this))
     return DeclarationOnly;
 
   // C99 6.9.2p2:
   //   A declaration of an object that has file scope without an initializer,
   //   and without a storage class specifier or the scs 'static', constitutes
   //   a tentative definition.
   // No such thing in C++.
   if (!C.getLangOpts().CPlusPlus && isFileVarDecl())

lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D) {
 
   // Emit global variable debug information.
   if (CGDebugInfo *DI = getModuleDebugInfo())
     if (getCodeGenOpts().getDebugInfo() >= CodeGenOptions::LimitedDebugInfo)
       DI->EmitGlobalVariable(GV, D);
 }
 
 static bool isVarDeclStrongDefinition(const ASTContext &Context,
-                                      const VarDecl *D, bool NoCommon) {
+                                      CodeGenModule &CGM, const VarDecl *D,
+                                      bool NoCommon) {
   // Don't give variables common linkage if -fno-common was specified unless it
   // was overridden by a NoCommon attribute.
   if ((NoCommon || D->hasAttr<NoCommonAttr>()) && !D->hasAttr<CommonAttr>())
     return true;
 
   // C11 6.9.2/2:
   //   A declaration of an identifier for an object that has file scope without
   //   an initializer, and without a storage-class specifier or with the
   //   storage-class specifier static, constitutes a tentative definition.
   if (D->getInit() || D->hasExternalStorage())
     return true;
 
   // A variable cannot be both common and exist in a section.
   if (D->hasAttr<SectionAttr>())
     return true;
 
   // Thread local vars aren't considered common linkage.
   if (D->getTLSKind())
     return true;
 
   // Tentative definitions marked with WeakImportAttr are true definitions.
   if (D->hasAttr<WeakImportAttr>())
     return true;
 
+  // A variable cannot be both common and exist in a comdat.
+  if (shouldBeInCOMDAT(CGM, *D))
+    return true;
+
   // Declarations with a required alignment do not have common linakge in MSVC
   // mode.
   if (Context.getLangOpts().MSVCCompat) {
     if (D->hasAttr<AlignedAttr>())
       return true;
     QualType VarType = D->getType();
     if (Context.isAlignmentRequired(VarType))
       return true;
llvm::GlobalValue::LinkageTypes CodeGenModule::getLLVMLinkageForDeclarator(
   // throw away these explicit instantiations.
   if (Linkage == GVA_StrongODR)
     return !Context.getLangOpts().AppleKext ? llvm::Function::WeakODRLinkage
                                             : llvm::Function::ExternalLinkage;
 
   // C++ doesn't have tentative definitions and thus cannot have common
   // linkage.
   if (!getLangOpts().CPlusPlus && isa<VarDecl>(D) &&
-      !isVarDeclStrongDefinition(Context, cast<VarDecl>(D),
+      !isVarDeclStrongDefinition(Context, *this, cast<VarDecl>(D),
                                  CodeGenOpts.NoCommon))
     return llvm::GlobalVariable::CommonLinkage;
 
   // selectany symbols are externally visible, so use weak instead of
   // linkonce.  MSVC optimizes away references to const selectany globals, so
   // all definitions should be the same and ODR linkage should be used.
   // http://msdn.microsoft.com/en-us/library/5tkz6s71.aspx
   if (D->hasAttr<SelectAnyAttr>())

test/CodeGen/ms-declspecs.c

 // RUN: %clang_cc1 -triple i386-pc-win32 %s -emit-llvm -fms-compatibility -o - | FileCheck %s
 
 __declspec(selectany) int x1 = 1;
 const __declspec(selectany) int x2 = 2;
 // CHECK: @x1 = weak_odr global i32 1, comdat, align 4
 // CHECK: @x2 = weak_odr constant i32 2, comdat, align 4
 
 // selectany turns extern variable declarations into definitions.
-extern __declspec(selectany) int x3;
+__declspec(selectany) int x3;
+extern __declspec(selectany) int x4;
 // CHECK: @x3 = weak_odr global i32 0, comdat, align 4
+// CHECK: @x4 = weak_odr global i32 0, comdat, align 4
 
 struct __declspec(align(16)) S {
   char x;
 };
 union { struct S s; } u;
 
 // CHECK: @u = {{.*}}zeroinitializer, align 16
 

test/CodeGen/ms-declspecs.cpp

 // RUN: %clang_cc1 -triple i386-pc-win32 %s -emit-llvm -fms-compatibility -o - | FileCheck %s
 
 // selectany turns extern "C" variable declarations into definitions.
 extern __declspec(selectany) int x1;
 extern "C" __declspec(selectany) int x2;
 extern "C++" __declspec(selectany) int x3;
 extern "C" {
 __declspec(selectany) int x4;
 }
+__declspec(selectany) int x5;
 // CHECK: @"\01?x1@@3HA" = weak_odr global i32 0, comdat, align 4
 // CHECK: @x2 = weak_odr global i32 0, comdat, align 4
 // CHECK: @"\01?x3@@3HA"  = weak_odr global i32 0, comdat, align 4
 // CHECK: @x4 = weak_odr global i32 0, comdat, align 4
+// CHECK: @"\01?x5@@3HA"  = weak_odr global i32 0, comdat, align 4