Warn on zero-sized structures inside extern "C" block (PR5065)

[sepavloff]

Summary

This patch adds warning on structures/unions that are empty or contain
only bit fields of zero size. Warnings are generated in C++ mode and
if only such type is defined inside extern "C" block.
The patch fixed PR5065.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

5721

Maybe name this warn_..._union_in_extern_c or similar?

lib/Sema/SemaDecl.cpp

12074–12075

You could also check that Record->getDeclContext()->getRedeclContext()->isFileContext() here (that is, it's not a local class nor a member class).

12076

This is redundant. In CPlusPlus mode, every RecordDecl is a CXXRecordDecl.

12105

Non-static data members can't have deduced types, so this is impossible.

12111–12132

It would be simpler to organize this as:

if (ZeroSize)
  Diag(..., getLangOpts().CPlusPlus ? diag::..._extern_c : diag::..._compat) ...

if (NonBitFields == 0 && !getLangOpts().CPlusPlus) {
  ...
}

[sepavloff]

Thank you for review.

include/clang/Basic/DiagnosticSemaKinds.td

5721

Renamed.

lib/Sema/SemaDecl.cpp

12074–12075

With this check the warning for inner struct would be absent in the following declaration:

struct pr5065_n2 {
    struct Inner {} x;
} q;

Expression sizeof(q.x) would give different results in C and C++.

12076

The check is removed.

12105

Removed.

[sepavloff]

Updated patch according to reviewer's notes. Added new tests.

[rsmith]

LGTM

lib/Sema/SemaDecl.cpp

12074–12075

OK, that's fair. On the other hand, this code means different things in C and C++ (in C, 'Inner' is a top-level type, not a nested type). Maybe we should have a separate warning for that?

[sepavloff]

Closed by commit rL194653 (authored by @sepavloff).

include/clang/AST/DeclBase.h

public:
   /// };
   /// @endcode
   /// Here, E is a transparent context, so its enumerator (Val1) will
   /// appear (semantically) that it is in the same context of E.
   /// Examples of transparent contexts include: enumerations (except for
   /// C++0x scoped enums), and C++ linkage specifications.
   bool isTransparentContext() const;
 
+  /// \brief Determines whether this context or some of its ancestors is a
+  /// linkage specification context that specifies C linkage.
+  bool isExternCContext() const;
+
+  /// \brief Determines whether this context or some of its ancestors is a
+  /// linkage specification context that specifies C++ linkage.
+  bool isExternCXXContext() const;
+
   /// \brief Determine whether this declaration context is equivalent
   /// to the declaration context DC.
   bool Equals(const DeclContext *DC) const {
     return DC && this->getPrimaryContext() == DC->getPrimaryContext();
   }
 
   /// \brief Determine whether this declaration context encloses the
   /// declaration context DC.

include/clang/Basic/DiagnosticGroups.td

 def NullConversion : DiagGroup<"null-conversion">;
 def ImplicitConversionFloatingPointToBool :
   DiagGroup<"implicit-conversion-floating-point-to-bool">;
 def BadArrayNewLength : DiagGroup<"bad-array-new-length">;
 def BuiltinMacroRedefined : DiagGroup<"builtin-macro-redefined">;
 def BuiltinRequiresHeader : DiagGroup<"builtin-requires-header">;
 def C99Compat : DiagGroup<"c99-compat">;
 def CXXCompat: DiagGroup<"c++-compat">;
+def ExternCCompat : DiagGroup<"extern-c-compat">;
 def GNUCaseRange : DiagGroup<"gnu-case-range">;
 def CastAlign : DiagGroup<"cast-align">;
 def : DiagGroup<"cast-qual">;
 def : DiagGroup<"char-align">;
 def Comment : DiagGroup<"comment">;
 def GNUComplexInteger : DiagGroup<"gnu-complex-integer">;
 def GNUConditionalOmittedOperand : DiagGroup<"gnu-conditional-omitted-operand">;
 def ConfigMacros : DiagGroup<"config-macros">;
def Most : DiagGroup<"most", [
     Trigraphs,
     Uninitialized,
     UnknownPragmas,
     Unused,
     VolatileRegisterVar,
     ObjCMissingSuperCalls,
     OverloadedVirtual,
     PrivateExtern,
-    SelTypeCast
+    SelTypeCast,
+    ExternCCompat
  ]>;
 
 // Thread Safety warnings 
 def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
 def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
 def ThreadSafetyPrecise    : DiagGroup<"thread-safety-precise">;
 def ThreadSafety : DiagGroup<"thread-safety",
                              [ThreadSafetyAttributes, 

include/clang/Basic/DiagnosticSemaKinds.td

def err_typecheck_cond_incompatible_operands_null : Error<
   "non-pointer operand type %0 incompatible with %select{NULL|nullptr}1">;
 def ext_empty_struct_union : Extension<
   "empty %select{struct|union}0 is a GNU extension">, InGroup<GNUEmptyStruct>;
 def ext_no_named_members_in_struct_union : Extension<
   "%select{struct|union}0 without named members is a GNU extension">, InGroup<GNUEmptyStruct>;
 def warn_zero_size_struct_union_compat : Warning<"%select{|empty }0"
   "%select{struct|union}1 has size 0 in C, %select{size 1|non-zero size}2 in C++">,
   InGroup<CXXCompat>, DefaultIgnore;
+def warn_zero_size_struct_union : Warning<"%select{|empty }0"
+  "%select{struct|union}1 has size 0 in C, %select{size 1|non-zero size}2 in C++">,
+  InGroup<ExternCCompat>;
 } // End of general sema category.
 
 // inline asm.
 let CategoryName = "Inline Assembly Issue" in {
   def err_asm_wide_character : Error<"wide string is invalid in 'asm'">;
   def err_asm_invalid_lvalue_in_output : Error<"invalid lvalue in asm output">;
   def err_asm_invalid_output_constraint : Error<
     "invalid output constraint '%0' in asm">;

lib/AST/Decl.cpp

 LanguageLinkage VarDecl::getLanguageLinkage() const {
   return getLanguageLinkageTemplate(*this);
 }
 
 bool VarDecl::isExternC() const {
   return isExternCTemplate(*this);
 }
 
-static bool isLinkageSpecContext(const DeclContext *DC,
-                                 LinkageSpecDecl::LanguageIDs ID) {
-  while (DC->getDeclKind() != Decl::TranslationUnit) {
-    if (DC->getDeclKind() == Decl::LinkageSpec)
-      return cast<LinkageSpecDecl>(DC)->getLanguage() == ID;
-    DC = DC->getParent();
-  }
-  return false;
-}
-
-template <typename T>
-static bool isInLanguageSpecContext(T *D, LinkageSpecDecl::LanguageIDs ID) {
-  return isLinkageSpecContext(D->getLexicalDeclContext(), ID);
-}
-
 bool VarDecl::isInExternCContext() const {
-  return isInLanguageSpecContext(this, LinkageSpecDecl::lang_c);
+  return getLexicalDeclContext()->isExternCContext();
 }
 
 bool VarDecl::isInExternCXXContext() const {
-  return isInLanguageSpecContext(this, LinkageSpecDecl::lang_cxx);
+  return getLexicalDeclContext()->isExternCXXContext();
 }
 
 VarDecl *VarDecl::getCanonicalDecl() { return getFirstDecl(); }
 
 VarDecl::DefinitionKind VarDecl::isThisDeclarationADefinition(
   ASTContext &C) const
 {
   // C++ [basic.def]p2:
LanguageLinkage FunctionDecl::getLanguageLinkage() const {
   return getLanguageLinkageTemplate(*this);
 }
 
 bool FunctionDecl::isExternC() const {
   return isExternCTemplate(*this);
 }
 
 bool FunctionDecl::isInExternCContext() const {
-  return isInLanguageSpecContext(this, LinkageSpecDecl::lang_c);
+  return getLexicalDeclContext()->isExternCContext();
 }
 
 bool FunctionDecl::isInExternCXXContext() const {
-  return isInLanguageSpecContext(this, LinkageSpecDecl::lang_cxx);
+  return getLexicalDeclContext()->isExternCXXContext();
 }
 
 bool FunctionDecl::isGlobal() const {
   if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(this))
     return Method->isStatic();
 
   if (getCanonicalDecl()->getStorageClass() == SC_Static)
     return false;

lib/AST/DeclBase.cpp

bool DeclContext::isTransparentContext() const {
   if (DeclKind == Decl::Enum)
     return !cast<EnumDecl>(this)->isScoped();
   else if (DeclKind == Decl::LinkageSpec)
     return true;
 
   return false;
 }
 
+static bool isLinkageSpecContext(const DeclContext *DC,
+                                 LinkageSpecDecl::LanguageIDs ID) {
+  while (DC->getDeclKind() != Decl::TranslationUnit) {
+    if (DC->getDeclKind() == Decl::LinkageSpec)
+      return cast<LinkageSpecDecl>(DC)->getLanguage() == ID;
+    DC = DC->getParent();
+  }
+  return false;
+}
+
+bool DeclContext::isExternCContext() const {
+  return isLinkageSpecContext(this, clang::LinkageSpecDecl::lang_c);
+}
+
+bool DeclContext::isExternCXXContext() const {
+  return isLinkageSpecContext(this, clang::LinkageSpecDecl::lang_cxx);
+}
+
 bool DeclContext::Encloses(const DeclContext *DC) const {
   if (getPrimaryContext() != this)
     return getPrimaryContext()->Encloses(DC);
 
   for (; DC; DC = DC->getParent())
     if (DC->getPrimaryContext() == this)
       return true;
   return false;

lib/Sema/SemaDecl.cpp

if (Record) {
     }
     
     if (!Completed)
       Record->completeDefinition();
 
     if (Record->hasAttrs())
       CheckAlignasUnderalignment(Record);
 
-    // Check if the structure/union declaration is a language extension.
+    // Check if the structure/union declaration is a type that can have zero
+    // size in C. For C this is a language extension, for C++ it may cause
+    // compatibility problems.
+    bool CheckForZeroSize;
     if (!getLangOpts().CPlusPlus) {
+      CheckForZeroSize = true;
+    } else {
+      // For C++ filter out types that cannot be referenced in C code.
+      if (Record->getLexicalDeclContext()->isExternCContext() &&
+          !Record->isDependentType()) {
+        if (CXXRecordDecl *CXXRec = dyn_cast<CXXRecordDecl>(Record)) {
+          CheckForZeroSize =
+              (CXXRec->method_begin() == CXXRec->method_end()) &&
+              CXXRec->getNumBases() == 0 &&
+              CXXRec->getNumVBases() == 0 &&
+              (CXXRec->getTagKind() == TTK_Struct ||
+               CXXRec->getTagKind() == TTK_Union) &&
+              CXXRec->getTemplateSpecializationKind() == TSK_Undeclared;
+        } else {
+          CheckForZeroSize = true;
+        }
+      }
+    }
+    if (CheckForZeroSize) {
       bool ZeroSize = true;
       bool IsEmpty = true;
       unsigned NonBitFields = 0;
       for (RecordDecl::field_iterator I = Record->field_begin(),
                                       E = Record->field_end();
            (NonBitFields == 0 || ZeroSize) && I != E; ++I) {
         IsEmpty = false;
         if (I->isUnnamedBitfield()) {
           if (I->getBitWidthValue(Context) > 0)
             ZeroSize = false;
         } else {
           ++NonBitFields;
           QualType FieldType = I->getType();
           if (FieldType->isIncompleteType() ||
+              FieldType->isDependentType()  ||
+              FieldType->isUndeducedType()  ||
               !Context.getTypeSizeInChars(FieldType).isZero())
             ZeroSize = false;
         }
       }
 
-      // Empty structs are an extension in C (C99 6.7.2.1p7), but are allowed in
-      // C++.
+      if (!getLangOpts().CPlusPlus) {
+        // Empty structs are an extension in C (C99 6.7.2.1p7), but are allowed
+        // in C++.
         if (ZeroSize)
           Diag(RecLoc, diag::warn_zero_size_struct_union_compat) << IsEmpty
               << Record->isUnion() << (NonBitFields > 1);
 
         // Structs without named members are extension in C (C99 6.7.2.1p7), but
         // are accepted by GCC.
         if (NonBitFields == 0) {
           if (IsEmpty)
             Diag(RecLoc, diag::ext_empty_struct_union) << Record->isUnion();
           else
-          Diag(RecLoc, diag::ext_no_named_members_in_struct_union) << Record->isUnion();
+            Diag(RecLoc, diag::ext_no_named_members_in_struct_union)
+              << Record->isUnion();
+        }
+      } else if (ZeroSize) {
+        // Empty structs are OK in C++ but will warn if its declaration is inside
+        // extern "C" block.
+        Diag(RecLoc, diag::warn_zero_size_struct_union) << IsEmpty
+          << Record->isUnion() << (NonBitFields > 1);
       }
     }
   } else {
     ObjCIvarDecl **ClsFields =
       reinterpret_cast<ObjCIvarDecl**>(RecFields.data());
     if (ObjCInterfaceDecl *ID = dyn_cast<ObjCInterfaceDecl>(EnclosingDecl)) {
       ID->setEndOfDefinitionLoc(RBrac);
       // Add ivar's to class's DeclContext.
       for (unsigned i = 0, e = RecFields.size(); i != e; ++i) {

test/CXX/basic/basic.lookup/basic.lookup.argdep/p2.cpp

namespace O {
   void test_global_scope_adl(Z z) {
     {
       int& ir = f(z);
     }
   }
 }
 
 extern "C" {
-  struct L { };
+  struct L { int x; };
 }
 
 void h(L); // expected-note{{candidate function}}
 
 namespace P {
   void h(L); // expected-note{{candidate function}}
   void test_transparent_context_adl(L l) {
     {

test/SemaCXX/extern-c.cpp

 }
 namespace N2 {
   extern "C" void different_kind_1(); // expected-error {{different kind of symbol}}
   extern "C" int different_kind_2; // expected-error {{different kind of symbol}}
 }
 
 // We allow all these even though the standard says they are ill-formed.
 extern "C" {
-  struct stat {};
+  struct stat {};   // expected-warning{{empty struct has size 0 in C, size 1 in C++}}
   void stat(struct stat);
 }
 namespace X {
   extern "C" {
     void stat(struct ::stat);
   }
 }
 int stat(int *p);

 }
 int global_var_vs_extern_c_fn_2; // expected-error {{conflicts with declaration with C language linkage}}
 int global_var_vs_extern_c_var_1; // expected-note {{here}}
 namespace X {
   extern "C" double global_var_vs_extern_c_var_1; // expected-error {{conflicts with declaration in global scope}}
   extern "C" double global_var_vs_extern_c_var_2; // expected-note {{here}}
 }
 int global_var_vs_extern_c_var_2; // expected-error {{conflicts with declaration with C language linkage}}
+
+extern "C" {
+  union pr5065_1 {}; // expected-warning{{empty union has size 0 in C, size 1 in C++}}
+  struct pr5065_2 { int: 0; }; // expected-warning{{struct has size 0 in C, size 1 in C++}}
+}

test/SemaCXX/storage-class.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 extern const int PR6495a = 42;
 extern int PR6495b = 42; // expected-warning{{'extern' variable has an initializer}}
 extern const int PR6495c[] = {42,43,44};
 
 extern struct Test1 {}; // expected-warning {{'extern' is not permitted on a declaration of a type}}
-extern "C" struct Test0 {}; // no warning
+extern "C" struct Test0 { int x; }; // no warning